[PATCH] gpio: mcp230xx: Introduce new driver

[Simon Glass]

Hi Sebastian,

On Tue, 6 Jul 2021 at 10:34, Sebastian Reichel
<sebastian.reichel at collabora.com> wrote:
>
> Introduce driver for I2C based MCP230xx GPIO chips, which are
> quite common and already well supported by the Linux kernel.
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel at collabora.com>
> ---
>  drivers/gpio/Kconfig         |  10 ++
>  drivers/gpio/Makefile        |   1 +
>  drivers/gpio/mcp230xx_gpio.c | 235 +++++++++++++++++++++++++++++++++++
>  3 files changed, 246 insertions(+)
>  create mode 100644 drivers/gpio/mcp230xx_gpio.c

Reviewed-by: Simon Glass <sjg at chromium.org>

a few nits below

>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index de4dc51d4b48..f93e736639bb 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -179,6 +179,16 @@ config LPC32XX_GPIO
>         help
>           Support for the LPC32XX GPIO driver.
>
> +config MCP230XX_GPIO
> +       bool "MCP230XX GPIO driver"
> +       depends on DM
> +       help
> +         Support for Microchip's MCP230XX I2C connected GPIO devices.
> +         The following chips are supported:
> +          - MCP23008
> +          - MCP23017
> +          - MCP23018
> +
>  config MSCC_SGPIO
>         bool "Microsemi Serial GPIO driver"
>         depends on DM_GPIO && SOC_VCOREIII
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 8541ba0b0aec..00ffcc753534 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_KIRKWOOD_GPIO)   += kw_gpio.o
>  obj-$(CONFIG_KONA_GPIO)        += kona_gpio.o
>  obj-$(CONFIG_MARVELL_GPIO)     += mvgpio.o
>  obj-$(CONFIG_MARVELL_MFP)      += mvmfp.o
> +obj-$(CONFIG_MCP230XX_GPIO)    += mcp230xx_gpio.o
>  obj-$(CONFIG_MXC_GPIO) += mxc_gpio.o
>  obj-$(CONFIG_MXS_GPIO) += mxs_gpio.o
>  obj-$(CONFIG_PCA953X)          += pca953x.o
> diff --git a/drivers/gpio/mcp230xx_gpio.c b/drivers/gpio/mcp230xx_gpio.c
> new file mode 100644
> index 000000000000..44def86ca7d9
> --- /dev/null
> +++ b/drivers/gpio/mcp230xx_gpio.c
> @@ -0,0 +1,235 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2021, Collabora Ltd.
> + * Copyright (C) 2021, General Electric Company
> + * Author(s): Sebastian Reichel <sebastian.reichel at collabora.com>
> + */
> +
> +#define LOG_CATEGORY UCLASS_GPIO
> +
> +#include <common.h>
> +#include <errno.h>
> +#include <dm.h>
> +#include <i2c.h>
> +#include <asm/gpio.h>
> +#include <dm/device_compat.h>
> +#include <dt-bindings/gpio/gpio.h>
> +
> +enum mcp230xx_type {
> +       UNKNOWN = 0,
> +       MCP23008,
> +       MCP23017,
> +       MCP23018,
> +};
> +
> +#define MCP230XX_IODIR 0x00
> +#define MCP230XX_GPPU 0x06
> +#define MCP230XX_GPIO 0x09
> +#define MCP230XX_OLAT 0x0a
> +
> +#define BANKSIZE 8
> +
> +static int mcp230xx_read(struct udevice *dev, uint reg, uint offset)
> +{
> +       struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> +       int bank = offset / BANKSIZE;
> +       int mask = 1 << (offset % BANKSIZE);
> +       int shift = (uc_priv->gpio_count / BANKSIZE) - 1;
> +       int ret;
> +
> +       ret = dm_i2c_reg_read(dev, (reg << shift) + bank);
> +       if (ret < 0)
> +               return ret;

blank line before final return

> +       return !!(ret & mask);
> +}
> +
> +static int mcp230xx_write(struct udevice *dev, uint reg, uint offset, bool val)
> +{
> +       struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> +       int bank = offset / BANKSIZE;
> +       int mask = 1 << (offset % BANKSIZE);
> +       int shift = (uc_priv->gpio_count / BANKSIZE) - 1;
> +       int regval;
> +
> +       regval = dm_i2c_reg_read(dev, (reg << shift) + bank);

Can I suggest adding something like

int i2c_reg_clrset(struct udevice *dev, uint offset, u32 clr, u32 set)

which would be useful in this situation. If you do that, add a test in
test/dm/i2c.c

> +       if (regval  < 0)
> +               return regval;
> +
> +       regval &= ~mask;
> +       if (val)
> +               regval |= mask;
> +
> +       return dm_i2c_reg_write(dev, (reg << shift) + bank, regval);

To my eye this would be better as

   reg << shift | bank

because + indicates addition. I suspect it is actually ORing in the addres?

> +}
> +
> +static int mcp230xx_get_value(struct udevice *dev, uint offset)
> +{
> +       int ret;
> +
> +       ret = mcp230xx_read(dev, MCP230XX_GPIO, offset);
> +       if (ret < 0) {
> +               dev_err(dev, "%s error: %d\n", __func__, ret);
> +               return ret;
> +       }
> +
> +       return ret;
> +}
> +
> +static int mcp230xx_set_value(struct udevice *dev, uint offset, int val)
> +{
> +       int ret;
> +
> +       ret = mcp230xx_write(dev, MCP230XX_GPIO, offset, val);
> +       if (ret < 0) {
> +               dev_err(dev, "%s error: %d\n", __func__, ret);
> +               return ret;
> +       }
> +
> +       return ret;
> +}
> +
> +static int mcp230xx_get_flags(struct udevice *dev, unsigned int offset,
> +                                 ulong *flags)
> +{
> +       int direction, pullup;
> +
> +       pullup = mcp230xx_read(dev, MCP230XX_GPPU, offset);
> +       if (pullup < 0) {
> +               dev_err(dev, "%s error: %d\n", __func__, pullup);
> +               return pullup;
> +       }
> +
> +       direction = mcp230xx_read(dev, MCP230XX_IODIR, offset);
> +       if (direction < 0) {
> +               dev_err(dev, "%s error: %d\n", __func__, direction);
> +               return direction;
> +       }
> +
> +       *flags = direction ? GPIOD_IS_IN : GPIOD_IS_OUT;
> +
> +       if (pullup)
> +               *flags |= GPIOD_PULL_UP;
> +
> +       return 0;
> +}
> +
> +static int mcp230xx_set_flags(struct udevice *dev, uint offset, ulong flags)
> +{
> +       int direction = !(flags & GPIOD_IS_OUT);
> +       int pullup = !!(flags & GPIOD_PULL_UP);

If you use bool you don't need the !!

> +       ulong supported_mask;
> +       int ret;
> +
> +       /* Note: active-low is ignored (handled by core) */
> +       supported_mask = GPIOD_ACTIVE_LOW | GPIOD_IS_IN | GPIOD_IS_OUT | GPIOD_PULL_UP;
> +       if (flags & ~supported_mask)
> +               return -EINVAL;
> +

[..]


Regards,
Simon