Messages in this thread | | | From | Brad Larson <> | Date | Sun, 22 Aug 2021 18:13:50 -0700 | Subject | Re: [PATCH 1/8] gpio: Add Elba SoC gpio driver for spi cs control |
| |
Hi Andy,
On Mon, Mar 29, 2021 at 3:40 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Mar 29, 2021 at 4:19 AM Brad Larson <brad@pensando.io> wrote: > > On Sun, Mar 7, 2021 at 11:21 AM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Thu, Mar 4, 2021 at 4:40 PM Brad Larson <brad@pensando.io> wrote: > > ... > > > > > +config GPIO_ELBA_SPICS > > > > + bool "Pensando Elba SPI chip-select" > > > > > > Can't it be a module? Why? > > > > All Elba SoC based platforms require this driver to be built-in to boot and > > removing the module would result in a variety of exceptions/errors. > > Needs to be at least in the commit message. > > > > > > > + depends on ARCH_PENSANDO_ELBA_SOC > > > > + help > > > > + Say yes here to support the Pensndo Elba SoC SPI chip-select driver > > > > > > Please give more explanation what it is and why users might need it, > > > and also tell users how the module will be named (if there is no > > > strong argument why it can't be a module). > > > > > Fixed the typo. > > Yeah, according to the above, you better elaborate what this module is > and why people would need it. > Also can be a good hint to add > default ARCH_MY_COOL_PLATFORM
Regarding the above module question and Kconfig definition, since I first looked at this and reviewed the comments I realized I should be using builtin. The file gpio/Kconfig is currently this
config GPIO_ELBA_SPICS def_bool y depends on ARCH_PENSANDO_ELBA_SOC || COMPILE_TEST
> ... > > > > > +#include <linux/of.h> > > > > > > It's not used here, but you missed mod_devicetable.h. > > > > Removed <linux/of.h>. There is no dependency on mod_devicetable.h. > > What do you mean? You don't use data structures from that? > of_device_id or other ID structures are defined there. Your module > works without them? > I typed the wrong filename. I do still have <linux/of.h>
> > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > + p->base = devm_ioremap_resource(&pdev->dev, res); > > > > > > p->base = devm_platform_ioremap_resource(pdev, 0); > > > > Implementation follows devm_ioremap_resource() example in lib/devres.c. > > So? How does this make it impossible to address my comment?
I was simply stating that I followed the recommended API per the source code although I don't recall if I was looking at 4.14, 5.10 or linux-next at the time. Changed to using devm_platform_ioremap_resource().
> > > > + if (IS_ERR(p->base)) { > > > > > > > + dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > > > > > Duplicate noisy message. > > > > > > > + return PTR_ERR(p->base); > > > > + }
Yep, I've removed the extraneous log message.
Regards, Brad
| |