Re: [PATCH 1/8] gpio: Add Elba SoC gpio driver for spi cs control

[Brad Larson]

Hi Andy,

On Sun, Mar 7, 2021 at 11:21 AM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
> On Thu, Mar 4, 2021 at 4:40 PM Brad Larson <brad@xxxxxxxxxxx> wrote:
> >
> > This GPIO driver is for the Pensando Elba SoC which
> > provides control of four chip selects on two SPI busses.
>
> I will try to avoid repeating otheris in their reviews, but my comments below.
>
> ...
>
> > +config GPIO_ELBA_SPICS
> > +       bool "Pensando Elba SPI chip-select"
>
> Can't it be a module? Why?
>
> > +       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).
>
> ...
>
> > +#include <linux/of.h>
>
> It's not used here, but you missed mod_devicetable.h.

Based on the feedback I realized this should not be a loadable module.
I should be using builtin_platform_driver(elba_spics_driver).
Currently I have this for gpio/Kconfig

config GPIO_ELBA_SPICS
        def_bool y
        depends on ARCH_PENSANDO_ELBA_SOC || COMPILE_TEST

> > +/*
> > + * pin:             3            2        |       1            0
> > + * bit:         7------6------5------4----|---3------2------1------0
> > + *     cs1  cs1_ovr  cs0  cs0_ovr |  cs1  cs1_ovr  cs0  cs0_ovr
> > + *                ssi1            |             ssi0
> > + */
> > +#define SPICS_PIN_SHIFT(pin)   (2 * (pin))
> > +#define SPICS_MASK(pin)                (0x3 << SPICS_PIN_SHIFT(pin))
>
> > +#define SPICS_SET(pin, val)    ((((val) << 1) | 0x1) << SPICS_PIN_SHIFT(pin))
>
> Isn't it easier to define as ((value) << (2 * (pin) + 1) | BIT(2 * (pin)))

Both are functionally correct.  I don't have a preference, do you want
this change?

> > +struct elba_spics_priv {
> > +       void __iomem *base;
> > +       spinlock_t lock;
>
> > +       struct gpio_chip chip;
>
> If you put it as a first member a container_of() becomes a no-op. OTOH
> dunno if there is any such container_of() use in the code.

There is no use of container_of() for this structure

> > +static int elba_spics_get_value(struct gpio_chip *chip, unsigned int pin)
> > +{
> > +       return -ENXIO;
>
> Hmm... Is it really acceptable error code here?

No it's not, thanks.  Changed to -ENOTSUPP as gpio output direction
only is supported.

> > +static int elba_spics_direction_input(struct gpio_chip *chip, unsigned int pin)
> > +{
> > +       return -ENXIO;
>
> Ditto.

Changed to ENOTSUPP

> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       p->base = devm_ioremap_resource(&pdev->dev, res);
>
> p->base = devm_platform_ioremap_resource(pdev, 0);

Changed to single call to devm_platform_ioremap_resource(pdev, 0)

> > +       if (IS_ERR(p->base)) {
>
> > +               dev_err(&pdev->dev, "failed to remap I/O memory\n");
>
> Duplicate noisy message.

Removed extra log message

> > +               return PTR_ERR(p->base);
> > +       }
>
> > +       ret = devm_gpiochip_add_data(&pdev->dev, &p->chip, p);
> > +       if (ret) {
> > +               dev_err(&pdev->dev, "unable to add gpio chip\n");
>
> > +               return ret;
> > +       }
> > +
> > +       dev_info(&pdev->dev, "elba spics registered\n");
> > +       return 0;
>
> if (ret)
>   dev_err(...);
> return ret;

Yes, made this change and will include in v3 patchset

--- a/drivers/gpio/gpio-elba-spics.c
+++ b/drivers/gpio/gpio-elba-spics.c
@@ -91,13 +91,9 @@ static int elba_spics_probe(struct platform_device *pdev)
        ret = devm_gpiochip_add_data(&pdev->dev, &p->chip, p);
-       if (ret) {
+       if (ret)
                dev_err(&pdev->dev, "unable to add gpio chip\n");
-               return ret;
-       }
-
-       dev_info(&pdev->dev, "elba spics registered\n");
-       return 0;
+       return ret;

Regards,
Brad