[PATCH] gpio: gpio-dwapb: add optional reset

[PATCH] gpio: gpio-dwapb: add optional reset

[Alan Tull]

Some platforms require reset to be released to allow register
access.

Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: Request shared controller
    return all errors from reset controller request
    drop conditions on asserting/deasserting reset
---
 drivers/gpio/gpio-dwapb.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index f051c45..0df8cdf 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -25,6 +25,7 @@
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
+#include <linux/reset.h>
 #include <linux/spinlock.h>
 #include <linux/platform_data/gpio-dwapb.h>
 #include <linux/slab.h>
@@ -97,6 +98,7 @@ struct dwapb_gpio {
 	unsigned int		nr_ports;
 	struct irq_domain	*domain;
 	unsigned int		flags;
+	struct reset_control	*rst;
 };
 
 static inline u32 gpio_reg_v2_convert(unsigned int offset)
@@ -608,6 +610,12 @@ static int dwapb_gpio_probe(struct platform_device *pdev)
 	gpio->dev = &pdev->dev;
 	gpio->nr_ports = pdata->nports;
 
+	gpio->rst = devm_reset_control_get_optional_shared(dev, NULL);
+	if (IS_ERR(gpio->rst))
+		return PTR_ERR(gpio->rst);
+
+	reset_control_deassert(gpio->rst);
+
 	gpio->ports = devm_kcalloc(&pdev->dev, gpio->nr_ports,
 				   sizeof(*gpio->ports), GFP_KERNEL);
 	if (!gpio->ports)
@@ -659,6 +667,7 @@ static int dwapb_gpio_remove(struct platform_device *pdev)
 
 	dwapb_gpio_unregister(gpio);
 	dwapb_irq_teardown(gpio);
+	reset_control_assert(gpio->rst);
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH] gpio: gpio-dwapb: add optional reset

[Alan Tull]

On Wed, Oct 11, 2017 at 11:34 AM, Alan Tull <atull@kernel.org> wrote:

Sorry, I forgot to specify PATCH v2 in the subject.

Alan

> Some platforms require reset to be released to allow register
> access.
>
> Signed-off-by: Alan Tull <atull@kernel.org>
> ---
> v2: Request shared controller
>     return all errors from reset controller request
>     drop conditions on asserting/deasserting reset
> ---
>  drivers/gpio/gpio-dwapb.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index f051c45..0df8cdf 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -25,6 +25,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/platform_device.h>
>  #include <linux/property.h>
> +#include <linux/reset.h>
>  #include <linux/spinlock.h>
>  #include <linux/platform_data/gpio-dwapb.h>
>  #include <linux/slab.h>
> @@ -97,6 +98,7 @@ struct dwapb_gpio {
>         unsigned int            nr_ports;
>         struct irq_domain       *domain;
>         unsigned int            flags;
> +       struct reset_control    *rst;
>  };
>
>  static inline u32 gpio_reg_v2_convert(unsigned int offset)
> @@ -608,6 +610,12 @@ static int dwapb_gpio_probe(struct platform_device *pdev)
>         gpio->dev = &pdev->dev;
>         gpio->nr_ports = pdata->nports;
>
> +       gpio->rst = devm_reset_control_get_optional_shared(dev, NULL);
> +       if (IS_ERR(gpio->rst))
> +               return PTR_ERR(gpio->rst);
> +
> +       reset_control_deassert(gpio->rst);
> +
>         gpio->ports = devm_kcalloc(&pdev->dev, gpio->nr_ports,
>                                    sizeof(*gpio->ports), GFP_KERNEL);
>         if (!gpio->ports)
> @@ -659,6 +667,7 @@ static int dwapb_gpio_remove(struct platform_device *pdev)
>
>         dwapb_gpio_unregister(gpio);
>         dwapb_irq_teardown(gpio);
> +       reset_control_assert(gpio->rst);
>
>         return 0;
>  }
> --
> 2.7.4
>

Re: [PATCH] gpio: gpio-dwapb: add optional reset

[Philipp Zabel]

On Wed, 2017-10-11 at 11:34 -0500, Alan Tull wrote:
> Some platforms require reset to be released to allow register
> access.
> 
> Signed-off-by: Alan Tull <atull@kernel.org>
> ---
> v2: Request shared controller
>     return all errors from reset controller request
>     drop conditions on asserting/deasserting reset

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

Could you also update the device tree binding docs [1] to mention the
optional resets phandle?

[1] Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt

regards
Philipp

> ---
>  drivers/gpio/gpio-dwapb.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index f051c45..0df8cdf 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -25,6 +25,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/platform_device.h>
>  #include <linux/property.h>
> +#include <linux/reset.h>
>  #include <linux/spinlock.h>
>  #include <linux/platform_data/gpio-dwapb.h>
>  #include <linux/slab.h>
> @@ -97,6 +98,7 @@ struct dwapb_gpio {
>  	unsigned int		nr_ports;
>  	struct irq_domain	*domain;
>  	unsigned int		flags;
> +	struct reset_control	*rst;
>  };
>  
>  static inline u32 gpio_reg_v2_convert(unsigned int offset)
> @@ -608,6 +610,12 @@ static int dwapb_gpio_probe(struct platform_device *pdev)
>  	gpio->dev = &pdev->dev;
>  	gpio->nr_ports = pdata->nports;
>  
> +	gpio->rst = devm_reset_control_get_optional_shared(dev, NULL);
> +	if (IS_ERR(gpio->rst))
> +		return PTR_ERR(gpio->rst);
> +
> +	reset_control_deassert(gpio->rst);
> +
>  	gpio->ports = devm_kcalloc(&pdev->dev, gpio->nr_ports,
>  				   sizeof(*gpio->ports), GFP_KERNEL);
>  	if (!gpio->ports)
> @@ -659,6 +667,7 @@ static int dwapb_gpio_remove(struct platform_device *pdev)
>  
>  	dwapb_gpio_unregister(gpio);
>  	dwapb_irq_teardown(gpio);
> +	reset_control_assert(gpio->rst);
>  
>  	return 0;
>  }

Re: [PATCH] gpio: gpio-dwapb: add optional reset

[Linus Walleij]

On Wed, Oct 11, 2017 at 6:34 PM, Alan Tull <atull@kernel.org> wrote:

> Some platforms require reset to be released to allow register
> access.
>
> Signed-off-by: Alan Tull <atull@kernel.org>
> ---
> v2: Request shared controller
>     return all errors from reset controller request
>     drop conditions on asserting/deasserting reset

This v2 version applied with Philipp's review tag, I also added
a blurb to the DT binding as Philipp asked so no need to fix that
separately.

Yours,
Linus Walleij

Re: [PATCH] gpio: gpio-dwapb: add optional reset

[Alan Tull]

On Thu, Oct 12, 2017 at 3:26 AM, Linus Walleij <linus.walleij@linaro.org> wrote:

Hi Linus, Phillipp,

> This v2 version applied with Philipp's review tag, I also added
> a blurb to the DT binding as Philipp asked so no need to fix that
> separately.
>
> Yours,
> Linus Walleij

Thanks!

Alan Tull

[PATCH] gpio: gpio-dwapb: add optional reset

[Alan Tull]

Some platforms require reset to be released to allow register
access.

Signed-off-by: Alan Tull <atull@kernel.org>
---
 drivers/gpio/gpio-dwapb.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 5cdb7bc..f7ac625 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -25,6 +25,7 @@
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
+#include <linux/reset.h>
 #include <linux/spinlock.h>
 #include <linux/platform_data/gpio-dwapb.h>
 #include <linux/slab.h>
@@ -98,6 +99,7 @@ struct dwapb_gpio {
 	unsigned int		nr_ports;
 	struct irq_domain	*domain;
 	unsigned int		flags;
+	struct reset_control	*rst;
 };
 
 static inline u32 gpio_reg_v2_convert(unsigned int offset)
@@ -612,6 +614,7 @@ static int dwapb_gpio_probe(struct platform_device *pdev)
 	int err;
 	struct device *dev = &pdev->dev;
 	struct dwapb_platform_data *pdata = dev_get_platdata(dev);
+	struct reset_control *rst;
 
 	if (!pdata) {
 		pdata = dwapb_gpio_get_pdata(dev);
@@ -629,6 +632,15 @@ static int dwapb_gpio_probe(struct platform_device *pdev)
 	gpio->dev = &pdev->dev;
 	gpio->nr_ports = pdata->nports;
 
+	rst = devm_reset_control_get_optional_exclusive(dev, NULL);
+	if (IS_ERR(rst)) {
+		if (PTR_ERR(rst) == -EPROBE_DEFER)
+			return PTR_ERR(rst);
+	} else {
+		reset_control_deassert(rst);
+		gpio->rst = rst;
+	}
+
 	gpio->ports = devm_kcalloc(&pdev->dev, gpio->nr_ports,
 				   sizeof(*gpio->ports), GFP_KERNEL);
 	if (!gpio->ports)
@@ -680,6 +692,8 @@ static int dwapb_gpio_remove(struct platform_device *pdev)
 
 	dwapb_gpio_unregister(gpio);
 	dwapb_irq_teardown(gpio);
+	if (gpio->rst)
+		reset_control_assert(gpio->rst);
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH] gpio: gpio-dwapb: add optional reset

[Linus Walleij]

On Tue, Oct 10, 2017 at 11:26 PM, Alan Tull <atull@kernel.org> wrote:

> Some platforms require reset to be released to allow register
> access.
>
> Signed-off-by: Alan Tull <atull@kernel.org>

Fair enough.

(...)
> +       rst = devm_reset_control_get_optional_exclusive(dev, NULL);
> +       if (IS_ERR(rst)) {
> +               if (PTR_ERR(rst) == -EPROBE_DEFER)
> +                       return PTR_ERR(rst);
> +       } else {
> +               reset_control_deassert(rst);
> +               gpio->rst = rst;
> +       }

I do not see why any error other than -EPROBE_DEFER
should be ignored?

I guess the _optional API returns NULL if there is no
reset line so it should be fine to just return the error on
any error.

> +       if (gpio->rst)
> +               reset_control_assert(gpio->rst);

Is this the right way to handle an optional reset line?

Yours,
Linus Walleij

Re: [PATCH] gpio: gpio-dwapb: add optional reset

[Philipp Zabel]

Hi Alan, Linus,

On Wed, 2017-10-11 at 10:31 +0200, Linus Walleij wrote:
> On Tue, Oct 10, 2017 at 11:26 PM, Alan Tull <atull@kernel.org> wrote:
> 
> > Some platforms require reset to be released to allow register
> > access.
> > 
> > Signed-off-by: Alan Tull <atull@kernel.org>
> 
> Fair enough.
> 
> (...)
> > +       rst = devm_reset_control_get_optional_exclusive(dev, NULL);

The way this reset control is used, it looks like you could use _shared
instead of _exclusive here. This relaxes the guarantees made by the API
a bit and may allow this driver to work with more reset controllers.

> > +       if (IS_ERR(rst)) {
> > +               if (PTR_ERR(rst) == -EPROBE_DEFER)
> > +                       return PTR_ERR(rst);

The _optional variant of reset_control_get returns NULL if no reset is
specified in the device tree. If an error value is returned, it is
always an actual error (invalid device tree contents, reset is specified
in the device tree but the driver returns an error, etc.).
This should just be:

	if (IS_ERR(rst))
		return PTR_ERR(rst);

> > +       } else {
> > +               reset_control_deassert(rst);
> > +               gpio->rst = rst;

And this should be made unconditional. reset_control_deassert just
ignores rst == NULL.

> > +       }
> 
> I do not see why any error other than -EPROBE_DEFER
> should be ignored?
> 
> I guess the _optional API returns NULL if there is no
> reset line so it should be fine to just return the error on
> any error.

Correct. The _optional API together with NULL reset control handles
allows to simplify handling of optional resets in the consumer drivers.

> > +       if (gpio->rst)
> > +               reset_control_assert(gpio->rst);
> 
> Is this the right way to handle an optional reset line?

Just as the deassert above, this should be made unconditional.

regards
Philipp

Re: [PATCH] gpio: gpio-dwapb: add optional reset

[Alan Tull]

On Wed, Oct 11, 2017 at 5:56 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:

Hi Linus, Phillipp,

Thanks for the review.

> Hi Alan, Linus,
>
> On Wed, 2017-10-11 at 10:31 +0200, Linus Walleij wrote:
>> On Tue, Oct 10, 2017 at 11:26 PM, Alan Tull <atull@kernel.org> wrote:
>>
>> > Some platforms require reset to be released to allow register
>> > access.
>> >
>> > Signed-off-by: Alan Tull <atull@kernel.org>
>>
>> Fair enough.
>>
>> (...)
>> > +       rst = devm_reset_control_get_optional_exclusive(dev, NULL);
>
> The way this reset control is used, it looks like you could use _shared
> instead of _exclusive here. This relaxes the guarantees made by the API
> a bit and may allow this driver to work with more reset controllers.

OK, will use devm_reset_control_get_optional_shared().

>
>> > +       if (IS_ERR(rst)) {
>> > +               if (PTR_ERR(rst) == -EPROBE_DEFER)
>> > +                       return PTR_ERR(rst);
>
> The _optional variant of reset_control_get returns NULL if no reset is
> specified in the device tree. If an error value is returned, it is
> always an actual error (invalid device tree contents, reset is specified
> in the device tree but the driver returns an error, etc.).
> This should just be:
>
>         if (IS_ERR(rst))
>                 return PTR_ERR(rst);

That's great!

>
>> > +       } else {
>> > +               reset_control_deassert(rst);
>> > +               gpio->rst = rst;
>
> And this should be made unconditional. reset_control_deassert just
> ignores rst == NULL.

Nice.

>
>> > +       }
>>
>> I do not see why any error other than -EPROBE_DEFER
>> should be ignored?
>>
>> I guess the _optional API returns NULL if there is no
>> reset line so it should be fine to just return the error on
>> any error.
>
> Correct. The _optional API together with NULL reset control handles
> allows to simplify handling of optional resets in the consumer drivers.
>
>> > +       if (gpio->rst)
>> > +               reset_control_assert(gpio->rst);
>>
>> Is this the right way to handle an optional reset line?
>
> Just as the deassert above, this should be made unconditional.

I've made the requested changes which shrinks the patch to be even smaller :)

Will send up v2 soon.

Thanks,
Alan

>
> regards
> Philipp