Re: [RFC PATCH] gpio: pca953x: Support for pcal6534

[Martyn Welch <martyn.welch@xxxxxxxxxxxxx>]

On Mon, 2022-08-22 at 11:56 +0300, Andy Shevchenko wrote:
> 
> 
> On Monday, August 22, 2022, Martyn Welch <martyn.welch@xxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxx> 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