[PATCH 1/4] pinctrl: Fix trivial spelling typo in a comment

[PATCH 1/4] pinctrl: Fix trivial spelling typo in a comment

[Charles Keepax]

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/pinctrl/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index d690465..0bf6392f 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -525,7 +525,7 @@ pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev,
 EXPORT_SYMBOL_GPL(pinctrl_find_gpio_range_from_pin);
 
 /**
- * pinctrl_remove_gpio_range() - remove a range of GPIOs fro a pin controller
+ * pinctrl_remove_gpio_range() - remove a range of GPIOs from a pin controller
  * @pctldev: pin controller device to remove the range from
  * @range: the GPIO range to remove
  */
-- 
2.1.4

[PATCH 2/4] pinctrl: samsung: Register pinctrl before GPIO

[Charles Keepax]

If we request a GPIO hog, then gpiochip_add_data will attempt to request
some of its own GPIOs. The driver also uses gpiochip_generic_request
which means that for any GPIO request to succeed the pinctrl needs to be
registered. Currently however the driver registers the GPIO and then the
pinctrl meaning all GPIO hog requests will fail, which then in turn causes
the whole driver to fail probe. Fix this up by ensuring we register the
pinctrl first.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/pinctrl/samsung/pinctrl-samsung.c | 32 +++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index d79eada..1134bc3 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -893,6 +893,19 @@ static int samsung_pinctrl_register(struct platform_device *pdev,
 	return 0;
 }
 
+/* unregister the pinctrl interface with the pinctrl subsystem */
+static int samsung_pinctrl_unregister(struct platform_device *pdev,
+				      struct samsung_pinctrl_drv_data *drvdata)
+{
+	struct samsung_pin_bank *bank = drvdata->pin_banks;
+	int i;
+
+	for (i = 0; i < drvdata->nr_banks; ++i, ++bank)
+		pinctrl_remove_gpio_range(drvdata->pctl_dev, &bank->grange);
+
+	return 0;
+}
+
 static const struct gpio_chip samsung_gpiolib_chip = {
 	.request = gpiochip_generic_request,
 	.free = gpiochip_generic_free,
@@ -939,19 +952,6 @@ static int samsung_gpiolib_register(struct platform_device *pdev,
 	return ret;
 }
 
-/* unregister the gpiolib interface with the gpiolib subsystem */
-static int samsung_gpiolib_unregister(struct platform_device *pdev,
-				      struct samsung_pinctrl_drv_data *drvdata)
-{
-	struct samsung_pin_bank *bank = drvdata->pin_banks;
-	int i;
-
-	for (i = 0; i < drvdata->nr_banks; ++i, ++bank)
-		gpiochip_remove(&bank->gpio_chip);
-
-	return 0;
-}
-
 /* retrieve the soc specific data */
 static const struct samsung_pin_ctrl *
 samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d,
@@ -1063,13 +1063,13 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
 			return PTR_ERR(drvdata->retention_ctrl);
 	}
 
-	ret = samsung_gpiolib_register(pdev, drvdata);
+	ret = samsung_pinctrl_register(pdev, drvdata);
 	if (ret)
 		return ret;
 
-	ret = samsung_pinctrl_register(pdev, drvdata);
+	ret = samsung_gpiolib_register(pdev, drvdata);
 	if (ret) {
-		samsung_gpiolib_unregister(pdev, drvdata);
+		samsung_pinctrl_unregister(pdev, drvdata);
 		return ret;
 	}
 
-- 
2.1.4

[PATCH 3/4] pinctrl: samsung: Use devres version of gpiochip_add_data

[Charles Keepax]

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/pinctrl/samsung/pinctrl-samsung.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index 1134bc3..f291cbf 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -936,20 +936,15 @@ static int samsung_gpiolib_register(struct platform_device *pdev,
 		gc->of_node = bank->of_node;
 		gc->label = bank->name;
 
-		ret = gpiochip_add_data(gc, bank);
+		ret = devm_gpiochip_add_data(&pdev->dev, gc, bank);
 		if (ret) {
 			dev_err(&pdev->dev, "failed to register gpio_chip %s, error code: %d\n",
 							gc->label, ret);
-			goto fail;
+			return ret;
 		}
 	}
 
 	return 0;
-
-fail:
-	for (--i, --bank; i >= 0; --i, --bank)
-		gpiochip_remove(&bank->gpio_chip);
-	return ret;
 }
 
 /* retrieve the soc specific data */
-- 
2.1.4

[PATCH 4/4] pinctrl: samsung: Remove unused local variable

[Charles Keepax]

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/pinctrl/samsung/pinctrl-samsung.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index f291cbf..eb08f30 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -566,13 +566,11 @@ static int samsung_gpio_set_direction(struct gpio_chip *gc,
 {
 	const struct samsung_pin_bank_type *type;
 	struct samsung_pin_bank *bank;
-	struct samsung_pinctrl_drv_data *drvdata;
 	void __iomem *reg;
 	u32 data, mask, shift;
 
 	bank = gpiochip_get_data(gc);
 	type = bank->type;
-	drvdata = bank->drvdata;
 
 	reg = bank->pctl_base + bank->pctl_offset
 			+ type->reg_offset[PINCFG_TYPE_FUNC];
-- 
2.1.4

Re: [PATCH 2/4] pinctrl: samsung: Register pinctrl before GPIO

[Krzysztof Kozlowski]

On Mon, Feb 13, 2017 at 10:11:04AM +0000, Charles Keepax wrote:
> If we request a GPIO hog, then gpiochip_add_data will attempt to request
> some of its own GPIOs. The driver also uses gpiochip_generic_request
> which means that for any GPIO request to succeed the pinctrl needs to be
> registered. Currently however the driver registers the GPIO and then the
> pinctrl meaning all GPIO hog requests will fail, which then in turn causes
> the whole driver to fail probe. Fix this up by ensuring we register the
> pinctrl first.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 32 +++++++++++++++----------------

I think the code makes sense altough the description describes
theoretical issue - we do not use GPIO hogs. Of course out of tree DTS
could use them... so maybe mention that this is not an existing case?

For the code itself:
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof


>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index d79eada..1134bc3 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -893,6 +893,19 @@ static int samsung_pinctrl_register(struct platform_device *pdev,
>  	return 0;
>  }
>  

Re: [PATCH 3/4] pinctrl: samsung: Use devres version of gpiochip_add_data

[Krzysztof Kozlowski]

On Mon, Feb 13, 2017 at 10:11:05AM +0000, Charles Keepax wrote:
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>

Empty? Please say WHY we want to use devres version of
gpiochip_add_data (e.g. for simplification of error paths).

Best regards,
Krzysztof


> ---
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index 1134bc3..f291cbf 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -936,20 +936,15 @@ static int samsung_gpiolib_register(struct platform_device *pdev,
>  		gc->of_node = bank->of_node;
>  		gc->label = bank->name;
>  
> -		ret = gpiochip_add_data(gc, bank);
> +		ret = devm_gpiochip_add_data(&pdev->dev, gc, bank);
>  		if (ret) {
>  			dev_err(&pdev->dev, "failed to register gpio_chip %s, error code: %d\n",
>  							gc->label, ret);
> -			goto fail;
> +			return ret;
>  		}
>  	}
>  
>  	return 0;
> -
> -fail:
> -	for (--i, --bank; i >= 0; --i, --bank)
> -		gpiochip_remove(&bank->gpio_chip);
> -	return ret;
>  }
>  
>  /* retrieve the soc specific data */
> -- 
> 2.1.4
> 

Re: [PATCH 4/4] pinctrl: samsung: Remove unused local variable

[Krzysztof Kozlowski]

On Mon, Feb 13, 2017 at 10:11:06AM +0000, Charles Keepax wrote:
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>

"The local variable drvdata is not used in samsung_gpio_set_direction()"?

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

> ---
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index f291cbf..eb08f30 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -566,13 +566,11 @@ static int samsung_gpio_set_direction(struct gpio_chip *gc,
>  {
>  	const struct samsung_pin_bank_type *type;
>  	struct samsung_pin_bank *bank;
> -	struct samsung_pinctrl_drv_data *drvdata;
>  	void __iomem *reg;
>  	u32 data, mask, shift;
>  
>  	bank = gpiochip_get_data(gc);
>  	type = bank->type;
> -	drvdata = bank->drvdata;
>  
>  	reg = bank->pctl_base + bank->pctl_offset
>  			+ type->reg_offset[PINCFG_TYPE_FUNC];
> -- 
> 2.1.4
> 

Re: [PATCH 4/4] pinctrl: samsung: Remove unused local variable

[Charles Keepax]

On Wed, Feb 15, 2017 at 07:43:03PM +0200, Krzysztof Kozlowski wrote:
> On Mon, Feb 13, 2017 at 10:11:06AM +0000, Charles Keepax wrote:
> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> 
> "The local variable drvdata is not used in samsung_gpio_set_direction()"?
> 
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> 

Thanks, will update the commit messages with your
recommendations and resend.

Thanks,
Charles

Re: [PATCH 1/4] pinctrl: Fix trivial spelling typo in a comment

[Linus Walleij]

On Mon, Feb 13, 2017 at 11:11 AM, Charles Keepax
<ckeepax@opensource.wolfsonmicro.com> wrote:

> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>

Patch applied.

Yours,
Linus Walleij