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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Dec 1, 2023 at 1:34 PM Nuno Sá <noname.nuno@xxxxxxxxx> 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.

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

> 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 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?

That's indeed a bit of an oddity...

If you implement .set_config and handle PIN_CONFIG_DRIVE_OPEN_DRAIN
for these lines then I think gpiolib will do the right thing for you.

Yours,
Linus Walleij





[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux