Messages in this thread | | | From | Linus Walleij <> | Date | Thu, 11 Oct 2018 19:01:13 +0200 | Subject | Re: [PATCH] gpio/regulator: Allow nonexclusive GPIO access |
| |
On Thu, Oct 11, 2018 at 4:43 PM Mark Brown <broonie@kernel.org> wrote: > On Thu, Oct 11, 2018 at 04:35:31PM +0200, Linus Walleij wrote: > > > + /* > > + * Some fixed regulators share the enable line between two > > + * regulators which makes it necessary to get a handle on the > > + * same descriptor for two different consumers. This will get > > + * the GPIO descriptor, but only the first call will initialize > > + * it so any flags such as inversion or open drain will only > > + * be set up by the first caller and assumed identical on the > > + * next caller. > > + * > > + * FIXME: find a better way to deal with this. > > + */ > > + gflags |= GPIOD_FLAGS_BIT_NONEXCLUSIVE; > > + > > It's not just fixed regulators that do this, often regulators with > register control can do this as well.
Yeah I thought that was implicit since the comment is inside fixed.c but I can change it, or you can edit when applying if you like.
> Since power up is often a > performance critical path but regulators tend to be controlled via slow > buses like I2C it's common to have register control combined with a GPIO > enable line so you don't need to use the bus to do the enables. That > should just be a case of adding this flag to all the drivers that have > already been converted to gpiod (including the core code that's in > regulator_ena_gpio_request() which I thought was coping with this > already) unless I'm missing something?
Sorry if I don't get it... we already have code in the core to check if the same gpiod is used by two regulators. regulator_ena_gpio_request() does this:
if (config->ena_gpiod) gpiod = config->ena_gpiod; else gpiod = gpio_to_desc(config->ena_gpio);
So after the change I made to fixed.c this comes in through config->ena_gpiod.
Later in this function:
list_for_each_entry(pin, ®ulator_ena_gpio_list, list) { if (pin->gpiod == gpiod) { rdev_dbg(rdev, "GPIO %d is already used\n", config->ena_gpio);
So there is the reference counting code, where we identify that the same descriptor is already in use.
The only change this patch really does is make it possible for the same gpiod to come in twice from fixed.c (as is proper).
If it's a simple and single consumer enable/disable disable line all is fine and it doesn't get reference counted etc, since there is one and only one consumer for the GPIO. gpiod_get[_optional]() is just fine in these cases.
When I straight out the FIXME:s I would add code to check that the flags passed to gpiolib are coherent and maybe store all users in an array instead of a simple char * inside the gpio descriptor, so we properly handle more than one user in gpiolib.
Possibly the reference counting should be moved over from regulator core as well: this would apply if there are more subsystems than regulator using the same GPIO with multiple consumers.
But that is for the future fixups.
Yours, Linus Walleij
| |