On Thu, Jul 14, 2022 at 10:47:27AM +0200, Nuno Sá wrote: > On Thu, 2022-07-14 at 16:27 +0800, Kent Gibson wrote: > > On Thu, Jul 14, 2022 at 09:14:21AM +0200, Nuno Sá wrote: > > > On Thu, 2022-07-14 at 12:20 +0800, Kent Gibson wrote: > > > > On Wed, Jul 13, 2022 at 08:36:38PM +0300, Andy Shevchenko wrote: > > > > > On Wed, Jul 13, 2022 at 03:14:18PM +0200, Nuno Sá wrote: > > > > > > This change prepares the gpio core to look at firmware flags > > > > > > and > > > > > > set > > > > > > 'FLAG_BIAS_DISABLE' if necessary. It works in similar way to > > > > > > 'GPIO_PULL_DOWN' and 'GPIO_PULL_UP'. > > > > > > > > > > ... > > > > > > > > > > > GPIO_PULL_UP = (1 << 4), > > > > > > GPIO_PULL_DOWN = (1 << 5), > > > > > > + GPIO_PULL_DISABLE = (1 << 6), > > > > > > > > > > To me it seems superfluous. You have already two flags: > > > > > PUp > > > > > PDown > > > > > When none is set --> Pdisable > > > > > > > > > > > > > Agree with Andy on this. The FLAG_BIAS_DISABLE was added, by me, > > > > to > > > > allow the cdev interface to support bias. cdev requires a "don't > > > > care" > > > > state, distinct from an explicit BIAS_DISABLE. > > > > The FLAG_BIAS_DISABLE allows gpiolib-cdev to communicate that to > > > > gpiolib, without altering the interpretation of the existing > > > > PULL_UP > > > > and > > > > PULL_DOWN flags. > > > > That is not an issue on the machine interface, where the two > > > > GPIO_PULL > > > > flags suffice. > > > > > > > > > > I see, but this means we can only disable the pin BIAS through > > > userspace. I might be wrong but I don't see a reason why it > > > wouldn't be > > > valid to do it from an in kernel path as we do for PULL-UPS and > > > PULL- > > > DOWNS > > > > > > > > > If you are looking for the place where FLAG_BIAS_DISABLE is set > > > > it is > > > > in > > > > gpio_v2_line_config_flags_to_desc_flags() in gpiolib-cdev.c. > > > > > > > > Referring to gpio_set_bias(), the only place in gpiolib the > > > > FLAG_BIAS_DISABLE is used, if neither FLAG_PULL_UP, > > > > FLAG_PULL_DOWN, > > > > nor FLAG_BIAS_DISABLE are set then the bias configuration remains > > > > unchanged (the don't care case) - no change is passed to the > > > > driver. > > > > Otherwise the corresponding PIN_CONFIG_BIAS flag is passed to the > > > > driver. > > > > > > > > > > Exactly, but note FLAG_BIAS_DISABLE can only be set from userspace > > > at > > > this point (IIUTC). If everyone agrees that should be case, so be > > > it. > > > But as I said, I just don't see why it's wrong to do it within the > > > kernel. > > > > > > > Believe it or not gpiolib-cdev is part of the kernel, not userspace - > > it > > just provides an interface to userspace. > > > > Yes, I do know that. But don't you still need a userspace process to > open the cdev and do the ioctl()? > Only if you want to drive the cdev interface, so not in this case. You would not use cdev, you would use gpiolib directly. > > Bias can be disabled by calling gpiod_direction_input() or > > gpiod_direction_output() after setting the FLAG_BIAS_DISABLE, as > > gpiolib-cdev does. > > > > Does that work for you? > > > > I'm not seeing how would this work... We would need to make gpiod > consumers having to do this. Something like: > > > desc = giod_get(); > set_bit(FLAG_BIAS_DISABLE, &desc->flags); > set_direction... > > In a nutshell. If that solves your immediate problem then you need to decide what problem your patch is trying to address. Cheers, Kent.
