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

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 1/25/23 15:55, Andy Shevchenko wrote:
> On Wed, Jan 25, 2023 at 1:40 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>> On 1/25/23 12:34, Andy Shevchenko wrote:
>>> On Wed, Jan 25, 2023 at 1:32 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>>> On 1/25/23 12:23, Andy Shevchenko wrote:
>>>>> On Wed, Jan 25, 2023 at 1:05 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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