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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 6 Dec 2022 14:48:29 +0100
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Edmund Berenson <edmund.berenson@...ix.com>,
        Mark Brown <broonie@...nel.org>
Cc:     Lukasz Zemla <Lukasz.Zemla@...dward.com>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH 2/2] gpio: max7317: Add gpio expander driver

Hi Edmund & Lukasz,

thanks for your patch!

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

(...)

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