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

[Bedel, Alban]

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:
> > > 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?

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.

> > > > +#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. The PCAL65xx we are talking about add a further register
block, so following the existing concept its not a new layout.

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

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

Ok

Alban

Attachment: signature.asc
Description: This is a digitally signed message part