| Message ID | 20181011143531.7195-1-linus.walleij@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpio/regulator: Allow nonexclusive GPIO access | expand |
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. 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?
Hi Linus, On 2018-10-11 16:35, Linus Walleij wrote: > This allows nonexclusive (simultaneous) access to a single > GPIO line for the fixed regulator enable line. This happens > when several regulators use the same GPIO for enabling and > disabling a regulator, and all need a handle on their GPIO > descriptor. > > This solution with a special flag is not entirely elegant > and should ideally be replaced by something more careful as > this makes it possible for several consumers to > enable/disable the same GPIO line to the left and right > without any consistency. The current use inside the regulator > core should however be fine as it takes special care to > handle this. > > For the state of the GPIO backend, this is still the > lesser evil compared to going back to global GPIO > numbers. > > Fixes: efdfeb079cc3 ("regulator: fixed: Convert to use GPIO descriptor only") > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > Cc: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- It looks to be fixing the Sii9234 probe issue, but I didn't do any further tests. I must go out the office and I will be back on Monday. I think You can add my: Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > Hi Marek, it would be great if you could try this on top > of linux-next and report if it solves your problem on the > Samsung Exynos. > > If it does I hope it will apply cleanly on the regulator > tree. > --- > drivers/gpio/gpiolib.c | 19 +++++++++++++++++-- > drivers/regulator/fixed.c | 13 +++++++++++++ > include/linux/gpio/consumer.h | 1 + > 3 files changed, 31 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 7c222df8f834..f82a741ff428 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -4144,8 +4144,23 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev, > * the device name as label > */ > status = gpiod_request(desc, con_id ? con_id : devname); > - if (status < 0) > - return ERR_PTR(status); > + if (status < 0) { > + if (status == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE) { > + /* > + * This happens when there are several consumers for > + * the same GPIO line: we just return here without > + * further initialization. It is a bit if a hack. > + * This is necessary to support fixed regulators. > + * > + * FIXME: Make this more sane and safe. > + */ > + dev_info(dev, "nonexclusive access to GPIO for %s\n", > + con_id); > + return desc; > + } else { > + return ERR_PTR(status); > + } > + } > > status = gpiod_configure_flags(desc, con_id, lookupflags, flags); > if (status < 0) { > diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c > index 7d639ad953b6..ccc29038f19a 100644 > --- a/drivers/regulator/fixed.c > +++ b/drivers/regulator/fixed.c > @@ -170,6 +170,19 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev) > gflags = GPIOD_OUT_LOW_OPEN_DRAIN; > } > > + /* > + * 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; > + > cfg.ena_gpiod = devm_gpiod_get_optional(&pdev->dev, NULL, gflags); > if (IS_ERR(cfg.ena_gpiod)) > return PTR_ERR(cfg.ena_gpiod); > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h > index 0f350616d372..f2f887795d43 100644 > --- a/include/linux/gpio/consumer.h > +++ b/include/linux/gpio/consumer.h > @@ -39,6 +39,7 @@ struct gpio_descs { > #define GPIOD_FLAGS_BIT_DIR_OUT BIT(1) > #define GPIOD_FLAGS_BIT_DIR_VAL BIT(2) > #define GPIOD_FLAGS_BIT_OPEN_DRAIN BIT(3) > +#define GPIOD_FLAGS_BIT_NONEXCLUSIVE BIT(4) > > /** > * Optional flags that can be passed to one of gpiod_* to configure direction Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
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
On Thu, Oct 11, 2018 at 07:01:13PM +0200, Linus Walleij wrote: > 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: > > 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. Other regulators that have GPIO control for their enables do their own requests (as does the core) so don't they all need to set the flag GPIOD_FLAGS_BIT_NONEXCLUSIVE when they request otherwise it'll only work in systems where the regulators after the first that request the GPIO are fixed regulators?
On Thu, Oct 11, 2018 at 7:54 PM Mark Brown <broonie@kernel.org> wrote: > On Thu, Oct 11, 2018 at 07:01:13PM +0200, Linus Walleij wrote: > > 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: > > > > 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. > > Other regulators that have GPIO control for their enables do their own > requests (as does the core) so don't they all need to set the flag > GPIOD_FLAGS_BIT_NONEXCLUSIVE when they request otherwise it'll only work > in systems where the regulators after the first that request the GPIO > are fixed regulators? Yeah ... I guess I assumed that the same enable line is not used for a fixed regulator and then some other regulator (which is not fixed). If this is common practice we need to account for it. Obviously I have no idea what is common practice :/ gpio-regulator.c will work fine since it passes in a global number (still) so it will just grab the desc out of the GPIO with the legacy function and go on. (I need to keep it in mind when refactoring that one later though!) Do we know some way to identify the affected systems or should we just tag on GPIOD_FLAGS_BIT_NONEXCLUSIVE on all the stuff we converted to descs to play it safe? (I can cook a patch.) Yours, Linus Walleij
Hi Linus, On 11/10/18 15:35, Linus Walleij wrote: > This allows nonexclusive (simultaneous) access to a single > GPIO line for the fixed regulator enable line. This happens > when several regulators use the same GPIO for enabling and > disabling a regulator, and all need a handle on their GPIO > descriptor. > > This solution with a special flag is not entirely elegant > and should ideally be replaced by something more careful as > this makes it possible for several consumers to > enable/disable the same GPIO line to the left and right > without any consistency. The current use inside the regulator > core should however be fine as it takes special care to > handle this. > > For the state of the GPIO backend, this is still the > lesser evil compared to going back to global GPIO > numbers. > > Fixes: efdfeb079cc3 ("regulator: fixed: Convert to use GPIO descriptor only") > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > Cc: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> This also fixes a regression I was seeing on Tegra124 Jetson TK1. So ... Tested-by: Jon Hunter <jonathanh@nvidia.com> > --- > Hi Marek, it would be great if you could try this on top > of linux-next and report if it solves your problem on the > Samsung Exynos. > > If it does I hope it will apply cleanly on the regulator > tree. > --- > drivers/gpio/gpiolib.c | 19 +++++++++++++++++-- > drivers/regulator/fixed.c | 13 +++++++++++++ > include/linux/gpio/consumer.h | 1 + > 3 files changed, 31 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 7c222df8f834..f82a741ff428 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -4144,8 +4144,23 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev, > * the device name as label > */ > status = gpiod_request(desc, con_id ? con_id : devname); > - if (status < 0) > - return ERR_PTR(status); > + if (status < 0) { > + if (status == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE) { > + /* > + * This happens when there are several consumers for > + * the same GPIO line: we just return here without > + * further initialization. It is a bit if a hack. > + * This is necessary to support fixed regulators. > + * > + * FIXME: Make this more sane and safe. > + */ > + dev_info(dev, "nonexclusive access to GPIO for %s\n", > + con_id); Nit-pick, for me the 'con_id' is NULL so should we use the same ternary operator here as above to print devname instead in this case? Cheers! Jon -- nvpublic
On Thu, Oct 11, 2018 at 08:41:16PM +0200, Linus Walleij wrote: > On Thu, Oct 11, 2018 at 7:54 PM Mark Brown <broonie@kernel.org> wrote: > > Other regulators that have GPIO control for their enables do their own > > requests (as does the core) so don't they all need to set the flag > > GPIOD_FLAGS_BIT_NONEXCLUSIVE when they request otherwise it'll only work > > in systems where the regulators after the first that request the GPIO > > are fixed regulators? > Yeah ... I guess I assumed that the same enable line is not used > for a fixed regulator and then some other regulator (which is not > fixed). > If this is common practice we need to account for it. Obviously > I have no idea what is common practice :/ > Do we know some way to identify the affected systems or > should we just tag on GPIOD_FLAGS_BIT_NONEXCLUSIVE > on all the stuff we converted to descs to play it safe? > (I can cook a patch.) I think we need to set the flag everywhere, previously we'd essentially been doing it by asking the GPIO framework if we resolved the same regulator as one we already had.
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 7c222df8f834..f82a741ff428 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -4144,8 +4144,23 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev, * the device name as label */ status = gpiod_request(desc, con_id ? con_id : devname); - if (status < 0) - return ERR_PTR(status); + if (status < 0) { + if (status == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE) { + /* + * This happens when there are several consumers for + * the same GPIO line: we just return here without + * further initialization. It is a bit if a hack. + * This is necessary to support fixed regulators. + * + * FIXME: Make this more sane and safe. + */ + dev_info(dev, "nonexclusive access to GPIO for %s\n", + con_id); + return desc; + } else { + return ERR_PTR(status); + } + } status = gpiod_configure_flags(desc, con_id, lookupflags, flags); if (status < 0) { diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c index 7d639ad953b6..ccc29038f19a 100644 --- a/drivers/regulator/fixed.c +++ b/drivers/regulator/fixed.c @@ -170,6 +170,19 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev) gflags = GPIOD_OUT_LOW_OPEN_DRAIN; } + /* + * 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; + cfg.ena_gpiod = devm_gpiod_get_optional(&pdev->dev, NULL, gflags); if (IS_ERR(cfg.ena_gpiod)) return PTR_ERR(cfg.ena_gpiod); diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h index 0f350616d372..f2f887795d43 100644 --- a/include/linux/gpio/consumer.h +++ b/include/linux/gpio/consumer.h @@ -39,6 +39,7 @@ struct gpio_descs { #define GPIOD_FLAGS_BIT_DIR_OUT BIT(1) #define GPIOD_FLAGS_BIT_DIR_VAL BIT(2) #define GPIOD_FLAGS_BIT_OPEN_DRAIN BIT(3) +#define GPIOD_FLAGS_BIT_NONEXCLUSIVE BIT(4) /** * Optional flags that can be passed to one of gpiod_* to configure direction
This allows nonexclusive (simultaneous) access to a single GPIO line for the fixed regulator enable line. This happens when several regulators use the same GPIO for enabling and disabling a regulator, and all need a handle on their GPIO descriptor. This solution with a special flag is not entirely elegant and should ideally be replaced by something more careful as this makes it possible for several consumers to enable/disable the same GPIO line to the left and right without any consistency. The current use inside the regulator core should however be fine as it takes special care to handle this. For the state of the GPIO backend, this is still the lesser evil compared to going back to global GPIO numbers. Fixes: efdfeb079cc3 ("regulator: fixed: Convert to use GPIO descriptor only") Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> Cc: Marek Szyprowski <m.szyprowski@samsung.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- Hi Marek, it would be great if you could try this on top of linux-next and report if it solves your problem on the Samsung Exynos. If it does I hope it will apply cleanly on the regulator tree. --- drivers/gpio/gpiolib.c | 19 +++++++++++++++++-- drivers/regulator/fixed.c | 13 +++++++++++++ include/linux/gpio/consumer.h | 1 + 3 files changed, 31 insertions(+), 2 deletions(-) -- 2.17.1