[PATCH 1/7] gpio: rcar: Add helper variable dev = &pdev->dev

[PATCH 1/7] gpio: rcar: Add helper variable dev = &pdev->dev

[Geert Uytterhoeven]

From: Geert Uytterhoeven <geert+renesas@glider.be>

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: linux-gpio@vger.kernel.org
---
 drivers/gpio/gpio-rcar.c |   32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
index ca76ce751540..03c91482432c 100644
--- a/drivers/gpio/gpio-rcar.c
+++ b/drivers/gpio/gpio-rcar.c
@@ -356,12 +356,13 @@ static int gpio_rcar_probe(struct platform_device *pdev)
 	struct resource *io, *irq;
 	struct gpio_chip *gpio_chip;
 	struct irq_chip *irq_chip;
-	const char *name = dev_name(&pdev->dev);
+	struct device *dev = &pdev->dev;
+	const char *name = dev_name(dev);
 	int ret;
 
-	p = devm_kzalloc(&pdev->dev, sizeof(*p), GFP_KERNEL);
+	p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
 	if (!p) {
-		dev_err(&pdev->dev, "failed to allocate driver data\n");
+		dev_err(dev, "failed to allocate driver data\n");
 		ret = -ENOMEM;
 		goto err0;
 	}
@@ -380,15 +381,14 @@ static int gpio_rcar_probe(struct platform_device *pdev)
 	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 
 	if (!io || !irq) {
-		dev_err(&pdev->dev, "missing IRQ or IOMEM\n");
+		dev_err(dev, "missing IRQ or IOMEM\n");
 		ret = -EINVAL;
 		goto err0;
 	}
 
-	p->base = devm_ioremap_nocache(&pdev->dev, io->start,
-				       resource_size(io));
+	p->base = devm_ioremap_nocache(dev, io->start, resource_size(io));
 	if (!p->base) {
-		dev_err(&pdev->dev, "failed to remap I/O memory\n");
+		dev_err(dev, "failed to remap I/O memory\n");
 		ret = -ENXIO;
 		goto err0;
 	}
@@ -402,7 +402,7 @@ static int gpio_rcar_probe(struct platform_device *pdev)
 	gpio_chip->set = gpio_rcar_set;
 	gpio_chip->to_irq = gpio_rcar_to_irq;
 	gpio_chip->label = name;
-	gpio_chip->dev = &pdev->dev;
+	gpio_chip->dev = dev;
 	gpio_chip->owner = THIS_MODULE;
 	gpio_chip->base = p->config.gpio_base;
 	gpio_chip->ngpio = p->config.number_of_pins;
@@ -421,30 +421,30 @@ static int gpio_rcar_probe(struct platform_device *pdev)
 					      &gpio_rcar_irq_domain_ops, p);
 	if (!p->irq_domain) {
 		ret = -ENXIO;
-		dev_err(&pdev->dev, "cannot initialize irq domain\n");
+		dev_err(dev, "cannot initialize irq domain\n");
 		goto err0;
 	}
 
-	if (devm_request_irq(&pdev->dev, irq->start,
-			     gpio_rcar_irq_handler, IRQF_SHARED, name, p)) {
-		dev_err(&pdev->dev, "failed to request IRQ\n");
+	if (devm_request_irq(dev, irq->start, gpio_rcar_irq_handler,
+			     IRQF_SHARED, name, p)) {
+		dev_err(dev, "failed to request IRQ\n");
 		ret = -ENOENT;
 		goto err1;
 	}
 
 	ret = gpiochip_add(gpio_chip);
 	if (ret) {
-		dev_err(&pdev->dev, "failed to add GPIO controller\n");
+		dev_err(dev, "failed to add GPIO controller\n");
 		goto err1;
 	}
 
-	dev_info(&pdev->dev, "driving %d GPIOs\n", p->config.number_of_pins);
+	dev_info(dev, "driving %d GPIOs\n", p->config.number_of_pins);
 
 	/* warn in case of mismatch if irq base is specified */
 	if (p->config.irq_base) {
 		ret = irq_find_mapping(p->irq_domain, 0);
 		if (p->config.irq_base != ret)
-			dev_warn(&pdev->dev, "irq base mismatch (%u/%u)\n",
+			dev_warn(dev, "irq base mismatch (%u/%u)\n",
 				 p->config.irq_base, ret);
 	}
 
@@ -452,7 +452,7 @@ static int gpio_rcar_probe(struct platform_device *pdev)
 		ret = gpiochip_add_pin_range(gpio_chip, p->config.pctl_name, 0,
 					     gpio_chip->base, gpio_chip->ngpio);
 		if (ret < 0)
-			dev_warn(&pdev->dev, "failed to add pin range\n");
+			dev_warn(dev, "failed to add pin range\n");
 	}
 
 	return 0;
-- 
1.7.9.5

Re: [PATCH 1/7] gpio: rcar: Add helper variable dev = &pdev->dev

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Thursday 27 March 2014 21:47:36 Geert Uytterhoeven wrote:
> From: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: linux-gpio@vger.kernel.org

I'm not sure if I would have bothered, but I have nothing against this :-)

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpio/gpio-rcar.c |   32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
> index ca76ce751540..03c91482432c 100644
> --- a/drivers/gpio/gpio-rcar.c
> +++ b/drivers/gpio/gpio-rcar.c
> @@ -356,12 +356,13 @@ static int gpio_rcar_probe(struct platform_device
> *pdev) struct resource *io, *irq;
>  	struct gpio_chip *gpio_chip;
>  	struct irq_chip *irq_chip;
> -	const char *name = dev_name(&pdev->dev);
> +	struct device *dev = &pdev->dev;
> +	const char *name = dev_name(dev);
>  	int ret;
> 
> -	p = devm_kzalloc(&pdev->dev, sizeof(*p), GFP_KERNEL);
> +	p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
>  	if (!p) {
> -		dev_err(&pdev->dev, "failed to allocate driver data\n");
> +		dev_err(dev, "failed to allocate driver data\n");
>  		ret = -ENOMEM;
>  		goto err0;
>  	}
> @@ -380,15 +381,14 @@ static int gpio_rcar_probe(struct platform_device
> *pdev) irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> 
>  	if (!io || !irq) {
> -		dev_err(&pdev->dev, "missing IRQ or IOMEM\n");
> +		dev_err(dev, "missing IRQ or IOMEM\n");
>  		ret = -EINVAL;
>  		goto err0;
>  	}
> 
> -	p->base = devm_ioremap_nocache(&pdev->dev, io->start,
> -				       resource_size(io));
> +	p->base = devm_ioremap_nocache(dev, io->start, resource_size(io));
>  	if (!p->base) {
> -		dev_err(&pdev->dev, "failed to remap I/O memory\n");
> +		dev_err(dev, "failed to remap I/O memory\n");
>  		ret = -ENXIO;
>  		goto err0;
>  	}
> @@ -402,7 +402,7 @@ static int gpio_rcar_probe(struct platform_device *pdev)
> gpio_chip->set = gpio_rcar_set;
>  	gpio_chip->to_irq = gpio_rcar_to_irq;
>  	gpio_chip->label = name;
> -	gpio_chip->dev = &pdev->dev;
> +	gpio_chip->dev = dev;
>  	gpio_chip->owner = THIS_MODULE;
>  	gpio_chip->base = p->config.gpio_base;
>  	gpio_chip->ngpio = p->config.number_of_pins;
> @@ -421,30 +421,30 @@ static int gpio_rcar_probe(struct platform_device
> *pdev) &gpio_rcar_irq_domain_ops, p);
>  	if (!p->irq_domain) {
>  		ret = -ENXIO;
> -		dev_err(&pdev->dev, "cannot initialize irq domain\n");
> +		dev_err(dev, "cannot initialize irq domain\n");
>  		goto err0;
>  	}
> 
> -	if (devm_request_irq(&pdev->dev, irq->start,
> -			     gpio_rcar_irq_handler, IRQF_SHARED, name, p)) {
> -		dev_err(&pdev->dev, "failed to request IRQ\n");
> +	if (devm_request_irq(dev, irq->start, gpio_rcar_irq_handler,
> +			     IRQF_SHARED, name, p)) {
> +		dev_err(dev, "failed to request IRQ\n");
>  		ret = -ENOENT;
>  		goto err1;
>  	}
> 
>  	ret = gpiochip_add(gpio_chip);
>  	if (ret) {
> -		dev_err(&pdev->dev, "failed to add GPIO controller\n");
> +		dev_err(dev, "failed to add GPIO controller\n");
>  		goto err1;
>  	}
> 
> -	dev_info(&pdev->dev, "driving %d GPIOs\n", p->config.number_of_pins);
> +	dev_info(dev, "driving %d GPIOs\n", p->config.number_of_pins);
> 
>  	/* warn in case of mismatch if irq base is specified */
>  	if (p->config.irq_base) {
>  		ret = irq_find_mapping(p->irq_domain, 0);
>  		if (p->config.irq_base != ret)
> -			dev_warn(&pdev->dev, "irq base mismatch (%u/%u)\n",
> +			dev_warn(dev, "irq base mismatch (%u/%u)\n",
>  				 p->config.irq_base, ret);
>  	}
> 
> @@ -452,7 +452,7 @@ static int gpio_rcar_probe(struct platform_device *pdev)
> ret = gpiochip_add_pin_range(gpio_chip, p->config.pctl_name, 0,
> gpio_chip->base, gpio_chip->ngpio);
>  		if (ret < 0)
> -			dev_warn(&pdev->dev, "failed to add pin range\n");
> +			dev_warn(dev, "failed to add pin range\n");
>  	}
> 
>  	return 0;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/7] gpio: rcar: Add helper variable dev = &pdev->dev

[Linus Walleij]

On Thu, Mar 27, 2014 at 9:47 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:

> From: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: linux-gpio@vger.kernel.org

Patch applied with Laurent's ACK.

Yours,
Linus Walleij

[PATCH 2/7] gpio: rcar: Add optional functional clock to bindings

[Geert Uytterhoeven]

From: Geert Uytterhoeven <geert+renesas@glider.be>

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: linux-gpio@vger.kernel.org
Cc: devicetree@vger.kernel.org
---
 .../devicetree/bindings/gpio/renesas,gpio-rcar.txt |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt b/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
index f61cef74a212..9ccbe50dcec2 100644
--- a/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
+++ b/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
@@ -21,6 +21,10 @@ Required Properties:
     GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported.
   - gpio-ranges: Range of pins managed by the GPIO controller.
 
+Optional properties:
+
+  - clocks: Must contain a reference to the functional clock.
+
 Please refer to gpio.txt in this directory for details of gpio-ranges property
 and the common GPIO bindings used by client devices.
 
-- 
1.7.9.5

Re: [PATCH 2/7] gpio: rcar: Add optional functional clock to bindings

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Thursday 27 March 2014 21:47:37 Geert Uytterhoeven wrote:
> From: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: linux-gpio@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> ---
>  .../devicetree/bindings/gpio/renesas,gpio-rcar.txt |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
> b/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt index
> f61cef74a212..9ccbe50dcec2 100644
> --- a/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
> +++ b/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
> @@ -21,6 +21,10 @@ Required Properties:
>      GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported.
>    - gpio-ranges: Range of pins managed by the GPIO controller.
> 
> +Optional properties:
> +
> +  - clocks: Must contain a reference to the functional clock.
> +

I would make the property mandatory. Obviously the driver needs to consider it 
as optional in order not to break the DT ABI, but the specification should 
make it mandatory in order to ensure that all future implementations will 
specify the clock.

>  Please refer to gpio.txt in this directory for details of gpio-ranges
> property and the common GPIO bindings used by client devices.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/7] gpio: rcar: Add optional functional clock to bindings

[Geert Uytterhoeven]

Hi Laurent,

On Fri, Mar 28, 2014 at 5:53 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> --- a/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
>> +++ b/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
>> @@ -21,6 +21,10 @@ Required Properties:
>>      GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported.
>>    - gpio-ranges: Range of pins managed by the GPIO controller.
>>
>> +Optional properties:
>> +
>> +  - clocks: Must contain a reference to the functional clock.
>> +
>
> I would make the property mandatory. Obviously the driver needs to consider it
> as optional in order not to break the DT ABI, but the specification should
> make it mandatory in order to ensure that all future implementations will
> specify the clock.

I think it has to stay optional: unless I misinterpreted the datasheet, r8a7778
doesn't have MSTP bits for the GPIO modules. I guess it's the same for
other R-Car Gen1 like r8a7779. So it looks like the bits were added in
R-Car Gen2.

Hence I'll just add ", if present", unless the Best Practice is to put such
properties under "Required properties" or "Recommended properties"?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/7] gpio: rcar: Add optional functional clock to bindings

[Laurent Pinchart]

Hi Geert,

On Monday 31 March 2014 11:55:52 Geert Uytterhoeven wrote:
> On Fri, Mar 28, 2014 at 5:53 PM, Laurent Pinchart wrote:
> >> --- a/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
> >> +++ b/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
> >> 
> >> @@ -21,6 +21,10 @@ Required Properties:
> >>      GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported.
> >>    - gpio-ranges: Range of pins managed by the GPIO controller.
> >> 
> >> +Optional properties:
> >> +
> >> +  - clocks: Must contain a reference to the functional clock.
> >> +
> > 
> > I would make the property mandatory. Obviously the driver needs to
> > consider it as optional in order not to break the DT ABI, but the
> > specification should make it mandatory in order to ensure that all future
> > implementations will specify the clock.
> 
> I think it has to stay optional: unless I misinterpreted the datasheet,
> r8a7778 doesn't have MSTP bits for the GPIO modules. I guess it's the same
> for other R-Car Gen1 like r8a7779. So it looks like the bits were added in
> R-Car Gen2.

Good point. It might also be that older SoCs have MSTP bits for the GPIO cores 
but don't document them. Anyway, we have no information, so we can't specify a 
clock.

> Hence I'll just add ", if present", unless the Best Practice is to put such
> properties under "Required properties" or "Recommended properties"?

I'm fine with keeping in under "Optional Properties". I would phrase it as 
"Must contain a reference to the functional clock. The property is mandatory 
if the hardware implements a controllable functional clock for the GPIO 
instance." (or something similar).

-- 
Regards,

Laurent Pinchart

[PATCH 3/7] gpio: rcar: Add minimal runtime PM support

[Geert Uytterhoeven]

From: Geert Uytterhoeven <geert+renesas@glider.be>

This is just enough to automatically enable the functional clock, if
present. Clock management during suspend/resume is still to be added.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: linux-gpio@vger.kernel.org
---
 drivers/gpio/gpio-rcar.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
index 03c91482432c..c6995f6c3c40 100644
--- a/drivers/gpio/gpio-rcar.c
+++ b/drivers/gpio/gpio-rcar.c
@@ -26,6 +26,7 @@
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_data/gpio-rcar.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/spinlock.h>
 #include <linux/slab.h>
 
@@ -377,6 +378,9 @@ static int gpio_rcar_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, p);
 
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
+
 	io = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 
@@ -460,6 +464,8 @@ static int gpio_rcar_probe(struct platform_device *pdev)
 err1:
 	irq_domain_remove(p->irq_domain);
 err0:
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
 	return ret;
 }
 
@@ -473,6 +479,8 @@ static int gpio_rcar_remove(struct platform_device *pdev)
 		return ret;
 
 	irq_domain_remove(p->irq_domain);
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 	return 0;
 }
 
-- 
1.7.9.5

Re: [PATCH 3/7] gpio: rcar: Add minimal runtime PM support

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Thursday 27 March 2014 21:47:38 Geert Uytterhoeven wrote:
> From: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> This is just enough to automatically enable the functional clock, if
> present. Clock management during suspend/resume is still to be added.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: linux-gpio@vger.kernel.org
> ---
>  drivers/gpio/gpio-rcar.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
> index 03c91482432c..c6995f6c3c40 100644
> --- a/drivers/gpio/gpio-rcar.c
> +++ b/drivers/gpio/gpio-rcar.c
> @@ -26,6 +26,7 @@
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/platform_data/gpio-rcar.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/spinlock.h>
>  #include <linux/slab.h>
> 
> @@ -377,6 +378,9 @@ static int gpio_rcar_probe(struct platform_device *pdev)
> 
>  	platform_set_drvdata(pdev, p);
> 
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);
> +
>  	io = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> 
> @@ -460,6 +464,8 @@ static int gpio_rcar_probe(struct platform_device *pdev)
> err1:
>  	irq_domain_remove(p->irq_domain);
>  err0:
> +	pm_runtime_put_sync(dev);

Do we really need the sync version, isn't pm_runtime_put() enough ?

> +	pm_runtime_disable(dev);
>  	return ret;
>  }
> 
> @@ -473,6 +479,8 @@ static int gpio_rcar_remove(struct platform_device
> *pdev) return ret;
> 
>  	irq_domain_remove(p->irq_domain);
> +	pm_runtime_put_sync(&pdev->dev);

Same comment here.

> +	pm_runtime_disable(&pdev->dev);
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/7] gpio: rcar: Add minimal runtime PM support

[Geert Uytterhoeven]

Hi Laurent,

On Fri, Mar 28, 2014 at 5:54 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> @@ -460,6 +464,8 @@ static int gpio_rcar_probe(struct platform_device *pdev)
>> err1:
>>       irq_domain_remove(p->irq_domain);
>>  err0:
>> +     pm_runtime_put_sync(dev);
>
> Do we really need the sync version, isn't pm_runtime_put() enough ?

No we don't, copy-and-paste from rcar-thermal (which I'll fix, too).

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds