[PATCH 0/2] DT binding support for ECAP & EHRPWM driver

[PATCH 0/2] DT binding support for ECAP & EHRPWM driver

[Philip, Avinash]

These patch set adds
1. DT binding support in drivers.
2. Add custom of_xlate function to request set period & polarity of
   pwm device from client drivers DT.

This has been tested on AM335X-EVM for PWM backlight based on tree
[1] merged with [2].
1. linux-next/master
2. linux-omap-dt/for_3.7/dts_part2

Philip, Avinash (2):
  pwm: pwm-tiecap: Add device-tree binding support for APWM driver
  pwm: pwm-tiehrpwm: Add device-tree binding support for EHRPWM driver

 .../devicetree/bindings/pwm/pwm-tiecap.txt         |   26 +++++
 .../devicetree/bindings/pwm/pwm-tiehrpwm.txt       |   26 +++++
 drivers/pwm/pwm-tiecap.c                           |  108 +++++++++++++++++++
 drivers/pwm/pwm-tiehrpwm.c                         |  110 ++++++++++++++++++++
 4 files changed, 270 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt

[PATCH 1/2] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

[Philip, Avinash]

Add support for device-tree binding and of_xlate for ECAP APWM driver.
of_xlate provides ECAP APWM polarity configuration from client driver
device-tree.
Also size of pwm-cells set to 3 to support pwm channel number, pwm
period & polarity configuration from device tree.

Add platform data with has_configspace as member. Set has_configspace as
true for platforms which has common config space for enabling clock
gating.

Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
---
:000000 100644 0000000... e93745d... A	Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
:100644 100644 081471f... 776b7ca... M	drivers/pwm/pwm-tiecap.c
:000000 100644 0000000... 0c19dcb... A	include/linux/platform_data/ti-pwmss.h
 .../devicetree/bindings/pwm/pwm-tiecap.txt         |   26 +++++
 drivers/pwm/pwm-tiecap.c                           |  105 ++++++++++++++++++++
 include/linux/platform_data/ti-pwmss.h             |   23 +++++
 3 files changed, 154 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt b/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
new file mode 100644
index 0000000..e93745d
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
@@ -0,0 +1,26 @@
+TI SOC ECAP based APWM controller
+
+Required properties:
+- compatible: Must be "ti,am33xx-ecap"
+- #pwm-cells: Should be 3. Number of cells being used to specify PWM property.
+  First cell specifies the per-chip index of the PWM to use, the second
+  cell is the period cycle in nanoseconds and the third cell is the polarity
+  of PWM output. Polarity 0 gives normal polarity and 1 gives inversed
+  polarity (inverse duty cycle)
+- reg: physical base address and size of the registers map. For am33xx,
+  2 register maps are present (ECAP register space & PWM subsystem common
+  config space). Order should be maintained with ECAP register map as first
+  entry & PWM subsystem common config space as second entry.
+
+Optional properties:
+- ti,hwmods: Name of the hwmod associated to the ECAP:
+  "ecap<x>", <x> being the 0-based instance number from the HW spec
+
+Example:
+
+ecap0: ecap@0 {
+	compatible = "ti,am33xx-ecap";
+	#pwm-cells = <3>;
+	reg = <0x48300100 0x80 0x48300000 0x10>;
+	ti,hwmods = "ecap0";
+};
diff --git a/drivers/pwm/pwm-tiecap.c b/drivers/pwm/pwm-tiecap.c
index 081471f..776b7ca 100644
--- a/drivers/pwm/pwm-tiecap.c
+++ b/drivers/pwm/pwm-tiecap.c
@@ -25,6 +25,9 @@
 #include <linux/clk.h>
 #include <linux/pm_runtime.h>
 #include <linux/pwm.h>
+#include <linux/of_device.h>
+#include <linux/platform_data/ti-pwmss.h>
+#include <linux/pinctrl/consumer.h>
 
 /* ECAP registers and bits definitions */
 #define CAP1			0x08
@@ -37,10 +40,16 @@
 #define ECCTL2_SYNC_SEL_DISA	(BIT(7) | BIT(6))
 #define ECCTL2_TSCTR_FREERUN	BIT(4)
 
+#define PWMSS_CLKCONFIG		8
+#define PWMSS_ECAP_CLK_EN	BIT(0)
+
+#define PWM_CELL_SIZE		3
+
 struct ecap_pwm_chip {
 	struct pwm_chip	chip;
 	unsigned int	clk_rate;
 	void __iomem	*mmio_base;
+	void __iomem	*config_base;
 };
 
 static inline struct ecap_pwm_chip *to_ecap_pwm_chip(struct pwm_chip *chip)
@@ -184,12 +193,60 @@ static const struct pwm_ops ecap_pwm_ops = {
 	.owner		= THIS_MODULE,
 };
 
+static struct pwm_device *of_ecap_xlate(struct pwm_chip *chip,
+		const struct of_phandle_args *args)
+{
+	struct pwm_device *pwm;
+
+	if (chip->of_pwm_n_cells < PWM_CELL_SIZE)
+		return ERR_PTR(-EINVAL);
+
+	if (args->args[0] >= chip->npwm)
+		return ERR_PTR(-EINVAL);
+
+	pwm = pwm_request_from_chip(chip, args->args[0], NULL);
+	if (IS_ERR(pwm))
+		return pwm;
+
+	pwm_set_period(pwm, args->args[1]);
+	pwm_set_polarity(pwm, args->args[2]);
+	return pwm;
+}
+
+static struct pwmss_platform_data am33xx_data = {
+	.has_configspace	= true,
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id ecap_of_match[] = {
+	{
+		.compatible	= "ti,am33xx-ecap",
+		.data		= &am33xx_data,
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, ecap_of_match);
+#endif
+
 static int __devinit ecap_pwm_probe(struct platform_device *pdev)
 {
 	int ret;
 	struct resource *r;
 	struct clk *clk;
 	struct ecap_pwm_chip *pc;
+	struct pwmss_platform_data *pdata = pdev->dev.platform_data;
+	const struct of_device_id *match;
+	u16 regval;
+	struct pinctrl *pinctrl;
+
+	match = of_match_device(of_match_ptr(ecap_of_match), &pdev->dev);
+
+	if (match)
+		pdata = (struct pwmss_platform_data *)match->data;
+
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl))
+		dev_warn(&pdev->dev, "failed to configure pins from driver\n");
 
 	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
 	if (!pc) {
@@ -211,6 +268,8 @@ static int __devinit ecap_pwm_probe(struct platform_device *pdev)
 
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &ecap_pwm_ops;
+	pc->chip.of_xlate = of_ecap_xlate;
+	pc->chip.of_pwm_n_cells = PWM_CELL_SIZE;
 	pc->chip.base = -1;
 	pc->chip.npwm = 1;
 
@@ -231,13 +290,56 @@ static int __devinit ecap_pwm_probe(struct platform_device *pdev)
 	}
 
 	pm_runtime_enable(&pdev->dev);
+
+	/*
+	 * Some platform has extra PWM-subsystem common config space
+	 * and requires special handling of clock gating.
+	 */
+	if (pdata && pdata->has_configspace) {
+		r = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+		if (!r) {
+			dev_err(&pdev->dev, "no memory resource defined\n");
+			ret = -ENODEV;
+			goto err_disable_clock;
+		}
+
+		pc->config_base = devm_ioremap(&pdev->dev, r->start,
+				resource_size(r));
+		if (!pc->config_base) {
+			dev_err(&pdev->dev, "failed to ioremap() registers\n");
+			ret = -EADDRNOTAVAIL;
+			goto err_disable_clock;
+		}
+
+		/* Enable ECAP clock gating at PWM-subsystem common config */
+		pm_runtime_get_sync(&pdev->dev);
+		regval = readw(pc->config_base + PWMSS_CLKCONFIG);
+		regval |= PWMSS_ECAP_CLK_EN;
+		writew(regval, pc->config_base + PWMSS_CLKCONFIG);
+		pm_runtime_put_sync(&pdev->dev);
+	}
+
 	platform_set_drvdata(pdev, pc);
 	return 0;
+
+err_disable_clock:
+	pm_runtime_disable(&pdev->dev);
+	return ret;
 }
 
 static int __devexit ecap_pwm_remove(struct platform_device *pdev)
 {
 	struct ecap_pwm_chip *pc = platform_get_drvdata(pdev);
+	u16 regval;
+
+	if (pc->config_base) {
+		/* Disable ECAP clock gating at PWM-subsystem common config */
+		pm_runtime_get_sync(&pdev->dev);
+		regval = readw(pc->config_base + PWMSS_CLKCONFIG);
+		regval &= ~PWMSS_ECAP_CLK_EN;
+		writew(regval, pc->config_base + PWMSS_CLKCONFIG);
+		pm_runtime_put_sync(&pdev->dev);
+	}
 
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
@@ -247,6 +349,9 @@ static int __devexit ecap_pwm_remove(struct platform_device *pdev)
 static struct platform_driver ecap_pwm_driver = {
 	.driver = {
 		.name = "ecap",
+#ifdef CONFIG_OF
+		.of_match_table = of_match_ptr(ecap_of_match),
+#endif
 	},
 	.probe = ecap_pwm_probe,
 	.remove = __devexit_p(ecap_pwm_remove),
diff --git a/include/linux/platform_data/ti-pwmss.h b/include/linux/platform_data/ti-pwmss.h
new file mode 100644
index 0000000..0c19dcb
--- /dev/null
+++ b/include/linux/platform_data/ti-pwmss.h
@@ -0,0 +1,23 @@
+/*
+ * TI PWM subsystem
+ *
+ * Copyright (C) {2012} Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _TI_PWMSS_H
+#define _TI_PWMSS_H
+
+struct pwmss_platform_data {
+	bool has_configspace; /* set true for platforms has config space */
+};
+
+#endif
-- 
1.7.0.4

[PATCH 2/2] pwm: pwm-tiehrpwm: Add device-tree binding support for EHRPWM driver

[Philip, Avinash]

Add support for device-tree binding and of_xlate for EHRWPM driver.
of_xlate provides EHRPWM polarity configuration from client driver
device-tree.
Also size of pwm-cells set to 3 to support pwm channel number, pwm
period & polarity configuration from device tree.

Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
---
:000000 100644 0000000... 05d9d63... A	Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
:100644 100644 caf00fe... ae23c2b... M	drivers/pwm/pwm-tiehrpwm.c
 .../devicetree/bindings/pwm/pwm-tiehrpwm.txt       |   26 +++++
 drivers/pwm/pwm-tiehrpwm.c                         |  107 ++++++++++++++++++++
 2 files changed, 133 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
new file mode 100644
index 0000000..05d9d63
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
@@ -0,0 +1,26 @@
+TI SOC EHRPWM based PWM controller
+
+Required properties:
+- compatible : Must be "ti,am33xx-ehrpwm"
+- #pwm-cells: Should be 3. Number of cells being used to specify PWM property.
+  First cell specifies the per-chip index of the PWM to use, the second
+  cell is the period cycle in nanoseconds and the third cell is the
+  polarity of PWM output. Polarity 0 gives normal polarity and 1 gives
+  inversed polarity (inverse duty cycle)
+- reg: physical base address and size of the registers map. For am33xx,
+  2 register maps are present (EHRPWM register space & PWM subsystem common
+  config space). Order should be maintained with EHRPWM register map as first
+  entry & PWM subsystem common config space as second entry.
+
+Optional properties:
+- ti,hwmods: Name of the hwmod associated to the EHRPWM:
+  "ehrpwm<x>", <x> being the 0-based instance number from the HW spec
+
+Example:
+
+ehrpwm0: ehrpwm@0 {
+	compatible = "ti,am33xx-ehrpwm";
+	#pwm-cells = <3>;
+	reg = <0x48300200 0x100 0x48300000 0x10>;
+	ti,hwmods = "ehrpwm0";
+};
diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index caf00fe..ae23c2b 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -25,6 +25,9 @@
 #include <linux/err.h>
 #include <linux/clk.h>
 #include <linux/pm_runtime.h>
+#include <linux/of_device.h>
+#include <linux/platform_data/ti-pwmss.h>
+#include <linux/pinctrl/consumer.h>
 
 /* EHRPWM registers and bits definitions */
 
@@ -107,12 +110,18 @@
 #define AQCSFRC_CSFA_FRCHIGH	BIT(1)
 #define AQCSFRC_CSFA_DISSWFRC	(BIT(1) | BIT(0))
 
+#define PWMSS_CLKCONFIG		0x8
+#define PWMSS_EHRPWM_CLK_EN	BIT(8)
+
+#define PWM_CELL_SIZE		3
+
 #define NUM_PWM_CHANNEL		2	/* EHRPWM channels */
 
 struct ehrpwm_pwm_chip {
 	struct pwm_chip	chip;
 	unsigned int	clk_rate;
 	void __iomem	*mmio_base;
+	void __iomem	*config_base;
 	unsigned long period_cycles[NUM_PWM_CHANNEL];
 	enum pwm_polarity polarity[NUM_PWM_CHANNEL];
 };
@@ -392,12 +401,60 @@ static const struct pwm_ops ehrpwm_pwm_ops = {
 	.owner		= THIS_MODULE,
 };
 
+static struct pwm_device *of_ehrpwm_xlate(struct pwm_chip *chip,
+		const struct of_phandle_args *args)
+{
+	struct pwm_device *pwm;
+
+	if (chip->of_pwm_n_cells < PWM_CELL_SIZE)
+		return ERR_PTR(-EINVAL);
+
+	if (args->args[0] >= chip->npwm)
+		return ERR_PTR(-EINVAL);
+
+	pwm = pwm_request_from_chip(chip, args->args[0], NULL);
+	if (IS_ERR(pwm))
+		return pwm;
+
+	pwm_set_period(pwm, args->args[1]);
+	pwm_set_polarity(pwm, args->args[2]);
+	return pwm;
+}
+
+static struct pwmss_platform_data am33xx_data = {
+	.has_configspace	= true,
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id ehrpwm_of_match[] = {
+	{
+		.compatible	= "ti,am33xx-ehrpwm",
+		.data		= &am33xx_data,
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, ehrpwm_of_match);
+#endif
+
 static int __devinit ehrpwm_pwm_probe(struct platform_device *pdev)
 {
 	int ret;
 	struct resource *r;
 	struct clk *clk;
 	struct ehrpwm_pwm_chip *pc;
+	struct pwmss_platform_data *pdata = pdev->dev.platform_data;
+	const struct of_device_id *match;
+	u16 regval;
+	struct pinctrl *pinctrl;
+
+	match = of_match_device(of_match_ptr(ehrpwm_of_match), &pdev->dev);
+
+	if (match)
+		pdata = (struct pwmss_platform_data *)match->data;
+
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl))
+		dev_warn(&pdev->dev, "failed to configure pins from driver\n");
 
 	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
 	if (!pc) {
@@ -419,6 +476,8 @@ static int __devinit ehrpwm_pwm_probe(struct platform_device *pdev)
 
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &ehrpwm_pwm_ops;
+	pc->chip.of_xlate = of_ehrpwm_xlate;
+	pc->chip.of_pwm_n_cells = PWM_CELL_SIZE;
 	pc->chip.base = -1;
 	pc->chip.npwm = NUM_PWM_CHANNEL;
 
@@ -439,13 +498,58 @@ static int __devinit ehrpwm_pwm_probe(struct platform_device *pdev)
 	}
 
 	pm_runtime_enable(&pdev->dev);
+
+	/*
+	 * Few platform has extra PWM-subsystem common config space
+	 * and requires special handling of clock gating.
+	 */
+
+	if (pdata && pdata->has_configspace) {
+
+		r = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+		if (!r) {
+			dev_err(&pdev->dev, "no memory resource defined\n");
+			ret = -ENODEV;
+			goto err_disable_clock;
+		}
+
+		pc->config_base = devm_ioremap(&pdev->dev, r->start,
+				resource_size(r));
+		if (!pc->config_base) {
+			dev_err(&pdev->dev, "failed to ioremap() registers\n");
+			ret = -EADDRNOTAVAIL;
+			goto err_disable_clock;
+		}
+
+		/* Enable EHRPWM clock gating at PWM-subsystem common config */
+		pm_runtime_get_sync(&pdev->dev);
+		regval = readw(pc->config_base + PWMSS_CLKCONFIG);
+		regval |= PWMSS_EHRPWM_CLK_EN;
+		writew(regval, pc->config_base + PWMSS_CLKCONFIG);
+		pm_runtime_put_sync(&pdev->dev);
+	}
+
 	platform_set_drvdata(pdev, pc);
 	return 0;
+
+err_disable_clock:
+	pm_runtime_disable(&pdev->dev);
+	return ret;
 }
 
 static int __devexit ehrpwm_pwm_remove(struct platform_device *pdev)
 {
 	struct ehrpwm_pwm_chip *pc = platform_get_drvdata(pdev);
+	u16 regval;
+
+	if (pc->config_base) {
+		/* Disable EHRPWM clock gating at PWM-subsystem common config */
+		pm_runtime_get_sync(&pdev->dev);
+		regval = readw(pc->config_base + PWMSS_CLKCONFIG);
+		regval &= ~PWMSS_EHRPWM_CLK_EN;
+		writew(regval, pc->config_base + PWMSS_CLKCONFIG);
+		pm_runtime_put_sync(&pdev->dev);
+	}
 
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
@@ -455,6 +559,9 @@ static int __devexit ehrpwm_pwm_remove(struct platform_device *pdev)
 static struct platform_driver ehrpwm_pwm_driver = {
 	.driver = {
 		.name = "ehrpwm",
+#ifdef CONFIG_OF
+		.of_match_table = of_match_ptr(ehrpwm_of_match),
+#endif
 	},
 	.probe = ehrpwm_pwm_probe,
 	.remove = __devexit_p(ehrpwm_pwm_remove),
-- 
1.7.0.4

Re: [PATCH 1/2] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

[Thierry Reding]

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

On Wed, Sep 26, 2012 at 04:57:42PM +0530, Philip, Avinash wrote:
[...]
> +#include <linux/platform_data/ti-pwmss.h>
[...]
> +static struct pwmss_platform_data am33xx_data = {
> +	.has_configspace	= true,
> +};

This structure is defined in a public header. I don't see why it has to,
given that it's only used to associate some data with an of_device_id
entry below. Since AM33xx never had anything but OF support in the
mainline kernel I don't think we should add platform data.

> +#ifdef CONFIG_OF
> +static const struct of_device_id ecap_of_match[] = {
> +	{
> +		.compatible	= "ti,am33xx-ecap",
> +		.data		= &am33xx_data,
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ecap_of_match);
> +#endif
> +

I don't quite see why we need to pass this platform data to the device
at all since there is no other variant anyway. I think this should only
be added if this turns out to be required at some point.

>  static int __devinit ecap_pwm_probe(struct platform_device *pdev)
>  {
>  	int ret;
>  	struct resource *r;
>  	struct clk *clk;
>  	struct ecap_pwm_chip *pc;
> +	struct pwmss_platform_data *pdata = pdev->dev.platform_data;
> +	const struct of_device_id *match;
> +	u16 regval;
> +	struct pinctrl *pinctrl;
> +
> +	match = of_match_device(of_match_ptr(ecap_of_match), &pdev->dev);

I've never seen of_match_ptr() used this way. Maybe you should think
about not #ifdef'ing OF specific code at all. That way ecap_of_match
will always exist and you could use the IS_ENABLED() macro to
conditionalize at compile time. The compiler will probably be clever
enough to optimize the ecap_of_match away if OF is not enabled.

Given my comment earlier, since AM33xx is OF only you might just as well
add a depends on OF to this driver and things will become a lot easier.

> +
> +	if (match)
> +		pdata = (struct pwmss_platform_data *)match->data;
> +
> +	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> +	if (IS_ERR(pinctrl))
> +		dev_warn(&pdev->dev, "failed to configure pins from driver\n");
>  
>  	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
>  	if (!pc) {
> @@ -211,6 +268,8 @@ static int __devinit ecap_pwm_probe(struct platform_device *pdev)
>  
>  	pc->chip.dev = &pdev->dev;
>  	pc->chip.ops = &ecap_pwm_ops;
> +	pc->chip.of_xlate = of_ecap_xlate;
> +	pc->chip.of_pwm_n_cells = PWM_CELL_SIZE;
>  	pc->chip.base = -1;
>  	pc->chip.npwm = 1;
>  
> @@ -231,13 +290,56 @@ static int __devinit ecap_pwm_probe(struct platform_device *pdev)
>  	}
>  
>  	pm_runtime_enable(&pdev->dev);
> +
> +	/*
> +	 * Some platform has extra PWM-subsystem common config space
> +	 * and requires special handling of clock gating.
> +	 */
> +	if (pdata && pdata->has_configspace) {
> +		r = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +		if (!r) {
> +			dev_err(&pdev->dev, "no memory resource defined\n");
> +			ret = -ENODEV;
> +			goto err_disable_clock;
> +		}
> +
> +		pc->config_base = devm_ioremap(&pdev->dev, r->start,
> +				resource_size(r));
> +		if (!pc->config_base) {
> +			dev_err(&pdev->dev, "failed to ioremap() registers\n");
> +			ret = -EADDRNOTAVAIL;
> +			goto err_disable_clock;
> +		}

Isn't this missing a request_mem_region()? I assume you don't do that
here because you need the same region in the EHRPWM driver, right? This
should be indication enough that the design is not right here. I think
we need a proper abstraction here. Can this not be done via PM runtime
support? If not, maybe this should be represented by clock objects since
the bit obviously enables a clock.

> +
> +		/* Enable ECAP clock gating at PWM-subsystem common config */
> +		pm_runtime_get_sync(&pdev->dev);
> +		regval = readw(pc->config_base + PWMSS_CLKCONFIG);
> +		regval |= PWMSS_ECAP_CLK_EN;
> +		writew(regval, pc->config_base + PWMSS_CLKCONFIG);
> +		pm_runtime_put_sync(&pdev->dev);
> +	}
> +
>  	platform_set_drvdata(pdev, pc);
>  	return 0;
> +
> +err_disable_clock:
> +	pm_runtime_disable(&pdev->dev);
> +	return ret;
>  }
>  
>  static int __devexit ecap_pwm_remove(struct platform_device *pdev)
>  {
>  	struct ecap_pwm_chip *pc = platform_get_drvdata(pdev);
> +	u16 regval;
> +
> +	if (pc->config_base) {
> +		/* Disable ECAP clock gating at PWM-subsystem common config */
> +		pm_runtime_get_sync(&pdev->dev);
> +		regval = readw(pc->config_base + PWMSS_CLKCONFIG);
> +		regval &= ~PWMSS_ECAP_CLK_EN;
> +		writew(regval, pc->config_base + PWMSS_CLKCONFIG);
> +		pm_runtime_put_sync(&pdev->dev);
> +	}
>  
>  	pm_runtime_put_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> @@ -247,6 +349,9 @@ static int __devexit ecap_pwm_remove(struct platform_device *pdev)
>  static struct platform_driver ecap_pwm_driver = {
>  	.driver = {
>  		.name = "ecap",
> +#ifdef CONFIG_OF
> +		.of_match_table = of_match_ptr(ecap_of_match),
> +#endif

If you use of_match_ptr() you don't need the additional #ifdef
CONFIG_OF. But as I already said I don't think you need this at all
anyway. You should really just depend on OF and be done with it.

Thierry

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

Re: [PATCH 2/2] pwm: pwm-tiehrpwm: Add device-tree binding support for EHRPWM driver

[Thierry Reding]

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

On Wed, Sep 26, 2012 at 04:57:43PM +0530, Philip, Avinash wrote:
> Add support for device-tree binding and of_xlate for EHRWPM driver.
> of_xlate provides EHRPWM polarity configuration from client driver
> device-tree.
> Also size of pwm-cells set to 3 to support pwm channel number, pwm
> period & polarity configuration from device tree.

Oh, I forgot to mention this in my reply to the previous patch, but you
should consistently use PWM to refer to PWM devices, so the above should
be "PWM channel number" and "PWM period & polarity". And the property is
named "#pwm-cells".

> Signed-off-by: Philip, Avinash <avinashphilip-l0cyMroinI0@public.gmane.org>
> ---
> :000000 100644 0000000... 05d9d63... A	Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
> :100644 100644 caf00fe... ae23c2b... M	drivers/pwm/pwm-tiehrpwm.c
>  .../devicetree/bindings/pwm/pwm-tiehrpwm.txt       |   26 +++++
>  drivers/pwm/pwm-tiehrpwm.c                         |  107 ++++++++++++++++++++
>  2 files changed, 133 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
> new file mode 100644
> index 0000000..05d9d63
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
> @@ -0,0 +1,26 @@
> +TI SOC EHRPWM based PWM controller
> +
> +Required properties:
> +- compatible : Must be "ti,am33xx-ehrpwm"
> +- #pwm-cells: Should be 3. Number of cells being used to specify PWM property.
> +  First cell specifies the per-chip index of the PWM to use, the second
> +  cell is the period cycle in nanoseconds and the third cell is the

"period cycle" doesn't make much sense. It should be "period" only. This
also applies to the previous patch.

> +  polarity of PWM output. Polarity 0 gives normal polarity and 1 gives
> +  inversed polarity (inverse duty cycle)

I don't think "inversed" exists. It should be either "inverted polarity"
or "inverse polarity".

> +- reg: physical base address and size of the registers map. For am33xx,
> +  2 register maps are present (EHRPWM register space & PWM subsystem common
> +  config space). Order should be maintained with EHRPWM register map as first
> +  entry & PWM subsystem common config space as second entry.
> +
> +Optional properties:
> +- ti,hwmods: Name of the hwmod associated to the EHRPWM:
> +  "ehrpwm<x>", <x> being the 0-based instance number from the HW spec

I don't see where this property is used. There is no code in this patch
that parses it.

> +static struct pwm_device *of_ehrpwm_xlate(struct pwm_chip *chip,
> +		const struct of_phandle_args *args)
> +{
> +	struct pwm_device *pwm;
> +
> +	if (chip->of_pwm_n_cells < PWM_CELL_SIZE)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (args->args[0] >= chip->npwm)
> +		return ERR_PTR(-EINVAL);
> +
> +	pwm = pwm_request_from_chip(chip, args->args[0], NULL);
> +	if (IS_ERR(pwm))
> +		return pwm;
> +
> +	pwm_set_period(pwm, args->args[1]);
> +	pwm_set_polarity(pwm, args->args[2]);
> +	return pwm;
> +}

This is an exact duplicate of the ECAP's of_xlate(). Maybe we should
make this part of the PWM core. If so it is probably safer to define the
values for the third cell as flags, where the polarity is encoded in bit
0, and make the function handle this accordingly to allow other bits to
be added in the future.

The same comments as for patch 1 apply to the rest of this patch as
well.

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 1/2] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

[Sekhar Nori]

Hi Thierry,

On 10/2/2012 11:30 AM, Thierry Reding wrote:
> On Wed, Sep 26, 2012 at 04:57:42PM +0530, Philip, Avinash wrote:
> [...]
>> +#include <linux/platform_data/ti-pwmss.h>
> [...]
>> +static struct pwmss_platform_data am33xx_data = {
>> +	.has_configspace	= true,
>> +};
> 
> This structure is defined in a public header. I don't see why it has to,
> given that it's only used to associate some data with an of_device_id
> entry below. Since AM33xx never had anything but OF support in the
> mainline kernel I don't think we should add platform data.

Avinash probably introduced platform data because the same PWM IP is
used in older DaVinci family SoCs (DA830 and DA850) which are not
converted to DT. There are existing boards for those SoCs  (supported in
mainline) which could benefit from this driver.

Thanks,
Sekhar

Re: [PATCH 1/2] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

[Thierry Reding]

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

On Tue, Oct 02, 2012 at 12:46:16PM +0530, Sekhar Nori wrote:
> Hi Thierry,
> 
> On 10/2/2012 11:30 AM, Thierry Reding wrote:
> > On Wed, Sep 26, 2012 at 04:57:42PM +0530, Philip, Avinash wrote:
> > [...]
> >> +#include <linux/platform_data/ti-pwmss.h>
> > [...]
> >> +static struct pwmss_platform_data am33xx_data = {
> >> +	.has_configspace	= true,
> >> +};
> > 
> > This structure is defined in a public header. I don't see why it has to,
> > given that it's only used to associate some data with an of_device_id
> > entry below. Since AM33xx never had anything but OF support in the
> > mainline kernel I don't think we should add platform data.
> 
> Avinash probably introduced platform data because the same PWM IP is
> used in older DaVinci family SoCs (DA830 and DA850) which are not
> converted to DT. There are existing boards for those SoCs  (supported in
> mainline) which could benefit from this driver.

Okay. If that's the case platform data should be added along with
support for those SoCs. Ideally, of course, the DaVinci boards would be
converted to DT first so we wouldn't have to introduce platform data
just to get rid of it when the conversion does take place. Until now it
seems like the boards have managed to get by without PWM support so
maybe they just don't use or need it?

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 1/2] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

[Sekhar Nori]

On 10/2/2012 1:37 PM, Thierry Reding wrote:
> On Tue, Oct 02, 2012 at 12:46:16PM +0530, Sekhar Nori wrote:
>> Hi Thierry,
>>
>> On 10/2/2012 11:30 AM, Thierry Reding wrote:
>>> On Wed, Sep 26, 2012 at 04:57:42PM +0530, Philip, Avinash wrote:
>>> [...]
>>>> +#include <linux/platform_data/ti-pwmss.h>
>>> [...]
>>>> +static struct pwmss_platform_data am33xx_data = {
>>>> +	.has_configspace	= true,
>>>> +};
>>>
>>> This structure is defined in a public header. I don't see why it has to,
>>> given that it's only used to associate some data with an of_device_id
>>> entry below. Since AM33xx never had anything but OF support in the
>>> mainline kernel I don't think we should add platform data.
>>
>> Avinash probably introduced platform data because the same PWM IP is
>> used in older DaVinci family SoCs (DA830 and DA850) which are not
>> converted to DT. There are existing boards for those SoCs  (supported in
>> mainline) which could benefit from this driver.
> 
> Okay. If that's the case platform data should be added along with
> support for those SoCs. Ideally, of course, the DaVinci boards would be
> converted to DT first so we wouldn't have to introduce platform data
> just to get rid of it when the conversion does take place. Until now it
> seems like the boards have managed to get by without PWM support so
> maybe they just don't use or need it?

On top of my head, the DA850 EVM uses PWM for LCD backlight control.
Attempts were made in the past to support PWM on this board back when
Bill Gatliff was attempting to get a framework accepted. So, I guess the
boards were waiting for a framework to materialize ;-)

I posted some patches to add basic DT support for DA850 EVM. It will be
a while before the entire board is converted over. Also, platforms like
DA830 have lesser number of users so it will likely not be converted at all.

It is fair to add platform data along with SoC support. Thanks!

Regards,
Sekhar

RE: [PATCH 1/2] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

[Philip, Avinash]

On Tue, Oct 02, 2012 at 11:30:14, Thierry Reding wrote:
> On Wed, Sep 26, 2012 at 04:57:42PM +0530, Philip, Avinash wrote:
> [...]
> > +#include <linux/platform_data/ti-pwmss.h>
> [...]
> > +static struct pwmss_platform_data am33xx_data = {
> > +	.has_configspace	= true,
> > +};
> 
> This structure is defined in a public header. I don't see why it has to,
> given that it's only used to associate some data with an of_device_id
> entry below. Since AM33xx never had anything but OF support in the
> mainline kernel I don't think we should add platform data.

I will correct it. I was going through Sekhar's reply.

> 
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id ecap_of_match[] = {
> > +	{
> > +		.compatible	= "ti,am33xx-ecap",
> > +		.data		= &am33xx_data,
> > +	},
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, ecap_of_match);
> > +#endif
> > +
> 
> I don't quite see why we need to pass this platform data to the device
> at all since there is no other variant anyway. I think this should only
> be added if this turns out to be required at some point.
> 
> >  static int __devinit ecap_pwm_probe(struct platform_device *pdev)
> >  {
> >  	int ret;
> >  	struct resource *r;
> >  	struct clk *clk;
> >  	struct ecap_pwm_chip *pc;
> > +	struct pwmss_platform_data *pdata = pdev->dev.platform_data;
> > +	const struct of_device_id *match;
> > +	u16 regval;
> > +	struct pinctrl *pinctrl;
> > +
> > +	match = of_match_device(of_match_ptr(ecap_of_match), &pdev->dev);
> 
> I've never seen of_match_ptr() used this way. Maybe you should think
> about not #ifdef'ing OF specific code at all. That way ecap_of_match
> will always exist and you could use the IS_ENABLED() macro to
> conditionalize at compile time. The compiler will probably be clever
> enough to optimize the ecap_of_match away if OF is not enabled.

I will correct it.

> 
> Given my comment earlier, since AM33xx is OF only you might just as well
> add a depends on OF to this driver and things will become a lot easier.

I will check & correct it.

> 
> > +
> > +	if (match)
> > +		pdata = (struct pwmss_platform_data *)match->data;
> > +
> > +	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> > +	if (IS_ERR(pinctrl))
> > +		dev_warn(&pdev->dev, "failed to configure pins from driver\n");
> >  
> >  	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> >  	if (!pc) {
> > @@ -211,6 +268,8 @@ static int __devinit ecap_pwm_probe(struct platform_device *pdev)
> >  
> >  	pc->chip.dev = &pdev->dev;
> >  	pc->chip.ops = &ecap_pwm_ops;
> > +	pc->chip.of_xlate = of_ecap_xlate;
> > +	pc->chip.of_pwm_n_cells = PWM_CELL_SIZE;
> >  	pc->chip.base = -1;
> >  	pc->chip.npwm = 1;
> >  
> > @@ -231,13 +290,56 @@ static int __devinit ecap_pwm_probe(struct platform_device *pdev)
> >  	}
> >  
> >  	pm_runtime_enable(&pdev->dev);
> > +
> > +	/*
> > +	 * Some platform has extra PWM-subsystem common config space
> > +	 * and requires special handling of clock gating.
> > +	 */
> > +	if (pdata && pdata->has_configspace) {
> > +		r = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +		if (!r) {
> > +			dev_err(&pdev->dev, "no memory resource defined\n");
> > +			ret = -ENODEV;
> > +			goto err_disable_clock;
> > +		}
> > +
> > +		pc->config_base = devm_ioremap(&pdev->dev, r->start,
> > +				resource_size(r));
> > +		if (!pc->config_base) {
> > +			dev_err(&pdev->dev, "failed to ioremap() registers\n");
> > +			ret = -EADDRNOTAVAIL;
> > +			goto err_disable_clock;
> > +		}
> 
> Isn't this missing a request_mem_region()? I assume you don't do that
> here because you need the same region in the EHRPWM driver, right?

request_mem_region() is avoided as this region is shared across PWM
sub modules ECAP & EHRPWM. 

> This should be indication enough that the design is not right here.
> I think we need a proper abstraction here. Can this not be done via
> PM runtime support? If not, maybe this should be represented by
> clock objects since the bit obviously enables a clock.

It is not done as part of PM runtime as this is has nothing to
do with clock tree of the SOC. The bits we were enabling here
should consider as an enable of the individual sub module as
part of IP integration. Hence we were handling these subsystem
module enable in the driver itself.

> 
> > +
> > +		/* Enable ECAP clock gating at PWM-subsystem common config */
> > +		pm_runtime_get_sync(&pdev->dev);
> > +		regval = readw(pc->config_base + PWMSS_CLKCONFIG);
> > +		regval |= PWMSS_ECAP_CLK_EN;
> > +		writew(regval, pc->config_base + PWMSS_CLKCONFIG);
> > +		pm_runtime_put_sync(&pdev->dev);
> > +	}
> > +
> >  	platform_set_drvdata(pdev, pc);
> >  	return 0;
> > +
> > +err_disable_clock:
> > +	pm_runtime_disable(&pdev->dev);
> > +	return ret;
> >  }
> >  
> >  static int __devexit ecap_pwm_remove(struct platform_device *pdev)
> >  {
> >  	struct ecap_pwm_chip *pc = platform_get_drvdata(pdev);
> > +	u16 regval;
> > +
> > +	if (pc->config_base) {
> > +		/* Disable ECAP clock gating at PWM-subsystem common config */
> > +		pm_runtime_get_sync(&pdev->dev);
> > +		regval = readw(pc->config_base + PWMSS_CLKCONFIG);
> > +		regval &= ~PWMSS_ECAP_CLK_EN;
> > +		writew(regval, pc->config_base + PWMSS_CLKCONFIG);
> > +		pm_runtime_put_sync(&pdev->dev);
> > +	}
> >  
> >  	pm_runtime_put_sync(&pdev->dev);
> >  	pm_runtime_disable(&pdev->dev);
> > @@ -247,6 +349,9 @@ static int __devexit ecap_pwm_remove(struct platform_device *pdev)
> >  static struct platform_driver ecap_pwm_driver = {
> >  	.driver = {
> >  		.name = "ecap",
> > +#ifdef CONFIG_OF
> > +		.of_match_table = of_match_ptr(ecap_of_match),
> > +#endif
> 
> If you use of_match_ptr() you don't need the additional #ifdef
> CONFIG_OF. But as I already said I don't think you need this at all
> anyway. You should really just depend on OF and be done with it.

Ok I understood & will correct it.

Thanks
Avinash

> 
> Thierry
> 

RE: [PATCH 2/2] pwm: pwm-tiehrpwm: Add device-tree binding support for EHRPWM driver

[Philip, Avinash]

On Tue, Oct 02, 2012 at 11:41:43, Thierry Reding wrote:
> On Wed, Sep 26, 2012 at 04:57:43PM +0530, Philip, Avinash wrote:
> > Add support for device-tree binding and of_xlate for EHRWPM driver.
> > of_xlate provides EHRPWM polarity configuration from client driver
> > device-tree.
> > Also size of pwm-cells set to 3 to support pwm channel number, pwm
> > period & polarity configuration from device tree.
> 
> Oh, I forgot to mention this in my reply to the previous patch, but you
> should consistently use PWM to refer to PWM devices, so the above should
> be "PWM channel number" and "PWM period & polarity". And the property is
> named "#pwm-cells".

I will correct it.

> 
> > Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> > ---
> > :000000 100644 0000000... 05d9d63... A	Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
> > :100644 100644 caf00fe... ae23c2b... M	drivers/pwm/pwm-tiehrpwm.c
> >  .../devicetree/bindings/pwm/pwm-tiehrpwm.txt       |   26 +++++
> >  drivers/pwm/pwm-tiehrpwm.c                         |  107 ++++++++++++++++++++
> >  2 files changed, 133 insertions(+), 0 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
> > new file mode 100644
> > index 0000000..05d9d63
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
> > @@ -0,0 +1,26 @@
> > +TI SOC EHRPWM based PWM controller
> > +
> > +Required properties:
> > +- compatible : Must be "ti,am33xx-ehrpwm"
> > +- #pwm-cells: Should be 3. Number of cells being used to specify PWM property.
> > +  First cell specifies the per-chip index of the PWM to use, the second
> > +  cell is the period cycle in nanoseconds and the third cell is the
> 
> "period cycle" doesn't make much sense. It should be "period" only. This
> also applies to the previous patch.

I will correct it.

> 
> > +  polarity of PWM output. Polarity 0 gives normal polarity and 1 gives
> > +  inversed polarity (inverse duty cycle)
> 
> I don't think "inversed" exists. It should be either "inverted polarity"
> or "inverse polarity".
> 
> > +- reg: physical base address and size of the registers map. For am33xx,
> > +  2 register maps are present (EHRPWM register space & PWM subsystem common
> > +  config space). Order should be maintained with EHRPWM register map as first
> > +  entry & PWM subsystem common config space as second entry.
> > +
> > +Optional properties:
> > +- ti,hwmods: Name of the hwmod associated to the EHRPWM:
> > +  "ehrpwm<x>", <x> being the 0-based instance number from the HW spec
> 
> I don't see where this property is used. There is no code in this patch
> that parses it.

This data used by omap_hwmod layer to create platform devices. This is part
of omap hwmod implementation.

> 
> > +static struct pwm_device *of_ehrpwm_xlate(struct pwm_chip *chip,
> > +		const struct of_phandle_args *args)
> > +{
> > +	struct pwm_device *pwm;
> > +
> > +	if (chip->of_pwm_n_cells < PWM_CELL_SIZE)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	if (args->args[0] >= chip->npwm)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	pwm = pwm_request_from_chip(chip, args->args[0], NULL);
> > +	if (IS_ERR(pwm))
> > +		return pwm;
> > +
> > +	pwm_set_period(pwm, args->args[1]);
> > +	pwm_set_polarity(pwm, args->args[2]);
> > +	return pwm;
> > +}
> 
> This is an exact duplicate of the ECAP's of_xlate(). Maybe we should
> make this part of the PWM core. If so it is probably safer to define the
> values for the third cell as flags, where the polarity is encoded in bit
> 0, and make the function handle this accordingly to allow other bits to
> be added in the future.

Custom of_xlate support is provided as suggested while the discussion of
"Adding support for configuring polarity in PWM framework".
https://lkml.org/lkml/2012/7/16/177

without custom of_xlate() support, PWM drivers has to populate
chip->of_pwm_n_cells = x;
as this is hard coded to 2 in pwm/core.c.

if (!chip->of_xlate) {
	chip->of_xlate = of_pwm_simple_xlate;
	chip->of_pwm_n_cells = 2;


Thanks
Avinash
 
> 
> The same comments as for patch 1 apply to the rest of this patch as
> well.
> 
> Thierry
> 

Re: [PATCH 1/2] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

[Thierry Reding]

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

On Mon, Oct 08, 2012 at 01:31:19PM +0000, Philip, Avinash wrote:
> On Tue, Oct 02, 2012 at 11:30:14, Thierry Reding wrote:
> > On Wed, Sep 26, 2012 at 04:57:42PM +0530, Philip, Avinash wrote:
[...]
> > > @@ -231,13 +290,56 @@ static int __devinit ecap_pwm_probe(struct platform_device *pdev)
> > >  	}
> > >  
> > >  	pm_runtime_enable(&pdev->dev);
> > > +
> > > +	/*
> > > +	 * Some platform has extra PWM-subsystem common config space
> > > +	 * and requires special handling of clock gating.
> > > +	 */
> > > +	if (pdata && pdata->has_configspace) {
> > > +		r = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > > +		if (!r) {
> > > +			dev_err(&pdev->dev, "no memory resource defined\n");
> > > +			ret = -ENODEV;
> > > +			goto err_disable_clock;
> > > +		}
> > > +
> > > +		pc->config_base = devm_ioremap(&pdev->dev, r->start,
> > > +				resource_size(r));
> > > +		if (!pc->config_base) {
> > > +			dev_err(&pdev->dev, "failed to ioremap() registers\n");
> > > +			ret = -EADDRNOTAVAIL;
> > > +			goto err_disable_clock;
> > > +		}
> > 
> > Isn't this missing a request_mem_region()? I assume you don't do that
> > here because you need the same region in the EHRPWM driver, right?
> 
> request_mem_region() is avoided as this region is shared across PWM
> sub modules ECAP & EHRPWM. 
> 
> > This should be indication enough that the design is not right here.
> > I think we need a proper abstraction here. Can this not be done via
> > PM runtime support? If not, maybe this should be represented by
> > clock objects since the bit obviously enables a clock.
> 
> It is not done as part of PM runtime as this is has nothing to
> do with clock tree of the SOC. The bits we were enabling here
> should consider as an enable of the individual sub module as
> part of IP integration. Hence we were handling these subsystem
> module enable in the driver itself.

My point remains valid: you shouldn't be able to access the same
register through two different drivers. That's one of the reasons, if
not the only reasen, why the request_mem_region() function exists. I
think you should add some abstraction to provide this functionality to
the drivers. I assume that eventually there will be more than just the
PWM cores that require access to this register.

Thierry

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

Re: [PATCH 2/2] pwm: pwm-tiehrpwm: Add device-tree binding support for EHRPWM driver

[Thierry Reding]

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

On Mon, Oct 08, 2012 at 01:31:20PM +0000, Philip, Avinash wrote:
> On Tue, Oct 02, 2012 at 11:41:43, Thierry Reding wrote:
> > On Wed, Sep 26, 2012 at 04:57:43PM +0530, Philip, Avinash wrote:
[...]
> > > +- reg: physical base address and size of the registers map. For am33xx,
> > > +  2 register maps are present (EHRPWM register space & PWM subsystem common
> > > +  config space). Order should be maintained with EHRPWM register map as first
> > > +  entry & PWM subsystem common config space as second entry.
> > > +
> > > +Optional properties:
> > > +- ti,hwmods: Name of the hwmod associated to the EHRPWM:
> > > +  "ehrpwm<x>", <x> being the 0-based instance number from the HW spec
> > 
> > I don't see where this property is used. There is no code in this patch
> > that parses it.
> 
> This data used by omap_hwmod layer to create platform devices. This is part
> of omap hwmod implementation.

Okay. I've heard about hwmod but I wasn't aware of how it was used in
the context of device tree.

> > > +static struct pwm_device *of_ehrpwm_xlate(struct pwm_chip *chip,
> > > +		const struct of_phandle_args *args)
> > > +{
> > > +	struct pwm_device *pwm;
> > > +
> > > +	if (chip->of_pwm_n_cells < PWM_CELL_SIZE)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	if (args->args[0] >= chip->npwm)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	pwm = pwm_request_from_chip(chip, args->args[0], NULL);
> > > +	if (IS_ERR(pwm))
> > > +		return pwm;
> > > +
> > > +	pwm_set_period(pwm, args->args[1]);
> > > +	pwm_set_polarity(pwm, args->args[2]);
> > > +	return pwm;
> > > +}
> > 
> > This is an exact duplicate of the ECAP's of_xlate(). Maybe we should
> > make this part of the PWM core. If so it is probably safer to define the
> > values for the third cell as flags, where the polarity is encoded in bit
> > 0, and make the function handle this accordingly to allow other bits to
> > be added in the future.
> 
> Custom of_xlate support is provided as suggested while the discussion of
> "Adding support for configuring polarity in PWM framework".
> https://lkml.org/lkml/2012/7/16/177
> 
> without custom of_xlate() support, PWM drivers has to populate
> chip->of_pwm_n_cells = x;
> as this is hard coded to 2 in pwm/core.c.
> 
> if (!chip->of_xlate) {
> 	chip->of_xlate = of_pwm_simple_xlate;
> 	chip->of_pwm_n_cells = 2;

It's absolutely fine to provide a custom implementation. All I'm saying
is that we should add a 3-cell variant of of_pwm_simple_xlate() instead
of having to duplicate it for every chip that supports inversion of the
polarity.

Maybe something like of_pwm_xlate_with_flags()?

Thierry

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

RE: [PATCH 1/2] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

[Philip, Avinash]

On Mon, Oct 08, 2012 at 19:09:51, Thierry Reding wrote:
> On Mon, Oct 08, 2012 at 01:31:19PM +0000, Philip, Avinash wrote:
> > On Tue, Oct 02, 2012 at 11:30:14, Thierry Reding wrote:
> > > On Wed, Sep 26, 2012 at 04:57:42PM +0530, Philip, Avinash wrote:
> [...]
> > > > @@ -231,13 +290,56 @@ static int __devinit ecap_pwm_probe(struct platform_device *pdev)
> > > >  	}
> > > >  
> > > >  	pm_runtime_enable(&pdev->dev);
> > > > +
> > > > +	/*
> > > > +	 * Some platform has extra PWM-subsystem common config space
> > > > +	 * and requires special handling of clock gating.
> > > > +	 */
> > > > +	if (pdata && pdata->has_configspace) {
> > > > +		r = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > > > +		if (!r) {
> > > > +			dev_err(&pdev->dev, "no memory resource defined\n");
> > > > +			ret = -ENODEV;
> > > > +			goto err_disable_clock;
> > > > +		}
> > > > +
> > > > +		pc->config_base = devm_ioremap(&pdev->dev, r->start,
> > > > +				resource_size(r));
> > > > +		if (!pc->config_base) {
> > > > +			dev_err(&pdev->dev, "failed to ioremap() registers\n");
> > > > +			ret = -EADDRNOTAVAIL;
> > > > +			goto err_disable_clock;
> > > > +		}
> > > 
> > > Isn't this missing a request_mem_region()? I assume you don't do that
> > > here because you need the same region in the EHRPWM driver, right?
> > 
> > request_mem_region() is avoided as this region is shared across PWM
> > sub modules ECAP & EHRPWM. 
> > 
> > > This should be indication enough that the design is not right here.
> > > I think we need a proper abstraction here. Can this not be done via
> > > PM runtime support? If not, maybe this should be represented by
> > > clock objects since the bit obviously enables a clock.
> > 
> > It is not done as part of PM runtime as this is has nothing to
> > do with clock tree of the SOC. The bits we were enabling here
> > should consider as an enable of the individual sub module as
> > part of IP integration. Hence we were handling these subsystem
> > module enable in the driver itself.
> 
> My point remains valid: you shouldn't be able to access the same
> register through two different drivers. That's one of the reasons, if
> not the only reasen, why the request_mem_region() function exists. I
> think you should add some abstraction to provide this functionality to
> the drivers. I assume that eventually there will be more than just the
> PWM cores that require access to this register.

Enabling of PWM sub modules from CONFIG space is only present in AM33xx
as part of IP integration (ECAP, EHRPWM & EQEP).

Enabling of sub modules (ECAP, EHRPWM & EQEP) should do in CONFIG space. 
Hence sub module drivers are accessing CONFIG space without reserving it 
Individually from drivers (request_mem_region()).

Can you describe/point how it can be handled in a separate Abstraction layer
as this is shared across ECAP, EHRPWM & EQEP (EQEP driver is not yet available).


Adding it as part of PM runtime support is not a right place.
In PWM-SS, CONFIG Space is "Shared" across different sub modules and hence
can't be considered as a part of clock tree.

Thanks
Avinash

> 
> Thierry
> 

Re: [PATCH 1/2] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

[Thierry Reding]

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

On Tue, Oct 09, 2012 at 12:36:53PM +0000, Philip, Avinash wrote:
> On Mon, Oct 08, 2012 at 19:09:51, Thierry Reding wrote:
> > On Mon, Oct 08, 2012 at 01:31:19PM +0000, Philip, Avinash wrote:
> > > On Tue, Oct 02, 2012 at 11:30:14, Thierry Reding wrote:
> > > > On Wed, Sep 26, 2012 at 04:57:42PM +0530, Philip, Avinash wrote:
> > [...]
> > > > > @@ -231,13 +290,56 @@ static int __devinit ecap_pwm_probe(struct platform_device *pdev)
> > > > >  	}
> > > > >  
> > > > >  	pm_runtime_enable(&pdev->dev);
> > > > > +
> > > > > +	/*
> > > > > +	 * Some platform has extra PWM-subsystem common config space
> > > > > +	 * and requires special handling of clock gating.
> > > > > +	 */
> > > > > +	if (pdata && pdata->has_configspace) {
> > > > > +		r = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > > > > +		if (!r) {
> > > > > +			dev_err(&pdev->dev, "no memory resource defined\n");
> > > > > +			ret = -ENODEV;
> > > > > +			goto err_disable_clock;
> > > > > +		}
> > > > > +
> > > > > +		pc->config_base = devm_ioremap(&pdev->dev, r->start,
> > > > > +				resource_size(r));
> > > > > +		if (!pc->config_base) {
> > > > > +			dev_err(&pdev->dev, "failed to ioremap() registers\n");
> > > > > +			ret = -EADDRNOTAVAIL;
> > > > > +			goto err_disable_clock;
> > > > > +		}
> > > > 
> > > > Isn't this missing a request_mem_region()? I assume you don't do that
> > > > here because you need the same region in the EHRPWM driver, right?
> > > 
> > > request_mem_region() is avoided as this region is shared across PWM
> > > sub modules ECAP & EHRPWM. 
> > > 
> > > > This should be indication enough that the design is not right here.
> > > > I think we need a proper abstraction here. Can this not be done via
> > > > PM runtime support? If not, maybe this should be represented by
> > > > clock objects since the bit obviously enables a clock.
> > > 
> > > It is not done as part of PM runtime as this is has nothing to
> > > do with clock tree of the SOC. The bits we were enabling here
> > > should consider as an enable of the individual sub module as
> > > part of IP integration. Hence we were handling these subsystem
> > > module enable in the driver itself.
> > 
> > My point remains valid: you shouldn't be able to access the same
> > register through two different drivers. That's one of the reasons, if
> > not the only reasen, why the request_mem_region() function exists. I
> > think you should add some abstraction to provide this functionality to
> > the drivers. I assume that eventually there will be more than just the
> > PWM cores that require access to this register.
> 
> Enabling of PWM sub modules from CONFIG space is only present in AM33xx
> as part of IP integration (ECAP, EHRPWM & EQEP).
> 
> Enabling of sub modules (ECAP, EHRPWM & EQEP) should do in CONFIG space. 
> Hence sub module drivers are accessing CONFIG space without reserving it 
> Individually from drivers (request_mem_region()).
> 
> Can you describe/point how it can be handled in a separate Abstraction layer
> as this is shared across ECAP, EHRPWM & EQEP (EQEP driver is not yet available).

Perhaps something like a global function to enable and disable the
clocks would be enough. You could for instance have a driver that
requests the config space memory region and provides this global
function so that it can be used by the ECAP, EHRPWM and EQEP.

A more elaborate scheme would possibly involve making the new device a
parent of the ECAP, EHRPWM and EQEP devices and have their drivers
lookup the parent to determine which configurator device to operate on.

Thierry

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