From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 36D96C433F5 for ; Sun, 9 Oct 2022 03:56:44 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 41EF384BD8; Sun, 9 Oct 2022 05:56:41 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="Ej9dSzxW"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id B826084DD0; Sun, 9 Oct 2022 05:56:39 +0200 (CEST) Received: from mail-qv1-xf2a.google.com (mail-qv1-xf2a.google.com [IPv6:2607:f8b0:4864:20::f2a]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 16A1C84C4B for ; Sun, 9 Oct 2022 05:56:36 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=seanga2@gmail.com Received: by mail-qv1-xf2a.google.com with SMTP id f14so5441694qvo.3 for ; Sat, 08 Oct 2022 20:56:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:from:user-agent:mime-version:date:message-id:from:to :cc:subject:date:message-id:reply-to; bh=tqhtVv95O51w8bO3TobyW8ip9YA5wOsthqIcgc3lSL8=; b=Ej9dSzxWMsV4kabf4fA2JbXPx82PK6CPh8oGeFLPYY0AKng0b+TCjzis2QNlrdBocT wHm+CCFNuweXFLh/20iK1nzeKF7HoYMa8yxvjdbz5FqWyvZl6JbLDeaSqIDiJwpWy7tH QN3rW17LQZGHqUA0uSsB6N/WhneNXWBGZ1OtXYJ+svaFRJknD1Zd5mJOC8ZNdvZSraKf pv8temrx/use1UzRvTb/LcdAhzgMWB+TP2FrtD/dJhukeoISbpBYUWL8WETz4mbPYjxl dK9YJx9dcMctHltMMyPW+EqBwuubAXh7QjZa76taaEXu2gVsuH9ZRfuA5YJ8C7+Ave/l LOww== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:from:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=tqhtVv95O51w8bO3TobyW8ip9YA5wOsthqIcgc3lSL8=; b=LHKBEnNKz74ReMsLvmPOFi0G84cG6kKRVRgG5acYT5SH0xHDWvqCGF2NHeUcjCj7I3 unxccp8Ghyr0NNBBH0JVk/UFLQqyz20x+hvsBfFEd34/gdcy03E9aqpzGCDgIHrfUAVH Fk6SvhTbrjh4v2qj7mIp6PumRVuPwFFx3a/CVYPNcQ/y8sYf+wus8E/iqiLzUlNXEyuG ISZz7r4e7gz9LpNMU3MfueDJvE6ZJQZeKLfM/MMjHrsi/vthvWagTu2KI23pk1dAlxP9 QVgw6MsA5tuN2s9ckbZ+6oFoCpKEBOnNiL20zPDaVpHN8OAFWEX5db1Hh18LT5eTn0dE snpA== X-Gm-Message-State: ACrzQf1dxwwkoZ8O8X9h/yYJY5Mbc9WXNB7TFdowGg17zhupuDdFcNfh EthGTpDZMFfw69u48ZhoiT4= X-Google-Smtp-Source: AMsMyM4wuXakNcN7DsstmDYgN5Zc8UCfzmCV4Ae8QO9SFDjPhTKRtdQxqQ5KWt+fKgmfaPZ8cJFqgw== X-Received: by 2002:ad4:4ea2:0:b0:4b1:86aa:e31a with SMTP id ed2-20020ad44ea2000000b004b186aae31amr10250553qvb.10.1665287794855; Sat, 08 Oct 2022 20:56:34 -0700 (PDT) Received: from [192.168.1.201] (pool-173-73-95-180.washdc.fios.verizon.net. [173.73.95.180]) by smtp.gmail.com with ESMTPSA id k11-20020a05620a0b8b00b006cbc6e1478csm6271156qkh.57.2022.10.08.20.56.33 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 08 Oct 2022 20:56:34 -0700 (PDT) Message-ID: Date: Sat, 8 Oct 2022 23:56:32 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 From: Sean Anderson Subject: Re: [PATCH v2] gpio: adp5585: add gpio driver for ADP5585 I/O Expander Controller To: "Alice Guo (OSS)" , sbabic@denx.de, festevam@gmail.com, sjg@chromium.org Cc: uboot-imx@nxp.com, u-boot@lists.denx.de References: <20221009031922.13510-1-alice.guo@oss.nxp.com> Content-Language: en-US In-Reply-To: <20221009031922.13510-1-alice.guo@oss.nxp.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean Hi Alice, On 10/8/22 23:19, Alice Guo (OSS) wrote: > From: Alice Guo > > 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 > --- > > 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 > + */ > + > +#include > +#include > +#include > +#include > + > +#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