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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

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