Message-ID: <370beac7c85da4f1b57d8d78715d2a0676d19cf9.camel@collabora.com>
Date:   Mon, 22 Aug 2022 09:36:52 +0100
From:   Martyn Welch <martyn.welch@...labora.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Collabora Kernel ML <kernel@...labora.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH] gpio: pca953x: Support for pcal6534

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@...labora.com> wrote:
> > 
> > The pcal6534[1] is a 34-bit I/O expander with more than a passing
> > resemblance to the pcal6524[2] currently supported by the gpio-
> > pca953x
> > driver, however whilst the registers seem to functionally match
> > perfectly, the alignment of the register banks in the chips address
> > space do not follow the pattern expected by the existing driver.
> > For
> 
> does not
> 
> > instance, as the chip provides 34 I/O, which requires bannks of 5
> > 8-bit
> > registers to provide input state, output state, etc. as do the 40
> > I/O
> > variants, however the 40 I/O variants layout the banks of registers
> > on
> > 8-byte boundaries, whilst the pcal6534 does not space out the banks
> > at
> > all. Additionally the extended functionality starts at 30h rather
> > than
> > 40h and I suspect there will be other similar differences that I've
> > not
> > yet discovered.
> 
> The below shouldn't be in the commit message, but rather in the
> comments (after the cutter '---' line below). And due to these two
> paragraphs I consider this as an RFC (and it is luckily marked like
> this), so, Bart, please do not apply this, we need more eyes and
> datasheet reading before going on this.
> 

Yep, not even close to mergeable, sent to the list mainly for comment
on whether to try and cram this into this driver or create a separate
driver for it.

> > I suspect that this may add some additional complexity to the
> > driver and
> > I'm not sure whether this will be welcome. I've done a few cursory
> > searches to see if there are other chips which follow the pattern
> > of the
> > pcal6534 and have so far only found the pi4ioe5v6534q[3], which
> > appears
> > to be funcitonaly identical to the pcal6534.
> > 
> > I'm currently wondering whether a submission to extend the pcal6534
> > is likely to be deemed acceptable. If so whether something like the
> 
> so, whether
> 
> > attached approach would be OK, or whether anyone has better ideas
> > on how
> > to achieve this. Alternatively I'd be happy to create a new driver
> > to
> > support the pcal6534 if that's deemed more appropriate.
> 
> > [1] https://www.nxp.com/docs/en/data-sheet/PCAL6534.pdf
> > [2] https://www.nxp.com/docs/en/data-sheet/PCAL6524.pdf
> > [3] https://www.diodes.com/assets/Datasheets/PI4IOE5V6534Q.pdf
> 
> Convert these to Datasheet: tags.
> 
> ...
> 
> >  #define PCA957X_TYPE           BIT(13)
> >  #define PCA_TYPE_MASK          GENMASK(15, 12)
> > 
> > +#define PCAL6534_ALIGN         BIT(16)
> 
> I believe it should be a chip TYPE.
> 

I didn't do this as functionality wise it seems to basically be
PCA953X_TYPE, just with the alignment of the registers being very
different. I could add a PCAL6534_TYPE if you prefer.

> ...
> 
> >         { "xra1202", 8  | PCA953X_TYPE },
> 
> > +       { "pi4ioe5v6534q", 34 | PCA953X_TYPE | PCA_LATCH_INT |
> > PCAL6534_ALIGN, },
> 
> What's this and why is it not ordered?
> 

The entries in pca953x_id[] appear to be ordered in the same order as
pca953x_dt_ids[], where the entries are grouped by manufacturer. This
chip is manufactured by diodes (as mentioned in the commit message), so
put it in it's own block following president. The manufacturers weren't
ordered alphabetically, so assumed ordered by when they were added.

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

> ...
> 
> > +       for (i = 0; i < NBANK(chip); i++) {
> >                 value[i] = bitmap_get_value8(val, i * BANK_SZ);
> > +               dev_err(&chip->client->dev, "value[%d] = %x\n", i,
> > value[i]);
> > +       }
> > +       dev_err(&chip->client->dev, "regaddr: %x\n", regaddr);
> 
> dev_err() ?!
> 

Quick and dirty debug. The code was included in RFC to help show the
path I was taking to include support for this device and is definitely
not ready for merging.

> ...
> 
> > +       { .compatible = "diodes,pi4ioe5v6534q", .data = OF_953X(34,
> > PCA_LATCH_INT | PCAL6534_ALIGN), },
> 
> As per above.
> 

Powered by blists - more mailing lists