Re: [PATCH 5/5] gpio: pca953x: Add support for PCAL6534 and compatible

From: Andy Shevchenko
Date: Wed Aug 31 2022 - 17:03:00 EST


On Mon, Aug 29, 2022 at 4:52 PM Martyn Welch <martyn.welch@xxxxxxxxxxxxx> 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()?

--
With Best Regards,
Andy Shevchenko