[PATCH 1/3] ARM: dts: Update board files for pwm support

[PATCH 1/3] ARM: dts: Update board files for pwm support

[Tony Prisk]

This patch adds pwm support to arch-vt8500 board files, and adds
the use-case of pwm-backlight.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
---
 arch/arm/boot/dts/vt8500-bv07.dts |    8 ++++++++
 arch/arm/boot/dts/vt8500.dtsi     |   29 +++++++++++++++++++++++++++++
 arch/arm/boot/dts/wm8505-ref.dts  |    8 ++++++++
 arch/arm/boot/dts/wm8505.dtsi     |   29 +++++++++++++++++++++++++++++
 arch/arm/boot/dts/wm8650-mid.dts  |    8 ++++++++
 arch/arm/boot/dts/wm8650.dtsi     |   17 +++++++++++++----
 6 files changed, 95 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/vt8500-bv07.dts b/arch/arm/boot/dts/vt8500-bv07.dts
index 567cf4e..3cba367 100644
--- a/arch/arm/boot/dts/vt8500-bv07.dts
+++ b/arch/arm/boot/dts/vt8500-bv07.dts
@@ -33,4 +33,12 @@
 			};
 		};
 	};
+
+	backlight {
+		compatible = "pwm-backlight";
+		pwms = <&pwm 0 50000>;
+
+		brightness-levels = <0 4 8 16 32 64 128 255>;
+		default-brightness-level = <5>;
+	};
 };
diff --git a/arch/arm/boot/dts/vt8500.dtsi b/arch/arm/boot/dts/vt8500.dtsi
index d8645e9..e196b2e 100644
--- a/arch/arm/boot/dts/vt8500.dtsi
+++ b/arch/arm/boot/dts/vt8500.dtsi
@@ -40,14 +40,43 @@
 				#address-cells = <1>;
 				#size-cells = <0>;
 
+				ref25: ref25M {
+					#clock-cells = <0>;
+					compatible = "fixed-clock";
+					clock-frequency = <25000000>;
+				};
+
 				ref24: ref24M {
 					#clock-cells = <0>;
 					compatible = "fixed-clock";
 					clock-frequency = <24000000>;
 				};
+
+				pllb: pllb {
+					#clock-cells = <0>;
+					compatible = "via,vt8500-pll-clock";
+					clocks = <&ref25>;
+					reg = <0x204>;
+				};
+
+				clkpwm: pwm {
+					#clock-cells = <0>;
+					compatible = "via,vt8500-device-clock";
+					clocks = <&pllb>;
+					divisor-reg = <0x348>;
+					enable-reg = <0x250>;
+					enable-bit = <14>;
+				};
 			};
 		};
 
+		pwm: pwm@d8220000 {
+			#pwm-cells = <2>;
+			compatible = "via,vt8500-pwm";
+			reg = <0xd8220000 0x100>;
+			clocks = <&clkpwm>;
+		};
+
 		timer@d8130100 {
 			compatible = "via,vt8500-timer";
 			reg = <0xd8130100 0x28>;
diff --git a/arch/arm/boot/dts/wm8505-ref.dts b/arch/arm/boot/dts/wm8505-ref.dts
index fd4e248..f51c0ed 100644
--- a/arch/arm/boot/dts/wm8505-ref.dts
+++ b/arch/arm/boot/dts/wm8505-ref.dts
@@ -33,4 +33,12 @@
 			};
 		};
 	};
+
+	backlight {
+		compatible = "pwm-backlight";
+		pwms = <&pwm 0 50000>;
+
+		brightness-levels = <0 4 8 16 32 64 128 255>;
+		default-brightness-level = <5>;
+	};
 };
diff --git a/arch/arm/boot/dts/wm8505.dtsi b/arch/arm/boot/dts/wm8505.dtsi
index b459691..83c8ec5 100644
--- a/arch/arm/boot/dts/wm8505.dtsi
+++ b/arch/arm/boot/dts/wm8505.dtsi
@@ -54,14 +54,43 @@
 				#address-cells = <1>;
 				#size-cells = <0>;
 
+				ref25: ref25M {
+					#clock-cells = <0>;
+					compatible = "fixed-clock";
+					clock-frequency = <25000000>;
+				};
+
 				ref24: ref24M {
 					#clock-cells = <0>;
 					compatible = "fixed-clock";
 					clock-frequency = <24000000>;
 				};
+
+				pllb: pllb {
+					#clock-cells = <0>;
+					compatible = "via,vt8500-pll-clock";
+					clocks = <&ref25>;
+					reg = <0x204>;
+				};
+
+				clkpwm: pwm {
+					#clock-cells = <0>;
+					compatible = "via,vt8500-device-clock";
+					clocks = <&pllb>;
+					divisor-reg = <0x348>;
+					enable-reg = <0x250>;
+					enable-bit = <10>;
+				};
 			};
 		};
 
+		pwm: pwm@d8220000 {
+			#pwm-cells = <2>;
+			compatible = "via,vt8500-pwm";
+			reg = <0xd8220000 0x100>;
+			clocks = <&clkpwm>;
+		};
+
 		timer@d8130100 {
 			compatible = "via,vt8500-timer";
 			reg = <0xd8130100 0x28>;
diff --git a/arch/arm/boot/dts/wm8650-mid.dts b/arch/arm/boot/dts/wm8650-mid.dts
index cefd938..7a05dd5 100644
--- a/arch/arm/boot/dts/wm8650-mid.dts
+++ b/arch/arm/boot/dts/wm8650-mid.dts
@@ -33,4 +33,12 @@
 			};
 		};
 	};
+
+	backlight {
+		compatible = "pwm-backlight";
+		pwms = <&pwm 0 50000>;
+
+		brightness-levels = <0 4 8 16 32 64 128 255>;
+		default-brightness-level = <5>;
+	};
 };
diff --git a/arch/arm/boot/dts/wm8650.dtsi b/arch/arm/boot/dts/wm8650.dtsi
index 83b9467..a25d240 100644
--- a/arch/arm/boot/dts/wm8650.dtsi
+++ b/arch/arm/boot/dts/wm8650.dtsi
@@ -75,14 +75,16 @@
 					reg = <0x204>;
 				};
 
-				arm: arm {
+				clkpwm: pwm {
 					#clock-cells = <0>;
 					compatible = "via,vt8500-device-clock";
-					clocks = <&plla>;
-					divisor-reg = <0x300>;
+					clocks = <&pllb>;
+					divisor-reg = <0x348>;
+					enable-reg = <0x250>;
+					enable-bit = <10>;
 				};
 
-				sdhc: sdhc {
+				clksdhc: sdhc {
 					#clock-cells = <0>;
 					compatible = "via,vt8500-device-clock";
 					clocks = <&pllb>;
@@ -94,6 +96,13 @@
 			};
 		};
 
+		pwm: pwm@d8220000 {
+			#pwm-cells = <2>;
+			compatible = "via,vt8500-pwm";
+			reg = <0xd8220000 0x100>;
+			clocks = <&clkpwm>;
+		};
+
 		timer@d8130100 {
 			compatible = "via,vt8500-timer";
 			reg = <0xd8130100 0x28>;
-- 
1.7.9.5

[PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support

[Tony Prisk]

This patch updates pwm-vt8500.c to support devicetree probing and
make use of the common clock subsystem.

Signed-off-by: Tony Prisk <linux-ci5G2KO2hbZ+pU9mqzGVBQ@public.gmane.org>
---
 drivers/pwm/pwm-vt8500.c |   79 ++++++++++++++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 28 deletions(-)

diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
index ad14389..c2a71ee 100644
--- a/drivers/pwm/pwm-vt8500.c
+++ b/drivers/pwm/pwm-vt8500.c
@@ -1,7 +1,8 @@
 /*
  * drivers/pwm/pwm-vt8500.c
  *
- *  Copyright (C) 2010 Alexey Charkov <alchark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ * Copyright (C) 2012 Tony Prisk <linux-ci5G2KO2hbZ+pU9mqzGVBQ@public.gmane.org>
+ * Copyright (C) 2010 Alexey Charkov <alchark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  *
  * This software is licensed under the terms of the GNU General Public
  * License version 2, as published by the Free Software Foundation, and
@@ -21,14 +22,24 @@
 #include <linux/io.h>
 #include <linux/pwm.h>
 #include <linux/delay.h>
+#include <linux/clk.h>
 
 #include <asm/div64.h>
 
-#define VT8500_NR_PWMS 4
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+
+/*
+ * SoC architecture allocates register space for 4 PWMs but only
+ * 2 are currently implemented.
+ */
+#define VT8500_NR_PWMS	2
 
 struct vt8500_chip {
 	struct pwm_chip chip;
 	void __iomem *base;
+	struct clk *clk;
 };
 
 #define to_vt8500_chip(chip)	container_of(chip, struct vt8500_chip, chip)
@@ -52,7 +63,7 @@ static int vt8500_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	unsigned long long c;
 	unsigned long period_cycles, prescale, pv, dc;
 
-	c = 25000000/2; /* wild guess --- need to implement clocks */
+	c = clk_get_rate(vt8500->clk);
 	c = c * period_ns;
 	do_div(c, 1000000000);
 	period_cycles = c;
@@ -107,12 +118,22 @@ static struct pwm_ops vt8500_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
-static int __devinit pwm_probe(struct platform_device *pdev)
+static const struct of_device_id vt8500_pwm_dt_ids[] = {
+	{ .compatible = "via,vt8500-pwm", },
+	{ /* Sentinel */ }
+};
+
+static int __devinit vt8500_pwm_probe(struct platform_device *pdev)
 {
 	struct vt8500_chip *chip;
-	struct resource *r;
+	struct device_node *np = pdev->dev.of_node;
 	int ret;
 
+	if (!np) {
+		dev_err(&pdev->dev, "invalid devicetree node\n");
+		return -EINVAL;
+	}
+
 	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
 	if (chip == NULL) {
 		dev_err(&pdev->dev, "failed to allocate memory\n");
@@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct platform_device *pdev)
 	chip->chip.ops = &vt8500_pwm_ops;
 	chip->chip.base = -1;
 	chip->chip.npwm = VT8500_NR_PWMS;
+	chip->clk = of_clk_get(np, 0);
 
-	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (r == NULL) {
-		dev_err(&pdev->dev, "no memory resource defined\n");
-		return -ENODEV;
+	if (!chip->clk) {
+		dev_err(&pdev->dev, "clock source not specified\n");
+		return -EINVAL;
 	}
 
-	chip->base = devm_request_and_ioremap(&pdev->dev, r);
-	if (chip->base == NULL)
+	chip->base = of_iomap(np, 0);
+	if (!chip->base) {
+		dev_err(&pdev->dev, "memory resource not available\n");
 		return -EADDRNOTAVAIL;
+	}
+
+	clk_prepare_enable(chip->clk);
 
 	ret = pwmchip_add(&chip->chip);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to add pwmchip\n");
 		return ret;
+	}
 
 	platform_set_drvdata(pdev, chip);
 	return ret;
 }
 
-static int __devexit pwm_remove(struct platform_device *pdev)
+static int __devexit vt8500_pwm_remove(struct platform_device *pdev)
 {
 	struct vt8500_chip *chip;
 
@@ -150,28 +177,24 @@ static int __devexit pwm_remove(struct platform_device *pdev)
 	if (chip == NULL)
 		return -ENODEV;
 
+	clk_disable_unprepare(chip->clk);
+
 	return pwmchip_remove(&chip->chip);
 }
 
-static struct platform_driver pwm_driver = {
+static struct platform_driver vt8500_pwm_driver = {
+	.probe		= vt8500_pwm_probe,
+	.remove		= __devexit_p(vt8500_pwm_remove),
 	.driver		= {
 		.name	= "vt8500-pwm",
 		.owner	= THIS_MODULE,
+		.of_match_table = vt8500_pwm_dt_ids,
 	},
-	.probe		= pwm_probe,
-	.remove		= __devexit_p(pwm_remove),
 };
 
-static int __init pwm_init(void)
-{
-	return platform_driver_register(&pwm_driver);
-}
-arch_initcall(pwm_init);
-
-static void __exit pwm_exit(void)
-{
-	platform_driver_unregister(&pwm_driver);
-}
-module_exit(pwm_exit);
+module_platform_driver(vt8500_pwm_driver);
 
-MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("VT8500 PWM Driver");
+MODULE_AUTHOR("Tony Prisk <linux-ci5G2KO2hbZ+pU9mqzGVBQ@public.gmane.org>");
+MODULE_LICENSE("GPL v2");
+MODULE_DEVICE_TABLE(of, vt8500_pwm_dt_ids);
-- 
1.7.9.5

Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6960 bytes --]

On Fri, Oct 19, 2012 at 11:38:54PM +1300, Tony Prisk wrote:
> This patch updates pwm-vt8500.c to support devicetree probing and
> make use of the common clock subsystem.
> 
> Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> ---
>  drivers/pwm/pwm-vt8500.c |   79 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 51 insertions(+), 28 deletions(-)

On the whole, this looks like a great cleanup. A few minor issues
inline.

I should start with the subject: please keep lowercase pwm: as the
prefix for consistency. Uppercase PWM is used to refer to PWM devices in
prose.

Also I'd rather see the clock subsystem changes and device tree changes
in separate patches, but since both are rather intertwined I won't force
the issue.

> diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
> index ad14389..c2a71ee 100644
> --- a/drivers/pwm/pwm-vt8500.c
> +++ b/drivers/pwm/pwm-vt8500.c
> @@ -1,7 +1,8 @@
>  /*
>   * drivers/pwm/pwm-vt8500.c
>   *
> - *  Copyright (C) 2010 Alexey Charkov <alchark@gmail.com>
> + * Copyright (C) 2012 Tony Prisk <linux@prisktech.co.nz>
> + * Copyright (C) 2010 Alexey Charkov <alchark@gmail.com>
>   *
>   * This software is licensed under the terms of the GNU General Public
>   * License version 2, as published by the Free Software Foundation, and
> @@ -21,14 +22,24 @@
>  #include <linux/io.h>
>  #include <linux/pwm.h>
>  #include <linux/delay.h>
> +#include <linux/clk.h>
>  
>  #include <asm/div64.h>
>  
> -#define VT8500_NR_PWMS 4
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +
> +/*
> + * SoC architecture allocates register space for 4 PWMs but only
> + * 2 are currently implemented.
> + */
> +#define VT8500_NR_PWMS	2
>  
>  struct vt8500_chip {
>  	struct pwm_chip chip;
>  	void __iomem *base;
> +	struct clk *clk;
>  };
>  
>  #define to_vt8500_chip(chip)	container_of(chip, struct vt8500_chip, chip)
> @@ -52,7 +63,7 @@ static int vt8500_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	unsigned long long c;
>  	unsigned long period_cycles, prescale, pv, dc;
>  
> -	c = 25000000/2; /* wild guess --- need to implement clocks */
> +	c = clk_get_rate(vt8500->clk);
>  	c = c * period_ns;
>  	do_div(c, 1000000000);
>  	period_cycles = c;
> @@ -107,12 +118,22 @@ static struct pwm_ops vt8500_pwm_ops = {
>  	.owner = THIS_MODULE,
>  };
>  
> -static int __devinit pwm_probe(struct platform_device *pdev)
> +static const struct of_device_id vt8500_pwm_dt_ids[] = {
> +	{ .compatible = "via,vt8500-pwm", },
> +	{ /* Sentinel */ }
> +};
> +
> +static int __devinit vt8500_pwm_probe(struct platform_device *pdev)

Since you're changing this line anyway, maybe you should drop __devinit
(and __devexit for the .remove() callback). HOTPLUG is always enabled
nowadays and will go away eventually, in which case these will need to
be removed anyway.

>  {
>  	struct vt8500_chip *chip;
> -	struct resource *r;
> +	struct device_node *np = pdev->dev.of_node;
>  	int ret;
>  
> +	if (!np) {
> +		dev_err(&pdev->dev, "invalid devicetree node\n");
> +		return -EINVAL;
> +	}
> +

This effectively makes DT support mandatory. Shouldn't you be adding a
"depends on OF" into the Kconfig section in that case?

>  	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>  	if (chip == NULL) {
>  		dev_err(&pdev->dev, "failed to allocate memory\n");
> @@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct platform_device *pdev)
>  	chip->chip.ops = &vt8500_pwm_ops;
>  	chip->chip.base = -1;
>  	chip->chip.npwm = VT8500_NR_PWMS;
> +	chip->clk = of_clk_get(np, 0);

I thought this was supposed to work transparently across OF and !OF
configurations by using just clk_get() or devm_clk_get()? I guess that
if the driver depends on OF, then this would be moot, but we should
probably stick to the standard usage anyway.

Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
add explicit clk_put() in the error cleanup paths. One more argument in
favour of using devm_clk_get() instead.

> -	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (r == NULL) {
> -		dev_err(&pdev->dev, "no memory resource defined\n");
> -		return -ENODEV;
> +	if (!chip->clk) {
> +		dev_err(&pdev->dev, "clock source not specified\n");
> +		return -EINVAL;
>  	}
>  
> -	chip->base = devm_request_and_ioremap(&pdev->dev, r);
> -	if (chip->base == NULL)
> +	chip->base = of_iomap(np, 0);

No need to change this. It should work with the standard calls as well.

> +	if (!chip->base) {
> +		dev_err(&pdev->dev, "memory resource not available\n");
>  		return -EADDRNOTAVAIL;
> +	}
> +
> +	clk_prepare_enable(chip->clk);

Why does the clock need to be enabled here? Shouldn't it be postponed to
the last possible moment to save power?

>  
>  	ret = pwmchip_add(&chip->chip);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to add pwmchip\n");
>  		return ret;
> +	}
>  
>  	platform_set_drvdata(pdev, chip);
>  	return ret;
>  }
>  
> -static int __devexit pwm_remove(struct platform_device *pdev)
> +static int __devexit vt8500_pwm_remove(struct platform_device *pdev)
>  {
>  	struct vt8500_chip *chip;
>  
> @@ -150,28 +177,24 @@ static int __devexit pwm_remove(struct platform_device *pdev)
>  	if (chip == NULL)
>  		return -ENODEV;
>  
> +	clk_disable_unprepare(chip->clk);
> +

Again, shouldn't it be more efficient power-wise to disable this as soon
as the last PWM device is disabled?

>  	return pwmchip_remove(&chip->chip);
>  }
>  
> -static struct platform_driver pwm_driver = {
> +static struct platform_driver vt8500_pwm_driver = {
> +	.probe		= vt8500_pwm_probe,
> +	.remove		= __devexit_p(vt8500_pwm_remove),

__devexit_p can also be removed.

>  	.driver		= {
>  		.name	= "vt8500-pwm",
>  		.owner	= THIS_MODULE,
> +		.of_match_table = vt8500_pwm_dt_ids,
>  	},
> -	.probe		= pwm_probe,
> -	.remove		= __devexit_p(pwm_remove),
>  };
>  
> -static int __init pwm_init(void)
> -{
> -	return platform_driver_register(&pwm_driver);
> -}
> -arch_initcall(pwm_init);
> -
> -static void __exit pwm_exit(void)
> -{
> -	platform_driver_unregister(&pwm_driver);
> -}
> -module_exit(pwm_exit);
> +module_platform_driver(vt8500_pwm_driver);
>  
> -MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("VT8500 PWM Driver");
> +MODULE_AUTHOR("Tony Prisk <linux@prisktech.co.nz>");
> +MODULE_LICENSE("GPL v2");

IANAL, but I think you need the approval of all authors of this code
before changing the license. But I see that the file header actually
says that this code is GPL v2, so maybe this change could be considered
a bugfix. =)

> +MODULE_DEVICE_TABLE(of, vt8500_pwm_dt_ids);

I think it is customary to put this right after the device table
definition.

> -- 
> 1.7.9.5
> 
> 
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support

[Tony Prisk]

Replies to your comments inline:

On Mon, 2012-10-22 at 08:34 +0200, Thierry Reding wrote:
...
> > -static int __devinit pwm_probe(struct platform_device *pdev)
> > +static const struct of_device_id vt8500_pwm_dt_ids[] = {
> > +	{ .compatible = "via,vt8500-pwm", },
> > +	{ /* Sentinel */ }
> > +};
> > +
> > +static int __devinit vt8500_pwm_probe(struct platform_device *pdev)
> 
> Since you're changing this line anyway, maybe you should drop __devinit
> (and __devexit for the .remove() callback). HOTPLUG is always enabled
> nowadays and will go away eventually, in which case these will need to
> be removed anyway.

Will do. I must say the inconstancy among comments is rather
frustrating. In another patch I sent out a few days ago (completely
unrelated to this), I told to add __devexit to a remove() function :\

> >  {
> >  	struct vt8500_chip *chip;
> > -	struct resource *r;
> > +	struct device_node *np = pdev->dev.of_node;
> >  	int ret;
> >  
> > +	if (!np) {
> > +		dev_err(&pdev->dev, "invalid devicetree node\n");
> > +		return -EINVAL;
> > +	}
> > +
> 
> This effectively makes DT support mandatory. Shouldn't you be adding a
> "depends on OF" into the Kconfig section in that case?
This driver depends on ARCH_VT8500, which only supports DT so a
dependency on OF seemed redundant. If you think its still necessary, let
me know and I'll add it anyway.
> 
> >  	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> >  	if (chip == NULL) {
> >  		dev_err(&pdev->dev, "failed to allocate memory\n");
> > @@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct platform_device *pdev)
> >  	chip->chip.ops = &vt8500_pwm_ops;
> >  	chip->chip.base = -1;
> >  	chip->chip.npwm = VT8500_NR_PWMS;
> > +	chip->clk = of_clk_get(np, 0);
> 
> I thought this was supposed to work transparently across OF and !OF
> configurations by using just clk_get() or devm_clk_get()? I guess that
> if the driver depends on OF, then this would be moot, but we should
> probably stick to the standard usage anyway.
> 
> Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
> add explicit clk_put() in the error cleanup paths. One more argument in
> favour of using devm_clk_get() instead.

Hmm good point. I stuck with of_ functions because its an OF only driver
and it seemed 'backward' to mix old code with new. It does pose the
question of 'why have of_clk_get() if existing functions work better'.
> 
> > -	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > -	if (r == NULL) {
> > -		dev_err(&pdev->dev, "no memory resource defined\n");
> > -		return -ENODEV;
> > +	if (!chip->clk) {
> > +		dev_err(&pdev->dev, "clock source not specified\n");
> > +		return -EINVAL;
> >  	}
> >  
> > -	chip->base = devm_request_and_ioremap(&pdev->dev, r);
> > -	if (chip->base == NULL)
> > +	chip->base = of_iomap(np, 0);
> 
> No need to change this. It should work with the standard calls as well.
Again, this was a conversion of use of_ functions rather than the 'old'
style.

> 
> > +	if (!chip->base) {
> > +		dev_err(&pdev->dev, "memory resource not available\n");
> >  		return -EADDRNOTAVAIL;
> > +	}
> > +
> > +	clk_prepare_enable(chip->clk);
> 
> Why does the clock need to be enabled here? Shouldn't it be postponed to
> the last possible moment to save power?
I didn't consider that - but the use case for everyone at present is
that they only need the PWM driver to control the backlight, and it's
going to be enabled at boot anyway - so one PWM will always be active.

Futureproofing is always good so I'll fix this.

...

> >  
> > -MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("VT8500 PWM Driver");
> > +MODULE_AUTHOR("Tony Prisk <linux@prisktech.co.nz>");
> > +MODULE_LICENSE("GPL v2");
> 
> IANAL, but I think you need the approval of all authors of this code
> before changing the license. But I see that the file header actually
> says that this code is GPL v2, so maybe this change could be considered
> a bugfix. =)

This is something I've already discussed with Alexey in regards to all
the existing drivers he has in mainline. Since I have taken over as
maintainer on this platform I have corrected the licenses as patch's
have gone through. As you pointed out, it was already GPLv2 in the
header, this is just a 'bugfix'.
> 
> > +MODULE_DEVICE_TABLE(of, vt8500_pwm_dt_ids);
> 
> I think it is customary to put this right after the device table
> definition.
Didn't know that - will fix.
> 
> > -- 
> > 1.7.9.5
> > 
> > 
> > 

Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support

[Tony Prisk]

On Mon, 2012-10-22 at 19:51 +1300, Tony Prisk wrote:
> > 
> > >  	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> > >  	if (chip == NULL) {
> > >  		dev_err(&pdev->dev, "failed to allocate memory\n");
> > > @@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct platform_device *pdev)
> > >  	chip->chip.ops = &vt8500_pwm_ops;
> > >  	chip->chip.base = -1;
> > >  	chip->chip.npwm = VT8500_NR_PWMS;
> > > +	chip->clk = of_clk_get(np, 0);
> > 
> > I thought this was supposed to work transparently across OF and !OF
> > configurations by using just clk_get() or devm_clk_get()? I guess that
> > if the driver depends on OF, then this would be moot, but we should
> > probably stick to the standard usage anyway.
> > 
> > Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
> > add explicit clk_put() in the error cleanup paths. One more argument in
> > favour of using devm_clk_get() instead.
> 
> Hmm good point. I stuck with of_ functions because its an OF only driver
> and it seemed 'backward' to mix old code with new. It does pose the
> question of 'why have of_clk_get() if existing functions work better'.

Was about to fix this but noticed why it wasn't like this to start
with :)

struct clk *devm_clk_get(struct device *dev, const char *id);
struct clk *of_clk_get(struct device_node *np, int index);

devm_clk_get requires me to 'get' the clock by name. arch-vt8500 (and I
believe a lot of other arch's) don't enforce names for clocks defined in
devicetree, therefore there is no way for me to know what name the clk
has unless I include in the binding that the clock must be named 'xxx'.

of_clk_get retrieves it by the dt-node + index, so it doesn't care as
long as its the 1st clock listed.


Welcome your feedback.

Regards
Tony P

Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2399 bytes --]

On Mon, Oct 22, 2012 at 08:09:07PM +1300, Tony Prisk wrote:
> On Mon, 2012-10-22 at 19:51 +1300, Tony Prisk wrote:
> > > 
> > > >  	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> > > >  	if (chip == NULL) {
> > > >  		dev_err(&pdev->dev, "failed to allocate memory\n");
> > > > @@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct platform_device *pdev)
> > > >  	chip->chip.ops = &vt8500_pwm_ops;
> > > >  	chip->chip.base = -1;
> > > >  	chip->chip.npwm = VT8500_NR_PWMS;
> > > > +	chip->clk = of_clk_get(np, 0);
> > > 
> > > I thought this was supposed to work transparently across OF and !OF
> > > configurations by using just clk_get() or devm_clk_get()? I guess that
> > > if the driver depends on OF, then this would be moot, but we should
> > > probably stick to the standard usage anyway.
> > > 
> > > Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
> > > add explicit clk_put() in the error cleanup paths. One more argument in
> > > favour of using devm_clk_get() instead.
> > 
> > Hmm good point. I stuck with of_ functions because its an OF only driver
> > and it seemed 'backward' to mix old code with new. It does pose the
> > question of 'why have of_clk_get() if existing functions work better'.
> 
> Was about to fix this but noticed why it wasn't like this to start
> with :)
> 
> struct clk *devm_clk_get(struct device *dev, const char *id);
> struct clk *of_clk_get(struct device_node *np, int index);
> 
> devm_clk_get requires me to 'get' the clock by name. arch-vt8500 (and I
> believe a lot of other arch's) don't enforce names for clocks defined in
> devicetree, therefore there is no way for me to know what name the clk
> has unless I include in the binding that the clock must be named 'xxx'.

I thought clk_get() was supposed to return the first clock specified in
DT if you pass NULL as the consumer name. I haven't tested this though.
And I haven't looked at the code.

> of_clk_get retrieves it by the dt-node + index, so it doesn't care as
> long as its the 1st clock listed.

So the usual way to do this, I believe, is:

	clocks = <&clk_foo>;
	clock-names = "foo";

Then use:

	clk = devm_clk_get(&pdev->dev, "foo");

And as I said above, I was under the impression that the default would
be to use the first clock if NULL was specified instead of "foo".

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support

[Tony Prisk]

On Mon, 2012-10-22 at 09:24 +0200, Thierry Reding wrote:
> On Mon, Oct 22, 2012 at 08:09:07PM +1300, Tony Prisk wrote:
> > On Mon, 2012-10-22 at 19:51 +1300, Tony Prisk wrote:
> > > > 
> > > > >  	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> > > > >  	if (chip == NULL) {
> > > > >  		dev_err(&pdev->dev, "failed to allocate memory\n");
> > > > > @@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct platform_device *pdev)
> > > > >  	chip->chip.ops = &vt8500_pwm_ops;
> > > > >  	chip->chip.base = -1;
> > > > >  	chip->chip.npwm = VT8500_NR_PWMS;
> > > > > +	chip->clk = of_clk_get(np, 0);
> > > > 
> > > > I thought this was supposed to work transparently across OF and !OF
> > > > configurations by using just clk_get() or devm_clk_get()? I guess that
> > > > if the driver depends on OF, then this would be moot, but we should
> > > > probably stick to the standard usage anyway.
> > > > 
> > > > Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
> > > > add explicit clk_put() in the error cleanup paths. One more argument in
> > > > favour of using devm_clk_get() instead.
> > > 
> > > Hmm good point. I stuck with of_ functions because its an OF only driver
> > > and it seemed 'backward' to mix old code with new. It does pose the
> > > question of 'why have of_clk_get() if existing functions work better'.
> > 
> > Was about to fix this but noticed why it wasn't like this to start
> > with :)
> > 
> > struct clk *devm_clk_get(struct device *dev, const char *id);
> > struct clk *of_clk_get(struct device_node *np, int index);
> > 
> > devm_clk_get requires me to 'get' the clock by name. arch-vt8500 (and I
> > believe a lot of other arch's) don't enforce names for clocks defined in
> > devicetree, therefore there is no way for me to know what name the clk
> > has unless I include in the binding that the clock must be named 'xxx'.
> 
> I thought clk_get() was supposed to return the first clock specified in
> DT if you pass NULL as the consumer name. I haven't tested this though.
> And I haven't looked at the code.
> 
> > of_clk_get retrieves it by the dt-node + index, so it doesn't care as
> > long as its the 1st clock listed.
> 
> So the usual way to do this, I believe, is:
> 
> 	clocks = <&clk_foo>;
> 	clock-names = "foo";
> 
> Then use:
> 
> 	clk = devm_clk_get(&pdev->dev, "foo");
> 
> And as I said above, I was under the impression that the default would
> be to use the first clock if NULL was specified instead of "foo".
> 
> Thierry

clock-names is an optional property (as defined in
bindings/clock/clock-bindings.txt) so relying on it is .. well,
unreliable.

What you say makes sense, but it means the binding document has to make
an optional property into a required property simply to use an 'old'
function when a new function would 'work' (granted not as well, as you
pointed out) without requiring the optional property.

Your subsystem - your rules. Let me know if I've managed to sway you or
not :)

Regards
Tony P

Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3778 bytes --]

On Mon, Oct 22, 2012 at 08:36:22PM +1300, Tony Prisk wrote:
> On Mon, 2012-10-22 at 09:24 +0200, Thierry Reding wrote:
> > On Mon, Oct 22, 2012 at 08:09:07PM +1300, Tony Prisk wrote:
> > > On Mon, 2012-10-22 at 19:51 +1300, Tony Prisk wrote:
> > > > > 
> > > > > >  	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> > > > > >  	if (chip == NULL) {
> > > > > >  		dev_err(&pdev->dev, "failed to allocate memory\n");
> > > > > > @@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct platform_device *pdev)
> > > > > >  	chip->chip.ops = &vt8500_pwm_ops;
> > > > > >  	chip->chip.base = -1;
> > > > > >  	chip->chip.npwm = VT8500_NR_PWMS;
> > > > > > +	chip->clk = of_clk_get(np, 0);
> > > > > 
> > > > > I thought this was supposed to work transparently across OF and !OF
> > > > > configurations by using just clk_get() or devm_clk_get()? I guess that
> > > > > if the driver depends on OF, then this would be moot, but we should
> > > > > probably stick to the standard usage anyway.
> > > > > 
> > > > > Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
> > > > > add explicit clk_put() in the error cleanup paths. One more argument in
> > > > > favour of using devm_clk_get() instead.
> > > > 
> > > > Hmm good point. I stuck with of_ functions because its an OF only driver
> > > > and it seemed 'backward' to mix old code with new. It does pose the
> > > > question of 'why have of_clk_get() if existing functions work better'.
> > > 
> > > Was about to fix this but noticed why it wasn't like this to start
> > > with :)
> > > 
> > > struct clk *devm_clk_get(struct device *dev, const char *id);
> > > struct clk *of_clk_get(struct device_node *np, int index);
> > > 
> > > devm_clk_get requires me to 'get' the clock by name. arch-vt8500 (and I
> > > believe a lot of other arch's) don't enforce names for clocks defined in
> > > devicetree, therefore there is no way for me to know what name the clk
> > > has unless I include in the binding that the clock must be named 'xxx'.
> > 
> > I thought clk_get() was supposed to return the first clock specified in
> > DT if you pass NULL as the consumer name. I haven't tested this though.
> > And I haven't looked at the code.
> > 
> > > of_clk_get retrieves it by the dt-node + index, so it doesn't care as
> > > long as its the 1st clock listed.
> > 
> > So the usual way to do this, I believe, is:
> > 
> > 	clocks = <&clk_foo>;
> > 	clock-names = "foo";
> > 
> > Then use:
> > 
> > 	clk = devm_clk_get(&pdev->dev, "foo");
> > 
> > And as I said above, I was under the impression that the default would
> > be to use the first clock if NULL was specified instead of "foo".
> > 
> > Thierry
> 
> clock-names is an optional property (as defined in
> bindings/clock/clock-bindings.txt) so relying on it is .. well,
> unreliable.
> 
> What you say makes sense, but it means the binding document has to make
> an optional property into a required property simply to use an 'old'
> function when a new function would 'work' (granted not as well, as you
> pointed out) without requiring the optional property.

Okay, I've just checked the core clock code, and indeed if you run
clk_get() with con_id set to NULL, you'll eventually call of_clk_get()
with index == 0. That's exactly what you want, right? The only setup
where this won't work out is if you need to handle multiple clocks, in
which case I think it would make sense to make the clock-names property
mandatory. But for this driver that won't be necessary, since it will
never use a second clock, right?

> Your subsystem - your rules. Let me know if I've managed to sway you or
> not :)

I'd rather persuade you than force the issue. =)

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v2] pwm: vt8500: Update vt8500 PWM driver support

[Tony Prisk]

This patch updates pwm-vt8500.c to support devicetree probing and
make use of the common clock subsystem.

A binding document describing the PWM controller found on
arch-vt8500 is also included.

Signed-off-by: Tony Prisk <linux-ci5G2KO2hbZ+pU9mqzGVBQ@public.gmane.org>
---
v2 changes:
Merged binding doc patch with main code patch
Fixes as requested by Thierry Reding.

 .../devicetree/bindings/pwm/vt8500-pwm.txt         |   17 ++++
 drivers/pwm/pwm-vt8500.c                           |   87 +++++++++++++++-----
 2 files changed, 82 insertions(+), 22 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pwm/vt8500-pwm.txt

diff --git a/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
new file mode 100644
index 0000000..bcc6367
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
@@ -0,0 +1,17 @@
+VIA/Wondermedia VT8500/WM8xxx series SoC PWM controller
+
+Required properties:
+- compatible: should be "via,vt8500-pwm"
+- reg: physical base address and length of the controller's registers
+- #pwm-cells: should be 2.  The first cell specifies the per-chip index
+  of the PWM to use and the second cell is the period in nanoseconds.
+- clocks: phandle to the PWM source clock
+
+Example:
+
+pwm1: pwm@d8220000 {
+	#pwm-cells = <2>;
+	compatible = "via,vt8500-pwm";
+	reg = <0xd8220000 0x1000>;
+	clocks = <&clkpwm>;
+};
diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
index ad14389..36fef69 100644
--- a/drivers/pwm/pwm-vt8500.c
+++ b/drivers/pwm/pwm-vt8500.c
@@ -1,7 +1,8 @@
 /*
  * drivers/pwm/pwm-vt8500.c
  *
- *  Copyright (C) 2010 Alexey Charkov <alchark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ * Copyright (C) 2012 Tony Prisk <linux-ci5G2KO2hbZ+pU9mqzGVBQ@public.gmane.org>
+ * Copyright (C) 2010 Alexey Charkov <alchark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  *
  * This software is licensed under the terms of the GNU General Public
  * License version 2, as published by the Free Software Foundation, and
@@ -21,14 +22,25 @@
 #include <linux/io.h>
 #include <linux/pwm.h>
 #include <linux/delay.h>
+#include <linux/clk.h>
 
 #include <asm/div64.h>
 
-#define VT8500_NR_PWMS 4
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+
+/*
+ * SoC architecture allocates register space for 4 PWMs but only
+ * 2 are currently implemented.
+ */
+#define VT8500_NR_PWMS	2
 
 struct vt8500_chip {
 	struct pwm_chip chip;
 	void __iomem *base;
+	struct clk *clk;
+	int enable_cnt;
 };
 
 #define to_vt8500_chip(chip)	container_of(chip, struct vt8500_chip, chip)
@@ -52,7 +64,7 @@ static int vt8500_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	unsigned long long c;
 	unsigned long period_cycles, prescale, pv, dc;
 
-	c = 25000000/2; /* wild guess --- need to implement clocks */
+	c = clk_get_rate(vt8500->clk);
 	c = c * period_ns;
 	do_div(c, 1000000000);
 	period_cycles = c;
@@ -87,6 +99,11 @@ static int vt8500_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct vt8500_chip *vt8500 = to_vt8500_chip(chip);
 
+	if (vt8500->enable_cnt == 0)
+		clk_enable(vt8500->clk);
+
+	vt8500->enable_cnt++;
+
 	pwm_busy_wait(vt8500->base + 0x40 + pwm->hwpwm, (1 << 0));
 	writel(5, vt8500->base + (pwm->hwpwm << 4));
 	return 0;
@@ -98,6 +115,11 @@ static void vt8500_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 
 	pwm_busy_wait(vt8500->base + 0x40 + pwm->hwpwm, (1 << 0));
 	writel(0, vt8500->base + (pwm->hwpwm << 4));
+
+	vt8500->enable_cnt--;
+
+	if (vt8500->enable_cnt == 0)
+		clk_disable(vt8500->clk);
 }
 
 static struct pwm_ops vt8500_pwm_ops = {
@@ -107,12 +129,25 @@ static struct pwm_ops vt8500_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
-static int __devinit pwm_probe(struct platform_device *pdev)
+static const struct of_device_id vt8500_pwm_dt_ids[] = {
+	{ .compatible = "via,vt8500-pwm", },
+	{ /* Sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, vt8500_pwm_dt_ids);
+
+static int vt8500_pwm_probe(struct platform_device *pdev)
 {
 	struct vt8500_chip *chip;
 	struct resource *r;
+	struct device_node *np = pdev->dev.of_node;
 	int ret;
 
+	if (!np) {
+		dev_err(&pdev->dev, "invalid devicetree node\n");
+		return -EINVAL;
+	}
+
 	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
 	if (chip == NULL) {
 		dev_err(&pdev->dev, "failed to allocate memory\n");
@@ -123,6 +158,13 @@ static int __devinit pwm_probe(struct platform_device *pdev)
 	chip->chip.ops = &vt8500_pwm_ops;
 	chip->chip.base = -1;
 	chip->chip.npwm = VT8500_NR_PWMS;
+	chip->clk = devm_clk_get(&pdev->dev, NULL);
+	chip->enable_cnt = 0;
+
+	if (!chip->clk) {
+		dev_err(&pdev->dev, "clock source not specified\n");
+		return -EINVAL;
+	}
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (r == NULL) {
@@ -131,18 +173,24 @@ static int __devinit pwm_probe(struct platform_device *pdev)
 	}
 
 	chip->base = devm_request_and_ioremap(&pdev->dev, r);
-	if (chip->base == NULL)
+	if (!chip->base) {
+		dev_err(&pdev->dev, "memory resource not available\n");
 		return -EADDRNOTAVAIL;
+	}
+
+	clk_prepare(chip->clk);
 
 	ret = pwmchip_add(&chip->chip);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to add pwmchip\n");
 		return ret;
+	}
 
 	platform_set_drvdata(pdev, chip);
 	return ret;
 }
 
-static int __devexit pwm_remove(struct platform_device *pdev)
+static int vt8500_pwm_remove(struct platform_device *pdev)
 {
 	struct vt8500_chip *chip;
 
@@ -150,28 +198,23 @@ static int __devexit pwm_remove(struct platform_device *pdev)
 	if (chip == NULL)
 		return -ENODEV;
 
+	clk_unprepare(chip->clk);
+
 	return pwmchip_remove(&chip->chip);
 }
 
-static struct platform_driver pwm_driver = {
+static struct platform_driver vt8500_pwm_driver = {
+	.probe		= vt8500_pwm_probe,
+	.remove		= vt8500_pwm_remove,
 	.driver		= {
 		.name	= "vt8500-pwm",
 		.owner	= THIS_MODULE,
+		.of_match_table = vt8500_pwm_dt_ids,
 	},
-	.probe		= pwm_probe,
-	.remove		= __devexit_p(pwm_remove),
 };
 
-static int __init pwm_init(void)
-{
-	return platform_driver_register(&pwm_driver);
-}
-arch_initcall(pwm_init);
-
-static void __exit pwm_exit(void)
-{
-	platform_driver_unregister(&pwm_driver);
-}
-module_exit(pwm_exit);
+module_platform_driver(vt8500_pwm_driver);
 
-MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("VT8500 PWM Driver");
+MODULE_AUTHOR("Tony Prisk <linux-ci5G2KO2hbZ+pU9mqzGVBQ@public.gmane.org>");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v2] pwm: vt8500: Update vt8500 PWM driver support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 7318 bytes --]

On Mon, Oct 22, 2012 at 09:13:35PM +1300, Tony Prisk wrote:
> This patch updates pwm-vt8500.c to support devicetree probing and
> make use of the common clock subsystem.
> 
> A binding document describing the PWM controller found on
> arch-vt8500 is also included.
> 
> Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> ---
> v2 changes:
> Merged binding doc patch with main code patch
> Fixes as requested by Thierry Reding.
> 
>  .../devicetree/bindings/pwm/vt8500-pwm.txt         |   17 ++++
>  drivers/pwm/pwm-vt8500.c                           |   87 +++++++++++++++-----
>  2 files changed, 82 insertions(+), 22 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/pwm/vt8500-pwm.txt

Looking better... just a few minor comments.

> diff --git a/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
> new file mode 100644
> index 0000000..bcc6367
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
> @@ -0,0 +1,17 @@
> +VIA/Wondermedia VT8500/WM8xxx series SoC PWM controller
> +
> +Required properties:
> +- compatible: should be "via,vt8500-pwm"
> +- reg: physical base address and length of the controller's registers
> +- #pwm-cells: should be 2.  The first cell specifies the per-chip index
> +  of the PWM to use and the second cell is the period in nanoseconds.
> +- clocks: phandle to the PWM source clock
> +
> +Example:
> +
> +pwm1: pwm@d8220000 {
> +	#pwm-cells = <2>;
> +	compatible = "via,vt8500-pwm";
> +	reg = <0xd8220000 0x1000>;
> +	clocks = <&clkpwm>;
> +};
> diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
> index ad14389..36fef69 100644
> --- a/drivers/pwm/pwm-vt8500.c
> +++ b/drivers/pwm/pwm-vt8500.c
> @@ -1,7 +1,8 @@
>  /*
>   * drivers/pwm/pwm-vt8500.c
>   *
> - *  Copyright (C) 2010 Alexey Charkov <alchark@gmail.com>
> + * Copyright (C) 2012 Tony Prisk <linux@prisktech.co.nz>
> + * Copyright (C) 2010 Alexey Charkov <alchark@gmail.com>
>   *
>   * This software is licensed under the terms of the GNU General Public
>   * License version 2, as published by the Free Software Foundation, and
> @@ -21,14 +22,25 @@
>  #include <linux/io.h>
>  #include <linux/pwm.h>
>  #include <linux/delay.h>
> +#include <linux/clk.h>
>  
>  #include <asm/div64.h>
>  
> -#define VT8500_NR_PWMS 4
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +
> +/*
> + * SoC architecture allocates register space for 4 PWMs but only
> + * 2 are currently implemented.
> + */
> +#define VT8500_NR_PWMS	2
>  
>  struct vt8500_chip {
>  	struct pwm_chip chip;
>  	void __iomem *base;
> +	struct clk *clk;
> +	int enable_cnt;

You don't need to keep a reference count yourself. The clock framework
does that for you.

> @@ -87,6 +99,11 @@ static int vt8500_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct vt8500_chip *vt8500 = to_vt8500_chip(chip);
>  
> +	if (vt8500->enable_cnt == 0)
> +		clk_enable(vt8500->clk);
> +
> +	vt8500->enable_cnt++;
> +

Again, reference counting isn't needed. But you should be checking for
the return value of clk_enable().

>  	pwm_busy_wait(vt8500->base + 0x40 + pwm->hwpwm, (1 << 0));
>  	writel(5, vt8500->base + (pwm->hwpwm << 4));
>  	return 0;
> @@ -98,6 +115,11 @@ static void vt8500_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  
>  	pwm_busy_wait(vt8500->base + 0x40 + pwm->hwpwm, (1 << 0));
>  	writel(0, vt8500->base + (pwm->hwpwm << 4));
> +
> +	vt8500->enable_cnt--;
> +
> +	if (vt8500->enable_cnt == 0)
> +		clk_disable(vt8500->clk);

Just clk_disable() will be enough.

> @@ -107,12 +129,25 @@ static struct pwm_ops vt8500_pwm_ops = {
>  	.owner = THIS_MODULE,
>  };
>  
> -static int __devinit pwm_probe(struct platform_device *pdev)
> +static const struct of_device_id vt8500_pwm_dt_ids[] = {
> +	{ .compatible = "via,vt8500-pwm", },
> +	{ /* Sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, vt8500_pwm_dt_ids);

I've more often seen this without a blank line between. But if you
prefer this for readability or whatever reasons feel free to keep it as
is.

> +
> +static int vt8500_pwm_probe(struct platform_device *pdev)
>  {
>  	struct vt8500_chip *chip;
>  	struct resource *r;
> +	struct device_node *np = pdev->dev.of_node;
>  	int ret;
>  
> +	if (!np) {
> +		dev_err(&pdev->dev, "invalid devicetree node\n");
> +		return -EINVAL;
> +	}
> +
>  	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>  	if (chip == NULL) {
>  		dev_err(&pdev->dev, "failed to allocate memory\n");
> @@ -123,6 +158,13 @@ static int __devinit pwm_probe(struct platform_device *pdev)
>  	chip->chip.ops = &vt8500_pwm_ops;
>  	chip->chip.base = -1;
>  	chip->chip.npwm = VT8500_NR_PWMS;
> +	chip->clk = devm_clk_get(&pdev->dev, NULL);
> +	chip->enable_cnt = 0;
> +
> +	if (!chip->clk) {

The proper way to check this is with IS_ERR_OR_NULL(clk) and return
PTR_ERR(clk) in case of failure.

> +		dev_err(&pdev->dev, "clock source not specified\n");
> +		return -EINVAL;
> +	}
>  
>  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (r == NULL) {
> @@ -131,18 +173,24 @@ static int __devinit pwm_probe(struct platform_device *pdev)
>  	}
>  
>  	chip->base = devm_request_and_ioremap(&pdev->dev, r);
> -	if (chip->base == NULL)
> +	if (!chip->base) {
> +		dev_err(&pdev->dev, "memory resource not available\n");

devm_request_and_ioremap() already outputs an error message in case of
failure, so no need to repeat it here.

>  		return -EADDRNOTAVAIL;
> +	}
> +
> +	clk_prepare(chip->clk);

clk_prepare() can fail, so you should check for errors.

>  
>  	ret = pwmchip_add(&chip->chip);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to add pwmchip\n");
>  		return ret;
> +	}
>  
>  	platform_set_drvdata(pdev, chip);
>  	return ret;
>  }
>  
> -static int __devexit pwm_remove(struct platform_device *pdev)
> +static int vt8500_pwm_remove(struct platform_device *pdev)
>  {
>  	struct vt8500_chip *chip;
>  
> @@ -150,28 +198,23 @@ static int __devexit pwm_remove(struct platform_device *pdev)
>  	if (chip == NULL)
>  		return -ENODEV;
>  
> +	clk_unprepare(chip->clk);
> +
>  	return pwmchip_remove(&chip->chip);
>  }
>  
> -static struct platform_driver pwm_driver = {
> +static struct platform_driver vt8500_pwm_driver = {
> +	.probe		= vt8500_pwm_probe,
> +	.remove		= vt8500_pwm_remove,
>  	.driver		= {
>  		.name	= "vt8500-pwm",
>  		.owner	= THIS_MODULE,
> +		.of_match_table = vt8500_pwm_dt_ids,
>  	},
> -	.probe		= pwm_probe,
> -	.remove		= __devexit_p(pwm_remove),
>  };
>  
> -static int __init pwm_init(void)
> -{
> -	return platform_driver_register(&pwm_driver);
> -}
> -arch_initcall(pwm_init);
> -
> -static void __exit pwm_exit(void)
> -{
> -	platform_driver_unregister(&pwm_driver);
> -}
> -module_exit(pwm_exit);
> +module_platform_driver(vt8500_pwm_driver);

Similar to my comment regarding the MODULE_DEVICE_TABLE() macro, I've
seen this usually without a blank line. But again that's mostly bike-
shedding and I'm find taking the patch with the blank line left in.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v3] pwm: vt8500: Update vt8500 PWM driver support

[Tony Prisk]

This patch updates pwm-vt8500.c to support devicetree probing and
make use of the common clock subsystem.

A binding document describing the PWM controller found on
arch-vt8500 is also included.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
---
 .../devicetree/bindings/pwm/vt8500-pwm.txt         |   17 ++++
 drivers/pwm/pwm-vt8500.c                           |   83 ++++++++++++++------
 2 files changed, 77 insertions(+), 23 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pwm/vt8500-pwm.txt

diff --git a/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
new file mode 100644
index 0000000..bcc6367
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
@@ -0,0 +1,17 @@
+VIA/Wondermedia VT8500/WM8xxx series SoC PWM controller
+
+Required properties:
+- compatible: should be "via,vt8500-pwm"
+- reg: physical base address and length of the controller's registers
+- #pwm-cells: should be 2.  The first cell specifies the per-chip index
+  of the PWM to use and the second cell is the period in nanoseconds.
+- clocks: phandle to the PWM source clock
+
+Example:
+
+pwm1: pwm@d8220000 {
+	#pwm-cells = <2>;
+	compatible = "via,vt8500-pwm";
+	reg = <0xd8220000 0x1000>;
+	clocks = <&clkpwm>;
+};
diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
index ad14389..ebbc41c 100644
--- a/drivers/pwm/pwm-vt8500.c
+++ b/drivers/pwm/pwm-vt8500.c
@@ -1,7 +1,8 @@
 /*
  * drivers/pwm/pwm-vt8500.c
  *
- *  Copyright (C) 2010 Alexey Charkov <alchark@gmail.com>
+ * Copyright (C) 2012 Tony Prisk <linux@prisktech.co.nz>
+ * Copyright (C) 2010 Alexey Charkov <alchark@gmail.com>
  *
  * This software is licensed under the terms of the GNU General Public
  * License version 2, as published by the Free Software Foundation, and
@@ -21,14 +22,24 @@
 #include <linux/io.h>
 #include <linux/pwm.h>
 #include <linux/delay.h>
+#include <linux/clk.h>
 
 #include <asm/div64.h>
 
-#define VT8500_NR_PWMS 4
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+
+/*
+ * SoC architecture allocates register space for 4 PWMs but only
+ * 2 are currently implemented.
+ */
+#define VT8500_NR_PWMS	2
 
 struct vt8500_chip {
 	struct pwm_chip chip;
 	void __iomem *base;
+	struct clk *clk;
 };
 
 #define to_vt8500_chip(chip)	container_of(chip, struct vt8500_chip, chip)
@@ -52,7 +63,7 @@ static int vt8500_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	unsigned long long c;
 	unsigned long period_cycles, prescale, pv, dc;
 
-	c = 25000000/2; /* wild guess --- need to implement clocks */
+	c = clk_get_rate(vt8500->clk);
 	c = c * period_ns;
 	do_div(c, 1000000000);
 	period_cycles = c;
@@ -87,6 +98,11 @@ static int vt8500_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct vt8500_chip *vt8500 = to_vt8500_chip(chip);
 
+	if (!clk_enable(vt8500->clk)) {
+		dev_err(chip->dev, "failed to enable clock\n");
+		return -EBUSY;
+	};
+
 	pwm_busy_wait(vt8500->base + 0x40 + pwm->hwpwm, (1 << 0));
 	writel(5, vt8500->base + (pwm->hwpwm << 4));
 	return 0;
@@ -98,6 +114,8 @@ static void vt8500_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 
 	pwm_busy_wait(vt8500->base + 0x40 + pwm->hwpwm, (1 << 0));
 	writel(0, vt8500->base + (pwm->hwpwm << 4));
+
+	clk_disable(vt8500->clk);
 }
 
 static struct pwm_ops vt8500_pwm_ops = {
@@ -107,12 +125,24 @@ static struct pwm_ops vt8500_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
-static int __devinit pwm_probe(struct platform_device *pdev)
+static const struct of_device_id vt8500_pwm_dt_ids[] = {
+	{ .compatible = "via,vt8500-pwm", },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, vt8500_pwm_dt_ids);
+
+static int vt8500_pwm_probe(struct platform_device *pdev)
 {
 	struct vt8500_chip *chip;
 	struct resource *r;
+	struct device_node *np = pdev->dev.of_node;
 	int ret;
 
+	if (!np) {
+		dev_err(&pdev->dev, "invalid devicetree node\n");
+		return -EINVAL;
+	}
+
 	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
 	if (chip == NULL) {
 		dev_err(&pdev->dev, "failed to allocate memory\n");
@@ -123,6 +153,12 @@ static int __devinit pwm_probe(struct platform_device *pdev)
 	chip->chip.ops = &vt8500_pwm_ops;
 	chip->chip.base = -1;
 	chip->chip.npwm = VT8500_NR_PWMS;
+	chip->clk = devm_clk_get(&pdev->dev, NULL);
+
+	if (IS_ERR_OR_NULL(chip->clk)) {
+		dev_err(&pdev->dev, "clock source not specified\n");
+		return PTR_ERR(chip->clk);
+	}
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (r == NULL) {
@@ -131,18 +167,25 @@ static int __devinit pwm_probe(struct platform_device *pdev)
 	}
 
 	chip->base = devm_request_and_ioremap(&pdev->dev, r);
-	if (chip->base == NULL)
+	if (!chip->base)
 		return -EADDRNOTAVAIL;
 
+	if (!clk_prepare(chip->clk)) {
+		dev_err(&pdev->dev, "failed to prepare clock\n");
+		return -EBUSY;
+	}
+
 	ret = pwmchip_add(&chip->chip);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to add pwmchip\n");
 		return ret;
+	}
 
 	platform_set_drvdata(pdev, chip);
 	return ret;
 }
 
-static int __devexit pwm_remove(struct platform_device *pdev)
+static int vt8500_pwm_remove(struct platform_device *pdev)
 {
 	struct vt8500_chip *chip;
 
@@ -150,28 +193,22 @@ static int __devexit pwm_remove(struct platform_device *pdev)
 	if (chip == NULL)
 		return -ENODEV;
 
+	clk_unprepare(chip->clk);
+
 	return pwmchip_remove(&chip->chip);
 }
 
-static struct platform_driver pwm_driver = {
+static struct platform_driver vt8500_pwm_driver = {
+	.probe		= vt8500_pwm_probe,
+	.remove		= vt8500_pwm_remove,
 	.driver		= {
 		.name	= "vt8500-pwm",
 		.owner	= THIS_MODULE,
+		.of_match_table = vt8500_pwm_dt_ids,
 	},
-	.probe		= pwm_probe,
-	.remove		= __devexit_p(pwm_remove),
 };
+module_platform_driver(vt8500_pwm_driver);
 
-static int __init pwm_init(void)
-{
-	return platform_driver_register(&pwm_driver);
-}
-arch_initcall(pwm_init);
-
-static void __exit pwm_exit(void)
-{
-	platform_driver_unregister(&pwm_driver);
-}
-module_exit(pwm_exit);
-
-MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("VT8500 PWM Driver");
+MODULE_AUTHOR("Tony Prisk <linux@prisktech.co.nz>");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v3] pwm: vt8500: Update vt8500 PWM driver support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1598 bytes --]

On Tue, Oct 23, 2012 at 07:10:24AM +1300, Tony Prisk wrote:
[...]
> @@ -87,6 +98,11 @@ static int vt8500_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct vt8500_chip *vt8500 = to_vt8500_chip(chip);
>  
> +	if (!clk_enable(vt8500->clk)) {
> +		dev_err(chip->dev, "failed to enable clock\n");
> +		return -EBUSY;
> +	};
> +

I don't think that works. The clock API returns 0 on success and a
negative error code on failure. So this should rather be something like:

	err = clk_enable(vt8500->clk);
	if (err < 0) {
		dev_err(chip->dev, "failed to enable clock: %d\n", err);
		return err;
	}

> @@ -123,6 +153,12 @@ static int __devinit pwm_probe(struct platform_device *pdev)
>  	chip->chip.ops = &vt8500_pwm_ops;
>  	chip->chip.base = -1;
>  	chip->chip.npwm = VT8500_NR_PWMS;
> +	chip->clk = devm_clk_get(&pdev->dev, NULL);
> +

The blank line should go above the call to devm_clk_get().

> +	if (IS_ERR_OR_NULL(chip->clk)) {
> +		dev_err(&pdev->dev, "clock source not specified\n");
> +		return PTR_ERR(chip->clk);
> +	}
[...]
> +	if (!clk_prepare(chip->clk)) {
> +		dev_err(&pdev->dev, "failed to prepare clock\n");
> +		return -EBUSY;
> +	}
> +

Same comment here. I wonder how this code can work, since if the clock
is properly prepared, then it will return 0, and the above will return
-EBUSY.

>  	ret = pwmchip_add(&chip->chip);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to add pwmchip\n");

Error messages can be considered prose, so this should be: "failed to
add PWM chip".

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3] pwm: vt8500: Update vt8500 PWM driver support

[Tony Prisk]

On Wed, 2012-10-24 at 00:14 +0200, Thierry Reding wrote:
> On Tue, Oct 23, 2012 at 07:10:24AM +1300, Tony Prisk wrote:
> [...]
> > @@ -87,6 +98,11 @@ static int vt8500_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> >  {
> >  	struct vt8500_chip *vt8500 = to_vt8500_chip(chip);
> >  
> > +	if (!clk_enable(vt8500->clk)) {
> > +		dev_err(chip->dev, "failed to enable clock\n");
> > +		return -EBUSY;
> > +	};
> > +
> 
> I don't think that works. The clock API returns 0 on success and a
> negative error code on failure. So this should rather be something like:
> 
> 	err = clk_enable(vt8500->clk);
> 	if (err < 0) {
> 		dev_err(chip->dev, "failed to enable clock: %d\n", err);
> 		return err;
> 	}
> 
> > @@ -123,6 +153,12 @@ static int __devinit pwm_probe(struct platform_device *pdev)
> >  	chip->chip.ops = &vt8500_pwm_ops;
> >  	chip->chip.base = -1;
> >  	chip->chip.npwm = VT8500_NR_PWMS;
> > +	chip->clk = devm_clk_get(&pdev->dev, NULL);
> > +
> 
> The blank line should go above the call to devm_clk_get().
> 
> > +	if (IS_ERR_OR_NULL(chip->clk)) {
> > +		dev_err(&pdev->dev, "clock source not specified\n");
> > +		return PTR_ERR(chip->clk);
> > +	}
> [...]
> > +	if (!clk_prepare(chip->clk)) {
> > +		dev_err(&pdev->dev, "failed to prepare clock\n");
> > +		return -EBUSY;
> > +	}
> > +
> 
> Same comment here. I wonder how this code can work, since if the clock
> is properly prepared, then it will return 0, and the above will return
> -EBUSY.
> 
> >  	ret = pwmchip_add(&chip->chip);
> > -	if (ret < 0)
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "failed to add pwmchip\n");
> 
> Error messages can be considered prose, so this should be: "failed to
> add PWM chip".
> 
> Thierry

I don't know why none of this caused a failure when boot tested. The
clock should have been disabled 'automagically' at bootup, and never
reenabled. *shrug* Fixed in new patch v3 (didn't notice there was
already a v3).

Regards
Tony P

Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support

[Tony Prisk]

On Mon, 2012-10-22 at 10:04 +0200, Thierry Reding wrote:
> On Mon, Oct 22, 2012 at 08:36:22PM +1300, Tony Prisk wrote:
> > On Mon, 2012-10-22 at 09:24 +0200, Thierry Reding wrote:
> > > On Mon, Oct 22, 2012 at 08:09:07PM +1300, Tony Prisk wrote:
> > > > On Mon, 2012-10-22 at 19:51 +1300, Tony Prisk wrote:
> > > > > > 
> > > > > > >  	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> > > > > > >  	if (chip == NULL) {
> > > > > > >  		dev_err(&pdev->dev, "failed to allocate memory\n");
> > > > > > > @@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct platform_device *pdev)
> > > > > > >  	chip->chip.ops = &vt8500_pwm_ops;
> > > > > > >  	chip->chip.base = -1;
> > > > > > >  	chip->chip.npwm = VT8500_NR_PWMS;
> > > > > > > +	chip->clk = of_clk_get(np, 0);
> > > > > > 
> > > > > > I thought this was supposed to work transparently across OF and !OF
> > > > > > configurations by using just clk_get() or devm_clk_get()? I guess that
> > > > > > if the driver depends on OF, then this would be moot, but we should
> > > > > > probably stick to the standard usage anyway.
> > > > > > 
> > > > > > Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
> > > > > > add explicit clk_put() in the error cleanup paths. One more argument in
> > > > > > favour of using devm_clk_get() instead.
> > > > > 
> > > > > Hmm good point. I stuck with of_ functions because its an OF only driver
> > > > > and it seemed 'backward' to mix old code with new. It does pose the
> > > > > question of 'why have of_clk_get() if existing functions work better'.
> > > > 
> > > > Was about to fix this but noticed why it wasn't like this to start
> > > > with :)
> > > > 
> > > > struct clk *devm_clk_get(struct device *dev, const char *id);
> > > > struct clk *of_clk_get(struct device_node *np, int index);
> > > > 
> > > > devm_clk_get requires me to 'get' the clock by name. arch-vt8500 (and I
> > > > believe a lot of other arch's) don't enforce names for clocks defined in
> > > > devicetree, therefore there is no way for me to know what name the clk
> > > > has unless I include in the binding that the clock must be named 'xxx'.
> > > 
> > > I thought clk_get() was supposed to return the first clock specified in
> > > DT if you pass NULL as the consumer name. I haven't tested this though.
> > > And I haven't looked at the code.
> > > 
> > > > of_clk_get retrieves it by the dt-node + index, so it doesn't care as
> > > > long as its the 1st clock listed.
> > > 
> > > So the usual way to do this, I believe, is:
> > > 
> > > 	clocks = <&clk_foo>;
> > > 	clock-names = "foo";
> > > 
> > > Then use:
> > > 
> > > 	clk = devm_clk_get(&pdev->dev, "foo");
> > > 
> > > And as I said above, I was under the impression that the default would
> > > be to use the first clock if NULL was specified instead of "foo".
> > > 
> > > Thierry
> > 
> > clock-names is an optional property (as defined in
> > bindings/clock/clock-bindings.txt) so relying on it is .. well,
> > unreliable.
> > 
> > What you say makes sense, but it means the binding document has to make
> > an optional property into a required property simply to use an 'old'
> > function when a new function would 'work' (granted not as well, as you
> > pointed out) without requiring the optional property.
> 
> Okay, I've just checked the core clock code, and indeed if you run
> clk_get() with con_id set to NULL, you'll eventually call of_clk_get()
> with index == 0. That's exactly what you want, right? The only setup
> where this won't work out is if you need to handle multiple clocks, in
> which case I think it would make sense to make the clock-names property
> mandatory. But for this driver that won't be necessary, since it will
> never use a second clock, right?
> 
> > Your subsystem - your rules. Let me know if I've managed to sway you or
> > not :)
> 
> I'd rather persuade you than force the issue. =)
> 
> Thierry

Further to the discussion, my preference is still for of_clk_get()
(although I've changed the patch anyway as you saw because it makes no
difference in this case) :)

clk_get(x, NULL) and devm_clk_get(x, NULL) both seems like 'hacks' to
allow platforms to convert to DT without having to update all their
drivers first. It only allows the first (default) clock, as your pointed
out. Getting a 2nd... clock relies on an optional property in DT (which
again, seems like it is there to support 'old' drivers) which allows you
to request clocks by name.

of_clk_get() on the other hand seems like a properly native DT function.
You don't need to know anything about the clock, as long as the correct
clock is specified in the correct order as documented by the binding.
Relying on 'pre-OF' code for a OF-only driver also seems
counter-intuitive.

Granted of_clk_get() doesn't provide the 'garbage-collection' of
devm_get_clk() but I think that is just an arguement to needing
of_devm_clk_get().

Just my two cents.

Regards
Tony P

Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 5797 bytes --]

On Tue, Oct 23, 2012 at 09:41:46PM +1300, Tony Prisk wrote:
> On Mon, 2012-10-22 at 10:04 +0200, Thierry Reding wrote:
> > On Mon, Oct 22, 2012 at 08:36:22PM +1300, Tony Prisk wrote:
> > > On Mon, 2012-10-22 at 09:24 +0200, Thierry Reding wrote:
> > > > On Mon, Oct 22, 2012 at 08:09:07PM +1300, Tony Prisk wrote:
> > > > > On Mon, 2012-10-22 at 19:51 +1300, Tony Prisk wrote:
> > > > > > > 
> > > > > > > >  	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> > > > > > > >  	if (chip == NULL) {
> > > > > > > >  		dev_err(&pdev->dev, "failed to allocate memory\n");
> > > > > > > > @@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct platform_device *pdev)
> > > > > > > >  	chip->chip.ops = &vt8500_pwm_ops;
> > > > > > > >  	chip->chip.base = -1;
> > > > > > > >  	chip->chip.npwm = VT8500_NR_PWMS;
> > > > > > > > +	chip->clk = of_clk_get(np, 0);
> > > > > > > 
> > > > > > > I thought this was supposed to work transparently across OF and !OF
> > > > > > > configurations by using just clk_get() or devm_clk_get()? I guess that
> > > > > > > if the driver depends on OF, then this would be moot, but we should
> > > > > > > probably stick to the standard usage anyway.
> > > > > > > 
> > > > > > > Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
> > > > > > > add explicit clk_put() in the error cleanup paths. One more argument in
> > > > > > > favour of using devm_clk_get() instead.
> > > > > > 
> > > > > > Hmm good point. I stuck with of_ functions because its an OF only driver
> > > > > > and it seemed 'backward' to mix old code with new. It does pose the
> > > > > > question of 'why have of_clk_get() if existing functions work better'.
> > > > > 
> > > > > Was about to fix this but noticed why it wasn't like this to start
> > > > > with :)
> > > > > 
> > > > > struct clk *devm_clk_get(struct device *dev, const char *id);
> > > > > struct clk *of_clk_get(struct device_node *np, int index);
> > > > > 
> > > > > devm_clk_get requires me to 'get' the clock by name. arch-vt8500 (and I
> > > > > believe a lot of other arch's) don't enforce names for clocks defined in
> > > > > devicetree, therefore there is no way for me to know what name the clk
> > > > > has unless I include in the binding that the clock must be named 'xxx'.
> > > > 
> > > > I thought clk_get() was supposed to return the first clock specified in
> > > > DT if you pass NULL as the consumer name. I haven't tested this though.
> > > > And I haven't looked at the code.
> > > > 
> > > > > of_clk_get retrieves it by the dt-node + index, so it doesn't care as
> > > > > long as its the 1st clock listed.
> > > > 
> > > > So the usual way to do this, I believe, is:
> > > > 
> > > > 	clocks = <&clk_foo>;
> > > > 	clock-names = "foo";
> > > > 
> > > > Then use:
> > > > 
> > > > 	clk = devm_clk_get(&pdev->dev, "foo");
> > > > 
> > > > And as I said above, I was under the impression that the default would
> > > > be to use the first clock if NULL was specified instead of "foo".
> > > > 
> > > > Thierry
> > > 
> > > clock-names is an optional property (as defined in
> > > bindings/clock/clock-bindings.txt) so relying on it is .. well,
> > > unreliable.
> > > 
> > > What you say makes sense, but it means the binding document has to make
> > > an optional property into a required property simply to use an 'old'
> > > function when a new function would 'work' (granted not as well, as you
> > > pointed out) without requiring the optional property.
> > 
> > Okay, I've just checked the core clock code, and indeed if you run
> > clk_get() with con_id set to NULL, you'll eventually call of_clk_get()
> > with index == 0. That's exactly what you want, right? The only setup
> > where this won't work out is if you need to handle multiple clocks, in
> > which case I think it would make sense to make the clock-names property
> > mandatory. But for this driver that won't be necessary, since it will
> > never use a second clock, right?
> > 
> > > Your subsystem - your rules. Let me know if I've managed to sway you or
> > > not :)
> > 
> > I'd rather persuade you than force the issue. =)
> > 
> > Thierry
> 
> Further to the discussion, my preference is still for of_clk_get()
> (although I've changed the patch anyway as you saw because it makes no
> difference in this case) :)
> 
> clk_get(x, NULL) and devm_clk_get(x, NULL) both seems like 'hacks' to
> allow platforms to convert to DT without having to update all their
> drivers first. It only allows the first (default) clock, as your pointed
> out. Getting a 2nd... clock relies on an optional property in DT (which
> again, seems like it is there to support 'old' drivers) which allows you
> to request clocks by name.
> 
> of_clk_get() on the other hand seems like a properly native DT function.
> You don't need to know anything about the clock, as long as the correct
> clock is specified in the correct order as documented by the binding.
> Relying on 'pre-OF' code for a OF-only driver also seems
> counter-intuitive.

I do agree with those arguments. What I was saying is that for drivers
which aren't DT only, of_clk_get() is not an option and that maybe
others would be encouraged by the example to not use the generic APIs
even if their driver could be used in non-DT setups. But maybe I'm
worrying needlessly.

That said, maybe somebody with a broader view of things like Arnd
(Cc'ed) could share his thoughts.

> Granted of_clk_get() doesn't provide the 'garbage-collection' of
> devm_get_clk() but I think that is just an arguement to needing
> of_devm_clk_get().

That can be remedied by adding a corresponding function, so the argument
doesn't really count.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support

[Russell King - ARM Linux]

On Tue, Oct 23, 2012 at 11:22:47AM +0200, Thierry Reding wrote:
> On Tue, Oct 23, 2012 at 09:41:46PM +1300, Tony Prisk wrote:
> > Further to the discussion, my preference is still for of_clk_get()
> > (although I've changed the patch anyway as you saw because it makes no
> > difference in this case) :)
> > 
> > clk_get(x, NULL) and devm_clk_get(x, NULL) both seems like 'hacks' to
> > allow platforms to convert to DT without having to update all their
> > drivers first. It only allows the first (default) clock, as your pointed
> > out. Getting a 2nd... clock relies on an optional property in DT (which
> > again, seems like it is there to support 'old' drivers) which allows you
> > to request clocks by name.
> > 
> > of_clk_get() on the other hand seems like a properly native DT function.
> > You don't need to know anything about the clock, as long as the correct
> > clock is specified in the correct order as documented by the binding.
> > Relying on 'pre-OF' code for a OF-only driver also seems
> > counter-intuitive.
> 
> I do agree with those arguments. What I was saying is that for drivers
> which aren't DT only, of_clk_get() is not an option and that maybe
> others would be encouraged by the example to not use the generic APIs
> even if their driver could be used in non-DT setups. But maybe I'm
> worrying needlessly.
> 
> That said, maybe somebody with a broader view of things like Arnd
> (Cc'ed) could share his thoughts.

As I have already said, the way the DT bindings were done for the clk
stuff was wrong.  A little thought put into it would've come up with
a much better solution which wouldn't have needed of_clk_get() at all.

How?

The arguments for clk_get() are:
1. the struct device, which you can get the OF-node from.
2. a _device_ _specific_ _clock_ _input_ _name_ (or NULL if there's only
   one.)

So, we have something that defines a hardware clock input name, which
can be used to generate a property name for OF.  So, what _could_ have
been done is this:

	clock-<input-name> = <&provider-node clk-output-index>;

where the property name is generated by:

	snprintf(prop, sizeof(prop), "clk-%s", name ? name : "default");

So I continue to assert that our current design is wrong - and it will
cause driver authors to pointlessly have to make a choice at every stage
between DT and non-DT based systems.

Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2899 bytes --]

On Tue, Oct 23, 2012 at 10:31:28AM +0100, Russell King - ARM Linux wrote:
> On Tue, Oct 23, 2012 at 11:22:47AM +0200, Thierry Reding wrote:
> > On Tue, Oct 23, 2012 at 09:41:46PM +1300, Tony Prisk wrote:
> > > Further to the discussion, my preference is still for of_clk_get()
> > > (although I've changed the patch anyway as you saw because it makes no
> > > difference in this case) :)
> > > 
> > > clk_get(x, NULL) and devm_clk_get(x, NULL) both seems like 'hacks' to
> > > allow platforms to convert to DT without having to update all their
> > > drivers first. It only allows the first (default) clock, as your pointed
> > > out. Getting a 2nd... clock relies on an optional property in DT (which
> > > again, seems like it is there to support 'old' drivers) which allows you
> > > to request clocks by name.
> > > 
> > > of_clk_get() on the other hand seems like a properly native DT function.
> > > You don't need to know anything about the clock, as long as the correct
> > > clock is specified in the correct order as documented by the binding.
> > > Relying on 'pre-OF' code for a OF-only driver also seems
> > > counter-intuitive.
> > 
> > I do agree with those arguments. What I was saying is that for drivers
> > which aren't DT only, of_clk_get() is not an option and that maybe
> > others would be encouraged by the example to not use the generic APIs
> > even if their driver could be used in non-DT setups. But maybe I'm
> > worrying needlessly.
> > 
> > That said, maybe somebody with a broader view of things like Arnd
> > (Cc'ed) could share his thoughts.
> 
> As I have already said, the way the DT bindings were done for the clk
> stuff was wrong.  A little thought put into it would've come up with
> a much better solution which wouldn't have needed of_clk_get() at all.
> 
> How?
> 
> The arguments for clk_get() are:
> 1. the struct device, which you can get the OF-node from.
> 2. a _device_ _specific_ _clock_ _input_ _name_ (or NULL if there's only
>    one.)
> 
> So, we have something that defines a hardware clock input name, which
> can be used to generate a property name for OF.  So, what _could_ have
> been done is this:
> 
> 	clock-<input-name> = <&provider-node clk-output-index>;
> 
> where the property name is generated by:
> 
> 	snprintf(prop, sizeof(prop), "clk-%s", name ? name : "default");

But we already have this, only with slightly different syntax:

	clocks = <&provider foo-index>, <&provider bar-index>;
	clock-names = "foo", "bar";

> So I continue to assert that our current design is wrong - and it will
> cause driver authors to pointlessly have to make a choice at every stage
> between DT and non-DT based systems.

I think the reason that Tony brought this up is that with this API, the
clock-names property becomes mandatory if you have more than one input
clock.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4680 bytes --]

On Mon, Oct 22, 2012 at 07:51:52PM +1300, Tony Prisk wrote:
> Replies to your comments inline:
> 
> On Mon, 2012-10-22 at 08:34 +0200, Thierry Reding wrote:
> ...
> > > -static int __devinit pwm_probe(struct platform_device *pdev)
> > > +static const struct of_device_id vt8500_pwm_dt_ids[] = {
> > > +	{ .compatible = "via,vt8500-pwm", },
> > > +	{ /* Sentinel */ }
> > > +};
> > > +
> > > +static int __devinit vt8500_pwm_probe(struct platform_device *pdev)
> > 
> > Since you're changing this line anyway, maybe you should drop __devinit
> > (and __devexit for the .remove() callback). HOTPLUG is always enabled
> > nowadays and will go away eventually, in which case these will need to
> > be removed anyway.
> 
> Will do. I must say the inconstancy among comments is rather
> frustrating. In another patch I sent out a few days ago (completely
> unrelated to this), I told to add __devexit to a remove() function :\

This is a rather recent development, so maybe not everyone knows about
it yet. You can look at the following commit for the details:

	45f035ab9b8f45aaf1eb2213218b7e9c14af3fc2

It's been in linux-next for about 6 weeks and has also gone into
3.7-rc1.

> > >  {
> > >  	struct vt8500_chip *chip;
> > > -	struct resource *r;
> > > +	struct device_node *np = pdev->dev.of_node;
> > >  	int ret;
> > >  
> > > +	if (!np) {
> > > +		dev_err(&pdev->dev, "invalid devicetree node\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > 
> > This effectively makes DT support mandatory. Shouldn't you be adding a
> > "depends on OF" into the Kconfig section in that case?
> This driver depends on ARCH_VT8500, which only supports DT so a
> dependency on OF seemed redundant. If you think its still necessary, let
> me know and I'll add it anyway.

Okay, in that case you don't need another dependency. I've recently seen
comments about the check for !np being unnecessary because it can't be
NULL if you depend on OF. I suppose there's some truth in it but to be
honest I haven't made up my mind about it yet.

> > >  	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> > >  	if (chip == NULL) {
> > >  		dev_err(&pdev->dev, "failed to allocate memory\n");
> > > @@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct platform_device *pdev)
> > >  	chip->chip.ops = &vt8500_pwm_ops;
> > >  	chip->chip.base = -1;
> > >  	chip->chip.npwm = VT8500_NR_PWMS;
> > > +	chip->clk = of_clk_get(np, 0);
> > 
> > I thought this was supposed to work transparently across OF and !OF
> > configurations by using just clk_get() or devm_clk_get()? I guess that
> > if the driver depends on OF, then this would be moot, but we should
> > probably stick to the standard usage anyway.
> > 
> > Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
> > add explicit clk_put() in the error cleanup paths. One more argument in
> > favour of using devm_clk_get() instead.
> 
> Hmm good point. I stuck with of_ functions because its an OF only driver
> and it seemed 'backward' to mix old code with new. It does pose the
> question of 'why have of_clk_get() if existing functions work better'.

I think wherever possible you should use the generic calls, since they
are (usually) explicitly designed to work with OF and non-OF and you
never know what your driver might end up being used for. Then again, if
the driver is VT8500 specific and VT8500 doesn't support anything but
device-tree I suppose using the of_*() functions would be okay. Maybe
adding devm_of_clk_get() would be a worthwhile addition.

You should also consider that other people may look at your driver as a
reference and may end up using the OF-only variants even if their driver
is used in non-DT setups. I'm not sure how valid that argument is,
though, since code review is supposed to catch those mistakes.

[...]
> > > -MODULE_LICENSE("GPL");
> > > +MODULE_DESCRIPTION("VT8500 PWM Driver");
> > > +MODULE_AUTHOR("Tony Prisk <linux@prisktech.co.nz>");
> > > +MODULE_LICENSE("GPL v2");
> > 
> > IANAL, but I think you need the approval of all authors of this code
> > before changing the license. But I see that the file header actually
> > says that this code is GPL v2, so maybe this change could be considered
> > a bugfix. =)
> 
> This is something I've already discussed with Alexey in regards to all
> the existing drivers he has in mainline. Since I have taken over as
> maintainer on this platform I have corrected the licenses as patch's
> have gone through. As you pointed out, it was already GPLv2 in the
> header, this is just a 'bugfix'.

Okay, works for me.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support

[Arnd Bergmann]

On Monday 22 October 2012, Thierry Reding wrote:
> On Mon, Oct 22, 2012 at 07:51:52PM +1300, Tony Prisk wrote:
> > Replies to your comments inline:
> > 
> > On Mon, 2012-10-22 at 08:34 +0200, Thierry Reding wrote:
> > ...
> > > > -static int __devinit pwm_probe(struct platform_device *pdev)
> > > > +static const struct of_device_id vt8500_pwm_dt_ids[] = {
> > > > + { .compatible = "via,vt8500-pwm", },
> > > > + { /* Sentinel */ }
> > > > +};
> > > > +
> > > > +static int __devinit vt8500_pwm_probe(struct platform_device *pdev)
> > > 
> > > Since you're changing this line anyway, maybe you should drop __devinit
> > > (and __devexit for the .remove() callback). HOTPLUG is always enabled
> > > nowadays and will go away eventually, in which case these will need to
> > > be removed anyway.
> > 
> > Will do. I must say the inconstancy among comments is rather
> > frustrating. In another patch I sent out a few days ago (completely
> > unrelated to this), I told to add __devexit to a remove() function :\
> 
> This is a rather recent development, so maybe not everyone knows about
> it yet. You can look at the following commit for the details:
> 
>         45f035ab9b8f45aaf1eb2213218b7e9c14af3fc2
> 
> It's been in linux-next for about 6 weeks and has also gone into
> 3.7-rc1.

As long as we get build warnings for leaving out the __devinit/__devexit
annotations, I would generally recommend putting them in. If we do a
patch to remove all of them, a couple extra instances will not cause
any more troubles than we already have.

	Arnd

Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]

On Mon, Oct 22, 2012 at 11:50:21AM +0000, Arnd Bergmann wrote:
> On Monday 22 October 2012, Thierry Reding wrote:
> > On Mon, Oct 22, 2012 at 07:51:52PM +1300, Tony Prisk wrote:
> > > Replies to your comments inline:
> > > 
> > > On Mon, 2012-10-22 at 08:34 +0200, Thierry Reding wrote:
> > > ...
> > > > > -static int __devinit pwm_probe(struct platform_device *pdev)
> > > > > +static const struct of_device_id vt8500_pwm_dt_ids[] = {
> > > > > + { .compatible = "via,vt8500-pwm", },
> > > > > + { /* Sentinel */ }
> > > > > +};
> > > > > +
> > > > > +static int __devinit vt8500_pwm_probe(struct platform_device *pdev)
> > > > 
> > > > Since you're changing this line anyway, maybe you should drop __devinit
> > > > (and __devexit for the .remove() callback). HOTPLUG is always enabled
> > > > nowadays and will go away eventually, in which case these will need to
> > > > be removed anyway.
> > > 
> > > Will do. I must say the inconstancy among comments is rather
> > > frustrating. In another patch I sent out a few days ago (completely
> > > unrelated to this), I told to add __devexit to a remove() function :\
> > 
> > This is a rather recent development, so maybe not everyone knows about
> > it yet. You can look at the following commit for the details:
> > 
> >         45f035ab9b8f45aaf1eb2213218b7e9c14af3fc2
> > 
> > It's been in linux-next for about 6 weeks and has also gone into
> > 3.7-rc1.
> 
> As long as we get build warnings for leaving out the __devinit/__devexit
> annotations, I would generally recommend putting them in. If we do a
> patch to remove all of them, a couple extra instances will not cause
> any more troubles than we already have.

I've never seen any build warnings for leaving __devinit/__devexit out.
Where does that happen?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support

[Arnd Bergmann]

On Monday 22 October 2012, Thierry Reding wrote:
> > As long as we get build warnings for leaving out the __devinit/__devexit
> > annotations, I would generally recommend putting them in. If we do a
> > patch to remove all of them, a couple extra instances will not cause
> > any more troubles than we already have.
> 
> I've never seen any build warnings for leaving __devinit/__devexit out.
> Where does that happen?

Section mismatches usually result into warnings from modpost, like

WARNING: modpost: Found 1 section mismatch(es).
To see full details build your kernel with:
'make CONFIG_DEBUG_SECTION_MISMATCH=y'

Actually doing that gives you an output like this (currently on exynos_defconfig):

$ make CONFIG_DEBUG_SECTION_MISMATCH=y
WARNING: drivers/pinctrl/built-in.o(.devinit.text+0x124): Section mismatch in reference from the function samsung_pinctrl_probe() to the function .init.text:samsung_gpiolib_register()
The function __devinit samsung_pinctrl_probe() references
a function __init samsung_gpiolib_register().
If samsung_gpiolib_register is only used by samsung_pinctrl_probe then
annotate samsung_gpiolib_register with a matching annotation.

or like this (now fixed in socfpga_defconfig):

WARNING: drivers/net/ethernet/stmicro/stmmac/stmmac.o(.text+0x5d4c): Section mismatch in reference from the function stmmac_pltfr_probe() to the function .devinit.text:stmmac_probe_config_dt()
The function stmmac_pltfr_probe() references
the function __devinit stmmac_probe_config_dt().
This is often because stmmac_pltfr_probe lacks a __devinit 
annotation or the annotation of stmmac_probe_config_dt is wrong.

I believe you normally don't get warnings for functions that could be
marked __devinit and only call regular functions, but there are
a couple of __devinit infrastructure functions that you can't call
from a function that isn't __init or __devinit.

	Arnd

Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2306 bytes --]

On Mon, Oct 22, 2012 at 01:52:08PM +0000, Arnd Bergmann wrote:
> On Monday 22 October 2012, Thierry Reding wrote:
> > > As long as we get build warnings for leaving out the __devinit/__devexit
> > > annotations, I would generally recommend putting them in. If we do a
> > > patch to remove all of them, a couple extra instances will not cause
> > > any more troubles than we already have.
> > 
> > I've never seen any build warnings for leaving __devinit/__devexit out.
> > Where does that happen?
> 
> Section mismatches usually result into warnings from modpost, like
> 
> WARNING: modpost: Found 1 section mismatch(es).
> To see full details build your kernel with:
> 'make CONFIG_DEBUG_SECTION_MISMATCH=y'
> 
> Actually doing that gives you an output like this (currently on exynos_defconfig):
> 
> $ make CONFIG_DEBUG_SECTION_MISMATCH=y
> WARNING: drivers/pinctrl/built-in.o(.devinit.text+0x124): Section mismatch in reference from the function samsung_pinctrl_probe() to the function .init.text:samsung_gpiolib_register()
> The function __devinit samsung_pinctrl_probe() references
> a function __init samsung_gpiolib_register().
> If samsung_gpiolib_register is only used by samsung_pinctrl_probe then
> annotate samsung_gpiolib_register with a matching annotation.
> 
> or like this (now fixed in socfpga_defconfig):
> 
> WARNING: drivers/net/ethernet/stmicro/stmmac/stmmac.o(.text+0x5d4c): Section mismatch in reference from the function stmmac_pltfr_probe() to the function .devinit.text:stmmac_probe_config_dt()
> The function stmmac_pltfr_probe() references
> the function __devinit stmmac_probe_config_dt().
> This is often because stmmac_pltfr_probe lacks a __devinit 
> annotation or the annotation of stmmac_probe_config_dt is wrong.
> 
> I believe you normally don't get warnings for functions that could be
> marked __devinit and only call regular functions, but there are
> a couple of __devinit infrastructure functions that you can't call
> from a function that isn't __init or __devinit.

Right. If you get those warnings you shouldn't be dropping the
annotations. But I don't think that is the case for this driver. Tony,
can you confirm that the driver still builds properly without warnings
if you drop the __devinit/__devexit?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support

[Tony Prisk]

On Mon, 2012-10-22 at 17:08 +0200, Thierry Reding wrote:
> On Mon, Oct 22, 2012 at 01:52:08PM +0000, Arnd Bergmann wrote:
> > On Monday 22 October 2012, Thierry Reding wrote:
> > > > As long as we get build warnings for leaving out the __devinit/__devexit
> > > > annotations, I would generally recommend putting them in. If we do a
> > > > patch to remove all of them, a couple extra instances will not cause
> > > > any more troubles than we already have.
> > > 
> > > I've never seen any build warnings for leaving __devinit/__devexit out.
> > > Where does that happen?
> > 
> > Section mismatches usually result into warnings from modpost, like
> > 
> > WARNING: modpost: Found 1 section mismatch(es).
> > To see full details build your kernel with:
> > 'make CONFIG_DEBUG_SECTION_MISMATCH=y'
> > 
> > Actually doing that gives you an output like this (currently on exynos_defconfig):
> > 
> > $ make CONFIG_DEBUG_SECTION_MISMATCH=y
> > WARNING: drivers/pinctrl/built-in.o(.devinit.text+0x124): Section mismatch in reference from the function samsung_pinctrl_probe() to the function .init.text:samsung_gpiolib_register()
> > The function __devinit samsung_pinctrl_probe() references
> > a function __init samsung_gpiolib_register().
> > If samsung_gpiolib_register is only used by samsung_pinctrl_probe then
> > annotate samsung_gpiolib_register with a matching annotation.
> > 
> > or like this (now fixed in socfpga_defconfig):
> > 
> > WARNING: drivers/net/ethernet/stmicro/stmmac/stmmac.o(.text+0x5d4c): Section mismatch in reference from the function stmmac_pltfr_probe() to the function .devinit.text:stmmac_probe_config_dt()
> > The function stmmac_pltfr_probe() references
> > the function __devinit stmmac_probe_config_dt().
> > This is often because stmmac_pltfr_probe lacks a __devinit 
> > annotation or the annotation of stmmac_probe_config_dt is wrong.
> > 
> > I believe you normally don't get warnings for functions that could be
> > marked __devinit and only call regular functions, but there are
> > a couple of __devinit infrastructure functions that you can't call
> > from a function that isn't __init or __devinit.
> 
> Right. If you get those warnings you shouldn't be dropping the
> annotations. But I don't think that is the case for this driver. Tony,
> can you confirm that the driver still builds properly without warnings
> if you drop the __devinit/__devexit?
> 
> Thierry
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Correct - it builds without warnings without __devinit/__devexit.

If it had introduced warnings when I tested it, I would have mentioned
it :)

Regards
Tony P

[PATCH 3/3] DOC: PWM: Adding binding document for via,vt8500-pwm

[Tony Prisk]

Add a binding document describing the PWM controller found
on arch-vt8500 supported SoCs.

Signed-off-by: Tony Prisk <linux-ci5G2KO2hbZ+pU9mqzGVBQ@public.gmane.org>
---
 .../devicetree/bindings/pwm/vt8500-pwm.txt         |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/vt8500-pwm.txt

diff --git a/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
new file mode 100644
index 0000000..e8ba133
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
@@ -0,0 +1,17 @@
+VIA/Wondermedia VT8500/WM8xxx series SoC PWM controller
+
+Required properties:
+- compatible: should be "via,vt8500-pwm"
+- reg: physical base address and length of the controller's registers
+- #pwm-cells: should be 2.  The first cell specifies the per-chip index
+  of the PWM to use and the second cell is the period in nanoseconds.
+- clocks: pHandle to the PWM source clock
+
+Example:
+
+pwm1: pwm@d8220000 {
+	#pwm-cells = <2>;
+	compatible = "via,vt8500-pwm";
+	reg = <0xd8220000 0x1000>;
+	clocks = <&clkpwm>;
+};
-- 
1.7.9.5

Re: [PATCH 3/3] DOC: PWM: Adding binding document for via,vt8500-pwm

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1231 bytes --]

On Fri, Oct 19, 2012 at 11:38:55PM +1300, Tony Prisk wrote:
> Add a binding document describing the PWM controller found
> on arch-vt8500 supported SoCs.
> 
> Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> ---
>  .../devicetree/bindings/pwm/vt8500-pwm.txt         |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/vt8500-pwm.txt

I think this can be folded into the previous patch. One other comment
below.

> diff --git a/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
> new file mode 100644
> index 0000000..e8ba133
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
> @@ -0,0 +1,17 @@
> +VIA/Wondermedia VT8500/WM8xxx series SoC PWM controller
> +
> +Required properties:
> +- compatible: should be "via,vt8500-pwm"
> +- reg: physical base address and length of the controller's registers
> +- #pwm-cells: should be 2.  The first cell specifies the per-chip index
> +  of the PWM to use and the second cell is the period in nanoseconds.
> +- clocks: pHandle to the PWM source clock

I think the common spelling is "phandle".

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/3] DOC: PWM: Adding binding document for via,vt8500-pwm

[Tony Prisk]

On Mon, 2012-10-22 at 08:35 +0200, Thierry Reding wrote:
> On Fri, Oct 19, 2012 at 11:38:55PM +1300, Tony Prisk wrote:
> > Add a binding document describing the PWM controller found
> > on arch-vt8500 supported SoCs.
> > 
> > Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> > ---
> >  .../devicetree/bindings/pwm/vt8500-pwm.txt         |   17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
> 
> I think this can be folded into the previous patch. One other comment
> below.
> 
> > diff --git a/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
> > new file mode 100644
> > index 0000000..e8ba133
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
> > @@ -0,0 +1,17 @@
> > +VIA/Wondermedia VT8500/WM8xxx series SoC PWM controller
> > +
> > +Required properties:
> > +- compatible: should be "via,vt8500-pwm"
> > +- reg: physical base address and length of the controller's registers
> > +- #pwm-cells: should be 2.  The first cell specifies the per-chip index
> > +  of the PWM to use and the second cell is the period in nanoseconds.
> > +- clocks: pHandle to the PWM source clock
> 
> I think the common spelling is "phandle".
> 
> Thierry

Will fix the capitalisation and merge this with patch 2.

Regards
Tony P

Re: [PATCH 1/3] ARM: dts: Update board files for pwm support

[Tony Prisk]

On Fri, 2012-10-19 at 23:38 +1300, Tony Prisk wrote:
> This patch adds pwm support to arch-vt8500 board files, and adds
> the use-case of pwm-backlight.
> 
> Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> ---
>  arch/arm/boot/dts/vt8500-bv07.dts |    8 ++++++++
>  arch/arm/boot/dts/vt8500.dtsi     |   29 +++++++++++++++++++++++++++++
>  arch/arm/boot/dts/wm8505-ref.dts  |    8 ++++++++
>  arch/arm/boot/dts/wm8505.dtsi     |   29 +++++++++++++++++++++++++++++
>  arch/arm/boot/dts/wm8650-mid.dts  |    8 ++++++++
>  arch/arm/boot/dts/wm8650.dtsi     |   17 +++++++++++++----
>  6 files changed, 95 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/vt8500-bv07.dts b/arch/arm/boot/dts/vt8500-bv07.dts
> index 567cf4e..3cba367 100644
> --- a/arch/arm/boot/dts/vt8500-bv07.dts
> +++ b/arch/arm/boot/dts/vt8500-bv07.dts
> @@ -33,4 +33,12 @@
>  			};
>  		};
>  	};
> +
> +	backlight {
> +		compatible = "pwm-backlight";
> +		pwms = <&pwm 0 50000>;
> +
> +		brightness-levels = <0 4 8 16 32 64 128 255>;
> +		default-brightness-level = <5>;
> +	};
>  };
> diff --git a/arch/arm/boot/dts/vt8500.dtsi b/arch/arm/boot/dts/vt8500.dtsi
> index d8645e9..e196b2e 100644
> --- a/arch/arm/boot/dts/vt8500.dtsi
> +++ b/arch/arm/boot/dts/vt8500.dtsi
> @@ -40,14 +40,43 @@
>  				#address-cells = <1>;
>  				#size-cells = <0>;
>  
> +				ref25: ref25M {
> +					#clock-cells = <0>;
> +					compatible = "fixed-clock";
> +					clock-frequency = <25000000>;
> +				};
> +
>  				ref24: ref24M {
>  					#clock-cells = <0>;
>  					compatible = "fixed-clock";
>  					clock-frequency = <24000000>;
>  				};
> +
> +				pllb: pllb {
> +					#clock-cells = <0>;
> +					compatible = "via,vt8500-pll-clock";
> +					clocks = <&ref25>;
> +					reg = <0x204>;
> +				};
> +
> +				clkpwm: pwm {
> +					#clock-cells = <0>;
> +					compatible = "via,vt8500-device-clock";
> +					clocks = <&pllb>;
> +					divisor-reg = <0x348>;
> +					enable-reg = <0x250>;
> +					enable-bit = <14>;
> +				};
>  			};
>  		};
>  
> +		pwm: pwm@d8220000 {
> +			#pwm-cells = <2>;
> +			compatible = "via,vt8500-pwm";
> +			reg = <0xd8220000 0x100>;
> +			clocks = <&clkpwm>;
> +		};
> +
>  		timer@d8130100 {
>  			compatible = "via,vt8500-timer";
>  			reg = <0xd8130100 0x28>;
> diff --git a/arch/arm/boot/dts/wm8505-ref.dts b/arch/arm/boot/dts/wm8505-ref.dts
> index fd4e248..f51c0ed 100644
> --- a/arch/arm/boot/dts/wm8505-ref.dts
> +++ b/arch/arm/boot/dts/wm8505-ref.dts
> @@ -33,4 +33,12 @@
>  			};
>  		};
>  	};
> +
> +	backlight {
> +		compatible = "pwm-backlight";
> +		pwms = <&pwm 0 50000>;
> +
> +		brightness-levels = <0 4 8 16 32 64 128 255>;
> +		default-brightness-level = <5>;
> +	};
>  };
> diff --git a/arch/arm/boot/dts/wm8505.dtsi b/arch/arm/boot/dts/wm8505.dtsi
> index b459691..83c8ec5 100644
> --- a/arch/arm/boot/dts/wm8505.dtsi
> +++ b/arch/arm/boot/dts/wm8505.dtsi
> @@ -54,14 +54,43 @@
>  				#address-cells = <1>;
>  				#size-cells = <0>;
>  
> +				ref25: ref25M {
> +					#clock-cells = <0>;
> +					compatible = "fixed-clock";
> +					clock-frequency = <25000000>;
> +				};
> +
>  				ref24: ref24M {
>  					#clock-cells = <0>;
>  					compatible = "fixed-clock";
>  					clock-frequency = <24000000>;
>  				};
> +
> +				pllb: pllb {
> +					#clock-cells = <0>;
> +					compatible = "via,vt8500-pll-clock";
> +					clocks = <&ref25>;
> +					reg = <0x204>;
> +				};
> +
> +				clkpwm: pwm {
> +					#clock-cells = <0>;
> +					compatible = "via,vt8500-device-clock";
> +					clocks = <&pllb>;
> +					divisor-reg = <0x348>;
> +					enable-reg = <0x250>;
> +					enable-bit = <10>;
> +				};
>  			};
>  		};
>  
> +		pwm: pwm@d8220000 {
> +			#pwm-cells = <2>;
> +			compatible = "via,vt8500-pwm";
> +			reg = <0xd8220000 0x100>;
> +			clocks = <&clkpwm>;
> +		};
> +
>  		timer@d8130100 {
>  			compatible = "via,vt8500-timer";
>  			reg = <0xd8130100 0x28>;
> diff --git a/arch/arm/boot/dts/wm8650-mid.dts b/arch/arm/boot/dts/wm8650-mid.dts
> index cefd938..7a05dd5 100644
> --- a/arch/arm/boot/dts/wm8650-mid.dts
> +++ b/arch/arm/boot/dts/wm8650-mid.dts
> @@ -33,4 +33,12 @@
>  			};
>  		};
>  	};
> +
> +	backlight {
> +		compatible = "pwm-backlight";
> +		pwms = <&pwm 0 50000>;
> +
> +		brightness-levels = <0 4 8 16 32 64 128 255>;
> +		default-brightness-level = <5>;
> +	};
>  };
> diff --git a/arch/arm/boot/dts/wm8650.dtsi b/arch/arm/boot/dts/wm8650.dtsi
> index 83b9467..a25d240 100644
> --- a/arch/arm/boot/dts/wm8650.dtsi
> +++ b/arch/arm/boot/dts/wm8650.dtsi
> @@ -75,14 +75,16 @@
>  					reg = <0x204>;
>  				};
>  
> -				arm: arm {
> +				clkpwm: pwm {
>  					#clock-cells = <0>;
>  					compatible = "via,vt8500-device-clock";
> -					clocks = <&plla>;
> -					divisor-reg = <0x300>;
> +					clocks = <&pllb>;
> +					divisor-reg = <0x348>;
> +					enable-reg = <0x250>;
> +					enable-bit = <10>;
>  				};
>  
> -				sdhc: sdhc {
> +				clksdhc: sdhc {
>  					#clock-cells = <0>;
>  					compatible = "via,vt8500-device-clock";
>  					clocks = <&pllb>;
> @@ -94,6 +96,13 @@
>  			};
>  		};
>  
> +		pwm: pwm@d8220000 {
> +			#pwm-cells = <2>;
> +			compatible = "via,vt8500-pwm";
> +			reg = <0xd8220000 0x100>;
> +			clocks = <&clkpwm>;
> +		};
> +
>  		timer@d8130100 {
>  			compatible = "via,vt8500-timer";
>  			reg = <0xd8130100 0x28>;

Please don't pull Patch 1/3 to any trees. I will include in a
pull-request for arm-soc with other dts updates later on.

Patch 2/3 + 3/3 can be taken now or Ack'd and I'll do a pull-request
later.

Regards
Tony P