Re: [PATCH v2 2/2] hwmon: ltc4282: add support for the LTC4282 chip

["Nuno Sá" <noname.nuno@gmail.com>]

On Fri, 2023-12-01 at 14:40 +0100, Linus Walleij wrote:
> On Fri, Dec 1, 2023 at 1:34 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > On Thu, 2023-11-30 at 21:15 +0100, Linus Walleij wrote:
> 
> > I did not used libgpiod but I did tested it with gpio-sysfs. Well, I could
> > effectively see the pull down behaviour but since my eval board has no pull-ups I
> > could not drive the line high.
> 
> libgpiod has the upside of offering you to set the pull down and open
> drain behaviour from userspace.
> 

Yeah, I can also just come up with a minimal test driver and devicetree.

> > > The gpiolib framework assumes we can do open drain emulation by
> > > setting lines as input. It is used as fallback unless the hardware has
> > > an explicit open drain setting.
> > 
> > Yeah, I did look at that after you pointed that out. There's just something I'm
> > still
> > not getting. This HW has no explicit open drain setting because open drain is all
> > that it is. So, I guess we could just specify the flag in devicetree so gpiolib
> > could
> > use the emulation
> > but I wonder how would we have things in case we have the HW setup
> > to drive the pin high (so having this as GPOs)?
> 
> If another device tree node uses:
> 
> foo-gpios = <&gpio0 5 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> 
> The result will be that gpiolib will emulate open drain.
> 
> From userspace libgpiod can do the same request.
> 
> > > > Also got me thinking if a gpi vs gpo devicetree property would make sense.
> > > > But I
> > > > would likely leave it more generic/relaxed for now (even though I think you
> > > > would
> > > > need to be creative and actually use more HW to have the possibility of using
> > > > these
> > > > pins as GPIs and GPOs at the same time).
> > > 
> > > We don't define that in the device tree currently, we just make the driver
> > > not support output on input-only pins and vice versa, by returning error
> > > codes on the .set_direction() callbacks.
> > 
> > I see, but in this case, the pins could be outputs depending on the HW setup but
> > there's no way for us to know that in the driver.
> 
> We just specify the line in the device tree, and we just use it as
> intended in the
> driver, if it is present, whether that is as input or output.
> 
> We do not try to over-protect users from misusing GPIO lines that have just
> one possible (electronic defined) mode. It would be over-engineering IMO.
> 

Fair enough...

> > And given the fact that (I think)
> > it's highly unlikely for pins like this to ever be GPIs and GPOs at the same
> > time, I
> > brought the devicetree property to define input and output only. So, roughly,
> > what I
> > have in mind now for the chip is;
> > 
> > .set_config() -> with PULL_DOWN and HIGH_IMPEDANCE support
> > .direction_input() -> This is important for gpio1 where we do have an hw setting
> > to
> > set the direction. On the other pins I was thinking in just forcing high-z. Or
> > maybe
> > can I just rely on gpio_set_bias()?
> 
> No just write some default set-up into the registers, that's fine.
> Or leave the power-on defaults.
> 
> > .direction_ouput() -> Would only matter for gpio1
> 
> The just return an error code for any other GPIO where this is called.
> 
> > .get/set_value() -> And in this case we just assume that high value might or
> > might
> > not be possible (depending on the hw setup). Note that reading the pin state is
> > always possible.
> 
> If a pins .direction_output() fails, .set_value() will not be called
> on it either.

This is where I lost you :(. So, I'm might be overcomplicating things but... Again,
the case where someone wired up HW so that we can actually use the pin to drive the
line high (having an external pull up). In that case, If I return error, then I won't
be able to effectively set the line high (as you said, set_value will not be called
on it either).

Now, I do understand that if we have the line flagged as GPIO_OPEN_DRAIN, then
gpiolib will switch the line to input which means we will set the line in high-z
which means that if we have a pull up, then the line will be high. I mean, it works
but it would be strange if someone wants to have the line as output high and after
trying to set the it high, it sees the pin moving to input. But if this is how it
should be, fine by me.

I do understand this is the definition of open drain so I guess someone should know
what to expect when operating with pins like this.

> 
> > This means that I assume we can have both directions but that is not really case
> > and
> > one needs to know what it is doing :). Or in cases like this, we just ignore the
> > possibility of having GPO's and we let gpiolib do the emulation?
> > 
> > Sounds reasonable or not really how I should handle this open-drain only pins?
> 
> Open drain-only pins would be pins that can be set to electric LOW (grounded)
> or High-Z. Is this what we have?
> 

Yes, that is the only thing we have. Meaning that there is no hw setting to set the
pins to open drain. Open drain is what they are. That is why I'm not seeing the point
in having PIN_CONFIG_DRIVE_OPEN_DRAIN implemented.

Anyways, I'll try to have something cooked next week. I'll be slow since the winter
(not even there yet) in Germany already got me!


- Nuno Sá