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

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

[Charles Keepax]

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

[PATCH v2 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. 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

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

[Charles Keepax]

The local variable drvdata is not used in samsung_gpio_set_direction.

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

Changes since v1:
 - Update commit message

 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 1134bc3..4ab20ec 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

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

[Charles Keepax]

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

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

[Krzysztof Kozlowski]

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

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

[Charles Keepax]

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

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

[Linus Walleij]

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

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

[Linus Walleij]

On Thu, Feb 16, 2017 at 2:27 PM, Charles Keepax
<ckeepax@opensource.wolfsonmicro.com> wrote:

> The local variable drvdata is not used in samsung_gpio_set_direction.
>
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>
> Changes since v1:
>  - Update commit message


Patch applied. for v4.12.

Yours,
Linus Walleij

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

[Charles Keepax]

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

[PATCH] pinctrl: samsung: Calculate GPIO base for pinctrl_add_gpio_range

[Charles Keepax]

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

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

[Tomasz Figa]

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

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

[Charles Keepax]

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

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

[Charles Keepax]

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

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

[Tomasz Figa]

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

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

[Charles Keepax]

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

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

[Linus Walleij]

On Thu, Feb 23, 2017 at 6:23 PM, Charles Keepax
<ckeepax@opensource.wolfsonmicro.com> wrote:
> 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.

OK I dropped it.

Resend it if you figure it out.

The Samsung driver traffic is increasing, and there are now
a bunch of developers starting to step on each others' toes.

Would you Samsung developers start considering someone who
can collect Samsung pin control patches and send them as
pull requests to me?

Yours,
Linus Walleij

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

[Krzysztof Kozlowski]

On Tue, Mar 14, 2017 at 3:44 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Feb 23, 2017 at 6:23 PM, Charles Keepax
> <ckeepax@opensource.wolfsonmicro.com> wrote:
>> 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.
>
> OK I dropped it.
>
> Resend it if you figure it out.
>
> The Samsung driver traffic is increasing, and there are now
> a bunch of developers starting to step on each others' toes.
>
> Would you Samsung developers start considering someone who
> can collect Samsung pin control patches and send them as
> pull requests to me?

For some reason, some days ago I had impression that Sylwester is
doing this already... but I was corrected that you are taking them
directly. If Tomasz and Sylwester are willing to do this, they got my
Ack.

If not, I can take it and set a separate repo on my kernel.org account.

Best regards,
Krzysztof

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

[Linus Walleij]

On Tue, Mar 14, 2017 at 2:58 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Tue, Mar 14, 2017 at 3:44 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Thu, Feb 23, 2017 at 6:23 PM, Charles Keepax
>> <ckeepax@opensource.wolfsonmicro.com> wrote:
>>> 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.
>>
>> OK I dropped it.
>>
>> Resend it if you figure it out.
>>
>> The Samsung driver traffic is increasing, and there are now
>> a bunch of developers starting to step on each others' toes.
>>
>> Would you Samsung developers start considering someone who
>> can collect Samsung pin control patches and send them as
>> pull requests to me?
>
> For some reason, some days ago I had impression that Sylwester is
> doing this already... but I was corrected that you are taking them
> directly. If Tomasz and Sylwester are willing to do this, they got my
> Ack.
>
> If not, I can take it and set a separate repo on my kernel.org account.

That would be great, if there will be a lot of Samsung pin control work
for this kernel cycle.

Yours,
Linus Walleij

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

[Krzysztof Kozlowski]

On Mon, Mar 06, 2017 at 04:49:09PM +0000, Charles Keepax wrote:
> 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.

Hi Charles,

Thanks for the work.

This is a follow up of:
https://patchwork.kernel.org/patch/9577147/
Right?

None of these two were applied so can you squash them, rebase, retest
and send again?

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On Fri, Feb 17, 2017 at 01:52:14PM +0000, Charles Keepax wrote:
> 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.

Can you resend with updated commit msg? I think it was not picked up by
Linus yet.

Best regards,
Krzysztof

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

[Charles Keepax]

On Mon, Mar 20, 2017 at 08:39:15PM +0200, Krzysztof Kozlowski wrote:
> On Mon, Mar 06, 2017 at 04:49:09PM +0000, Charles Keepax wrote:
> > 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.
> 
> Hi Charles,
> 
> Thanks for the work.
> 
> This is a follow up of:
> https://patchwork.kernel.org/patch/9577147/
> Right?
> 
> None of these two were applied so can you squash them, rebase, retest
> and send again?
> 

Yeah no problem should be able to resend later today hopefully.

Thanks,
Charles

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

[Linus Walleij]

On Mon, Mar 20, 2017 at 7:44 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Fri, Feb 17, 2017 at 01:52:14PM +0000, Charles Keepax wrote:
>> 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.
>
> Can you resend with updated commit msg? I think it was not picked up by
> Linus yet.

I'm expecting you to pick it up and send to me by pull request now I guess,
or did we agree that a Samsung patches wouldn't be too voluminous this
cycle?

If you're OK with it, can we proceed to use you as Samsung patch
collection point for this kernel cycle from this moment on?

Yours,
Linus Walleij

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

[Krzysztof Kozlowski]

On Thu, Mar 23, 2017 at 11:47 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Mon, Mar 20, 2017 at 7:44 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On Fri, Feb 17, 2017 at 01:52:14PM +0000, Charles Keepax wrote:
> >> 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.
> >
> > Can you resend with updated commit msg? I think it was not picked up by
> > Linus yet.
>
> I'm expecting you to pick it up and send to me by pull request now I guess,
> or did we agree that a Samsung patches wouldn't be too voluminous this
> cycle?
>
> If you're OK with it, can we proceed to use you as Samsung patch
> collection point for this kernel cycle from this moment on?

I will take it, especially that more things are coming. I already
applied patch from Chanwoo which should go to fixes (initially I put
it on wrong branch).

Best regards,
Krzysztof