Re: [RFC PATCH] gpio: pca953x: Support for pcal6534

From: Andy Shevchenko
Date: Fri Aug 19 2022 - 18:36:13 EST


On Wed, Aug 17, 2022 at 2:29 PM Martyn Welch <martyn.welch@xxxxxxxxxxxxx> 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