linux-kernel - Re: [RFC PATCH] gpio: pca953x: Support for pcal6534

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] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 20 Aug 2022 01:35:30 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Martyn Welch <martyn.welch@...labora.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Collabora Kernel ML <kernel@...labora.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH] gpio: pca953x: Support for pcal6534

On Wed, Aug 17, 2022 at 2:29 PM Martyn Welch <martyn.welch@...labora.com> wrote:
>
> The pcal6534[1] is a 34-bit I/O expander with more than a passing
> resemblance to the pcal6524[2] currently supported by the gpio-pca953x
> driver, however whilst the registers seem to functionally match
> perfectly, the alignment of the register banks in the chips address
> space do not follow the pattern expected by the existing driver. For

does not

> instance, as the chip provides 34 I/O, which requires bannks of 5 8-bit
> registers to provide input state, output state, etc. as do the 40 I/O
> variants, however the 40 I/O variants layout the banks of registers on
> 8-byte boundaries, whilst the pcal6534 does not space out the banks at
> all. Additionally the extended functionality starts at 30h rather than
> 40h and I suspect there will be other similar differences that I've not
> yet discovered.

The below shouldn't be in the commit message, but rather in the
comments (after the cutter '---' line below). And due to these two
paragraphs I consider this as an RFC (and it is luckily marked like
this), so, Bart, please do not apply this, we need more eyes and
datasheet reading before going on this.

> I suspect that this may add some additional complexity to the driver and
> I'm not sure whether this will be welcome. I've done a few cursory
> searches to see if there are other chips which follow the pattern of the
> pcal6534 and have so far only found the pi4ioe5v6534q[3], which appears
> to be funcitonaly identical to the pcal6534.
>
> I'm currently wondering whether a submission to extend the pcal6534
> is likely to be deemed acceptable. If so whether something like the

so, whether

> attached approach would be OK, or whether anyone has better ideas on how
> to achieve this. Alternatively I'd be happy to create a new driver to
> support the pcal6534 if that's deemed more appropriate.

> [1] https://www.nxp.com/docs/en/data-sheet/PCAL6534.pdf
> [2] https://www.nxp.com/docs/en/data-sheet/PCAL6524.pdf
> [3] https://www.diodes.com/assets/Datasheets/PI4IOE5V6534Q.pdf

Convert these to Datasheet: tags.

...

>  #define PCA957X_TYPE           BIT(13)
>  #define PCA_TYPE_MASK          GENMASK(15, 12)
>
> +#define PCAL6534_ALIGN         BIT(16)

I believe it should be a chip TYPE.

...

>         { "xra1202", 8  | PCA953X_TYPE },

> +       { "pi4ioe5v6534q", 34 | PCA953X_TYPE | PCA_LATCH_INT | PCAL6534_ALIGN, },

What's this and why is it not ordered?

...

> -       int bank_shift = pca953x_bank_shift(chip);
> -       int bank = (reg & REG_ADDR_MASK) >> bank_shift;
> -       int offset = reg & (BIT(bank_shift) - 1);
> +       int bank;
> +       int offset;
> +
> +       if (chip->driver_data & PCAL6534_ALIGN) {
> +               bank = (reg & REG_ADDR_MASK) / NBANK(chip);
> +               offset = reg - (bank * NBANK(chip));
> +       } else {
> +               int bank_shift = pca953x_bank_shift(chip);
> +               bank = (reg & REG_ADDR_MASK) >> bank_shift;
> +               offset = reg & (BIT(bank_shift) - 1);
> +       }

I'm wondering if it can be moved to bank_shift()  and possibly a new
helper to get an offset.

...

> +       for (i = 0; i < NBANK(chip); i++) {
>                 value[i] = bitmap_get_value8(val, i * BANK_SZ);
> +               dev_err(&chip->client->dev, "value[%d] = %x\n", i, value[i]);
> +       }
> +       dev_err(&chip->client->dev, "regaddr: %x\n", regaddr);

dev_err() ?!

...

> +       { .compatible = "diodes,pi4ioe5v6534q", .data = OF_953X(34, PCA_LATCH_INT | PCAL6534_ALIGN), },

As per above.

-- 
With Best Regards,
Andy Shevchenko