devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] *** Add support for TI TCA6418 GPIO chip ***
@ 2025-06-17 20:44 Maria Garcia
  2025-06-17 20:44 ` [PATCH 1/2] dt-bindings: gpio: pca95xx: add TI TCA6418 Maria Garcia
  2025-06-17 20:44 ` [PATCH 2/2] gpio: pca953x: Add support for " Maria Garcia
  0 siblings, 2 replies; 7+ messages in thread
From: Maria Garcia @ 2025-06-17 20:44 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Maria Garcia, Maria Garcia

This series introduces support for the Texas Instruments TCA6418 
GPIO expander chip to the gpio-pca953x driver. It also includes the 
necessary device tree binding definition.

Signed-off-by: Maria Garcia <mariagarcia7293@gmail.com>
---
Maria Garcia (2):
  dt-bindings: gpio: pca95xx: add TI TCA6418
  gpio: pca953x: Add support for TI TCA6418

 .../bindings/gpio/gpio-pca95xx.yaml           |   1 +
 drivers/gpio/gpio-pca953x.c                   | 110 ++++++++++++++++--
 2 files changed, 99 insertions(+), 12 deletions(-)

-- 
2.43.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] dt-bindings: gpio: pca95xx: add TI TCA6418
  2025-06-17 20:44 [PATCH 0/2] *** Add support for TI TCA6418 GPIO chip *** Maria Garcia
@ 2025-06-17 20:44 ` Maria Garcia
  2025-06-18 15:58   ` Conor Dooley
  2025-06-17 20:44 ` [PATCH 2/2] gpio: pca953x: Add support for " Maria Garcia
  1 sibling, 1 reply; 7+ messages in thread
From: Maria Garcia @ 2025-06-17 20:44 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Maria Garcia, Maria Garcia

The TCA6418E is a 18-channel I2C I/O expander with integrated ESD
protection.

Signed-off-by: Maria Garcia <mariagarcia7293@gmail.com>
---
 Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml b/Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml
index 4d3f52f8d1b8..12134c737ad8 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml
+++ b/Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml
@@ -68,6 +68,7 @@ properties:
               - ti,pca9536
               - ti,tca6408
               - ti,tca6416
+              - ti,tca6418
               - ti,tca6424
               - ti,tca9535
               - ti,tca9538
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] gpio: pca953x: Add support for TI TCA6418
  2025-06-17 20:44 [PATCH 0/2] *** Add support for TI TCA6418 GPIO chip *** Maria Garcia
  2025-06-17 20:44 ` [PATCH 1/2] dt-bindings: gpio: pca95xx: add TI TCA6418 Maria Garcia
@ 2025-06-17 20:44 ` Maria Garcia
  2025-06-25 11:53   ` Bartosz Golaszewski
  1 sibling, 1 reply; 7+ messages in thread
From: Maria Garcia @ 2025-06-17 20:44 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Maria Garcia, Maria Garcia

The TI TCA6418 is a 18-channel I2C I/O expander. It is slightly
different to other models from the same family, such as TCA6416,
but has enough in common with them to make it work with just a
few tweaks, which are explained in the code's documentation.

Signed-off-by: Maria Garcia <mariagarcia7293@gmail.com>
---
 drivers/gpio/gpio-pca953x.c | 110 ++++++++++++++++++++++++++++++++----
 1 file changed, 98 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index b852e4997629..9fe28d7ba667 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -38,6 +38,10 @@
 #define PCA953X_INVERT		0x02
 #define PCA953X_DIRECTION	0x03
 
+#define TCA6418_INPUT		0x14
+#define TCA6418_OUTPUT		0x17
+#define TCA6418_DIRECTION	0x23
+
 #define REG_ADDR_MASK		GENMASK(5, 0)
 #define REG_ADDR_EXT		BIT(6)
 #define REG_ADDR_AI		BIT(7)
@@ -76,7 +80,8 @@
 #define PCA953X_TYPE		BIT(12)
 #define PCA957X_TYPE		BIT(13)
 #define PCAL653X_TYPE		BIT(14)
-#define PCA_TYPE_MASK		GENMASK(15, 12)
+#define TCA6418_TYPE		BIT(16)
+#define PCA_TYPE_MASK		GENMASK(16, 12)
 
 #define PCA_CHIP_TYPE(x)	((x) & PCA_TYPE_MASK)
 
@@ -115,6 +120,7 @@ static const struct i2c_device_id pca953x_id[] = {
 	{ "pca6107", 8  | PCA953X_TYPE | PCA_INT, },
 	{ "tca6408", 8  | PCA953X_TYPE | PCA_INT, },
 	{ "tca6416", 16 | PCA953X_TYPE | PCA_INT, },
+	{ "tca6418", 18 | TCA6418_TYPE | PCA_INT, },
 	{ "tca6424", 24 | PCA953X_TYPE | PCA_INT, },
 	{ "tca9538", 8  | PCA953X_TYPE | PCA_INT, },
 	{ "tca9539", 16 | PCA953X_TYPE | PCA_INT, },
@@ -204,6 +210,13 @@ static const struct pca953x_reg_config pca957x_regs = {
 	.invert = PCA957X_INVRT,
 };
 
+static const struct pca953x_reg_config tca6418_regs = {
+	.direction = TCA6418_DIRECTION,
+	.output = TCA6418_OUTPUT,
+	.input = TCA6418_INPUT,
+	.invert = 0xFF, /* Does not apply */
+};
+
 struct pca953x_chip {
 	unsigned gpio_start;
 	struct mutex i2c_lock;
@@ -237,6 +250,21 @@ static int pca953x_bank_shift(struct pca953x_chip *chip)
 	return fls((chip->gpio_chip.ngpio - 1) / BANK_SZ);
 }
 
+/* Helper function to get the correct bit mask for a given offset and chip type.
+ * The TCA6418's input, output, and direction banks have a peculiar bit order:
+ * the first byte uses reversed bit order, while the second byte uses standard order.
+ */
+static inline u8 pca953x_get_bit_mask(struct pca953x_chip *chip, unsigned int offset)
+{
+	unsigned int bit_pos_in_bank = offset % BANK_SZ;
+	int msb = BANK_SZ - 1;
+
+	if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE && offset <= msb)
+		return BIT(msb - bit_pos_in_bank);
+	else
+		return BIT(bit_pos_in_bank);
+}
+
 #define PCA953x_BANK_INPUT	BIT(0)
 #define PCA953x_BANK_OUTPUT	BIT(1)
 #define PCA953x_BANK_POLARITY	BIT(2)
@@ -353,6 +381,23 @@ static bool pcal6534_check_register(struct pca953x_chip *chip, unsigned int reg,
 	return true;
 }
 
+/* TCA6418 breaks the PCA953x register order rule */
+static bool tca6418_check_register(struct pca953x_chip *chip, unsigned int reg,
+				   u32 access_type_mask)
+{
+	/*  Valid Input Registers - BIT(0) for readable access */
+	if (reg >= TCA6418_INPUT && reg < (TCA6418_INPUT + NBANK(chip)))
+		return (access_type_mask & BIT(0));
+	/*  Valid Output Registers - BIT(1) for writeable access */
+	if (reg >= TCA6418_OUTPUT && reg < (TCA6418_OUTPUT + NBANK(chip)))
+		return (access_type_mask & (BIT(0) | BIT(1)));
+	/*  Valid Direction Registers - BIT(2) for volatile access */
+	if (reg >= TCA6418_DIRECTION && reg < (TCA6418_DIRECTION + NBANK(chip)))
+		return (access_type_mask & (BIT(0) | BIT(1)));
+
+	return false;
+}
+
 static bool pca953x_readable_register(struct device *dev, unsigned int reg)
 {
 	struct pca953x_chip *chip = dev_get_drvdata(dev);
@@ -362,6 +407,9 @@ static bool pca953x_readable_register(struct device *dev, unsigned int reg)
 		bank = PCA957x_BANK_INPUT | PCA957x_BANK_OUTPUT |
 		       PCA957x_BANK_POLARITY | PCA957x_BANK_CONFIG |
 		       PCA957x_BANK_BUSHOLD;
+	} else if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE) {
+		/* BIT(0) to indicate read access */
+		return tca6418_check_register(chip, reg, BIT(0));
 	} else {
 		bank = PCA953x_BANK_INPUT | PCA953x_BANK_OUTPUT |
 		       PCA953x_BANK_POLARITY | PCA953x_BANK_CONFIG;
@@ -384,6 +432,9 @@ static bool pca953x_writeable_register(struct device *dev, unsigned int reg)
 	if (PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE) {
 		bank = PCA957x_BANK_OUTPUT | PCA957x_BANK_POLARITY |
 			PCA957x_BANK_CONFIG | PCA957x_BANK_BUSHOLD;
+	} else if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE) {
+		/* BIT(1) for write access */
+		return tca6418_check_register(chip, reg, BIT(1));
 	} else {
 		bank = PCA953x_BANK_OUTPUT | PCA953x_BANK_POLARITY |
 			PCA953x_BANK_CONFIG;
@@ -403,6 +454,9 @@ static bool pca953x_volatile_register(struct device *dev, unsigned int reg)
 
 	if (PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE)
 		bank = PCA957x_BANK_INPUT;
+	else if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE)
+		/* BIT(2) for volatile access */
+		return tca6418_check_register(chip, reg, BIT(2));
 	else
 		bank = PCA953x_BANK_INPUT;
 
@@ -489,6 +543,15 @@ static u8 pcal6534_recalc_addr(struct pca953x_chip *chip, int reg, int off)
 	return pinctrl + addr + (off / BANK_SZ);
 }
 
+static u8 tca6418_recalc_addr(struct pca953x_chip *chip, int reg_base, int offset)
+{
+	/* reg_base will be TCA6418_INPUT, TCA6418_OUTPUT, or TCA6418_DIRECTION
+	 * offset is the global GPIO line offset (0-17)
+	 * BANK_SZ is 8 for TCA6418 (8 bits per register bank)
+	 */
+	return reg_base + (offset / BANK_SZ);
+}
+
 static int pca953x_write_regs(struct pca953x_chip *chip, int reg, unsigned long *val)
 {
 	u8 regaddr = chip->recalc_addr(chip, reg, 0);
@@ -529,11 +592,14 @@ static int pca953x_gpio_direction_input(struct gpio_chip *gc, unsigned off)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
 	u8 dirreg = chip->recalc_addr(chip, chip->regs->direction, off);
-	u8 bit = BIT(off % BANK_SZ);
+	u8 bit = pca953x_get_bit_mask(chip, off);
 
 	guard(mutex)(&chip->i2c_lock);
 
-	return regmap_write_bits(chip->regmap, dirreg, bit, bit);
+	if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE)
+		return regmap_write_bits(chip->regmap, dirreg, bit, 0);
+	else
+		return regmap_write_bits(chip->regmap, dirreg, bit, bit);
 }
 
 static int pca953x_gpio_direction_output(struct gpio_chip *gc,
@@ -542,7 +608,7 @@ static int pca953x_gpio_direction_output(struct gpio_chip *gc,
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
 	u8 dirreg = chip->recalc_addr(chip, chip->regs->direction, off);
 	u8 outreg = chip->recalc_addr(chip, chip->regs->output, off);
-	u8 bit = BIT(off % BANK_SZ);
+	u8 bit = pca953x_get_bit_mask(chip, off);
 	int ret;
 
 	guard(mutex)(&chip->i2c_lock);
@@ -552,15 +618,20 @@ static int pca953x_gpio_direction_output(struct gpio_chip *gc,
 	if (ret)
 		return ret;
 
-	/* then direction */
-	return regmap_write_bits(chip->regmap, dirreg, bit, 0);
+	/* then direction
+	 * (in/out logic is inverted on TCA6418)
+	 */
+	if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE)
+		return regmap_write_bits(chip->regmap, dirreg, bit, bit);
+	else
+		return regmap_write_bits(chip->regmap, dirreg, bit, 0);
 }
 
 static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
 	u8 inreg = chip->recalc_addr(chip, chip->regs->input, off);
-	u8 bit = BIT(off % BANK_SZ);
+	u8 bit = pca953x_get_bit_mask(chip, off);
 	u32 reg_val;
 	int ret;
 
@@ -577,7 +648,7 @@ static int pca953x_gpio_set_value(struct gpio_chip *gc, unsigned int off,
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
 	u8 outreg = chip->recalc_addr(chip, chip->regs->output, off);
-	u8 bit = BIT(off % BANK_SZ);
+	u8 bit = pca953x_get_bit_mask(chip, off);
 
 	guard(mutex)(&chip->i2c_lock);
 
@@ -588,7 +659,7 @@ static int pca953x_gpio_get_direction(struct gpio_chip *gc, unsigned off)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
 	u8 dirreg = chip->recalc_addr(chip, chip->regs->direction, off);
-	u8 bit = BIT(off % BANK_SZ);
+	u8 bit = pca953x_get_bit_mask(chip, off);
 	u32 reg_val;
 	int ret;
 
@@ -597,10 +668,17 @@ static int pca953x_gpio_get_direction(struct gpio_chip *gc, unsigned off)
 	if (ret < 0)
 		return ret;
 
-	if (reg_val & bit)
+	/* (in/out logic is inverted on TCA6418) */
+	if (reg_val & bit) {
+		if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE)
+			return GPIO_LINE_DIRECTION_OUT;
+		else
+			return GPIO_LINE_DIRECTION_IN;
+	}
+	if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE)
 		return GPIO_LINE_DIRECTION_IN;
-
-	return GPIO_LINE_DIRECTION_OUT;
+	else
+		return GPIO_LINE_DIRECTION_OUT;
 }
 
 static int pca953x_gpio_get_multiple(struct gpio_chip *gc,
@@ -1120,6 +1198,11 @@ static int pca953x_probe(struct i2c_client *client)
 	if (PCA_CHIP_TYPE(chip->driver_data) == PCAL653X_TYPE) {
 		chip->recalc_addr = pcal6534_recalc_addr;
 		chip->check_reg = pcal6534_check_register;
+	}  else if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE) {
+		chip->recalc_addr = tca6418_recalc_addr;
+		/* We don't assign chip->check_reg = tca6418_check_register directly here.
+		 * Instead, the wrappers handle the dispatch based on PCA_CHIP_TYPE.
+		 */
 	} else {
 		chip->recalc_addr = pca953x_recalc_addr;
 		chip->check_reg = pca953x_check_register;
@@ -1157,6 +1240,8 @@ static int pca953x_probe(struct i2c_client *client)
 	if (PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE) {
 		chip->regs = &pca957x_regs;
 		ret = device_pca957x_init(chip);
+	} else if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE) {
+		chip->regs = &tca6418_regs;
 	} else {
 		chip->regs = &pca953x_regs;
 		ret = device_pca95xx_init(chip);
@@ -1325,6 +1410,7 @@ static const struct of_device_id pca953x_dt_ids[] = {
 	{ .compatible = "ti,pca9536", .data = OF_953X( 4, 0), },
 	{ .compatible = "ti,tca6408", .data = OF_953X( 8, PCA_INT), },
 	{ .compatible = "ti,tca6416", .data = OF_953X(16, PCA_INT), },
+	{ .compatible = "ti,tca6418", .data = (void *)(18 | TCA6418_TYPE | PCA_INT), },
 	{ .compatible = "ti,tca6424", .data = OF_953X(24, PCA_INT), },
 	{ .compatible = "ti,tca9535", .data = OF_953X(16, PCA_INT), },
 	{ .compatible = "ti,tca9538", .data = OF_953X( 8, PCA_INT), },
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] dt-bindings: gpio: pca95xx: add TI TCA6418
  2025-06-17 20:44 ` [PATCH 1/2] dt-bindings: gpio: pca95xx: add TI TCA6418 Maria Garcia
@ 2025-06-18 15:58   ` Conor Dooley
  0 siblings, 0 replies; 7+ messages in thread
From: Conor Dooley @ 2025-06-18 15:58 UTC (permalink / raw)
  To: Maria Garcia
  Cc: linux-gpio, devicetree, linux-kernel, Linus Walleij,
	Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Maria Garcia

[-- Attachment #1: Type: text/plain, Size: 54 bytes --]



Acked-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] gpio: pca953x: Add support for TI TCA6418
  2025-06-17 20:44 ` [PATCH 2/2] gpio: pca953x: Add support for " Maria Garcia
@ 2025-06-25 11:53   ` Bartosz Golaszewski
  2025-07-03 13:07     ` Bartosz Golaszewski
  0 siblings, 1 reply; 7+ messages in thread
From: Bartosz Golaszewski @ 2025-06-25 11:53 UTC (permalink / raw)
  To: Maria Garcia
  Cc: linux-gpio, devicetree, linux-kernel, Linus Walleij, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Maria Garcia

On Tue, Jun 17, 2025 at 10:44 PM Maria Garcia <mariagarcia7293@gmail.com> wrote:
>
> The TI TCA6418 is a 18-channel I2C I/O expander. It is slightly
> different to other models from the same family, such as TCA6416,
> but has enough in common with them to make it work with just a
> few tweaks, which are explained in the code's documentation.
>
> Signed-off-by: Maria Garcia <mariagarcia7293@gmail.com>
> ---

Thanks, looks good overall. Just a few nits below.

>
> +/* Helper function to get the correct bit mask for a given offset and chip type.
> + * The TCA6418's input, output, and direction banks have a peculiar bit order:
> + * the first byte uses reversed bit order, while the second byte uses standard order.
> + */
> +static inline u8 pca953x_get_bit_mask(struct pca953x_chip *chip, unsigned int offset)
> +{
> +       unsigned int bit_pos_in_bank = offset % BANK_SZ;
> +       int msb = BANK_SZ - 1;
> +
> +       if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE && offset <= msb)
> +               return BIT(msb - bit_pos_in_bank);
> +       else
> +               return BIT(bit_pos_in_bank);

No need for else.

>  static bool pca953x_readable_register(struct device *dev, unsigned int reg)
>  {
>         struct pca953x_chip *chip = dev_get_drvdata(dev);
> @@ -362,6 +407,9 @@ static bool pca953x_readable_register(struct device *dev, unsigned int reg)
>                 bank = PCA957x_BANK_INPUT | PCA957x_BANK_OUTPUT |
>                        PCA957x_BANK_POLARITY | PCA957x_BANK_CONFIG |
>                        PCA957x_BANK_BUSHOLD;
> +       } else if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE) {
> +               /* BIT(0) to indicate read access */
> +               return tca6418_check_register(chip, reg, BIT(0));
>         } else {
>                 bank = PCA953x_BANK_INPUT | PCA953x_BANK_OUTPUT |
>                        PCA953x_BANK_POLARITY | PCA953x_BANK_CONFIG;
> @@ -384,6 +432,9 @@ static bool pca953x_writeable_register(struct device *dev, unsigned int reg)
>         if (PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE) {
>                 bank = PCA957x_BANK_OUTPUT | PCA957x_BANK_POLARITY |
>                         PCA957x_BANK_CONFIG | PCA957x_BANK_BUSHOLD;
> +       } else if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE) {
> +               /* BIT(1) for write access */
> +               return tca6418_check_register(chip, reg, BIT(1));
>         } else {
>                 bank = PCA953x_BANK_OUTPUT | PCA953x_BANK_POLARITY |
>                         PCA953x_BANK_CONFIG;

Can you convert these to a switch statement for better readability? I
have a slight preference for cases over ifelserry.

> @@ -403,6 +454,9 @@ static bool pca953x_volatile_register(struct device *dev, unsigned int reg)
>
>         if (PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE)
>                 bank = PCA957x_BANK_INPUT;
> +       else if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE)
> +               /* BIT(2) for volatile access */
> +               return tca6418_check_register(chip, reg, BIT(2));
>         else
>                 bank = PCA953x_BANK_INPUT;
>
> @@ -489,6 +543,15 @@ static u8 pcal6534_recalc_addr(struct pca953x_chip *chip, int reg, int off)
>         return pinctrl + addr + (off / BANK_SZ);
>  }
>
> +static u8 tca6418_recalc_addr(struct pca953x_chip *chip, int reg_base, int offset)
> +{
> +       /* reg_base will be TCA6418_INPUT, TCA6418_OUTPUT, or TCA6418_DIRECTION
> +        * offset is the global GPIO line offset (0-17)
> +        * BANK_SZ is 8 for TCA6418 (8 bits per register bank)
> +        */

Please use regular kernel comments, not the networking style.

> +       return reg_base + (offset / BANK_SZ);
> +}
> +
>  static int pca953x_write_regs(struct pca953x_chip *chip, int reg, unsigned long *val)
>  {
>         u8 regaddr = chip->recalc_addr(chip, reg, 0);
> @@ -529,11 +592,14 @@ static int pca953x_gpio_direction_input(struct gpio_chip *gc, unsigned off)
>  {
>         struct pca953x_chip *chip = gpiochip_get_data(gc);
>         u8 dirreg = chip->recalc_addr(chip, chip->regs->direction, off);
> -       u8 bit = BIT(off % BANK_SZ);
> +       u8 bit = pca953x_get_bit_mask(chip, off);
>
>         guard(mutex)(&chip->i2c_lock);
>
> -       return regmap_write_bits(chip->regmap, dirreg, bit, bit);
> +       if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE)
> +               return regmap_write_bits(chip->regmap, dirreg, bit, 0);
> +       else
> +               return regmap_write_bits(chip->regmap, dirreg, bit, bit);
>  }
>

No need for else.

>  static int pca953x_gpio_direction_output(struct gpio_chip *gc,
> @@ -542,7 +608,7 @@ static int pca953x_gpio_direction_output(struct gpio_chip *gc,
>         struct pca953x_chip *chip = gpiochip_get_data(gc);
>         u8 dirreg = chip->recalc_addr(chip, chip->regs->direction, off);
>         u8 outreg = chip->recalc_addr(chip, chip->regs->output, off);
> -       u8 bit = BIT(off % BANK_SZ);
> +       u8 bit = pca953x_get_bit_mask(chip, off);
>         int ret;
>
>         guard(mutex)(&chip->i2c_lock);
> @@ -552,15 +618,20 @@ static int pca953x_gpio_direction_output(struct gpio_chip *gc,
>         if (ret)
>                 return ret;
>
> -       /* then direction */
> -       return regmap_write_bits(chip->regmap, dirreg, bit, 0);
> +       /* then direction
> +        * (in/out logic is inverted on TCA6418)
> +        */
> +       if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE)
> +               return regmap_write_bits(chip->regmap, dirreg, bit, bit);
> +       else
> +               return regmap_write_bits(chip->regmap, dirreg, bit, 0);
>  }

Same here.

>
>  static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
>  {
>         struct pca953x_chip *chip = gpiochip_get_data(gc);
>         u8 inreg = chip->recalc_addr(chip, chip->regs->input, off);
> -       u8 bit = BIT(off % BANK_SZ);
> +       u8 bit = pca953x_get_bit_mask(chip, off);
>         u32 reg_val;
>         int ret;
>
> @@ -577,7 +648,7 @@ static int pca953x_gpio_set_value(struct gpio_chip *gc, unsigned int off,
>  {
>         struct pca953x_chip *chip = gpiochip_get_data(gc);
>         u8 outreg = chip->recalc_addr(chip, chip->regs->output, off);
> -       u8 bit = BIT(off % BANK_SZ);
> +       u8 bit = pca953x_get_bit_mask(chip, off);
>
>         guard(mutex)(&chip->i2c_lock);
>
> @@ -588,7 +659,7 @@ static int pca953x_gpio_get_direction(struct gpio_chip *gc, unsigned off)
>  {
>         struct pca953x_chip *chip = gpiochip_get_data(gc);
>         u8 dirreg = chip->recalc_addr(chip, chip->regs->direction, off);
> -       u8 bit = BIT(off % BANK_SZ);
> +       u8 bit = pca953x_get_bit_mask(chip, off);
>         u32 reg_val;
>         int ret;
>
> @@ -597,10 +668,17 @@ static int pca953x_gpio_get_direction(struct gpio_chip *gc, unsigned off)
>         if (ret < 0)
>                 return ret;
>
> -       if (reg_val & bit)
> +       /* (in/out logic is inverted on TCA6418) */
> +       if (reg_val & bit) {
> +               if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE)
> +                       return GPIO_LINE_DIRECTION_OUT;
> +               else
> +                       return GPIO_LINE_DIRECTION_IN;

Same here.

> +       }
> +       if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE)
>                 return GPIO_LINE_DIRECTION_IN;
> -
> -       return GPIO_LINE_DIRECTION_OUT;
> +       else
> +               return GPIO_LINE_DIRECTION_OUT;


And here.

>  }
>
>  static int pca953x_gpio_get_multiple(struct gpio_chip *gc,
> @@ -1120,6 +1198,11 @@ static int pca953x_probe(struct i2c_client *client)
>         if (PCA_CHIP_TYPE(chip->driver_data) == PCAL653X_TYPE) {
>                 chip->recalc_addr = pcal6534_recalc_addr;
>                 chip->check_reg = pcal6534_check_register;
> +       }  else if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE) {
> +               chip->recalc_addr = tca6418_recalc_addr;
> +               /* We don't assign chip->check_reg = tca6418_check_register directly here.
> +                * Instead, the wrappers handle the dispatch based on PCA_CHIP_TYPE.
> +                */
>         } else {
>                 chip->recalc_addr = pca953x_recalc_addr;
>                 chip->check_reg = pca953x_check_register;
> @@ -1157,6 +1240,8 @@ static int pca953x_probe(struct i2c_client *client)
>         if (PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE) {
>                 chip->regs = &pca957x_regs;
>                 ret = device_pca957x_init(chip);
> +       } else if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE) {
> +               chip->regs = &tca6418_regs;
>         } else {

Same thing about if-elses.

>                 chip->regs = &pca953x_regs;
>                 ret = device_pca95xx_init(chip);
> @@ -1325,6 +1410,7 @@ static const struct of_device_id pca953x_dt_ids[] = {
>         { .compatible = "ti,pca9536", .data = OF_953X( 4, 0), },
>         { .compatible = "ti,tca6408", .data = OF_953X( 8, PCA_INT), },
>         { .compatible = "ti,tca6416", .data = OF_953X(16, PCA_INT), },
> +       { .compatible = "ti,tca6418", .data = (void *)(18 | TCA6418_TYPE | PCA_INT), },
>         { .compatible = "ti,tca6424", .data = OF_953X(24, PCA_INT), },
>         { .compatible = "ti,tca9535", .data = OF_953X(16, PCA_INT), },
>         { .compatible = "ti,tca9538", .data = OF_953X( 8, PCA_INT), },
> --
> 2.43.0
>

Bartosz

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 0/2] *** Add support for TI TCA6418 GPIO chip ***
@ 2025-07-01 10:05 Maria Garcia
  0 siblings, 0 replies; 7+ messages in thread
From: Maria Garcia @ 2025-07-01 10:05 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Maria Garcia, Maria Garcia

This series introduces support for the Texas Instruments TCA6418 
GPIO expander chip, added to the gpio-pca953x driver. It also includes the 
necessary device tree binding definition.

Signed-off-by: Maria Garcia <mariagarcia7293@gmail.com>
---
Changes in v2:
 - gpio-pca953x.c: address review feedback: remove some else statements, 
   turn some if-else statements into switch statements.
Maria Garcia (2):
  dt-bindings: gpio: pca95xx: add TI TCA6418
  gpio: pca953x: Add support for TI TCA6418

 .../bindings/gpio/gpio-pca95xx.yaml           |   1 +
 drivers/gpio/gpio-pca953x.c                   | 139 +++++++++++++++---
 2 files changed, 121 insertions(+), 19 deletions(-)

-- 
2.43.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] gpio: pca953x: Add support for TI TCA6418
  2025-06-25 11:53   ` Bartosz Golaszewski
@ 2025-07-03 13:07     ` Bartosz Golaszewski
  0 siblings, 0 replies; 7+ messages in thread
From: Bartosz Golaszewski @ 2025-07-03 13:07 UTC (permalink / raw)
  To: Maria Garcia
  Cc: linux-gpio, devicetree, linux-kernel, Linus Walleij, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Maria Garcia

On Wed, Jun 25, 2025 at 1:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> >
> > +static u8 tca6418_recalc_addr(struct pca953x_chip *chip, int reg_base, int offset)
> > +{
> > +       /* reg_base will be TCA6418_INPUT, TCA6418_OUTPUT, or TCA6418_DIRECTION
> > +        * offset is the global GPIO line offset (0-17)
> > +        * BANK_SZ is 8 for TCA6418 (8 bits per register bank)
> > +        */
>
> Please use regular kernel comments, not the networking style.
>

I asked for this under version 1. It's still not fixed in version 3.

Bart

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2025-07-03 13:08 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-17 20:44 [PATCH 0/2] *** Add support for TI TCA6418 GPIO chip *** Maria Garcia
2025-06-17 20:44 ` [PATCH 1/2] dt-bindings: gpio: pca95xx: add TI TCA6418 Maria Garcia
2025-06-18 15:58   ` Conor Dooley
2025-06-17 20:44 ` [PATCH 2/2] gpio: pca953x: Add support for " Maria Garcia
2025-06-25 11:53   ` Bartosz Golaszewski
2025-07-03 13:07     ` Bartosz Golaszewski
  -- strict thread matches above, loose matches on Subject: below --
2025-07-01 10:05 [PATCH 0/2] *** Add support for TI TCA6418 GPIO chip *** Maria Garcia

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).