linux-kernel - Re: [PATCH v3 2/2] gpio: pca953x: Add support for TI TCA6418

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAMRc=MdEr+bP1y9DHYa-qrXGJT3-zEvemWm7FzHvkqyfBZC2bw@mail.gmail.com>
Date: Thu, 3 Jul 2025 15:10:11 +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 v3 2/2] gpio: pca953x: Add support for TI TCA6418

On Tue, Jul 1, 2025 at 6:16 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>
> ---
>
> +/* 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.
> + */

No networking-style comments in drivers/gpio/ please.

> +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);

Since you're going to respin it anyway, please add newlines between
one return here and elsewhere.

>
> +/* 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));

Same here, please sprinkle in some newlines when returning for better
readability.

Bart