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

[Andy Shevchenko]

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:
> > Hint: don't forget to include reviewers from previous version
>
> I added you to the CC list for the new patch, did I miss someone else?

Then we are fine, thanks!

> > On Thu, Feb 11, 2021 at 7:52 PM Alban Bedel <alban.bedel@xxxxxxxx>
> > wrote:
> > > From a quick glance at various datasheets the PCAL6524 and the
> > > PCAL6534 seems to be the only chips in this family that support
> > > setting the drive mode of single pins. Other chips either don't
> > > support it at all, or can only set the drive mode of whole banks,
> > > which doesn't map to the GPIO API.
> > >
> > > Add a new flag, PCAL65xx_REGS, to mark chips that have the extra
> > > registers needed for this feature. Then mark the needed register
> > > banks
> > > as readable and writable, here we don't set OUT_CONF as writable,
> > > although it is, as we only need to read it. Finally add a function
> > > that configures the OUT_INDCONF register when the GPIO API sets the
> > > drive mode of the pins.

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?

...

> > > +#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).

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

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

> > > +#define PCAL65xx_BANK_INDOUT_CONF BIT(8 + 12)
> >
> > IND is a bit ambiguous based on the description below.
> > After you elaborate, I probably can propose better naming.
>
> It's derived from the register name in the datasheet which is
> "Individual pin output port X configuration register".

Since we have already register definitions, if should follow existing
pattern, i.e. OUT_INDCONF.

-- 
With Best Regards,
Andy Shevchenko