lkml.org 
[lkml]   [2022]   [Aug]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC PATCH] gpio: pca953x: Support for pcal6534
    From
    Date
    On Mon, 2022-08-22 at 11:56 +0300, Andy Shevchenko wrote:
    >
    >
    > On Monday, August 22, 2022, Martyn Welch <martyn.welch@collabora.com>
    > 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@collabora.com> 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
    \
     
     \ /
      Last update: 2022-08-24 16:54    [W:6.700 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site