| Message ID | 20181012125412.21324-1-linus.walleij@linaro.org |
|---|---|
| State | Accepted |
| Commit | b0ce7b29bfcd090ddba476f45a75ec0a797b048a |
| Headers | show |
| Series | [v2] regulator/gpio: Allow nonexclusive GPIO access | expand |
Hi Linus, + Laurent, as he reviewed most of that driver code Sorry, I'm going slightly OT with this, but please read below. On Fri, Oct 12, 2018 at 02:54:12PM +0200, 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. > I might have a use case for shared GPIO lines used as 'simple' reset signal for camera devices and not for regulators. See drivers/media/i2c/ov772x.c FIXME note in power_on() function at: https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov772x.c#L832 The reset line is in this case is passed to the driver by board file: https://elixir.bootlin.com/linux/latest/source/arch/sh/boards/mach-migor/setup.c#L350 As you can see PTT3 is used for both sensors (I know, questionable HW design...) Do you think extending gpiod_lookup_flags with this newly introduced NONEXCLUSIVE one is an acceptable solution to avoid handling this in the sensor driver like we're doing today? Please note this is an ancient architecture that boots from board files, but the same might happen in modern designs with OF support. Is there any clean way to handle shared GPIOs I might not be aware of for those systems? Thanks j > Cc: Marek Szyprowski <m.szyprowski@samsung.com> > Cc: Jon Hunter <jonathanh@nvidia.com> > Fixes: efdfeb079cc3 ("regulator: fixed: Convert to use GPIO descriptor only") > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > Tested-by: Jon Hunter <jonathanh@nvidia.com> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > ChangeLog v1->v2: > - Fix the print string to use ternary operator and alternative > text. > - Collect Tested-by's from affected systems. > - Mark: I tested to apply this on the regulator tree pulled > in my for-next branches for GPIO and pin control on top and > it seems to work! Could you apply it? > --- > 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..56178af4ecd9 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 ? con_id : devname); > + 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 > -- > 2.17.2 > >
On Fri, Oct 12, 2018 at 04:26:12PM +0200, jacopo mondi wrote: > Sorry, I'm going slightly OT with this, but please read below. > On Fri, Oct 12, 2018 at 02:54:12PM +0200, Linus Walleij wrote: > > This allows nonexclusive (simultaneous) access to a single > > GPIO line for the fixed regulator enable line. This happens > I might have a use case for shared GPIO lines used as 'simple' reset > signal for camera devices and not for regulators. This recently came up in ASoC too with audio CODECs sharing reset lines, there was a discussion started with the reset API maintainer though no response yet. CCing in Cheng-yi who had that problem. Not deleting context for that. > See drivers/media/i2c/ov772x.c FIXME note in power_on() function at: > https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov772x.c#L832 > > The reset line is in this case is passed to the driver by board file: > https://elixir.bootlin.com/linux/latest/source/arch/sh/boards/mach-migor/setup.c#L350 > > As you can see PTT3 is used for both sensors (I know, questionable > HW design...) > > Do you think extending gpiod_lookup_flags with this newly introduced > NONEXCLUSIVE one is an acceptable solution to avoid handling this in > the sensor driver like we're doing today? One problem you've got there is that you need the two devices to know about each other so they coordinate their use of the reset line. This was relatively easy in the sysetm Cheng-yi has as it's just one driver but what if there's mutiple drivers? That's relatively likely with audio where you might have something like a CODEC with a separate power amplifier. The regulator enable stuff is handling this in the core but it's less clear where to put that for reset lines. > Please note this is an ancient architecture that boots from board > files, but the same might happen in modern designs with OF support. Is > there any clean way to handle shared GPIOs I might not be aware of for > those systems?
Hi Mark, On Fri, Oct 12, 2018 at 06:44:24PM +0200, Mark Brown wrote: > On Fri, Oct 12, 2018 at 04:26:12PM +0200, jacopo mondi wrote: > > > Sorry, I'm going slightly OT with this, but please read below. > > > On Fri, Oct 12, 2018 at 02:54:12PM +0200, Linus Walleij wrote: > > > This allows nonexclusive (simultaneous) access to a single > > > GPIO line for the fixed regulator enable line. This happens > > > I might have a use case for shared GPIO lines used as 'simple' reset > > signal for camera devices and not for regulators. > > This recently came up in ASoC too with audio CODECs sharing reset lines, > there was a discussion started with the reset API maintainer though no > response yet. CCing in Cheng-yi who had that problem. Not deleting > context for that. > Thanks > > See drivers/media/i2c/ov772x.c FIXME note in power_on() function at: > > https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov772x.c#L832 > > > > The reset line is in this case is passed to the driver by board file: > > https://elixir.bootlin.com/linux/latest/source/arch/sh/boards/mach-migor/setup.c#L350 > > > > As you can see PTT3 is used for both sensors (I know, questionable > > HW design...) > > > > Do you think extending gpiod_lookup_flags with this newly introduced > > NONEXCLUSIVE one is an acceptable solution to avoid handling this in > > the sensor driver like we're doing today? > > One problem you've got there is that you need the two devices to know > about each other so they coordinate their use of the reset line. This That's exactly why the current implementation in those drivers is not even sub-optimal :) > was relatively easy in the sysetm Cheng-yi has as it's just one driver > but what if there's mutiple drivers? That's relatively likely with > audio where you might have something like a CODEC with a separate power > amplifier. The regulator enable stuff is handling this in the core but > it's less clear where to put that for reset lines. > Isn't DT the natural place where to define a reset line (or any kind of shared GPIO line) as shared? And for non-OF archs, the board file? Maybe I should start by adding the NONEXCLUSIVE flags to the ones accepted by gpio lookup tables and see how it looks? Thanks j > > Please note this is an ancient architecture that boots from board > > files, but the same might happen in modern designs with OF support. Is > > there any clean way to handle shared GPIOs I might not be aware of for > > those systems?
On Fri, Oct 12, 2018 at 6:44 PM Mark Brown <broonie@kernel.org> wrote: > On Fri, Oct 12, 2018 at 04:26:12PM +0200, jacopo mondi wrote: > > Do you think extending gpiod_lookup_flags with this newly introduced > > NONEXCLUSIVE one is an acceptable solution to avoid handling this in > > the sensor driver like we're doing today? > > One problem you've got there is that you need the two devices to know > about each other so they coordinate their use of the reset line. This > was relatively easy in the sysetm Cheng-yi has as it's just one driver > but what if there's mutiple drivers? That's relatively likely with > audio where you might have something like a CODEC with a separate power > amplifier. The regulator enable stuff is handling this in the core but > it's less clear where to put that for reset lines. Yes spot on. What we need to do for that to work is to move the reference counting for shared lines over to gpiolib as well, else every subsystem that needs to share a GPIO line essentially has to reimplement it. If the consumers are in different subsystems they would even have to share a reference count and this would lead to a big mess. So I was thinking to pull that over to gpiolib for the next kernel :) Hopefully I could do that without breaking anything, but I don't have a good track record on that so I think the fine people who saw this breakage will have to help me out. Yours, Linus Walleij
Hello, On Friday, 12 October 2018 19:44:24 EEST Mark Brown wrote: > On Fri, Oct 12, 2018 at 04:26:12PM +0200, jacopo mondi wrote: > > Sorry, I'm going slightly OT with this, but please read below. > > > > On Fri, Oct 12, 2018 at 02:54:12PM +0200, Linus Walleij wrote: > > > This allows nonexclusive (simultaneous) access to a single > > > GPIO line for the fixed regulator enable line. This happens > > > > I might have a use case for shared GPIO lines used as 'simple' reset > > signal for camera devices and not for regulators. > > This recently came up in ASoC too with audio CODECs sharing reset lines, > there was a discussion started with the reset API maintainer though no > response yet. CCing in Cheng-yi who had that problem. Not deleting > context for that. > > > See drivers/media/i2c/ov772x.c FIXME note in power_on() function at: > > https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov772x.c# > > L832 > > > > The reset line is in this case is passed to the driver by board file: > > https://elixir.bootlin.com/linux/latest/source/arch/sh/boards/mach-migor/s > > etup.c#L350 > > > > As you can see PTT3 is used for both sensors (I know, questionable > > HW design...) > > > > Do you think extending gpiod_lookup_flags with this newly introduced > > NONEXCLUSIVE one is an acceptable solution to avoid handling this in > > the sensor driver like we're doing today? > > One problem you've got there is that you need the two devices to know > about each other so they coordinate their use of the reset line. This > was relatively easy in the sysetm Cheng-yi has as it's just one driver > but what if there's mutiple drivers? That's relatively likely with > audio where you might have something like a CODEC with a separate power > amplifier. The regulator enable stuff is handling this in the core but > it's less clear where to put that for reset lines. I've seen other boards where two components sharing a reset signal have an active low reset for one, and an active high reset for the other one. Only one of the two can be out of reset at a time. That's probably considered as "clever" by the hardware engineers, but is awful to support for us. The core issue in my opinion is that we need code to handle this, and since the removal of board files there is no place anymore for such code. Board drivers exist in drivers/staging/board/, but that's hardly a solution moving forward (the TODO file explicitly states that removal of that code is the end goal). > > Please note this is an ancient architecture that boots from board > > files, but the same might happen in modern designs with OF support. Is > > there any clean way to handle shared GPIOs I might not be aware of for > > those systems? -- Regards, Laurent Pinchart
On Tue, Oct 16, 2018 at 1:08 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > I've seen other boards where two components sharing a reset signal have an > active low reset for one, and an active high reset for the other one. Only one > of the two can be out of reset at a time. That's probably considered as > "clever" by the hardware engineers, but is awful to support for us. Haha what a feat. If/when we run into that we simply invent a new flag like GPIOD_ACTIVE_AMBIGUOUS. > The core issue in my opinion is that we need code to handle this, and since > the removal of board files there is no place anymore for such code. Board > drivers exist in drivers/staging/board/, but that's hardly a solution moving > forward (the TODO file explicitly states that removal of that code is the end > goal). Yeah I kind of already concluded that I need to pull the multiple user reference counting out of the regulator core and take it over to the GPIO subsystem so I'm going to attempt that for the next kernel cycle. It's a neat feature anyways. Yours, Linus Walleij
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 7c222df8f834..56178af4ecd9 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 ? con_id : devname); + 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. Cc: Marek Szyprowski <m.szyprowski@samsung.com> Cc: Jon Hunter <jonathanh@nvidia.com> Fixes: efdfeb079cc3 ("regulator: fixed: Convert to use GPIO descriptor only") Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> Tested-by: Jon Hunter <jonathanh@nvidia.com> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- ChangeLog v1->v2: - Fix the print string to use ternary operator and alternative text. - Collect Tested-by's from affected systems. - Mark: I tested to apply this on the regulator tree pulled in my for-next branches for GPIO and pin control on top and it seems to work! Could you apply it? --- 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.2