diff mbox

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

Message ID 1487251636-2480-2-git-send-email-ckeepax@opensource.wolfsonmicro.com
State New
Headers show

Commit Message

Charles Keepax Feb. 16, 2017, 1:27 p.m. UTC 
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(-)

Comments

Linus Walleij Feb. 22, 2017, 2:37 p.m. UTC | #1 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Charles Keepax Feb. 23, 2017, 5:23 p.m. UTC | #2 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij March 14, 2017, 1:44 p.m. UTC | #3 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski March 14, 2017, 1:58 p.m. UTC | #4 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij March 16, 2017, 3:28 p.m. UTC | #5 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
 	}