| Message ID | 20220817112818.787771-1-martyn.welch@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] gpio: pca953x: Support for pcal6534 | expand |
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.
On Mon, 2022-08-22 at 11:56 +0300, Andy Shevchenko wrote: > > > On Monday, August 22, 2022, Martyn Welch <martyn.welch@collabora.com> > wrote: > > On Sat, 2022-08-20 at 01:35 +0300, Andy Shevchenko wrote: > > > On Wed, Aug 17, 2022 at 2:29 PM Martyn Welch > > > <martyn.welch@collabora.com> 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
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), }, { } };
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 <martyn.welch@collabora.com> --- 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(-)