Re: [PATCH 1/5] gpio: tps68470: Fix tps68470_gpio_get() reading from the wrong register

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Tue, Nov 29, 2022 at 01:19:57PM +0100, Hans de Goede wrote:
> On 11/29/22 12:56, Andy Shevchenko wrote:
> > On Tue, Nov 29, 2022 at 1:27 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >> On 11/29/22 11:22, Andy Shevchenko wrote:
> >>> On Mon, Nov 28, 2022 at 11:44 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>>>
> >>>> For the regular GPIO pins the value should be read from TPS68470_REG_GPDI,
> >>>> so that the actual value of the pin is read, rather then the value the pin
> >>>
> >>> than
> >>
> >> Ack.
> >>
> >>>> would output when put in output mode.
> >>>
> >>> I don't see it here and haven't checked the context, but the idea is
> >>> to check the direction and return either input (if pin is in input
> >>> mode) or [cached] output.If it's the case, the patch looks good to me.
> >>
> >> No the idea is to always actually use the input register when reading
> >> the pins, independent of the input/output mode. Instead of always
> >> reading the [cached] output register value.
> > 
> > But why? This makes a little sense to me.
> 
> I don't understand what your problem is with this patch ?
> 
> This is standard behavior for GPIO drivers, the get() callback
> always reads the actual pin values when there is a registers
> to get the actual pin-values. AFAIK this is no different from what
> countless other GPIO drivers do ?

If the GPIO is requested is output and the pin in the HiZ mode, what value
should you return?

> >> The input buffer will still work when the device is in output mode
> > 
> > Does this hardware support HiZ?
> 
> Yes the 7 standard GPIO pins can be put in input mode, aka HiZ mode.

No, input and HiZ are two different states. If the hardware doesn't
support the latter, then your patch doesn't make any difference indeed, except
involving "in" buffer for "out" value. Which I consider an unneeded
step instead of reading [cached] "out" value.

[cached] here is used in a broader sense: either SW cache, or read back value
from the GPIO out register (not all hardware support that).

> >> and if somehow an external force is pushing the pin low/high against
> >> the output value then the input buffer will correctly reflect this
> >> (assuming the output is current limited and thus the magic smoke
> >> stays inside the chip).
> > 
> > Exactly, when smoke comes out, the hardware is broken and code,
> > whatever is it, makes no difference at all.
> 
> The GPIO part of the TPS68470 supports open-drain outputs, to correctly
> get the actual value on the pin from the get() callback, the GPDI
> register must be used. And being able to detect some outside chip
> forcing the line low in open-drain mode is important to be able to
> e.g. implement a software I2C master.

Right, but for push-pull mode this is not needed, correct?

> As I mentioned above actually using the input buffer value in
> the get() method is standard behavior for GPIO drivers, exactly
> for reasons like allowing a sw I2C master implementation to
> detect clock stretching or conflicts (in the multi master case).

> I really don't understand what is so controversial about this
> patch?

There are couple of remarks above:
1) what to read at the real HiZ mode ("in" and "out" buffers are disconnected
   from the pin) in case hardware supports it;
2) why to read "out" value via "in" buffer in case of push-pull mode.

> Note the datasheet describes the GPDO / GPDI bit 0  values as:
> 
> GPDO bit 0: "Control of the GPIO0 output"
> GPDI bit 0: "State of the GPIO0 input"
> 
> So clearly GPDI is the correct register to use for the get()
> callback, which is what this patch is doing.

-- 
With Best Regards,
Andy Shevchenko