Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210128153601.153126-1-alban.bedel@aerq.com>
In-Reply-To: <20210128153601.153126-1-alban.bedel@aerq.com>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Tue, 2 Feb 2021 14:57:31 +0100
Message-ID: <CACRpkdaP8-mnXuBZRKad53tvGrS0BdfTRKNezr0mhRVf8qkYig@mail.gmail.com>
Subject: Re: [PATCH] gpio: pca953x: add support for open drain pins on PCAL6524
To:     Alban Bedel <alban.bedel@aerq.com>
Cc:     Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jan 28, 2021 at 4:36 PM Alban Bedel <alban.bedel@aerq.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@aerq.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...

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

> +       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)?

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

> +       /* Configure the drive mode */
> +       ret = regmap_write_bits(chip->regmap, out_indconf_reg, mask, val);

Yours,
Linus Walleij