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
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.
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
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
Signed-off-by: Martyn Welch <[email protected]>
---
Just to note that I'm currently awaiting delivery if hardware, so the
below patch is not just incomplete, it also untested.
drivers/gpio/gpio-pca953x.c | 46 ++++++++++++++++++++++++++++++-------
1 file changed, 38 insertions(+), 8 deletions(-)
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index ecd7d169470b..413bcda68935 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -68,6 +68,8 @@
#define PCA957X_TYPE BIT(13)
#define PCA_TYPE_MASK GENMASK(15, 12)
+#define PCAL6534_ALIGN BIT(16)
+
#define PCA_CHIP_TYPE(x) ((x) & PCA_TYPE_MASK)
static const struct i2c_device_id pca953x_id[] = {
@@ -91,6 +93,7 @@ static const struct i2c_device_id pca953x_id[] = {
{ "pcal6416", 16 | PCA953X_TYPE | PCA_LATCH_INT, },
{ "pcal6524", 24 | PCA953X_TYPE | PCA_LATCH_INT, },
+ { "pcal6534", 34 | PCA953X_TYPE | PCA_LATCH_INT | PCAL6534_ALIGN, },
{ "pcal9535", 16 | PCA953X_TYPE | PCA_LATCH_INT, },
{ "pcal9554b", 8 | PCA953X_TYPE | PCA_LATCH_INT, },
{ "pcal9555a", 16 | PCA953X_TYPE | PCA_LATCH_INT, },
@@ -107,6 +110,8 @@ static const struct i2c_device_id pca953x_id[] = {
{ "tca9539", 16 | PCA953X_TYPE | PCA_INT, },
{ "tca9554", 8 | PCA953X_TYPE | PCA_INT, },
{ "xra1202", 8 | PCA953X_TYPE },
+
+ { "pi4ioe5v6534q", 34 | PCA953X_TYPE | PCA_LATCH_INT | PCAL6534_ALIGN, },
{ }
};
MODULE_DEVICE_TABLE(i2c, pca953x_id);
@@ -266,10 +271,19 @@ static int pca953x_bank_shift(struct pca953x_chip *chip)
static bool pca953x_check_register(struct pca953x_chip *chip, unsigned int reg,
u32 checkbank)
{
- 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);
+ }
+ /* TODO: This needs looking at for PCAL6534 */
/* Special PCAL extended register check. */
if (reg & REG_ADDR_EXT) {
if (!(chip->driver_data & PCA_PCAL))
@@ -381,10 +395,20 @@ static const struct regmap_config pca953x_ai_i2c_regmap = {
static u8 pca953x_recalc_addr(struct pca953x_chip *chip, int reg, int off)
{
- int bank_shift = pca953x_bank_shift(chip);
- int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
- int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
- u8 regaddr = pinctrl | addr | (off / BANK_SZ);
+ int bank_shift;
+ int addr;
+ int pinctrl;
+ u8 regaddr;
+
+ if (chip->driver_data & PCAL6534_ALIGN) {
+ addr = (reg & PCAL_GPIO_MASK) * NBANK(chip);
+ } else {
+ bank_shift = pca953x_bank_shift(chip);
+ addr = (reg & PCAL_GPIO_MASK) << bank_shift;
+ }
+ /* TODO: Do we need to handle the pinctrl offset differently for pcal6534? */
+ pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
+ regaddr = pinctrl | addr | (off / BANK_SZ);
return regaddr;
}
@@ -395,8 +419,11 @@ static int pca953x_write_regs(struct pca953x_chip *chip, int reg, unsigned long
u8 value[MAX_BANK];
int i, ret;
- for (i = 0; i < NBANK(chip); i++)
+ 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);
ret = regmap_bulk_write(chip->regmap, regaddr, value, NBANK(chip));
if (ret < 0) {
@@ -1239,6 +1266,7 @@ static const struct of_device_id pca953x_dt_ids[] = {
{ .compatible = "nxp,pcal6416", .data = OF_953X(16, PCA_LATCH_INT), },
{ .compatible = "nxp,pcal6524", .data = OF_953X(24, PCA_LATCH_INT), },
+ { .compatible = "nxp,pcal6534", .data = OF_953X(34, PCA_LATCH_INT | PCAL6534_ALIGN), },
{ .compatible = "nxp,pcal9535", .data = OF_953X(16, PCA_LATCH_INT), },
{ .compatible = "nxp,pcal9554b", .data = OF_953X( 8, PCA_LATCH_INT), },
{ .compatible = "nxp,pcal9555a", .data = OF_953X(16, PCA_LATCH_INT), },
@@ -1261,6 +1289,8 @@ static const struct of_device_id pca953x_dt_ids[] = {
{ .compatible = "onnn,pca9655", .data = OF_953X(16, PCA_INT), },
{ .compatible = "exar,xra1202", .data = OF_953X( 8, 0), },
+
+ { .compatible = "diodes,pi4ioe5v6534q", .data = OF_953X(34, PCA_LATCH_INT | PCAL6534_ALIGN), },
{ }
};
--
2.35.1
On Wed, Aug 17, 2022 at 2:29 PM Martyn Welch <[email protected]> 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
On Sat, 2022-08-20 at 01:35 +0300, Andy Shevchenko wrote:
> On Wed, Aug 17, 2022 at 2:29 PM Martyn Welch
> <[email protected]> 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.
>
Yep, not even close to mergeable, sent to the list mainly for comment
on whether to try and cram this into this driver or create a separate
driver for it.
> > 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.
>
I didn't do this as functionality wise it seems to basically be
PCA953X_TYPE, just with the alignment of the registers being very
different. I could add a PCAL6534_TYPE if you prefer.
> ...
>
> > { "xra1202", 8 | PCA953X_TYPE },
>
> > + { "pi4ioe5v6534q", 34 | PCA953X_TYPE | PCA_LATCH_INT |
> > PCAL6534_ALIGN, },
>
> What's this and why is it not ordered?
>
The entries in pca953x_id[] appear to be ordered in the same order as
pca953x_dt_ids[], where the entries are grouped by manufacturer. This
chip is manufactured by diodes (as mentioned in the commit message), so
put it in it's own block following president. The manufacturers weren't
ordered alphabetically, so assumed ordered by when they were added.
> ...
>
> > - 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.
>
Due to the different register spacing, I don't think these chips obey
any offset based rules. For the record, I've done a bit more work here
to get it returning the correct values for all the extended registers.
What I currently have is this (which I don't particularly like and
would be open to alternative implementations):
static u8 pca953x_recalc_addr(struct pca953x_chip *chip, int reg, int
off)
{
- int bank_shift = pca953x_bank_shift(chip);
- int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
- int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
- u8 regaddr = pinctrl | addr | (off / BANK_SZ);
+ int bank_shift;
+ int addr;
+ int pinctrl;
+ u8 regaddr;
+
+ if (chip->driver_data & PCAL6534_ALIGN) {
+ addr = (reg & PCAL_GPIO_MASK) * NBANK(chip);
+
+ switch(reg) {
+ case PCAL953X_OUT_STRENGTH:
+ case PCAL953X_IN_LATCH:
+ case PCAL953X_PULL_EN:
+ case PCAL953X_PULL_SEL:
+ case PCAL953X_INT_MASK:
+ case PCAL953X_INT_STAT:
+ case PCAL953X_OUT_CONF:
+ pinctrl = ((reg & PCAL_PINCTRL_MASK) >> 1) +
0x20;
+ break;
+ case PCAL6524_INT_EDGE:
+ case PCAL6524_INT_CLR:
+ case PCAL6524_IN_STATUS:
+ case PCAL6524_OUT_INDCONF:
+ case PCAL6524_DEBOUNCE:
+ pinctrl = ((reg & PCAL_PINCTRL_MASK) >> 1) +
0x1c;
+ break;
+ }
+ regaddr = pinctrl + addr + (off / BANK_SZ);
+ } else {
+ bank_shift = pca953x_bank_shift(chip);
+ addr = (reg & PCAL_GPIO_MASK) << bank_shift;
+ pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
+ regaddr = pinctrl | addr | (off / BANK_SZ);
+ }
return regaddr;
}
As I said, whilst the functionality of this chip seems to closely match
some of the others driven by this driver, the register offsets are
quite different and hard to incorporate cleanly in this driver due to
the way it determines register locations.
> ...
>
> > + 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() ?!
>
Quick and dirty debug. The code was included in RFC to help show the
path I was taking to include support for this device and is definitely
not ready for merging.
> ...
>
> > + { .compatible = "diodes,pi4ioe5v6534q", .data = OF_953X(34,
> > PCA_LATCH_INT | PCAL6534_ALIGN), },
>
> As per above.
>
On Mon, 2022-08-22 at 11:56 +0300, Andy Shevchenko wrote:
>
>
> On Monday, August 22, 2022, Martyn Welch <[email protected]>
> wrote:
> > On Sat, 2022-08-20 at 01:35 +0300, Andy Shevchenko wrote:
> > > On Wed, Aug 17, 2022 at 2:29 PM Martyn Welch
> > > <[email protected]> wrote:
> > > >
> > > > - 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.
> > >
> >
> > Due to the different register spacing, I don't think these chips
> > obey
> > any offset based rules. For the record, I've done a bit more work
> > here
> > to get it returning the correct values for all the extended
> > registers.
> > What I currently have is this (which I don't particularly like and
> > would be open to alternative implementations):
> >
> >
> > static u8 pca953x_recalc_addr(struct pca953x_chip *chip, int reg,
> > int
> > off)
> > {
> > - int bank_shift = pca953x_bank_shift(chip);
> > - int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
> > - int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
> > - u8 regaddr = pinctrl | addr | (off / BANK_SZ);
> > + int bank_shift;
> > + int addr;
> > + int pinctrl;
> > + u8 regaddr;
> > +
> > + if (chip->driver_data & PCAL6534_ALIGN) {
> > + addr = (reg & PCAL_GPIO_MASK) * NBANK(chip);
> > +
> > + switch(reg) {
> > + case PCAL953X_OUT_STRENGTH:
> > + case PCAL953X_IN_LATCH:
> > + case PCAL953X_PULL_EN:
> > + case PCAL953X_PULL_SEL:
> > + case PCAL953X_INT_MASK:
> > + case PCAL953X_INT_STAT:
> > + case PCAL953X_OUT_CONF:
> > + pinctrl = ((reg & PCAL_PINCTRL_MASK) >> 1)
> > +
> > 0x20;
> > + break;
> > + case PCAL6524_INT_EDGE:
> > + case PCAL6524_INT_CLR:
> > + case PCAL6524_IN_STATUS:
> > + case PCAL6524_OUT_INDCONF:
> > + case PCAL6524_DEBOUNCE:
> > + pinctrl = ((reg & PCAL_PINCTRL_MASK) >> 1)
> > +
> > 0x1c;
> > + break;
> > + }
> > + regaddr = pinctrl + addr + (off / BANK_SZ);
> > + } else {
> > + bank_shift = pca953x_bank_shift(chip);
> > + addr = (reg & PCAL_GPIO_MASK) << bank_shift;
> > + pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
> > + regaddr = pinctrl | addr | (off / BANK_SZ);
> > + }
> >
> > return regaddr;
> > }
> >
> > As I said, whilst the functionality of this chip seems to closely
> > match
> > some of the others driven by this driver, the register offsets are
> > quite different and hard to incorporate cleanly in this driver due
> > to
> > the way it determines register locations.
> >
>
>
> I think it can be done much easier with the specific regmap callbacks
> specific to these kind of chips.
>
Are you thinking of defining functions via struct regmap_bus? If so,
I'm not sure how this helps. Unless I've miss understood how that would
work, those would come into play after regmap_bulk_write(), etc are
called, by which point the desired (and in this case wrong) offset will
have already been calculated in pca953x_recalc_addr().
Am I missing something?
Martyn