| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMRc=Mftput7DO+nmOA0yMcB0SvtsDf5U25ukkMVuOnV4XfX=g@mail.gmail.com>
Date: Wed, 25 Jun 2025 13:53:51 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Maria Garcia <mariagarcia7293@...il.com>
Cc: linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, Linus Walleij <linus.walleij@...aro.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Maria Garcia <mgarcia@...ox.com>
Subject: Re: [PATCH 2/2] gpio: pca953x: Add support for TI TCA6418
On Tue, Jun 17, 2025 at 10:44 PM Maria Garcia <mariagarcia7293@...il.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@...il.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
Powered by blists - more mailing lists