Re: [PATCH v2] gpio: adp5585: add gpio driver for ADP5585 I/O Expander Controller

[Sean Anderson <seanga2@gmail.com>]

Hi Alice,

On 10/8/22 23:19, Alice Guo (OSS) wrote:
> From: Alice Guo <alice.guo@nxp.com>
> 
> Add gpio driver for ADP5585 I/O Expander Controller. The ADP5585 is a 10
> input/output port expander and can be used to increase the number of
> I/Os available to a processor.
> 
> Signed-off-by: Alice Guo <alice.guo@nxp.com>
> ---
> 
> Changes for v2:
>   - add a commit log
>   - remove unrelated change
>   - remove "on i.MX platform" in Kconfig file
> 
>   drivers/gpio/Kconfig        |   6 +
>   drivers/gpio/Makefile       |   1 +
>   drivers/gpio/adp5585_gpio.c | 238 ++++++++++++++++++++++++++++++++++++
>   3 files changed, 245 insertions(+)
>   create mode 100644 drivers/gpio/adp5585_gpio.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index c949f9d2f7..c38022d01c 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -605,4 +605,10 @@ config TURRIS_OMNIA_MCU
>   	help
>   	   Support for GPIOs on MCU connected to Turris Omnia via i2c.
>   
> +config ADP5585_GPIO
> +	bool "ADP5585 GPIO driver"
> +	depends on DM_GPIO && DM_I2C
> +	help
> +	  Support ADP5585 GPIO expander.
> +
>   endif
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 9d718a554e..2f60b98384 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -75,3 +75,4 @@ obj-$(CONFIG_SL28CPLD_GPIO)	+= sl28cpld-gpio.o
>   obj-$(CONFIG_ZYNQMP_GPIO_MODEPIN)	+= zynqmp_gpio_modepin.o
>   obj-$(CONFIG_SLG7XL45106_I2C_GPO)	+= gpio_slg7xl45106.o
>   obj-$(CONFIG_$(SPL_TPL_)TURRIS_OMNIA_MCU)	+= turris_omnia_mcu.o
> +obj-$(CONFIG_ADP5585_GPIO)	+= adp5585_gpio.o
> diff --git a/drivers/gpio/adp5585_gpio.c b/drivers/gpio/adp5585_gpio.c
> new file mode 100644
> index 0000000000..ea0cb75459
> --- /dev/null
> +++ b/drivers/gpio/adp5585_gpio.c
> @@ -0,0 +1,238 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2022 NXP
> + *
> + * ADP5585 I/O Expander Controller
> + *
> + * Author: Alice Guo <alice.guo@nxp.com>
> + */
> +
> +#include <asm/gpio.h>
> +#include <dm.h>
> +#include <dt-bindings/gpio/gpio.h>
> +#include <i2c.h>
> +
> +#define ADP5585_ID			0x00
> +#define ADP5585_INT_STATUS		0x01
> +#define ADP5585_STATUS			0x02
> +#define ADP5585_FIFO_1			0x03
> +#define ADP5585_FIFO_2			0x04
> +#define ADP5585_FIFO_3			0x05
> +#define ADP5585_FIFO_4			0x06
> +#define ADP5585_FIFO_5			0x07
> +#define ADP5585_FIFO_6			0x08
> +#define ADP5585_FIFO_7			0x09
> +#define ADP5585_FIFO_8			0x0A
> +#define ADP5585_FIFO_9			0x0B
> +#define ADP5585_FIFO_10			0x0C
> +#define ADP5585_FIFO_11			0x0D
> +#define ADP5585_FIFO_12			0x0E
> +#define ADP5585_FIFO_13			0x0F
> +#define ADP5585_FIFO_14			0x10
> +#define ADP5585_FIFO_15			0x11
> +#define ADP5585_FIFO_16			0x12
> +#define ADP5585_GPI_INT_STAT_A		0x13
> +#define ADP5585_GPI_INT_STAT_B		0x14
> +#define ADP5585_GPI_STATUS_A		0x15
> +#define ADP5585_GPI_STATUS_B		0x16
> +#define ADP5585_RPULL_CONFIG_A		0x17
> +#define ADP5585_RPULL_CONFIG_B		0x18
> +#define ADP5585_RPULL_CONFIG_C		0x19
> +#define ADP5585_RPULL_CONFIG_D		0x1A
> +#define ADP5585_GPI_INT_LEVEL_A		0x1B
> +#define ADP5585_GPI_INT_LEVEL_B		0x1C
> +#define ADP5585_GPI_EVENT_EN_A		0x1D
> +#define ADP5585_GPI_EVENT_EN_B		0x1E
> +#define ADP5585_GPI_INTERRUPT_EN_A	0x1F
> +#define ADP5585_GPI_INTERRUPT_EN_B	0x20
> +#define ADP5585_DEBOUNCE_DIS_A		0x21
> +#define ADP5585_DEBOUNCE_DIS_B		0x22
> +#define ADP5585_GPO_DATA_OUT_A		0x23
> +#define ADP5585_GPO_DATA_OUT_B		0x24
> +#define ADP5585_GPO_OUT_MODE_A		0x25
> +#define ADP5585_GPO_OUT_MODE_B		0x26
> +#define ADP5585_GPIO_DIRECTION_A	0x27
> +#define ADP5585_GPIO_DIRECTION_B	0x28
> +#define ADP5585_RESET1_EVENT_A		0x29
> +#define ADP5585_RESET1_EVENT_B		0x2A
> +#define ADP5585_RESET1_EVENT_C		0x2B
> +#define ADP5585_RESET2_EVENT_A		0x2C
> +#define ADP5585_RESET2_EVENT_B		0x2D
> +#define ADP5585_RESET_CFG		0x2E
> +#define ADP5585_PWM_OFFT_LOW		0x2F
> +#define ADP5585_PWM_OFFT_HIGH		0x30
> +#define ADP5585_PWM_ONT_LOW		0x31
> +#define ADP5585_PWM_ONT_HIGH		0x32
> +#define ADP5585_PWM_CFG			0x33
> +#define ADP5585_LOGIC_CFG		0x34
> +#define ADP5585_LOGIC_FF_CFG		0x35
> +#define ADP5585_LOGIC_INT_EVENT_EN	0x36
> +#define ADP5585_POLL_PTIME_CFG		0x37
> +#define ADP5585_PIN_CONFIG_A		0x38
> +#define ADP5585_PIN_CONFIG_B		0x39
> +#define ADP5585_PIN_CONFIG_D		0x3A
> +#define ADP5585_GENERAL_CFG		0x3B
> +#define ADP5585_INT_EN			0x3C
> +
> +#define ADP5585_MAXGPIO			10
> +#define ADP5585_BANK(offs)		((offs) > 4)
> +#define ADP5585_BIT(offs)		((offs) > 4 ? \
> +					1u << ((offs) - 5) : 1u << (offs))

I had a look at Analog's website, and it looks like there are several
other parts in this family [1]. While this strategy for allocating GPIOs
may be fine for this part, other parts use all GPIOs in a bank. If
someone wanted to add support for these parts, they would need to add
special cases to ensure that GPIO numbering remained the same for the
5585. I suggest using a scheme like

	#define ADP558X_BANK(offs)	((offs) >> 3)
	#define ADP558X_BIT(offs)	((offs) & 7)

This will allow for easier porting. It does allow for some "invalid"
GPIOs. From what I can tell, GPIOs are allocated from lowest to highest.
So you could do something like

	struct adp558x_cfg {
		int bank_gpios[3];
	};

	static struct adp558x_cfg adp5585_cfg = {
		.bank_gpios = { 6, 4 },
	};

	static const struct udevice_id adp5585_ids[] = {
		{ .compatible = "adp5585", data = (ulong)&adp5585_cfg },
		{ }
	};

which you can then read during probe() and check in xlate(). Actually,
are you sure the condition you have is correct? The upper bank seems to
be the one with 4 GPIOs.

[1] https://www.analog.com/en/parametricsearch/11270

> +struct adp5585_plat {
> +	fdt_addr_t addr;
> +	u8 id;
> +	u8 dat_out[2];
> +	u8 dir[2];
> +};

The plat data struct should be used for configuration data. For example,
the address definitely belongs here. But dat_out and dir belong in
private data. See below wrt my comments on id.

> +
> +static int adp5585_direction_input(struct udevice *dev, unsigned int offset)
> +{
> +	int ret;
> +	unsigned int bank;
> +	struct adp5585_plat *plat = dev_get_plat(dev);
> +
> +	bank = ADP5585_BANK(offset);
> +
> +	plat->dir[bank] &= ~ADP5585_BIT(offset);
> +	ret = dm_i2c_write(dev, ADP5585_GPIO_DIRECTION_A + bank, &plat->dir[bank], 1);
> +
> +	return ret;
> +}
> +
> +static int adp5585_direction_output(struct udevice *dev, unsigned int offset,
> +				    int value)
> +{
> +	int ret;
> +	unsigned int bank, bit;
> +	struct adp5585_plat *plat = dev_get_plat(dev);
> +
> +	bank =  ADP5585_BANK(offset);
> +	bit = ADP5585_BIT(offset);
> +
> +	plat->dir[bank] |= bit;
> +
> +	if (value)
> +		plat->dat_out[bank] |= bit;
> +	else
> +		plat->dat_out[bank] &= ~bit;
> +
> +	ret = dm_i2c_write(dev, ADP5585_GPO_DATA_OUT_A + bank, &plat->dat_out[bank], 1);
> +	ret |= dm_i2c_write(dev, ADP5585_GPIO_DIRECTION_A + bank, &plat->dir[bank], 1);
> +
> +	return ret;
> +}
> +
> +static int adp5585_get_value(struct udevice *dev, unsigned int offset)
> +{
> +	struct adp5585_plat *plat = dev_get_plat(dev);
> +	unsigned int bank = ADP5585_BANK(offset);
> +	unsigned int bit = ADP5585_BIT(offset);
> +	u8 val;
> +
> +	if (plat->dir[bank] & bit)
> +		val = plat->dat_out[bank];
> +	else
> +		dm_i2c_read(dev, ADP5585_GPI_STATUS_A + bank, &val, 1);
> +
> +	return !!(val & bit);
> +}
> +
> +static int adp5585_set_value(struct udevice *dev, unsigned int offset, int value)
> +{
> +	int ret;
> +	unsigned int bank, bit;
> +	struct adp5585_plat *plat = dev_get_plat(dev);
> +
> +	bank =  ADP5585_BANK(offset);
> +	bit = ADP5585_BIT(offset);
> +
> +	if (value)
> +		plat->dat_out[bank] |= bit;
> +	else
> +		plat->dat_out[bank] &= ~bit;
> +
> +	ret = dm_i2c_write(dev, ADP5585_GPO_DATA_OUT_A + bank, &plat->dat_out[bank], 1);
> +
> +	return ret;
> +}
> +
> +static int adp5585_get_function(struct udevice *dev, unsigned int offset)
> +{
> +	unsigned int bank, bit, dir;
> +	struct adp5585_plat *plat = dev_get_plat(dev);
> +
> +	bank =  ADP5585_BANK(offset);
> +	bit = ADP5585_BIT(offset);
> +	dir = plat->dir[bank] & bit;
> +
> +	if (!dir)
> +		return GPIOF_INPUT;
> +	else
> +		return GPIOF_OUTPUT;
> +}
> +
> +static int adp5585_xlate(struct udevice *dev, struct gpio_desc *desc,
> +			 struct ofnode_phandle_args *args)
> +{
> +	desc->offset =  args->args[0];
> +	desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0;
> +
> +	return 0;
> +}
> +
> +static const struct dm_gpio_ops adp5585_ops = {
> +	.direction_input	= adp5585_direction_input,
> +	.direction_output	= adp5585_direction_output,
> +	.get_value		= adp5585_get_value,
> +	.set_value		= adp5585_set_value,
> +	.get_function		= adp5585_get_function,
> +	.xlate			= adp5585_xlate,
> +};
> +
> +static int adp5585_probe(struct udevice *dev)
> +{
> +	struct adp5585_plat *plat = dev_get_plat(dev);
> +	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> +	int ret;
> +
> +	if (!plat)
> +		return 0;
> +
> +	plat->addr = dev_read_addr(dev);
> +	if (plat->addr == FDT_ADDR_T_NONE)
> +		return -EINVAL;
> +
> +	ret = dm_i2c_read(dev, ADP5585_ID, &plat->id, 1);

Is this ever used? Maybe you should check to make sure the ID matches
the compatible. You could also add an "autodetect" compatible. See the
handling of abracon,abx80x in [2] for an example.

[2] https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/rtc/abx80x.c#L470

> +	if (ret < 0)
> +		return ret;
> +
> +	uc_priv->gpio_count = ADP5585_MAXGPIO;
> +	uc_priv->bank_name = "adp5585-gpio";
> +
> +	for (int i = 0; i < 2; i++) {
> +		ret = dm_i2c_read(dev, ADP5585_GPO_DATA_OUT_A + i, &plat->dat_out[i], 1);
> +		if (ret)
> +			return ret;
> +
> +		ret = dm_i2c_read(dev, ADP5585_GPIO_DIRECTION_A + i, &plat->dir[i], 1);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct udevice_id adp5585_ids[] = {
> +	{ .compatible = "adp5585" },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(adp5585) = {
> +	.name	= "adp5585",
> +	.id	= UCLASS_GPIO,
> +	.of_match	= adp5585_ids,
> +	.probe	= adp5585_probe,
> +	.ops	= &adp5585_ops,
> +	.plat_auto	= sizeof(struct adp5585_plat),
> +};

--Sean