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

[Bedel, Alban]

On Tue, 2021-02-02 at 14:57 +0100, Linus Walleij wrote:
> On Thu, Jan 28, 2021 at 4:36 PM Alban Bedel <alban.bedel@xxxxxxxx>
> wrote:
> 
> > From a quick glance at various datasheet the PCAL6524 seems to be
> > the
> > only chip in this familly 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, PCAL6524, 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 configure the OUT_INDCONF register when the GPIO API set the
> > drive mode of the pins.
> > 
> > Signed-off-by: Alban Bedel <alban.bedel@xxxxxxxx>
> 
> Thats's nice!
> 
> > + *     Output port configuration       0x40 + 7 * bank_size    R
> > + *
> > + *   - PCAL6524 with individual pin configuration
> > + *     Individual pin output config    0x40 + 12 * bank_size   RW
> 
> So this will become 0x70? It's a bit hard for me this weird
> register layout...

Correct.

> > +static int pcal6524_gpio_set_drive_mode(struct pca953x_chip *chip,
> > +                                       unsigned int offset,
> > +                                       unsigned long config)
> > +{
> > +       u8 out_conf_reg = pca953x_recalc_addr(
> > +               chip, PCAL953X_OUT_CONF, 0);
> > +       u8 out_indconf_reg = pca953x_recalc_addr(
> > +               chip, PCAL6524_OUT_INDCONF, offset);
> > +       u8 mask = BIT(offset % BANK_SZ), val;
> 
> Split to two variable declarations please, this is hard to read.

Will do.

> > +       unsigned int out_conf;
> > +       int ret;
> 
> So we set mask to the bit index for the line we want to affect.
> 
> > +       if (config == PIN_CONFIG_DRIVE_OPEN_DRAIN)
> > +               val = mask;
> > +       else if (config == PIN_CONFIG_DRIVE_PUSH_PULL)
> > +               val = 0;
> > +       else
> > +               return -EINVAL;
> 
> And this makes sense, we set it to 1 to enable open drain.
> 
> > +       /* Invert the value if ODENn is set */
> > +       ret = regmap_read(chip->regmap, out_conf_reg, &out_conf);
> > +       if (ret)
> > +               goto exit;
> > +       if (out_conf & BIT(offset / BANK_SZ))
> 
> I suppose this could be written if (out_conf & mask)?

No, the mask in ODENn is per bank (group of 8 pins) and not per pin.

> > +               val ^= mask;
> 
> Invert? Why?
> 
> The datasheet says:
> 
>   "If the ODENx bit is set at logic 0 (push-pull), any bit set to
> logic 1
>   in the IOCRx register will reverse the output state of that pin
> only
>   to open-drain. When ODENx bit is set at logic 1 (open-drain), a
>   logic 1 in IOCRx will set that pin to push-pull."
> 
> So your logic is accounting for the fact that someone go and set
> one of the bits in ODENx to 1, but aren't they all by default set
> to zero (or should be programmed by the driver to zero)
> so that you can control open drain individually here by simply
> setting the corresponding bit to 1 for open drain and 0 for
> push-pull?

Yes the ODENx bits are 0 by default, but the bootloader might have
changed them for example. Currently the driver doesn't do any reset so
I think it make sense to correctly handle this case as it doesn't bring
that much extra complexity.

Alban

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