| Message ID | 20220829133923.1114555-5-martyn.welch@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Mon, Aug 29, 2022 at 3:39 PM Martyn Welch <martyn.welch@collabora.com> wrote: > > Add support for the NXP PCAL6534 and Diodes Inc. PI4IOE5V6534Q. These > devices, which have identical register layouts and features, are broadly a > 34-bit version of the PCAL6524. > > However, whilst the registers are broadly what you'd expect for a 34-bit > version of the PCAL6524, the spacing of the registers has been > compacted. This has the unfortunate effect of breaking the bit shift > based mechanism that is employed to work out register locations used by > the other chips supported by this driver, resulting in special handling > needing to be introduced in pca953x_recalc_addr() and > pca953x_check_register(). > > Datasheet: https://www.nxp.com/docs/en/data-sheet/PCAL6534.pdf > Datasheet: https://www.diodes.com/assets/Datasheets/PI4IOE5V6534Q.pdf > Signed-off-by: Martyn Welch <martyn.welch@collabora.com> > --- Is this series complete? I don't have patch 1/5 in my inbox and neither does patchwork. Bart
On Wed, 2022-08-31 at 13:57 +0200, Bartosz Golaszewski wrote: > On Mon, Aug 29, 2022 at 3:39 PM Martyn Welch > <martyn.welch@collabora.com> wrote: > > > > Add support for the NXP PCAL6534 and Diodes Inc. PI4IOE5V6534Q. > > These > > devices, which have identical register layouts and features, are > > broadly a > > 34-bit version of the PCAL6524. > > > > However, whilst the registers are broadly what you'd expect for a > > 34-bit > > version of the PCAL6524, the spacing of the registers has been > > compacted. This has the unfortunate effect of breaking the bit > > shift > > based mechanism that is employed to work out register locations > > used by > > the other chips supported by this driver, resulting in special > > handling > > needing to be introduced in pca953x_recalc_addr() and > > pca953x_check_register(). > > > > Datasheet: https://www.nxp.com/docs/en/data-sheet/PCAL6534.pdf > > Datasheet: > > https://www.diodes.com/assets/Datasheets/PI4IOE5V6534Q.pdf > > Signed-off-by: Martyn Welch <martyn.welch@collabora.com> > > --- > > Is this series complete? I don't have patch 1/5 in my inbox and > neither does patchwork. > I used get_maintainers to generate the recipients, it's a patch for the binding documentation relating to the driver so didn't get sent to the same lists: https://lore.kernel.org/lkml/20220829133923.1114555-3-martyn.welch@collabora.com/T/ Martyn > Bart
On Wed, Aug 31, 2022 at 6:30 PM Martyn Welch <martyn.welch@collabora.com> wrote: > > On Wed, 2022-08-31 at 13:57 +0200, Bartosz Golaszewski wrote: > > On Mon, Aug 29, 2022 at 3:39 PM Martyn Welch > > <martyn.welch@collabora.com> wrote: > > > > > > Add support for the NXP PCAL6534 and Diodes Inc. PI4IOE5V6534Q. > > > These > > > devices, which have identical register layouts and features, are > > > broadly a > > > 34-bit version of the PCAL6524. > > > > > > However, whilst the registers are broadly what you'd expect for a > > > 34-bit > > > version of the PCAL6524, the spacing of the registers has been > > > compacted. This has the unfortunate effect of breaking the bit > > > shift > > > based mechanism that is employed to work out register locations > > > used by > > > the other chips supported by this driver, resulting in special > > > handling > > > needing to be introduced in pca953x_recalc_addr() and > > > pca953x_check_register(). > > > > > > Datasheet: https://www.nxp.com/docs/en/data-sheet/PCAL6534.pdf > > > Datasheet: > > > https://www.diodes.com/assets/Datasheets/PI4IOE5V6534Q.pdf > > > Signed-off-by: Martyn Welch <martyn.welch@collabora.com> > > > --- > > > > Is this series complete? I don't have patch 1/5 in my inbox and > > neither does patchwork. > > > > I used get_maintainers to generate the recipients, it's a patch for the > binding documentation relating to the driver so didn't get sent to the > same lists: > > https://lore.kernel.org/lkml/20220829133923.1114555-3-martyn.welch@collabora.com/T/ > > Martyn > > > Bart > Ah, ok. Just for completeness, please send the entire series to everyone unless it's lots of patches. Bart
On Mon, Aug 29, 2022 at 4:52 PM Martyn Welch <martyn.welch@collabora.com> wrote: > > Add support for the NXP PCAL6534 and Diodes Inc. PI4IOE5V6534Q. These > devices, which have identical register layouts and features, are broadly a > 34-bit version of the PCAL6524. > > However, whilst the registers are broadly what you'd expect for a 34-bit > version of the PCAL6524, the spacing of the registers has been > compacted. This has the unfortunate effect of breaking the bit shift > based mechanism that is employed to work out register locations used by > the other chips supported by this driver, resulting in special handling > needing to be introduced in pca953x_recalc_addr() and > pca953x_check_register(). This still needs an alternative deep review. I'll do my best to get into it sooner than later. ... > #define PCA953X_TYPE BIT(12) > #define PCA957X_TYPE BIT(13) > +#define PCAL653X_TYPE BIT(14) > #define PCA_TYPE_MASK GENMASK(15, 12) > > + Stray change. > #define PCA_CHIP_TYPE(x) ((x) & PCA_TYPE_MASK) ... > 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 (PCA_CHIP_TYPE(chip->driver_data) == PCAL653X_TYPE) { > + if (reg > 0x2f) { > + /* > + * Reserved block between 14h and 2Fh does not align on > + * expected bank boundaries like other devices. > + */ > + int temp = reg - 0x30; > + > + bank = temp / NBANK(chip); > + offset = temp - (bank * NBANK(chip)); > + bank += 8; > + } else if (reg > 0x53) { > + /* Handle lack of reserved registers after output port > + * configuration register to form a bank. > + */ > + int temp = reg - 0x54; > + > + bank = temp / NBANK(chip); > + offset = temp - (bank * NBANK(chip)); > + bank += 16; Can we rather put this into a separate function? > + } else { > + bank = reg / NBANK(chip); > + offset = reg - (bank * NBANK(chip)); > + } > + } else { > + int bank_shift = pca953x_bank_shift(chip); > > - /* Special PCAL extended register check. */ > - if (reg & REG_ADDR_EXT) { > - if (!(chip->driver_data & PCA_PCAL)) > - return false; > - bank += 8; > + bank = (reg & REG_ADDR_MASK) >> bank_shift; > + offset = reg & (BIT(bank_shift) - 1); > + > + /* Special PCAL extended register check. */ > + if (reg & REG_ADDR_EXT) { > + if (!(chip->driver_data & PCA_PCAL)) > + return false; > + bank += 8; > + } > } All the same, split this to be like if (current) do_current_things else do_new_type ... > 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 addr; > + int pinctrl; > + u8 regaddr; > + > + if (PCA_CHIP_TYPE(chip->driver_data) == PCAL653X_TYPE) { > + /* The PCAL6534 and compatible chips have altered bank alignment that doesn't > + * fit within the bit shifting scheme used for other devices. > + */ > + 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 { > + int 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); > + } Looking at all these, why not add the callbacks for recalc_addr and check_reg and assign them in the ->probe()?
On Thu, 2022-09-01 at 00:02 +0300, Andy Shevchenko wrote: > On Mon, Aug 29, 2022 at 4:52 PM Martyn Welch > <martyn.welch@collabora.com> wrote: > > > > Add support for the NXP PCAL6534 and Diodes Inc. PI4IOE5V6534Q. > > These > > devices, which have identical register layouts and features, are > > broadly a > > 34-bit version of the PCAL6524. > > > > However, whilst the registers are broadly what you'd expect for a > > 34-bit > > version of the PCAL6524, the spacing of the registers has been > > compacted. This has the unfortunate effect of breaking the bit > > shift > > based mechanism that is employed to work out register locations > > used by > > the other chips supported by this driver, resulting in special > > handling > > needing to be introduced in pca953x_recalc_addr() and > > pca953x_check_register(). > > This still needs an alternative deep review. I'll do my best to get > into it sooner than later. > Thanks much appreciated. ... > > 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 addr; > > + int pinctrl; > > + u8 regaddr; > > + > > + if (PCA_CHIP_TYPE(chip->driver_data) == PCAL653X_TYPE) { > > + /* The PCAL6534 and compatible chips have altered > > bank alignment that doesn't > > + * fit within the bit shifting scheme used for > > other devices. > > + */ > > + 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 { > > + int 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); > > + } > > Looking at all these, why not add the callbacks for recalc_addr and > check_reg and assign them in the ->probe()? > Yeah, that sounds like a good plan. I'll look into doing that. Martyn
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c index 19a8eb94a629..ef1f0a603007 100644 --- a/drivers/gpio/gpio-pca953x.c +++ b/drivers/gpio/gpio-pca953x.c @@ -66,8 +66,10 @@ #define PCA_LATCH_INT (PCA_PCAL | PCA_INT) #define PCA953X_TYPE BIT(12) #define PCA957X_TYPE BIT(13) +#define PCAL653X_TYPE BIT(14) #define PCA_TYPE_MASK GENMASK(15, 12) + #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 | PCAL653X_TYPE | PCA_LATCH_INT, }, { "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 | PCAL653X_TYPE | PCA_LATCH_INT, }, { } }; MODULE_DEVICE_TABLE(i2c, pca953x_id); @@ -261,20 +266,56 @@ static int pca953x_bank_shift(struct pca953x_chip *chip) * - Registers with bit 0x80 set, the AI bit * The bit is cleared and the registers fall into one of the * categories above. + * + * Unfortunately, whilst the PCAL6534 chip (and compatibles) broadly follow + * the same register layout as the PCAL6524, the spacing of the registers has + * been fundamentally altered by compacting them and thus does not obey the + * same rules, including being able to use bit shifting to determine bank. + * These chips hence need special handling here. */ 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 (PCA_CHIP_TYPE(chip->driver_data) == PCAL653X_TYPE) { + if (reg > 0x2f) { + /* + * Reserved block between 14h and 2Fh does not align on + * expected bank boundaries like other devices. + */ + int temp = reg - 0x30; + + bank = temp / NBANK(chip); + offset = temp - (bank * NBANK(chip)); + bank += 8; + } else if (reg > 0x53) { + /* Handle lack of reserved registers after output port + * configuration register to form a bank. + */ + int temp = reg - 0x54; + + bank = temp / NBANK(chip); + offset = temp - (bank * NBANK(chip)); + bank += 16; + } else { + bank = reg / NBANK(chip); + offset = reg - (bank * NBANK(chip)); + } + } else { + int bank_shift = pca953x_bank_shift(chip); - /* Special PCAL extended register check. */ - if (reg & REG_ADDR_EXT) { - if (!(chip->driver_data & PCA_PCAL)) - return false; - bank += 8; + bank = (reg & REG_ADDR_MASK) >> bank_shift; + offset = reg & (BIT(bank_shift) - 1); + + /* Special PCAL extended register check. */ + if (reg & REG_ADDR_EXT) { + if (!(chip->driver_data & PCA_PCAL)) + return false; + bank += 8; + } } /* Register is not in the matching bank. */ @@ -381,10 +422,42 @@ 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 addr; + int pinctrl; + u8 regaddr; + + if (PCA_CHIP_TYPE(chip->driver_data) == PCAL653X_TYPE) { + /* The PCAL6534 and compatible chips have altered bank alignment that doesn't + * fit within the bit shifting scheme used for other devices. + */ + 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 { + int 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; } @@ -1215,6 +1288,7 @@ static int pca953x_resume(struct device *dev) #endif /* convenience to stop overlong match-table lines */ +#define OF_653X(__nrgpio, __int) ((void *)(__nrgpio | PCAL653X_TYPE | __int)) #define OF_953X(__nrgpio, __int) (void *)(__nrgpio | PCA953X_TYPE | __int) #define OF_957X(__nrgpio, __int) (void *)(__nrgpio | PCA957X_TYPE | __int) @@ -1239,6 +1313,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_653X(34, PCA_LATCH_INT), }, { .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 +1336,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_653X(34, PCA_LATCH_INT), }, { } };
Add support for the NXP PCAL6534 and Diodes Inc. PI4IOE5V6534Q. These devices, which have identical register layouts and features, are broadly a 34-bit version of the PCAL6524. However, whilst the registers are broadly what you'd expect for a 34-bit version of the PCAL6524, the spacing of the registers has been compacted. This has the unfortunate effect of breaking the bit shift based mechanism that is employed to work out register locations used by the other chips supported by this driver, resulting in special handling needing to be introduced in pca953x_recalc_addr() and pca953x_check_register(). Datasheet: https://www.nxp.com/docs/en/data-sheet/PCAL6534.pdf Datasheet: https://www.diodes.com/assets/Datasheets/PI4IOE5V6534Q.pdf Signed-off-by: Martyn Welch <martyn.welch@collabora.com> --- drivers/gpio/gpio-pca953x.c | 101 +++++++++++++++++++++++++++++++----- 1 file changed, 89 insertions(+), 12 deletions(-)