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
