gpio: gpio-dwapb: add optional reset

PATCH In review Alan Tull 3 participants 4 messages Oct 10, 2017 – Oct 11, 2017

Subsystems (2)

GPIO Synopsys DesignWare apb GPIO driver

lore

Share & tools

lore

Alan Tull

author

2017-10-10 21:26 UTC #0 lore raw

PATCH 1/1 gpio: gpio-dwapb: add optional reset

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

Signed-off-by Alan Tull <[email protected]>

---
 drivers/gpio/gpio-dwapb.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

drivers/gpio/gpio-dwapb.c +14 -0

index 5cdb7bc..f7ac625 100644
@@ -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;
 }

Signature · 1 line

2.7.4

Linus Walleij

2017-10-11 08:31 UTC #1 lore raw

Quoted 4 lines · Alan Tull

Some platforms require reset to be released to allow register access.
Signed-off-by Alan Tull <[email protected]>

Fair enough.

(...)

drivers/gpio/gpio-dwapb.c static int dwapb_gpio_probe(struct platform_device *pdev) · line 642

Quoted 8 lines

drivers/gpio/gpio-dwapb.c +8 -0

+       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.

drivers/gpio/gpio-dwapb.c static int dwapb_gpio_remove(struct platform_device *pdev) · line 696

Quoted 2 lines

drivers/gpio/gpio-dwapb.c +2 -0

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

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

Yours,
Linus Walleij

Philipp Zabel

2017-10-11 10:56 UTC #2 lore raw

Hi Alan, Linus,

Quoted 11 lines · Linus Walleij

Quoted 4 lines · Alan Tull
Some platforms require reset to be released to allow register access.
Signed-off-by Alan Tull <[email protected]>
Fair enough.
(...)
Quoted 1 line
+ 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.

Quoted 3 lines

drivers/gpio/gpio-dwapb.c static int dwapb_gpio_probe(struct platform_device *pdev) · line 638
Quoted 3 lines
drivers/gpio/gpio-dwapb.c +3 -0
+       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);

Quoted 3 lines

drivers/gpio/gpio-dwapb.c static int dwapb_gpio_probe(struct platform_device *pdev) · line 641
Quoted 3 lines
drivers/gpio/gpio-dwapb.c +3 -0
+       } else {
+               reset_control_deassert(rst);
+               gpio->rst = rst;

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

Quoted 8 lines

Quoted 1 line
+ }
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.

Quoted 4 lines

drivers/gpio/gpio-dwapb.c static int dwapb_gpio_remove(struct platform_device *pdev) · line 696
Quoted 2 lines
drivers/gpio/gpio-dwapb.c +2 -0
+       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

Alan Tull

author

2017-10-11 16:06 UTC #3 lore raw

On Wed, Oct 11, 2017 at 5:56 AM, Philipp Zabel <[email protected]> wrote:

Hi Linus, Phillipp,

Thanks for the review.

Quoted 10 lines

Hi Alan, Linus,
Quoted 7 lines · Linus Walleij
Quoted 4 lines · Alan Tull
Some platforms require reset to be released to allow register access.
Signed-off-by Alan Tull <[email protected]>

Quoted 8 lines

Quoted 4 lines
Fair enough.
(...)
Quoted 1 line
+ 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().

Quoted 5 lines

Quoted 4 lines
drivers/gpio/gpio-dwapb.c static int dwapb_gpio_probe(struct platform_device *pdev) · line 638
Quoted 4 lines
drivers/gpio/gpio-dwapb.c +3 -0
+       if (IS_ERR(rst)) {
+               if (PTR_ERR(rst) == -EPROBE_DEFER)
+                       return PTR_ERR(rst);

Quoted 8 lines

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!

Quoted 7 lines

Quoted 4 lines
drivers/gpio/gpio-dwapb.c static int dwapb_gpio_probe(struct platform_device *pdev) · line 641
Quoted 4 lines
drivers/gpio/gpio-dwapb.c +3 -0
+       } else {
+               reset_control_deassert(rst);
+               gpio->rst = rst;
And this should be made unconditional. reset_control_deassert just ignores rst == NULL.

Nice.

Quoted 13 lines

Quoted 9 lines
Quoted 2 lines
+ }
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.

Quoted 6 lines

Quoted 4 lines
drivers/gpio/gpio-dwapb.c static int dwapb_gpio_remove(struct platform_device *pdev) · line 696
Quoted 2 lines
drivers/gpio/gpio-dwapb.c +2 -0
+       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

Quoted 3 lines

regards
Philipp