Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1582573 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 1/4] pinctrl: Fix trivial spelling typo in a comment Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-02-16 14:30 +0100
    [PATCH v2 4/4] pinctrl: samsung: Use devres version of gpiochip_add_data Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-02-16 14:30 +0100
      Re: [PATCH v2 4/4] pinctrl: samsung: Use devres version of  gpiochip_add_data Krzysztof Kozlowski <krzk@kernel.org> - 2017-02-17 14:40 +0100
        Re: [PATCH v2 4/4] pinctrl: samsung: Use devres version of  gpiochip_add_data Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-02-17 15:00 +0100
    [PATCH v2 2/4] pinctrl: samsung: Register pinctrl before GPIO Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-02-16 14:30 +0100
      Re: [PATCH v2 2/4] pinctrl: samsung: Register pinctrl before GPIO Linus Walleij <linus.walleij@linaro.org> - 2017-02-22 15:40 +0100
        Re: [PATCH v2 2/4] pinctrl: samsung: Register pinctrl before GPIO Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-02-23 18:30 +0100
          [PATCH] pinctrl: samsung: Calculate GPIO base for pinctrl_add_gpio_range Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-02-23 19:00 +0100
            Re: [PATCH] pinctrl: samsung: Calculate GPIO base for pinctrl_add_gpio_range Tomasz Figa <tomasz.figa@gmail.com> - 2017-02-27 03:20 +0100
              Re: [PATCH] pinctrl: samsung: Calculate GPIO base for  pinctrl_add_gpio_range Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-02-28 10:10 +0100
                [PATCH v2] pinctrl: samsung: Calculate GPIO base for pinctrl_add_gpio_range Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-02-28 18:30 +0100
                  Re: [PATCH v2] pinctrl: samsung: Calculate GPIO base for pinctrl_add_gpio_range Tomasz Figa <tomasz.figa@gmail.com> - 2017-03-04 12:30 +0100
                    Re: [PATCH v2] pinctrl: samsung: Calculate GPIO base for  pinctrl_add_gpio_range Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-03-06 17:50 +0100

#1582573 — [PATCH v2 1/4] pinctrl: Fix trivial spelling typo in a comment

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

No changes since v1.

 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

[toc] | [next] | [standalone]

#1582574 — [PATCH v2 4/4] pinctrl: samsung: Use devres version of gpiochip_add_data

Use devm_gpiochip_add_data to simplify the error path in
samsung_gpiolib_register. Additionally this would also fix a leak if
the pinctrl driver was unbound, although admittedly I can't see any
good use-case for doing so, but the driver does currently allow it.

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

Changes since v1:
 - Update commit message.

 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 4ab20ec..eb08f30 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -934,20 +934,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

[toc] | [prev] | [next] | [standalone]

#1583412 — Re: [PATCH v2 4/4] pinctrl: samsung: Use devres version of gpiochip_add_data

On Thu, Feb 16, 2017 at 01:27:16PM +0000, Charles Keepax wrote:
> Use devm_gpiochip_add_data to simplify the error path in
> samsung_gpiolib_register. Additionally this would also fix a leak if
> the pinctrl driver was unbound, although admittedly I can't see any
> good use-case for doing so, but the driver does currently allow it.

Driver does not allow unbinding (.suppress_bind_attrs = true)...

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

Best regards,
Krzysztof

[toc] | [prev] | [next] | [standalone]

#1583420 — Re: [PATCH v2 4/4] pinctrl: samsung: Use devres version of gpiochip_add_data

On Fri, Feb 17, 2017 at 03:35:04PM +0200, Krzysztof Kozlowski wrote:
> On Thu, Feb 16, 2017 at 01:27:16PM +0000, Charles Keepax wrote:
> > Use devm_gpiochip_add_data to simplify the error path in
> > samsung_gpiolib_register. Additionally this would also fix a leak if
> > the pinctrl driver was unbound, although admittedly I can't see any
> > good use-case for doing so, but the driver does currently allow it.
> 
> Driver does not allow unbinding (.suppress_bind_attrs = true)...
> 

Oops... sorry missed that.

Thanks,
Charles

[toc] | [prev] | [next] | [standalone]

#1582578 — [PATCH v2 2/4] pinctrl: samsung: Register pinctrl before GPIO

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. Although currently there are no users of GPIO hogs in
mainline there are plenty of Samsung based boards that are widely used
for development purposes of other hardware. Indeed we hit this issue
whilst attaching some additional hardware to an Arndale system.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
---

Changes since v1:
 - Updated commit message

 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

[toc] | [prev] | [next] | [standalone]

#1586206 — Re: [PATCH v2 2/4] pinctrl: samsung: Register pinctrl before GPIO

On Thu, Feb 16, 2017 at 2:27 PM, Charles Keepax
<ckeepax@opensource.wolfsonmicro.com> 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. Although currently there are no users of GPIO hogs in
> mainline there are plenty of Samsung based boards that are widely used
> for development purposes of other hardware. Indeed we hit this issue
> whilst attaching some additional hardware to an Arndale system.
>
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>
> Changes since v1:
>  - Updated commit message

Patch applied. Will not be visible in -next until after the merge window
though.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1587041 — Re: [PATCH v2 2/4] pinctrl: samsung: Register pinctrl before GPIO

On Wed, Feb 22, 2017 at 03:37:37PM +0100, Linus Walleij wrote:
> On Thu, Feb 16, 2017 at 2:27 PM, Charles Keepax
> <ckeepax@opensource.wolfsonmicro.com> 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. Although currently there are no users of GPIO hogs in
> > mainline there are plenty of Samsung based boards that are widely used
> > for development purposes of other hardware. Indeed we hit this issue
> > whilst attaching some additional hardware to an Arndale system.
> >
> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> > ---
> >
> > Changes since v1:
> >  - Updated commit message
> 
> Patch applied. Will not be visible in -next until after the merge window
> though.
> 

Apologies but I think you might be best to drop this patch for
now, it seems this causes the ranges passed to
pinctrl_add_gpio_range to have the wrong .base, whilst I can
actually see no ill effects from this on Arndale. I suspect this
patch may be only part of the solution and may potentially cause
issues for others even though it seems fine for me.

It does seem to there is some circular dependancy issues between
setting up the pinctrl and setting up the gpio which crops up in
the case of gpio hogs that may be more fundamental.

Thanks,
Charles

[toc] | [prev] | [next] | [standalone]

#1587057 — [PATCH] pinctrl: samsung: Calculate GPIO base for pinctrl_add_gpio_range

As the pinctrl is now added before the GPIOs are registered we need to
manually calculate what the GPIO base will be, otherwise the base for
each gpio_range will be set to zero. Fortunately the driver
already assigns a GPIO base, in samsung_gpiolib_register, and uses the
same calculation it does for the pin_base. Meaning the two will always
be the same and allowing us to reuse the pinbase and avoid the issue.

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

Ok I might have spoken to soon there looks like there is a simple
way to fix this up, at least in this case. It would be much more
of an issue if the driver allocated its GPIO base dynamically.

Thanks,
Charles

 drivers/pinctrl/samsung/pinctrl-samsung.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index ddc8d6b..864d8b4d 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -882,7 +882,7 @@ static int samsung_pinctrl_register(struct platform_device *pdev,
 		pin_bank->grange.id = bank;
 		pin_bank->grange.pin_base = drvdata->pin_base
 						+ pin_bank->pin_base;
-		pin_bank->grange.base = pin_bank->gpio_chip.base;
+		pin_bank->grange.base = pin_bank->grange.pin_base;
 		pin_bank->grange.npins = pin_bank->gpio_chip.ngpio;
 		pin_bank->grange.gc = &pin_bank->gpio_chip;
 		pinctrl_add_gpio_range(drvdata->pctl_dev, &pin_bank->grange);
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1588486 — Re: [PATCH] pinctrl: samsung: Calculate GPIO base for pinctrl_add_gpio_range

Hi Charles,

2017-02-24 2:54 GMT+09:00 Charles Keepax <ckeepax@opensource.wolfsonmicro.com>:
> As the pinctrl is now added before the GPIOs are registered we need to
> manually calculate what the GPIO base will be, otherwise the base for
> each gpio_range will be set to zero. Fortunately the driver
> already assigns a GPIO base, in samsung_gpiolib_register, and uses the
> same calculation it does for the pin_base. Meaning the two will always
> be the same and allowing us to reuse the pinbase and avoid the issue.

Please see my comment inline.

>
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>
> Ok I might have spoken to soon there looks like there is a simple
> way to fix this up, at least in this case. It would be much more
> of an issue if the driver allocated its GPIO base dynamically.
>
> Thanks,
> Charles
>
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index ddc8d6b..864d8b4d 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -882,7 +882,7 @@ static int samsung_pinctrl_register(struct platform_device *pdev,
>                 pin_bank->grange.id = bank;
>                 pin_bank->grange.pin_base = drvdata->pin_base
>                                                 + pin_bank->pin_base;
> -               pin_bank->grange.base = pin_bank->gpio_chip.base;
> +               pin_bank->grange.base = pin_bank->grange.pin_base;

If we are not reading the base from the GPIO bank anymore, maybe it
could make sense to actually make samsung_gpiolib_register() use
bank->grange.base as gc->base? This way we would avoid explicitly
numbering two times.

Best regards,
Tomasz

[toc] | [prev] | [next] | [standalone]

#1589301 — Re: [PATCH] pinctrl: samsung: Calculate GPIO base for pinctrl_add_gpio_range

On Mon, Feb 27, 2017 at 11:13:59AM +0900, Tomasz Figa wrote:
> > diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> > index ddc8d6b..864d8b4d 100644
> > --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> > +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> > @@ -882,7 +882,7 @@ static int samsung_pinctrl_register(struct platform_device *pdev,
> >                 pin_bank->grange.id = bank;
> >                 pin_bank->grange.pin_base = drvdata->pin_base
> >                                                 + pin_bank->pin_base;
> > -               pin_bank->grange.base = pin_bank->gpio_chip.base;
> > +               pin_bank->grange.base = pin_bank->grange.pin_base;
> 
> If we are not reading the base from the GPIO bank anymore, maybe it
> could make sense to actually make samsung_gpiolib_register() use
> bank->grange.base as gc->base? This way we would avoid explicitly
> numbering two times.
> 

Yeah no problem I can respin with that included.

Thanks,
Charles

[toc] | [prev] | [next] | [standalone]

#1589644 — [PATCH v2] pinctrl: samsung: Calculate GPIO base for pinctrl_add_gpio_range

As the pinctrl is now added before the GPIOs are registered we need to
manually calculate what the GPIO base will be, otherwise the base for
each gpio_range will be set to zero. Fortunately the driver
already assigns a GPIO base, in samsung_gpiolib_register, and uses the
same calculation it does for the pin_base. Meaning the two will always
be the same and allowing us to reuse the pinbase and avoid the issue.

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

Changes since v1:
 - Use grange.base in samsung_gpiolib_register to make it more
   clear the two are related in the driver.

Thanks,
Charles


 drivers/pinctrl/samsung/pinctrl-samsung.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index ddc8d6b..27d5157 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -882,7 +882,7 @@ static int samsung_pinctrl_register(struct platform_device *pdev,
 		pin_bank->grange.id = bank;
 		pin_bank->grange.pin_base = drvdata->pin_base
 						+ pin_bank->pin_base;
-		pin_bank->grange.base = pin_bank->gpio_chip.base;
+		pin_bank->grange.base = pin_bank->grange.pin_base;
 		pin_bank->grange.npins = pin_bank->gpio_chip.ngpio;
 		pin_bank->grange.gc = &pin_bank->gpio_chip;
 		pinctrl_add_gpio_range(drvdata->pctl_dev, &pin_bank->grange);
@@ -928,7 +928,7 @@ static int samsung_gpiolib_register(struct platform_device *pdev,
 		bank->gpio_chip = samsung_gpiolib_chip;
 
 		gc = &bank->gpio_chip;
-		gc->base = drvdata->pin_base + bank->pin_base;
+		gc->base = bank->grange.base;
 		gc->ngpio = bank->nr_pins;
 		gc->parent = &pdev->dev;
 		gc->of_node = bank->of_node;
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1592461 — Re: [PATCH v2] pinctrl: samsung: Calculate GPIO base for pinctrl_add_gpio_range

Hi Charles,

2017-03-01 2:04 GMT+09:00 Charles Keepax <ckeepax@opensource.wolfsonmicro.com>:
> As the pinctrl is now added before the GPIOs are registered we need to
> manually calculate what the GPIO base will be, otherwise the base for
> each gpio_range will be set to zero. Fortunately the driver
> already assigns a GPIO base, in samsung_gpiolib_register, and uses the
> same calculation it does for the pin_base. Meaning the two will always
> be the same and allowing us to reuse the pinbase and avoid the issue.

Sorry, I didn't notice before and I don't see the offending patch in ,
but you should add

Fixes: XXXXXXXXXXXX ("pinctrl: Patch subject")

if you intend to submit this patch separately. Otherwise, maybe this
can be just squashed?

>
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>
> Changes since v1:
>  - Use grange.base in samsung_gpiolib_register to make it more
>    clear the two are related in the driver.

Other than the above:

Acked-by: Tomasz Figa <tomasz.figa@gmail.com>

Best regards,
Tomasz

[toc] | [prev] | [next] | [standalone]

#1593532 — Re: [PATCH v2] pinctrl: samsung: Calculate GPIO base for pinctrl_add_gpio_range

On Sat, Mar 04, 2017 at 08:20:11PM +0900, Tomasz Figa wrote:
> Hi Charles,
> 
> 2017-03-01 2:04 GMT+09:00 Charles Keepax <ckeepax@opensource.wolfsonmicro.com>:
> > As the pinctrl is now added before the GPIOs are registered we need to
> > manually calculate what the GPIO base will be, otherwise the base for
> > each gpio_range will be set to zero. Fortunately the driver
> > already assigns a GPIO base, in samsung_gpiolib_register, and uses the
> > same calculation it does for the pin_base. Meaning the two will always
> > be the same and allowing us to reuse the pinbase and avoid the issue.
> 
> Sorry, I didn't notice before and I don't see the offending patch in ,
> but you should add
> 
> Fixes: XXXXXXXXXXXX ("pinctrl: Patch subject")
> 
> if you intend to submit this patch separately. Otherwise, maybe this
> can be just squashed?
> 

Yeah apologies for that as the original patch hasn't showed up in
the tree yet I couldn't pull a commit ID to add the fixes tag.
Squashing it in is probably the best way to go.

Thanks,
Charles

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web