Re: [PATCH v2] gpio: pca953x: add support for open drain pins on PCAL6524

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Wed, Feb 17, 2021 at 3:11 PM Bedel, Alban <alban.bedel@xxxxxxxx> wrote:
> On Tue, 2021-02-16 at 19:50 +0200, Andy Shevchenko wrote:
> > On Tue, Feb 16, 2021 at 6:37 PM Bedel, Alban <alban.bedel@xxxxxxxx>
> > wrote:
> > > On Mon, 2021-02-15 at 14:53 +0200, Andy Shevchenko wrote:
> > > > On Thu, Feb 11, 2021 at 7:52 PM Alban Bedel <alban.bedel@xxxxxxxx
> > > > wrote:

...

> > Before continuing on this, have you considered to split this
> > particular chip to a real pin controller and use the existing driver
> > only for GPIO part of the functionality?
>
> No, this driver already use the ->set_config() mechanism so adding
> another property is trivial. On the other hand having a pinctrl driver
> would be a massive undertaking as the pinctrl API is quiet complex
> iirc.

> Furthermore, unless I'm missing something, that would not allow a
> consumer to request an open drain GPIO which is what I'm after.

Hmm... Linus, is it so?

...

> > > > > +#define PCAL65xx_REGS          BIT(10)
> > > >
> > > > Can we have it as a _TYPE, please?
> > >
> > > Let's please take a closer look at these macros and what they mean.
> > > Currently we have 3 possible set of functions that are indicated by
> > > setting bits in driver_data using the PCA_xxx macros:
> > >
> > > - Basic register only: 0
> > > - With interrupt support: PCA_INT
> > > - With latching interrupt regs: PCA_INT | PCA_PCAL = PCA_LATCH_INT
> > >
> > > This patch then add a forth case:
> > >
> > > - With pin config regs: PCA_INT | PCA_PCAL |
> > > $MACRO_WE_ARE_DICUSSING
> > >
> > > Then there is the PCA953X_TYPE and PCA957X_TYPE macros which
> > > indicate
> > > the need for a different regmap config and register layout.
> >
> > Exactly, and you have a different register layout (coincidentally
> > overlaps with the original PCA953x).
>
> We have 2 layout for the base registers, the "mixed up registers" of
> the PCA957x and the "standard" of the PCA953x. Then we have the
> PCALxxxx chips which extend the base PCA953x registers with further
> registers for better interrupt handling. These are not treated as a new
> type in the current code, but as an extra feature on top of the
> PCA953x.

Yes, because they are about interrupts AFAICS.

>  The PCAL65xx we are talking about add a further register
> block, so following the existing concept its not a new layout.

Yes, with one important detail, i.e. it extends the "mixed up"
registers, it's not a separate "feature" in this sense. The separate
"feature" can be, for example, PWM registers. I admit that this most
of the angle of preference how to draw a line between the features.

I prefer to see it as a type because of two things (in the current code):
 - OF_9*() macros take only two arguments, second of which is Interrupt related
 - PCA_INT group of bits is about Interrupt only

Your proposal will disrupt the code (more invasive).

Actually I prefer to see this chip as a pin controller, but it will be
a longer road to pass, I suppose.

...

> > > These are
> > > accessed using the PCA_CHIP_TYPE() and are used as an integer
> > > value,
> > > not as bit-field like the above ones. If we had a struct instead of
> > > a
> > > packed integer that would be a different field.
> >
> > How come? It's a bitmask.
>
> The definitions use BIT() but all evaluations of PCA_CHIP_TYPE() use
> the equality operator.

I don't get how it's related. It's a bitmap and each bit uniquely
defines the type.

...

> > > I'll change it to PCAL65xx_TYPE if you insist, but that seems very
> > > wrong to me to use the same naming convention for macros meant for
> > > different fields.
> >
> > To me it's the opposite. The 6524 defines a new type (which has some
> > registers others don't have). We even have already definitions of
> > those special registers. I think TYPE is a better approach here.
>
> Let's look at pca953x_writeable_register() which I think clearly show
> how chips variants are currently handled. This is just one example but
> the rest of the code follows the same concept.
>
> static bool pca953x_writeable_register(struct device *dev, unsigned int reg)
> {
>         struct pca953x_chip *chip = dev_get_drvdata(dev);
>         u32 bank;
>
>         if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE) {
>                 bank = PCA953x_BANK_OUTPUT | PCA953x_BANK_POLARITY |
>                         PCA953x_BANK_CONFIG;
>         } else {
>                 bank = PCA957x_BANK_OUTPUT | PCA957x_BANK_POLARITY |
>                         PCA957x_BANK_CONFIG | PCA957x_BANK_BUSHOLD;
>         }
>
>         if (chip->driver_data & PCA_PCAL)
>                 bank |= PCAL9xxx_BANK_IN_LATCH | PCAL9xxx_BANK_PULL_EN |
>                         PCAL9xxx_BANK_PULL_SEL | PCAL9xxx_BANK_IRQ_MASK;
>
> +       if (chip->driver_data & PCAL65xx_REGS)
> +               bank |= PCAL65xx_BANK_INDOUT_CONF;
> +
>         return pca953x_check_register(chip, reg, bank);
> }
>
> The chip we are talking about further extend the PCAL registers, so it
> is currently covered by PCA953X_TYPE as base type and has the PCA_PCAL
> bit set. I really fails to see how this new type would nicely fit in
> the existing code.

Use switch-case instead of if-else-if and it will bring you a better
picture (not sure about __fallthrough be good or bad here).

switch (PCA_CHIP_TYPE(chip->driver_data)) {
case PCA6524_TYPE:
 ...
 __fallthrough; // perhaps better is to explicitly show what's going on
case PCA953X_TYPE:
default: // originally default seems PCA957X, but I guess this makes more sense
 ...
break;
case PCA957X_TYPE:
...
break;
}

-- 
With Best Regards,
Andy Shevchenko