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> <CACRpkdaP8-mnXuBZRKad53tvGrS0BdfTRKNezr0mhRVf8qkYig@mail.gmail.com>
 <93036ef781d20df4c6017178cc545702bd0f42bc.camel@aerq.com>
In-Reply-To: <93036ef781d20df4c6017178cc545702bd0f42bc.camel@aerq.com>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Tue, 2 Feb 2021 20:21:34 +0100
Message-ID: <CACRpkdZhOWxbOdfC3sh9BXj9dWTcm3gPuGjzz+T96==+G2-i2g@mail.gmail.com>
Subject: Re: [PATCH] gpio: pca953x: add support for open drain pins on PCAL6524
To:     "Bedel, Alban" <alban.bedel@aerq.com>
Cc:     "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "bgolaszewski@baylibre.com" <bgolaszewski@baylibre.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Feb 2, 2021 at 6:45 PM Bedel, Alban <alban.bedel@aerq.com> wrote:
> On Tue, 2021-02-02 at 14:57 +0100, Linus Walleij wrote:

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

Ah I see it now (confused % for / ...)

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

Hm. I guess you're right if that is the style of the driver overall
(don't touch bootloader/reset defaults).

We don't use that style for interrupt registers because we don't
want interrupts to randomly fire, instead we usually disable them
all so the driver and Linux keeps track on what is enabled. But for
bias etc, I guess it is OK. But consider the option of just writing
0 into all the ODENx registers at probe(). I'm not gonna complain
if you really want it like this though.

Yours,
Linus Walleij