Hi Andy,

On Mon, Mar 29, 2021 at 3:40 AM Andy Shevchenko
[off-list ref] wrote:

On Mon, Mar 29, 2021 at 4:19 AM Brad Larson [off-list ref] wrote:

quoted

On Sun, Mar 7, 2021 at 11:21 AM Andy Shevchenko
[off-list ref] wrote:

quoted

On Thu, Mar 4, 2021 at 4:40 PM Brad Larson [off-list ref] wrote:

...

quoted

quoted

quoted

+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.

quoted

quoted

quoted

+       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

...

quoted

quoted

quoted

+#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>

quoted

quoted

quoted

+       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().

quoted

quoted

quoted

+       if (IS_ERR(p->base)) {

quoted

+               dev_err(&pdev->dev, "failed to remap I/O memory\n");

Duplicate noisy message.

quoted

+               return PTR_ERR(p->base);
+       }

Yep, I've removed the extraneous log message.

Regards,
Brad