[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACRpkdYnbQfbAhFCeNh6dD_4zfPPxcEREX9QOMEqKu5tVDVK1A@mail.gmail.com>
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
Powered by blists - more mailing lists