Hi, On 11/29/22 12:56, Andy Shevchenko wrote: > On Tue, Nov 29, 2022 at 1:27 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: >> On 11/29/22 11:22, Andy Shevchenko wrote: >>> On Mon, Nov 28, 2022 at 11:44 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 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
