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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 2 Feb 2021 17:45:21 +0000
From:   "Bedel, Alban" <alban.bedel@...q.com>
To:     "linus.walleij@...aro.org" <linus.walleij@...aro.org>
CC:     "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "bgolaszewski@...libre.com" <bgolaszewski@...libre.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] gpio: pca953x: add support for open drain pins on
 PCAL6524

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@...q.com>
> 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@...q.com>
> 
> 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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)