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

[Martyn Welch <martyn.welch@xxxxxxxxxxxxx>]

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@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.
> 

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