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.
>
> > +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.
> > + 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.
> > +#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.
> ...
>
> > +/*
> > + * 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)))
>
> ...
>
> > +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()
> > +static int elba_spics_get_value(struct gpio_chip *chip, unsigned int pin)
> > +{
> > + return -ENXIO;
>
> Hmm... Is it really acceptable error code here?
>
> > +static int elba_spics_direction_input(struct gpio_chip *chip, unsigned int pin)
> > +{
> > + return -ENXIO;
>
> Ditto.
>
Changed both 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);
Implementation follows devm_ioremap_resource() example in lib/devres.c.
> > + if (IS_ERR(p->base)) {
>
> > + dev_err(&pdev->dev, "failed to remap I/O memory\n");
>
> Duplicate noisy 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;
Cleaned this up in patchset v2.
