Hi Sebastian,
On Tue, 6 Jul 2021 at 10:34, Sebastian Reichel sebastian.reichel@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@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@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 DMhelpSupport for Microchip's MCP230XX I2C connected GPIO devices.The following chips are supported:- MCP23008- MCP23017- MCP23018config 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@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