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

  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
would output when put in output mode.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpio/gpio-tps68470.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

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

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

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpio/gpio-tps68470.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-tps68470.c b/drivers/gpio/gpio-tps68470.c
> index aaddcabe9b35..778a72cf800c 100644
> --- a/drivers/gpio/gpio-tps68470.c
> +++ b/drivers/gpio/gpio-tps68470.c
> @@ -30,7 +30,7 @@ static int tps68470_gpio_get(struct gpio_chip *gc, unsigned int offset)
>  {
>         struct tps68470_gpio_data *tps68470_gpio = gpiochip_get_data(gc);
>         struct regmap *regmap = tps68470_gpio->tps68470_regmap;
> -       unsigned int reg = TPS68470_REG_GPDO;
> +       unsigned int reg = TPS68470_REG_GPDI;
>         int val, ret;
>
>         if (offset >= TPS68470_N_REGULAR_GPIO) {
> --
> 2.38.1
>
  

Hi,

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.

The input buffer will still work when the device is in output mode
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).

Regards,

Hans



> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/gpio/gpio-tps68470.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpio/gpio-tps68470.c b/drivers/gpio/gpio-tps68470.c
>> index aaddcabe9b35..778a72cf800c 100644
>> --- a/drivers/gpio/gpio-tps68470.c
>> +++ b/drivers/gpio/gpio-tps68470.c
>> @@ -30,7 +30,7 @@ static int tps68470_gpio_get(struct gpio_chip *gc, unsigned int offset)
>>  {
>>         struct tps68470_gpio_data *tps68470_gpio = gpiochip_get_data(gc);
>>         struct regmap *regmap = tps68470_gpio->tps68470_regmap;
>> -       unsigned int reg = TPS68470_REG_GPDO;
>> +       unsigned int reg = TPS68470_REG_GPDI;
>>         int val, ret;
>>
>>         if (offset >= TPS68470_N_REGULAR_GPIO) {
>> --
>> 2.38.1
>>
> 
>
  

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.

> The input buffer will still work when the device is in output mode

Does this hardware support HiZ?

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

Hi,

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 ?

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

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

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?

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.

Regards,

Hans
  

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.
  

Hi,

On 11/29/22 13:34, Andy Shevchenko wrote:
> 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 actual value measured on the pin. The only output + HiZ mode is
the GPDO register saying the pin should be high while it is configured
as an open-drain pin and then get() should return the actual value
on the pin which might be high (pulled-up) or low, because of
being pulled down by another open-drain output on the same bus
(e.g. I2C) and presumably the caller wants to know the actual
pin value to detect e.g. clock stretching with I2C.

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

See above.

> [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;

If your definition of real HiZ mode is the input buffer also
being disconnected from the pin, then AFAICT from the datasheet
this hw does not support real (see above for what I consider
HiZ mode).

> 2) why to read "out" value via "in" buffer in case of push-pull mode.

Because it keeps the code KISS, because even then the bus
might be short-circuited to ground?

A better question is why would a GPIO API user ever call get()
on a pin in push-pull mode. The only answer I can come up with
is to get *the actual pin value* to e.g. detect a short-circuit
on the bus to gnd / Vdd.

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

Regards,

Hans
  

Hi Hans

On 28/11/2022 21:44, Hans de Goede 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
> would output when put in output mode.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>


Good spot:


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>


Just building the series to test it too

> ---
>   drivers/gpio/gpio-tps68470.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-tps68470.c b/drivers/gpio/gpio-tps68470.c
> index aaddcabe9b35..778a72cf800c 100644
> --- a/drivers/gpio/gpio-tps68470.c
> +++ b/drivers/gpio/gpio-tps68470.c
> @@ -30,7 +30,7 @@ static int tps68470_gpio_get(struct gpio_chip *gc, unsigned int offset)
>   {
>   	struct tps68470_gpio_data *tps68470_gpio = gpiochip_get_data(gc);
>   	struct regmap *regmap = tps68470_gpio->tps68470_regmap;
> -	unsigned int reg = TPS68470_REG_GPDO;
> +	unsigned int reg = TPS68470_REG_GPDI;
>   	int val, ret;
>   
>   	if (offset >= TPS68470_N_REGULAR_GPIO) {