Messages in this thread | | | From | Andy Shevchenko <> | Date | Sat, 20 Aug 2022 01:35:30 +0300 | Subject | Re: [RFC PATCH] gpio: pca953x: Support for pcal6534 |
| |
On Wed, Aug 17, 2022 at 2:29 PM Martyn Welch <martyn.welch@collabora.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
| |