On Tue, Feb 2, 2021 at 6:45 PM Bedel, Alban [off-list ref] wrote:

On Tue, 2021-02-02 at 14:57 +0100, Linus Walleij wrote:

quoted

quoted

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

quoted

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