Re: [PATCH] regulator: core: do not put managed GPIOd:s

[Charles Keepax]

On Thu, Nov 22, 2018 at 05:04:21PM +0100, Linus Walleij wrote:
> Some drivers have been converted to pass GPIO descriptors
> rather than GPIO numbers to the regulator core. We should
> not issue gpiod_put() on those descriptors, but rather
> let the driver reference count it with devm_* if they so
> desire.
> 
> Currently the regulator core issues gpiod_put() on all
> descriptors as it assumes it was obtained with the
> sequence gpio_request_one()/gpio_to_desc(), as
> gpio_request_one() will be equivalent to gpiod_get().
> 
> We introduce a helper bool that deal with this situation
> by making sure the core only issue gpiod_put() if the
> GPIO was requested from within the core itself.
> 
> A subsequent patch set will delete legacy GPIO handling
> and get rid of this ugliness, so it is a stepgap patch.
> 
> Fixes: e45e290a882e ("regulator: core: Support passing an initialized GPIO enable descriptor")
> Cc: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> Cc: Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>
> Reported-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> Reported-by: Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> Mark: I will rebase my v7 series for removing legacy
> GPIO on this if it solves Marek's probel, it can be applied
> for fixes if need be but I don't know how big this problem is.
> ---

Sent my patch chain anyway although the middle patch is basically
identical to this one, I really don't mind which one we use.

I think we also want to add some additional handling into the
GPIO core as well. Since whilst this patch will resolve the
situation for wm8994 here, it doesn't cover the case of actually
shared GPIOs. This patch means the drivers will still own their
GPIOs so if two drivers requested the same GPIO both will call a
gpiod_put for that GPIO, causing the same issue. In my chain I
add some very primitive reference counting to the GPIO core.

>  drivers/regulator/core.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 03a03763457c..05bb2db6cff5 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -80,6 +80,7 @@ struct regulator_map {
>  struct regulator_enable_gpio {
>  	struct list_head list;
>  	struct gpio_desc *gpiod;
> +	bool gpiod_needs_put;

I went with locally_requested here seemed better to be
descriptive of the situation rather than the required action,
also a bit flag at the end with the other bit flags.

>  	u32 enable_count;	/* a number of enabled shared GPIO */
>  	u32 request_count;	/* a number of requested shared GPIO */
>  	unsigned int ena_gpio_invert:1;
> @@ -2247,6 +2248,8 @@ static int regulator_ena_gpio_request(struct regulator_dev *rdev,
>  	}
>  
>  	pin->gpiod = gpiod;
> +	if (!config->ena_gpiod)
> +		pin->gpiod_needs_put = true;
>  	pin->ena_gpio_invert = config->ena_gpio_invert;
>  	list_add(&pin->list, &regulator_ena_gpio_list);
>  
> @@ -2268,7 +2271,8 @@ static void regulator_ena_gpio_free(struct regulator_dev *rdev)
>  		if (pin->gpiod == rdev->ena_pin->gpiod) {
>  			if (pin->request_count <= 1) {
>  				pin->request_count = 0;
> -				gpiod_put(pin->gpiod);
> +				if (pin->gpiod_needs_put)
> +					gpiod_put(pin->gpiod);
>  				list_del(&pin->list);
>  				kfree(pin);
>  				rdev->ena_pin = NULL;
> -- 
> 2.19.1

Thanks,
Charles