Re: [PATCH 2/2] gpio: max7317: Add gpio expander driver

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Edmund & Lukasz,

thanks for your patch!

On Tue, Dec 6, 2022 at 1:45 PM Edmund Berenson
<edmund.berenson@xxxxxxxxx> wrote:
>
> Add driver for maxim MAX7317 SPI-Interfaced 10 Port
> GPIO Expander.
>
> Co-developed-by: Lukasz Zemla <Lukasz.Zemla@xxxxxxxxxxxx>
> Signed-off-by: Lukasz Zemla <Lukasz.Zemla@xxxxxxxxxxxx>
> Signed-off-by: Edmund Berenson <edmund.berenson@xxxxxxxxx>

(...)

> +config GPIO_MAX7317
> +       tristate "Maxim MAX7317 GPIO expander"
> +       depends on SPI

Skip this. Just put it inside the section:

menu "SPI GPIO expanders"
        depends on SPI_MASTER

But consider this:

select REGMAP_SPI

> +/* A write to the MAX7317 means one message with one transfer */
> +static int max7317_spi_write(struct device *dev, unsigned int reg,
> +                               unsigned int val)
> +{
> +       struct spi_device *spi = to_spi_device(dev);
> +       u16 word = ((reg & 0x7F) << 8) | (val & 0xFF);
> +
> +       return spi_write_then_read(spi, &word, sizeof(word), NULL, 0);
> +}
> +
> +/* A read from the MAX7317 means two transfers, with chip select going high between them. */
> +static int max7317_spi_read(struct device *dev, unsigned int reg)
> +{
> +       int ret;
> +       u16 word;
> +       struct spi_device *spi = to_spi_device(dev);
> +
> +       word = 0x8000 | (reg << 8);
> +
> +       /* First transfer: ask to read register */
> +       ret = spi_write(spi, &word, sizeof(word));
> +       if (ret)
> +               return ret;
> +
> +       /* Second transfer: read register value */
> +       ret = spi_read(spi, &word, sizeof(word));
> +       if (ret)
> +               return ret;
> +
> +       return word & 0xff;
> +}

I bet this is standard SPI addressing of some kind, if in doubt Mark Brown
will know. Look into REGMAP_SPI and use regmap accessors in the driver.
Look at: drivers/gpio/gpio-xra1403.c for an example.

> +static int max7317_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct max7317 *ts = gpiochip_get_data(chip);
> +       unsigned int reg, val, mask, shift;

Switch to unsigned long when using regmap.

> +       int inputs;
> +
> +       if (offset < 8) {
> +               reg = REG_CODE_READ_PORTS_7_TO_0;
> +               mask = 1u << offset;

mask = BIT(offset);

> +               shift = offset;
> +       } else {
> +               reg = REG_CODE_READ_PORTS_9_TO_8;
> +               mask = 1u << (offset - 8);

mask = BIT(offset - 8);

> +               shift = offset - 8;
> +       }
> +
> +       mutex_lock(&ts->lock);
> +       inputs = max7317_spi_read(ts->dev, reg);
> +       mutex_unlock(&ts->lock);

Regmap also contains a mutex already so you will just need a oneliner
for things like this.

> +       val = ((unsigned int)inputs & mask) >> shift;
> +       return (int)val;

Ugh get rid of this shifting and casting.

I tend to do:

return !!(inputs & mask);

That will clamp whatever value to a bool.

Once you have converted the driver to use REGMAP_SPI, take a
look at GPIO_REGMAP in drivers/gpio/gpio-regmap.c and see if
you can also use that helper to slim this down even more. (Uncertain
about that, but check it!)

Yours,
Linus Walleij