| Message ID | 20181128104350.31902-1-linus.walleij@linaro.org |
|---|---|
| Headers | show |
| Series | Regulator ena_gpiod fixups | expand |
On Wed, Nov 28, 2018 at 11:43:40AM +0100, Linus Walleij wrote: > drivers/gpio/gpiolib.h | 6 ----- > drivers/regulator/da9211-regulator.c | 4 +-- > drivers/regulator/fixed.c | 4 ++- > drivers/regulator/lm363x-regulator.c | 8 ++++-- > drivers/regulator/lp8788-ldo.c | 4 ++- > drivers/regulator/max77686-regulator.c | 3 +-- > drivers/regulator/max8952.c | 8 +++--- > drivers/regulator/max8973-regulator.c | 12 ++++++--- > drivers/regulator/s5m8767.c | 37 ++++++++++++++++++-------- > drivers/regulator/tps65090-regulator.c | 10 +++---- > include/linux/gpio/consumer.h | 13 +++++++++ > 11 files changed, 72 insertions(+), 37 deletions(-) It looks like the patches are assuming the regulator core, doesn't free the GPIO on an error, however that is not true in all cases. If only a single regulator has requested the GPIO then all the error paths after the call to regulator_ena_gpio_request in regulator_register will free the GPIO. Although this is not the case if more than one regulator has requested the GPIO. Thanks, charles
On Wed, Nov 28, 2018 at 4:22 PM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > It looks like the patches are assuming the regulator core, > doesn't free the GPIO on an error, however that is not true in > all cases. If only a single regulator has requested the GPIO then > all the error paths after the call to regulator_ena_gpio_request > in regulator_register will free the GPIO. I guess part of it is that I should make sure not to gpiod_put() if the [devm_]regulator_register() fails, I will go over the series with that in mind! Essentially the semantic is that the [devm_]regulator_register() call will immediately take ownership of the descriptor and place it in the regulator core. I'll check! > Although this is not the > case if more than one regulator has requested the GPIO. This should be fine since the regulator core refcounts it, when all the other regulators drops it, gpiod_put() will be called as the refcount goes down to 0. Yours, Linus Walleij
On Thu, Nov 29, 2018 at 04:52:04PM +0100, Linus Walleij wrote: > Essentially the semantic is that the [devm_]regulator_register() > call will immediately take ownership of the descriptor > and place it in the regulator core. I think that's going to be the easiest thing to work with, it's more pain in the regulator core but simpler for all the users which is probably a good balance.
śr., 28 lis 2018 o 11:43 Linus Walleij <linus.walleij@linaro.org> napisał(a): > > We noticed a refcounting fight between the kernel device > core devm_* and the regulator core refcounting, pertaining > to enable GPIOd:s that may be shared between multiple > regulators. > > Fix this with a series that hand it all over to the > regulator core and remove any devm_* stuff pertaining > to these GPIOs. > > This includes a patch to gpiolib to make gpiod_get_from_node() > available. Just go ahead and apply this to the regulator > tree. > > If these need to go for fixes or not is up to the > regulator maintainer, but all commits have a proper > Fixes: tag in case they would. I have noted in the > four last commits that the gpiolib patch is a > prerequisite. > > Linus Walleij (10): > regulator: fixed: Let core handle GPIO descriptor > regulator: lm363x: Let core handle GPIO descriptor > regulator: lp8788-ldo: Let core handle GPIO descriptor > regulator: max8952: Let core handle GPIO descriptor > regulator: max8973: Let core handle GPIO descriptor > gpio: Export gpiod_get_from_of_node() > regulator: da9211: Let core handle GPIO descriptors > regulator: max77686: Let core handle GPIO descriptor > regulator: s5m8767: Let core handle GPIO descriptors > regulator: tps65090: Let core handle GPIO descriptors > > drivers/gpio/gpiolib.h | 6 ----- > drivers/regulator/da9211-regulator.c | 4 +-- > drivers/regulator/fixed.c | 4 ++- > drivers/regulator/lm363x-regulator.c | 8 ++++-- > drivers/regulator/lp8788-ldo.c | 4 ++- > drivers/regulator/max77686-regulator.c | 3 +-- > drivers/regulator/max8952.c | 8 +++--- > drivers/regulator/max8973-regulator.c | 12 ++++++--- > drivers/regulator/s5m8767.c | 37 ++++++++++++++++++-------- > drivers/regulator/tps65090-regulator.c | 10 +++---- > include/linux/gpio/consumer.h | 13 +++++++++ > 11 files changed, 72 insertions(+), 37 deletions(-) > > -- > 2.19.1 > I'm wondering if instead of using the non-devm variants of gpiod_get_*() routines, we shouldn't provide helpers in the regulator framework that would be named accordingly, for example: regulator_gpiod_get_optional() etc. even if all they do is call the relevant gpiolib function. Those helpers could then be documented as passing the control over GPIO lines over to the regulator subsystem. The reason for that is that most driver developers will automatically use devm functions whenever available and having a single non-devm function without any comment used in a driver normally using devres looks like a bug. Expect people sending "fixes" in a couple months. Bart
On Thu, Nov 29, 2018 at 07:38:20PM +0100, Bartosz Golaszewski wrote: > I'm wondering if instead of using the non-devm variants of > gpiod_get_*() routines, we shouldn't provide helpers in the regulator > framework that would be named accordingly, for example: > regulator_gpiod_get_optional() etc. even if all they do is call the > relevant gpiolib function. Those helpers could then be documented as > passing the control over GPIO lines over to the regulator subsystem. > The reason for that is that most driver developers will automatically > use devm functions whenever available and having a single non-devm > function without any comment used in a driver normally using devres > looks like a bug. Expect people sending "fixes" in a couple months. I predict that people would then immediately start demanding devm_ variants of that function...
czw., 29 lis 2018 o 20:01 Mark Brown <broonie@kernel.org> napisał(a): > > On Thu, Nov 29, 2018 at 07:38:20PM +0100, Bartosz Golaszewski wrote: > > > I'm wondering if instead of using the non-devm variants of > > gpiod_get_*() routines, we shouldn't provide helpers in the regulator > > framework that would be named accordingly, for example: > > regulator_gpiod_get_optional() etc. even if all they do is call the > > relevant gpiolib function. Those helpers could then be documented as > > passing the control over GPIO lines over to the regulator subsystem. > > > The reason for that is that most driver developers will automatically > > use devm functions whenever available and having a single non-devm > > function without any comment used in a driver normally using devres > > looks like a bug. Expect people sending "fixes" in a couple months. > > I predict that people would then immediately start demanding devm_ > variants of that function... At least we could document it in the code. If I wouldn't know about the reason for not using devm and saw a stray gpiod_get() without a corresponding put() I'd probably send a patch to fix it, but if I saw something like regulator_gpiod_get(), I'd look at what this routine does. Matter of taste I guess, but I'd prefer the latter. At the very least we could add a comment to each call saying that the regulator framework will take care of that resource. Bart
On Fri, Nov 30, 2018 at 9:35 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> At least we could document it in the code.
I've put some comments in my patch set, I will check if I can add some
more in the less-than-obvious spots.
Yours,
Linus Walleij