[PATCH v2 resend 0/2] gpio: tps68470: 2 bugfixes

[PATCH v2 resend 0/2] gpio: tps68470: 2 bugfixes

[Hans de Goede]

Hi All,

My previous gpio-tps68470 patch series, which tried to model the LED
outputs as GPIOs, included these 2 bugfixes. The patch to model the
LED outputs as GPIOs has been nacked (*), but these 2 bugfixes are
still valid and it would be good if we can get them merged.

Regards,

Hans

*) A new series from Kate, modelling these as LED class devices is pending


Hans de Goede (2):
  gpio: tps68470: Fix tps68470_gpio_get() reading from the wrong
    register
  gpio: tps68470: Make tps68470_gpio_output() always set the initial
    value

 drivers/gpio/gpio-tps68470.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

-- 
2.39.0

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

[Hans de Goede]

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.

Fixes: 275b13a65547 ("gpio: Add support for TPS68470 GPIOs")
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Tested-by: Daniel Scally <dan.scally@ideasonboard.com>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Add Fixes tag
---
 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.39.0

[PATCH v2 resend 2/2] gpio: tps68470: Make tps68470_gpio_output() always set the initial value

[Hans de Goede]

Make tps68470_gpio_output() call tps68470_gpio_set() for output-only pins
too, so that the initial value passed to gpiod_direction_output() is
honored for these pins too.

Fixes: 275b13a65547 ("gpio: Add support for TPS68470 GPIOs")
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Tested-by: Daniel Scally <dan.scally@ideasonboard.com>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Add Fixes tag
- Add Andy's Reviewed-by
---
 drivers/gpio/gpio-tps68470.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-tps68470.c b/drivers/gpio/gpio-tps68470.c
index 778a72cf800c..2ca86fbe1d84 100644
--- a/drivers/gpio/gpio-tps68470.c
+++ b/drivers/gpio/gpio-tps68470.c
@@ -91,13 +91,13 @@ static int tps68470_gpio_output(struct gpio_chip *gc, unsigned int offset,
 	struct tps68470_gpio_data *tps68470_gpio = gpiochip_get_data(gc);
 	struct regmap *regmap = tps68470_gpio->tps68470_regmap;
 
+	/* Set the initial value */
+	tps68470_gpio_set(gc, offset, value);
+
 	/* rest are always outputs */
 	if (offset >= TPS68470_N_REGULAR_GPIO)
 		return 0;
 
-	/* Set the initial value */
-	tps68470_gpio_set(gc, offset, value);
-
 	return regmap_update_bits(regmap, TPS68470_GPIO_CTL_REG_A(offset),
 				 TPS68470_GPIO_MODE_MASK,
 				 TPS68470_GPIO_MODE_OUT_CMOS);
-- 
2.39.0

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 resend 2/2] gpio: tps68470: Make tps68470_gpio_output() always set the initial value

[Andy Shevchenko]

On Wed, Jan 25, 2023 at 1:05 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Make tps68470_gpio_output() call tps68470_gpio_set() for output-only pins
> too, so that the initial value passed to gpiod_direction_output() is
> honored for these pins too.
>
> Fixes: 275b13a65547 ("gpio: Add support for TPS68470 GPIOs")
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Yes, just to confirm that this patch is correct in my opinion.
Thank you!

> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> Tested-by: Daniel Scally <dan.scally@ideasonboard.com>
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> - Add Fixes tag
> - Add Andy's Reviewed-by
> ---
>  drivers/gpio/gpio-tps68470.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpio-tps68470.c b/drivers/gpio/gpio-tps68470.c
> index 778a72cf800c..2ca86fbe1d84 100644
> --- a/drivers/gpio/gpio-tps68470.c
> +++ b/drivers/gpio/gpio-tps68470.c
> @@ -91,13 +91,13 @@ static int tps68470_gpio_output(struct gpio_chip *gc, unsigned int offset,
>         struct tps68470_gpio_data *tps68470_gpio = gpiochip_get_data(gc);
>         struct regmap *regmap = tps68470_gpio->tps68470_regmap;
>
> +       /* Set the initial value */
> +       tps68470_gpio_set(gc, offset, value);
> +
>         /* rest are always outputs */
>         if (offset >= TPS68470_N_REGULAR_GPIO)
>                 return 0;
>
> -       /* Set the initial value */
> -       tps68470_gpio_set(gc, offset, value);
> -
>         return regmap_update_bits(regmap, TPS68470_GPIO_CTL_REG_A(offset),
>                                  TPS68470_GPIO_MODE_MASK,
>                                  TPS68470_GPIO_MODE_OUT_CMOS);
> --
> 2.39.0
>


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Jan 25, 2023 at 1:23 PM Andy Shevchenko
<andy.shevchenko@gmail.com> 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.

Just realized that what we are missing is BIDI settings for the
direction. That would solve a lot of these kinds of grey areas. Linus?
Bart?


-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

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.

Regards,

Hans

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

[Andy Shevchenko]

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

That's why we need some flags to be sure we are doing the right thing.

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

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

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

[Andy Shevchenko]

On Wed, Jan 25, 2023 at 1:40 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 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!

I don't see how you can make it work for GPO pins. get() method should
work for them too. Probably I'm missing this part. At least the choice
of the registers has to rely on direction, correct?

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 1/25/23 15:55, Andy Shevchenko wrote:
> On Wed, Jan 25, 2023 at 1:40 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> 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!
> 
> I don't see how you can make it work for GPO pins. get() method should
> work for them too. Probably I'm missing this part. At least the choice
> of the registers has to rely on direction, correct?

You mean the special output-only pins which are earmarked
for usage as sensor-reset, etc. ? That is handled already:

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_GPDI;
        int val, ret;

        if (offset >= TPS68470_N_REGULAR_GPIO) {
                offset -= TPS68470_N_REGULAR_GPIO;
                reg = TPS68470_REG_SGPO;
        }

	ret = regmap_read(regmap, reg, &val);
	... (error handling)

	return !!(val & BIT(offset));
}

Notice the offset >= TPS68470_N_REGULAR_GPIO that checks for
the output only pins.

Regards,

Hans