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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 1/25/23 12:34, Andy Shevchenko wrote:
> On Wed, Jan 25, 2023 at 1:32 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 1/25/23 12:23, Andy Shevchenko wrote:
>>> On Wed, Jan 25, 2023 at 1:05 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 than the value the pin
>>>> would output when put in output mode.
>>>
>>> It really depends. I think it's a wrong perception and brings nothing
>>> to software. If we output, we know what we program, so reading back
>>> returns us what we _assume_ should be on the pin under normal
>>> circumstances. The difference is OD/OS/OE/OC cases where we output>>> only one possible value.
>>>
>>>> Fixes: 275b13a65547 ("gpio: Add support for TPS68470 GPIOs")
>>>
>>> Is it really fixing anything? Can we have more?
>>>
>>> P.S. Before doing this, I would have a clarification in the
>>> documentation. Sorry that I have had no time to respond to my series
>>> regarding that. But it seems we have a strong disagreement in the
>>> area.
>>
>> I know disagree on some of the semantics of gpiod_get_value() but 99.9%
>> of the consumers of gpiod_get_value() are going to deal with pins
>> which are either in input mode, or in some non push-pull (e.g.
>> open-collector for i2c) mode. And in those cases reading GPDI
>> os the right thing to do.
>>
>> Calling gpiod_get_value() when the pin is in push-pull mode really
>> does not make much sense, so there will be very little if any users
>> of that. And this patch fixes the 99.9% other (potential) users
>> while not breaking the push-pull case since even then reading GPDI
>> should still return the right value.
> 
> Some hardware may not even allow what you are doing here.
> For example when input and output direction is the same bit in the
> register. Another case when the input buffer is simply disabled by the
> driver for a reason (power consumption, etc).

But we are not talking some hw here, we are talking about this
specific driver, which has separate registers for setting
the output and reading the pin value.

The current behavior of this driver is *totally* broken wrt
gpiod_get_value() and this fix actually makes it work!

Regards,

Hans