| Message ID | 20221128214408.165726-2-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/5] gpio: tps68470: Fix tps68470_gpio_get() reading from the wrong register | expand |
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
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) {
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(-)