[PATCH v3] PWM: PXA: add device tree support to PWM driver

[PATCH v3] PWM: PXA: add device tree support to PWM driver

[Mike Dunn]

This patch adds device tree support to the PXA's PWM driver.  Only an OF match
table is added; nothing needs to be extracted from the device tree node.  The
existing ID table is reused for the match table data.

Tested on a Palm Treo 680 (both platform data and DT cases).

Signed-off-by: Mike Dunn <mikedunn-kFrNdAxtuftBDgjK7y7TUQ@public.gmane.org>
---
Changle log:
v3:
- remove support for the polarity flag
- remove per-chip pwm index cell; define custom of_xlate()
   (now #pwm-cells = <1>)
- "compatible" strings for all devices added to OF match table
- various stylistic changes recommended by reviewers

v2:
- of_match_table contains only the "pxa250-pwm" compatible string; require one
  device instance per pwm
- add Documentation/devicetree/bindings/pwm/pxa-pwm.txt
- add support for polarity flag in DT and implement set_polarity() method
  (the treo 680 inverts the signal between pwm out and backlight)
- return -EINVAL instead of -ENODEV if platform data or DT node not found
- output dev_info string if platform data missing
- expanded CC list of patch

 Documentation/devicetree/bindings/pwm/pxa-pwm.txt | 31 ++++++++++++
 arch/arm/boot/dts/pxa27x.dtsi                     | 24 +++++++++
 drivers/pwm/pwm-pxa.c                             | 62 +++++++++++++++++++++++
 3 files changed, 117 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pxa-pwm.txt

diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
new file mode 100644
index 0000000..6fcf90c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
@@ -0,0 +1,31 @@
+Marvell PWM controller
+
+Required properties:
+- compatible: should be one of:
+  - "marvell,pxa250-pwm"
+  - "marvell,pxa270-pwm"
+  - "marvell,pxa168-pwm"
+  - "marvell,pxa910-pwm"
+- reg: physical base address and length of the registers used by the PWM channel
+  NB: One device instance must be created for each PWM that is used, so the
+  length covers only the register window for one PWM output, not that of the
+  entire PWM controller.  Currently length is 0x10 for all supported devices.
+- #pwm-cells: should be 1.  This cell is used to specify the period in
+  nanoseconds.  (Because one device instance is created for each PWM output,
+  the per-chip index is superflous and not used.)
+
+Example PWM device node:
+
+pwm0: pwm@40b00000 {
+	compatible = "marvell,pxa250-pwm";
+	reg = <0x40b00000 0x10>;
+	#pwm-cells = <1>;
+};
+
+Example PWM client node:
+
+backlight {
+	compatible = "pwm-backlight";
+	pwms = <&pwm0 5000000>;
+	...
+}
diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
index d7c5d72..5057c41 100644
--- a/arch/arm/boot/dts/pxa27x.dtsi
+++ b/arch/arm/boot/dts/pxa27x.dtsi
@@ -10,5 +10,29 @@
 			marvell,intc-priority;
 			marvell,intc-nr-irqs = <34>;
 		};
+
+		pwm0: pwm@40b00000 {
+			compatible = "marvell,pxa270-pwm";
+			reg = <0x40b00000 0x10>;
+			#pwm-cells = <1>;
+		};
+
+		pwm1: pwm@40b00010 {
+			compatible = "marvell,pxa270-pwm";
+			reg = <0x40b00010 0x10>;
+			#pwm-cells = <1>;
+		};
+
+		pwm2: pwm@40c00000 {
+			compatible = "marvell,pxa270-pwm";
+			reg = <0x40c00000 0x10>;
+			#pwm-cells = <1>;
+		};
+
+		pwm3: pwm@40c00010 {
+			compatible = "marvell,pxa270-pwm";
+			reg = <0x40c00010 0x10>;
+			#pwm-cells = <1>;
+		};
 	};
 };
diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index a4d2164..91b1cff 100644
--- a/drivers/pwm/pwm-pxa.c
+++ b/drivers/pwm/pwm-pxa.c
@@ -19,6 +19,7 @@
 #include <linux/clk.h>
 #include <linux/io.h>
 #include <linux/pwm.h>
+#include <linux/of_device.h>
 
 #include <asm/div64.h>
 
@@ -124,6 +125,59 @@ static struct pwm_ops pxa_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
+#ifdef CONFIG_OF
+/*
+ * Device tree users must create one device instance for each pwm channel.
+ * Hence we dispense with the HAS_SECONDARY_PWM and "tell" the original driver
+ * code that this is a single channel pxa25x-pwm.  Currently all devices are
+ * supported identically.
+ */
+static struct of_device_id pwm_of_match[] = {
+	{ .compatible = "marvell,pxa250-pwm", .data = &pwm_id_table[0]},
+	{ .compatible = "marvell,pxa270-pwm", .data = &pwm_id_table[0]},
+	{ .compatible = "marvell,pxa168-pwm", .data = &pwm_id_table[0]},
+	{ .compatible = "marvell,pxa910-pwm", .data = &pwm_id_table[0]},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pwm_of_match);
+
+static const struct platform_device_id *pxa_pwm_get_id_dt(struct device *dev)
+{
+	const struct of_device_id *id = of_match_device(pwm_of_match, dev);
+	if (id)
+		return (const struct platform_device_id *)id->data;
+	else
+		return NULL;
+}
+
+struct pwm_device *
+pxa_pwm_of_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
+{
+	struct pwm_device *pwm;
+
+	pwm = pwm_request_from_chip(pc, 0, NULL);
+	if (IS_ERR(pwm))
+		return pwm;
+
+	pwm_set_period(pwm, args->args[0]);
+
+	return pwm;
+}
+
+#else  /* !CONFIG_OF */
+static const struct platform_device_id *pxa_pwm_get_id_dt(struct device *dev)
+{
+	dev_err(dev, "missing platform data\n");
+	return NULL;
+}
+
+struct pwm_device *
+pxa_pwm_of_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
+{
+	return NULL;
+}
+#endif
+
 static int pwm_probe(struct platform_device *pdev)
 {
 	const struct platform_device_id *id = platform_get_device_id(pdev);
@@ -131,6 +185,11 @@ static int pwm_probe(struct platform_device *pdev)
 	struct resource *r;
 	int ret = 0;
 
+	if (id == NULL)		/* using device tree */
+		id = pxa_pwm_get_id_dt(&pdev->dev);
+	if (id == NULL)
+		return -EINVAL;
+
 	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
 	if (pwm == NULL) {
 		dev_err(&pdev->dev, "failed to allocate memory\n");
@@ -145,6 +204,8 @@ static int pwm_probe(struct platform_device *pdev)
 	pwm->chip.ops = &pxa_pwm_ops;
 	pwm->chip.base = -1;
 	pwm->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
+	pwm->chip.of_xlate = pxa_pwm_of_xlate;
+	pwm->chip.of_pwm_n_cells = 1;
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pwm->mmio_base = devm_ioremap_resource(&pdev->dev, r);
@@ -176,6 +237,7 @@ static struct platform_driver pwm_driver = {
 	.driver		= {
 		.name	= "pxa25x-pwm",
 		.owner	= THIS_MODULE,
+		.of_match_table	= of_match_ptr(pwm_of_match),
 	},
 	.probe		= pwm_probe,
 	.remove		= pwm_remove,
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] PWM: PXA: add device tree support to PWM driver

[Mike Dunn]

This patch adds device tree support to the PXA's PWM driver.  Only an OF match
table is added; nothing needs to be extracted from the device tree node.  The
existing ID table is reused for the match table data.

Tested on a Palm Treo 680 (both platform data and DT cases).

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
Changle log:
v3:
- remove support for the polarity flag
- remove per-chip pwm index cell; define custom of_xlate()
   (now #pwm-cells = <1>)
- "compatible" strings for all devices added to OF match table
- various stylistic changes recommended by reviewers

v2:
- of_match_table contains only the "pxa250-pwm" compatible string; require one
  device instance per pwm
- add Documentation/devicetree/bindings/pwm/pxa-pwm.txt
- add support for polarity flag in DT and implement set_polarity() method
  (the treo 680 inverts the signal between pwm out and backlight)
- return -EINVAL instead of -ENODEV if platform data or DT node not found
- output dev_info string if platform data missing
- expanded CC list of patch

 Documentation/devicetree/bindings/pwm/pxa-pwm.txt | 31 ++++++++++++
 arch/arm/boot/dts/pxa27x.dtsi                     | 24 +++++++++
 drivers/pwm/pwm-pxa.c                             | 62 +++++++++++++++++++++++
 3 files changed, 117 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pxa-pwm.txt

diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
new file mode 100644
index 0000000..6fcf90c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
@@ -0,0 +1,31 @@
+Marvell PWM controller
+
+Required properties:
+- compatible: should be one of:
+  - "marvell,pxa250-pwm"
+  - "marvell,pxa270-pwm"
+  - "marvell,pxa168-pwm"
+  - "marvell,pxa910-pwm"
+- reg: physical base address and length of the registers used by the PWM channel
+  NB: One device instance must be created for each PWM that is used, so the
+  length covers only the register window for one PWM output, not that of the
+  entire PWM controller.  Currently length is 0x10 for all supported devices.
+- #pwm-cells: should be 1.  This cell is used to specify the period in
+  nanoseconds.  (Because one device instance is created for each PWM output,
+  the per-chip index is superflous and not used.)
+
+Example PWM device node:
+
+pwm0: pwm@40b00000 {
+	compatible = "marvell,pxa250-pwm";
+	reg = <0x40b00000 0x10>;
+	#pwm-cells = <1>;
+};
+
+Example PWM client node:
+
+backlight {
+	compatible = "pwm-backlight";
+	pwms = <&pwm0 5000000>;
+	...
+}
diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
index d7c5d72..5057c41 100644
--- a/arch/arm/boot/dts/pxa27x.dtsi
+++ b/arch/arm/boot/dts/pxa27x.dtsi
@@ -10,5 +10,29 @@
 			marvell,intc-priority;
 			marvell,intc-nr-irqs = <34>;
 		};
+
+		pwm0: pwm@40b00000 {
+			compatible = "marvell,pxa270-pwm";
+			reg = <0x40b00000 0x10>;
+			#pwm-cells = <1>;
+		};
+
+		pwm1: pwm@40b00010 {
+			compatible = "marvell,pxa270-pwm";
+			reg = <0x40b00010 0x10>;
+			#pwm-cells = <1>;
+		};
+
+		pwm2: pwm@40c00000 {
+			compatible = "marvell,pxa270-pwm";
+			reg = <0x40c00000 0x10>;
+			#pwm-cells = <1>;
+		};
+
+		pwm3: pwm@40c00010 {
+			compatible = "marvell,pxa270-pwm";
+			reg = <0x40c00010 0x10>;
+			#pwm-cells = <1>;
+		};
 	};
 };
diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index a4d2164..91b1cff 100644
--- a/drivers/pwm/pwm-pxa.c
+++ b/drivers/pwm/pwm-pxa.c
@@ -19,6 +19,7 @@
 #include <linux/clk.h>
 #include <linux/io.h>
 #include <linux/pwm.h>
+#include <linux/of_device.h>
 
 #include <asm/div64.h>
 
@@ -124,6 +125,59 @@ static struct pwm_ops pxa_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
+#ifdef CONFIG_OF
+/*
+ * Device tree users must create one device instance for each pwm channel.
+ * Hence we dispense with the HAS_SECONDARY_PWM and "tell" the original driver
+ * code that this is a single channel pxa25x-pwm.  Currently all devices are
+ * supported identically.
+ */
+static struct of_device_id pwm_of_match[] = {
+	{ .compatible = "marvell,pxa250-pwm", .data = &pwm_id_table[0]},
+	{ .compatible = "marvell,pxa270-pwm", .data = &pwm_id_table[0]},
+	{ .compatible = "marvell,pxa168-pwm", .data = &pwm_id_table[0]},
+	{ .compatible = "marvell,pxa910-pwm", .data = &pwm_id_table[0]},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pwm_of_match);
+
+static const struct platform_device_id *pxa_pwm_get_id_dt(struct device *dev)
+{
+	const struct of_device_id *id = of_match_device(pwm_of_match, dev);
+	if (id)
+		return (const struct platform_device_id *)id->data;
+	else
+		return NULL;
+}
+
+struct pwm_device *
+pxa_pwm_of_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
+{
+	struct pwm_device *pwm;
+
+	pwm = pwm_request_from_chip(pc, 0, NULL);
+	if (IS_ERR(pwm))
+		return pwm;
+
+	pwm_set_period(pwm, args->args[0]);
+
+	return pwm;
+}
+
+#else  /* !CONFIG_OF */
+static const struct platform_device_id *pxa_pwm_get_id_dt(struct device *dev)
+{
+	dev_err(dev, "missing platform data\n");
+	return NULL;
+}
+
+struct pwm_device *
+pxa_pwm_of_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
+{
+	return NULL;
+}
+#endif
+
 static int pwm_probe(struct platform_device *pdev)
 {
 	const struct platform_device_id *id = platform_get_device_id(pdev);
@@ -131,6 +185,11 @@ static int pwm_probe(struct platform_device *pdev)
 	struct resource *r;
 	int ret = 0;
 
+	if (id == NULL)		/* using device tree */
+		id = pxa_pwm_get_id_dt(&pdev->dev);
+	if (id == NULL)
+		return -EINVAL;
+
 	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
 	if (pwm == NULL) {
 		dev_err(&pdev->dev, "failed to allocate memory\n");
@@ -145,6 +204,8 @@ static int pwm_probe(struct platform_device *pdev)
 	pwm->chip.ops = &pxa_pwm_ops;
 	pwm->chip.base = -1;
 	pwm->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
+	pwm->chip.of_xlate = pxa_pwm_of_xlate;
+	pwm->chip.of_pwm_n_cells = 1;
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pwm->mmio_base = devm_ioremap_resource(&pdev->dev, r);
@@ -176,6 +237,7 @@ static struct platform_driver pwm_driver = {
 	.driver		= {
 		.name	= "pxa25x-pwm",
 		.owner	= THIS_MODULE,
+		.of_match_table	= of_match_ptr(pwm_of_match),
 	},
 	.probe		= pwm_probe,
 	.remove		= pwm_remove,
-- 
1.8.1.5

[PATCH v3] PWM: PXA: add device tree support to PWM driver

[Mike Dunn]

This patch adds device tree support to the PXA's PWM driver.  Only an OF match
table is added; nothing needs to be extracted from the device tree node.  The
existing ID table is reused for the match table data.

Tested on a Palm Treo 680 (both platform data and DT cases).

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
Changle log:
v3:
- remove support for the polarity flag
- remove per-chip pwm index cell; define custom of_xlate()
   (now #pwm-cells = <1>)
- "compatible" strings for all devices added to OF match table
- various stylistic changes recommended by reviewers

v2:
- of_match_table contains only the "pxa250-pwm" compatible string; require one
  device instance per pwm
- add Documentation/devicetree/bindings/pwm/pxa-pwm.txt
- add support for polarity flag in DT and implement set_polarity() method
  (the treo 680 inverts the signal between pwm out and backlight)
- return -EINVAL instead of -ENODEV if platform data or DT node not found
- output dev_info string if platform data missing
- expanded CC list of patch

 Documentation/devicetree/bindings/pwm/pxa-pwm.txt | 31 ++++++++++++
 arch/arm/boot/dts/pxa27x.dtsi                     | 24 +++++++++
 drivers/pwm/pwm-pxa.c                             | 62 +++++++++++++++++++++++
 3 files changed, 117 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pxa-pwm.txt

diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
new file mode 100644
index 0000000..6fcf90c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
@@ -0,0 +1,31 @@
+Marvell PWM controller
+
+Required properties:
+- compatible: should be one of:
+  - "marvell,pxa250-pwm"
+  - "marvell,pxa270-pwm"
+  - "marvell,pxa168-pwm"
+  - "marvell,pxa910-pwm"
+- reg: physical base address and length of the registers used by the PWM channel
+  NB: One device instance must be created for each PWM that is used, so the
+  length covers only the register window for one PWM output, not that of the
+  entire PWM controller.  Currently length is 0x10 for all supported devices.
+- #pwm-cells: should be 1.  This cell is used to specify the period in
+  nanoseconds.  (Because one device instance is created for each PWM output,
+  the per-chip index is superflous and not used.)
+
+Example PWM device node:
+
+pwm0: pwm at 40b00000 {
+	compatible = "marvell,pxa250-pwm";
+	reg = <0x40b00000 0x10>;
+	#pwm-cells = <1>;
+};
+
+Example PWM client node:
+
+backlight {
+	compatible = "pwm-backlight";
+	pwms = <&pwm0 5000000>;
+	...
+}
diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
index d7c5d72..5057c41 100644
--- a/arch/arm/boot/dts/pxa27x.dtsi
+++ b/arch/arm/boot/dts/pxa27x.dtsi
@@ -10,5 +10,29 @@
 			marvell,intc-priority;
 			marvell,intc-nr-irqs = <34>;
 		};
+
+		pwm0: pwm at 40b00000 {
+			compatible = "marvell,pxa270-pwm";
+			reg = <0x40b00000 0x10>;
+			#pwm-cells = <1>;
+		};
+
+		pwm1: pwm at 40b00010 {
+			compatible = "marvell,pxa270-pwm";
+			reg = <0x40b00010 0x10>;
+			#pwm-cells = <1>;
+		};
+
+		pwm2: pwm at 40c00000 {
+			compatible = "marvell,pxa270-pwm";
+			reg = <0x40c00000 0x10>;
+			#pwm-cells = <1>;
+		};
+
+		pwm3: pwm at 40c00010 {
+			compatible = "marvell,pxa270-pwm";
+			reg = <0x40c00010 0x10>;
+			#pwm-cells = <1>;
+		};
 	};
 };
diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index a4d2164..91b1cff 100644
--- a/drivers/pwm/pwm-pxa.c
+++ b/drivers/pwm/pwm-pxa.c
@@ -19,6 +19,7 @@
 #include <linux/clk.h>
 #include <linux/io.h>
 #include <linux/pwm.h>
+#include <linux/of_device.h>
 
 #include <asm/div64.h>
 
@@ -124,6 +125,59 @@ static struct pwm_ops pxa_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
+#ifdef CONFIG_OF
+/*
+ * Device tree users must create one device instance for each pwm channel.
+ * Hence we dispense with the HAS_SECONDARY_PWM and "tell" the original driver
+ * code that this is a single channel pxa25x-pwm.  Currently all devices are
+ * supported identically.
+ */
+static struct of_device_id pwm_of_match[] = {
+	{ .compatible = "marvell,pxa250-pwm", .data = &pwm_id_table[0]},
+	{ .compatible = "marvell,pxa270-pwm", .data = &pwm_id_table[0]},
+	{ .compatible = "marvell,pxa168-pwm", .data = &pwm_id_table[0]},
+	{ .compatible = "marvell,pxa910-pwm", .data = &pwm_id_table[0]},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pwm_of_match);
+
+static const struct platform_device_id *pxa_pwm_get_id_dt(struct device *dev)
+{
+	const struct of_device_id *id = of_match_device(pwm_of_match, dev);
+	if (id)
+		return (const struct platform_device_id *)id->data;
+	else
+		return NULL;
+}
+
+struct pwm_device *
+pxa_pwm_of_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
+{
+	struct pwm_device *pwm;
+
+	pwm = pwm_request_from_chip(pc, 0, NULL);
+	if (IS_ERR(pwm))
+		return pwm;
+
+	pwm_set_period(pwm, args->args[0]);
+
+	return pwm;
+}
+
+#else  /* !CONFIG_OF */
+static const struct platform_device_id *pxa_pwm_get_id_dt(struct device *dev)
+{
+	dev_err(dev, "missing platform data\n");
+	return NULL;
+}
+
+struct pwm_device *
+pxa_pwm_of_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
+{
+	return NULL;
+}
+#endif
+
 static int pwm_probe(struct platform_device *pdev)
 {
 	const struct platform_device_id *id = platform_get_device_id(pdev);
@@ -131,6 +185,11 @@ static int pwm_probe(struct platform_device *pdev)
 	struct resource *r;
 	int ret = 0;
 
+	if (id == NULL)		/* using device tree */
+		id = pxa_pwm_get_id_dt(&pdev->dev);
+	if (id == NULL)
+		return -EINVAL;
+
 	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
 	if (pwm == NULL) {
 		dev_err(&pdev->dev, "failed to allocate memory\n");
@@ -145,6 +204,8 @@ static int pwm_probe(struct platform_device *pdev)
 	pwm->chip.ops = &pxa_pwm_ops;
 	pwm->chip.base = -1;
 	pwm->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
+	pwm->chip.of_xlate = pxa_pwm_of_xlate;
+	pwm->chip.of_pwm_n_cells = 1;
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pwm->mmio_base = devm_ioremap_resource(&pdev->dev, r);
@@ -176,6 +237,7 @@ static struct platform_driver pwm_driver = {
 	.driver		= {
 		.name	= "pxa25x-pwm",
 		.owner	= THIS_MODULE,
+		.of_match_table	= of_match_ptr(pwm_of_match),
 	},
 	.probe		= pwm_probe,
 	.remove		= pwm_remove,
-- 
1.8.1.5

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Marek Vasut]

Dear Mike Dunn,

> This patch adds device tree support to the PXA's PWM driver.  Only an OF
> match table is added; nothing needs to be extracted from the device tree
> node.  The existing ID table is reused for the match table data.
> 
> Tested on a Palm Treo 680 (both platform data and DT cases).
> 
> Signed-off-by: Mike Dunn <mikedunn-kFrNdAxtuftBDgjK7y7TUQ@public.gmane.org>
> ---
> Changle log:
> v3:
> - remove support for the polarity flag
> - remove per-chip pwm index cell; define custom of_xlate()
>    (now #pwm-cells = <1>)
> - "compatible" strings for all devices added to OF match table
> - various stylistic changes recommended by reviewers
> 
> v2:
> - of_match_table contains only the "pxa250-pwm" compatible string; require
> one device instance per pwm
> - add Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> - add support for polarity flag in DT and implement set_polarity() method
>   (the treo 680 inverts the signal between pwm out and backlight)
> - return -EINVAL instead of -ENODEV if platform data or DT node not found
> - output dev_info string if platform data missing
> - expanded CC list of patch
> 
>  Documentation/devicetree/bindings/pwm/pxa-pwm.txt | 31 ++++++++++++
>  arch/arm/boot/dts/pxa27x.dtsi                     | 24 +++++++++
>  drivers/pwm/pwm-pxa.c                             | 62
> +++++++++++++++++++++++ 3 files changed, 117 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt new file mode 100644
> index 0000000..6fcf90c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> @@ -0,0 +1,31 @@
> +Marvell PWM controller
> +
> +Required properties:
> +- compatible: should be one of:
> +  - "marvell,pxa250-pwm"
> +  - "marvell,pxa270-pwm"
> +  - "marvell,pxa168-pwm"
> +  - "marvell,pxa910-pwm"

This really is something I dont quite understand. Why should the driver list 
_every_ _single_ existing CPU that contains such PWM block? Is there any 
agreement about that? For me, it'd make much more sense to list only the CPUs 
where the IP block actually changed in some way, so that the differences can be 
discerned that way.

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Marek Vasut]

Dear Mike Dunn,

> This patch adds device tree support to the PXA's PWM driver.  Only an OF
> match table is added; nothing needs to be extracted from the device tree
> node.  The existing ID table is reused for the match table data.
> 
> Tested on a Palm Treo 680 (both platform data and DT cases).
> 
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> ---
> Changle log:
> v3:
> - remove support for the polarity flag
> - remove per-chip pwm index cell; define custom of_xlate()
>    (now #pwm-cells = <1>)
> - "compatible" strings for all devices added to OF match table
> - various stylistic changes recommended by reviewers
> 
> v2:
> - of_match_table contains only the "pxa250-pwm" compatible string; require
> one device instance per pwm
> - add Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> - add support for polarity flag in DT and implement set_polarity() method
>   (the treo 680 inverts the signal between pwm out and backlight)
> - return -EINVAL instead of -ENODEV if platform data or DT node not found
> - output dev_info string if platform data missing
> - expanded CC list of patch
> 
>  Documentation/devicetree/bindings/pwm/pxa-pwm.txt | 31 ++++++++++++
>  arch/arm/boot/dts/pxa27x.dtsi                     | 24 +++++++++
>  drivers/pwm/pwm-pxa.c                             | 62
> +++++++++++++++++++++++ 3 files changed, 117 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt new file mode 100644
> index 0000000..6fcf90c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> @@ -0,0 +1,31 @@
> +Marvell PWM controller
> +
> +Required properties:
> +- compatible: should be one of:
> +  - "marvell,pxa250-pwm"
> +  - "marvell,pxa270-pwm"
> +  - "marvell,pxa168-pwm"
> +  - "marvell,pxa910-pwm"

This really is something I dont quite understand. Why should the driver list 
_every_ _single_ existing CPU that contains such PWM block? Is there any 
agreement about that? For me, it'd make much more sense to list only the CPUs 
where the IP block actually changed in some way, so that the differences can be 
discerned that way.

Best regards,
Marek Vasut

[PATCH v3] PWM: PXA: add device tree support to PWM driver

[Marek Vasut]

Dear Mike Dunn,

> This patch adds device tree support to the PXA's PWM driver.  Only an OF
> match table is added; nothing needs to be extracted from the device tree
> node.  The existing ID table is reused for the match table data.
> 
> Tested on a Palm Treo 680 (both platform data and DT cases).
> 
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> ---
> Changle log:
> v3:
> - remove support for the polarity flag
> - remove per-chip pwm index cell; define custom of_xlate()
>    (now #pwm-cells = <1>)
> - "compatible" strings for all devices added to OF match table
> - various stylistic changes recommended by reviewers
> 
> v2:
> - of_match_table contains only the "pxa250-pwm" compatible string; require
> one device instance per pwm
> - add Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> - add support for polarity flag in DT and implement set_polarity() method
>   (the treo 680 inverts the signal between pwm out and backlight)
> - return -EINVAL instead of -ENODEV if platform data or DT node not found
> - output dev_info string if platform data missing
> - expanded CC list of patch
> 
>  Documentation/devicetree/bindings/pwm/pxa-pwm.txt | 31 ++++++++++++
>  arch/arm/boot/dts/pxa27x.dtsi                     | 24 +++++++++
>  drivers/pwm/pwm-pxa.c                             | 62
> +++++++++++++++++++++++ 3 files changed, 117 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt new file mode 100644
> index 0000000..6fcf90c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> @@ -0,0 +1,31 @@
> +Marvell PWM controller
> +
> +Required properties:
> +- compatible: should be one of:
> +  - "marvell,pxa250-pwm"
> +  - "marvell,pxa270-pwm"
> +  - "marvell,pxa168-pwm"
> +  - "marvell,pxa910-pwm"

This really is something I dont quite understand. Why should the driver list 
_every_ _single_ existing CPU that contains such PWM block? Is there any 
agreement about that? For me, it'd make much more sense to list only the CPUs 
where the IP block actually changed in some way, so that the differences can be 
discerned that way.

Best regards,
Marek Vasut

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Mike Dunn]

On 09/15/2013 07:07 AM, Marek Vasut wrote:
> Dear Mike Dunn,
> 
>> This patch adds device tree support to the PXA's PWM driver.  Only an OF
>> match table is added; nothing needs to be extracted from the device tree
>> node.  The existing ID table is reused for the match table data.
>>
>> Tested on a Palm Treo 680 (both platform data and DT cases).
>>
>> Signed-off-by: Mike Dunn <mikedunn-kFrNdAxtuftBDgjK7y7TUQ@public.gmane.org>
>> ---
>> Changle log:
>> v3:
>> - remove support for the polarity flag
>> - remove per-chip pwm index cell; define custom of_xlate()
>>    (now #pwm-cells = <1>)
>> - "compatible" strings for all devices added to OF match table
>> - various stylistic changes recommended by reviewers
>>
>> v2:
>> - of_match_table contains only the "pxa250-pwm" compatible string; require
>> one device instance per pwm
>> - add Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>> - add support for polarity flag in DT and implement set_polarity() method
>>   (the treo 680 inverts the signal between pwm out and backlight)
>> - return -EINVAL instead of -ENODEV if platform data or DT node not found
>> - output dev_info string if platform data missing
>> - expanded CC list of patch
>>
>>  Documentation/devicetree/bindings/pwm/pxa-pwm.txt | 31 ++++++++++++
>>  arch/arm/boot/dts/pxa27x.dtsi                     | 24 +++++++++
>>  drivers/pwm/pwm-pxa.c                             | 62
>> +++++++++++++++++++++++ 3 files changed, 117 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>> b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt new file mode 100644
>> index 0000000..6fcf90c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>> @@ -0,0 +1,31 @@
>> +Marvell PWM controller
>> +
>> +Required properties:
>> +- compatible: should be one of:
>> +  - "marvell,pxa250-pwm"
>> +  - "marvell,pxa270-pwm"
>> +  - "marvell,pxa168-pwm"
>> +  - "marvell,pxa910-pwm"
> 
> This really is something I dont quite understand. Why should the driver list 
> _every_ _single_ existing CPU that contains such PWM block? Is there any 
> agreement about that? For me, it'd make much more sense to list only the CPUs 
> where the IP block actually changed in some way, so that the differences can be 
> discerned that way.

I believe that this was Stephen's suggestion.

I actually don't object myself.  For the price of a few strings, it
- clearly shows which SoCs the driver supports
- ensures that any future differences are handled cleanly (e.g., if a hw bug in
one is discovered and a work-around is implemented)
- keeps thngs clean if support for another processor which does have pwm hw
differences is added

Especially the third point... otherwise, you have the case of a somewhat
confusing many-to-one mapping of processors to compatible strings.

Thanks,
Mike

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Mike Dunn]

On 09/15/2013 07:07 AM, Marek Vasut wrote:
> Dear Mike Dunn,
> 
>> This patch adds device tree support to the PXA's PWM driver.  Only an OF
>> match table is added; nothing needs to be extracted from the device tree
>> node.  The existing ID table is reused for the match table data.
>>
>> Tested on a Palm Treo 680 (both platform data and DT cases).
>>
>> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
>> ---
>> Changle log:
>> v3:
>> - remove support for the polarity flag
>> - remove per-chip pwm index cell; define custom of_xlate()
>>    (now #pwm-cells = <1>)
>> - "compatible" strings for all devices added to OF match table
>> - various stylistic changes recommended by reviewers
>>
>> v2:
>> - of_match_table contains only the "pxa250-pwm" compatible string; require
>> one device instance per pwm
>> - add Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>> - add support for polarity flag in DT and implement set_polarity() method
>>   (the treo 680 inverts the signal between pwm out and backlight)
>> - return -EINVAL instead of -ENODEV if platform data or DT node not found
>> - output dev_info string if platform data missing
>> - expanded CC list of patch
>>
>>  Documentation/devicetree/bindings/pwm/pxa-pwm.txt | 31 ++++++++++++
>>  arch/arm/boot/dts/pxa27x.dtsi                     | 24 +++++++++
>>  drivers/pwm/pwm-pxa.c                             | 62
>> +++++++++++++++++++++++ 3 files changed, 117 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>> b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt new file mode 100644
>> index 0000000..6fcf90c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>> @@ -0,0 +1,31 @@
>> +Marvell PWM controller
>> +
>> +Required properties:
>> +- compatible: should be one of:
>> +  - "marvell,pxa250-pwm"
>> +  - "marvell,pxa270-pwm"
>> +  - "marvell,pxa168-pwm"
>> +  - "marvell,pxa910-pwm"
> 
> This really is something I dont quite understand. Why should the driver list 
> _every_ _single_ existing CPU that contains such PWM block? Is there any 
> agreement about that? For me, it'd make much more sense to list only the CPUs 
> where the IP block actually changed in some way, so that the differences can be 
> discerned that way.

I believe that this was Stephen's suggestion.

I actually don't object myself.  For the price of a few strings, it
- clearly shows which SoCs the driver supports
- ensures that any future differences are handled cleanly (e.g., if a hw bug in
one is discovered and a work-around is implemented)
- keeps thngs clean if support for another processor which does have pwm hw
differences is added

Especially the third point... otherwise, you have the case of a somewhat
confusing many-to-one mapping of processors to compatible strings.

Thanks,
Mike

[PATCH v3] PWM: PXA: add device tree support to PWM driver

[Mike Dunn]

On 09/15/2013 07:07 AM, Marek Vasut wrote:
> Dear Mike Dunn,
> 
>> This patch adds device tree support to the PXA's PWM driver.  Only an OF
>> match table is added; nothing needs to be extracted from the device tree
>> node.  The existing ID table is reused for the match table data.
>>
>> Tested on a Palm Treo 680 (both platform data and DT cases).
>>
>> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
>> ---
>> Changle log:
>> v3:
>> - remove support for the polarity flag
>> - remove per-chip pwm index cell; define custom of_xlate()
>>    (now #pwm-cells = <1>)
>> - "compatible" strings for all devices added to OF match table
>> - various stylistic changes recommended by reviewers
>>
>> v2:
>> - of_match_table contains only the "pxa250-pwm" compatible string; require
>> one device instance per pwm
>> - add Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>> - add support for polarity flag in DT and implement set_polarity() method
>>   (the treo 680 inverts the signal between pwm out and backlight)
>> - return -EINVAL instead of -ENODEV if platform data or DT node not found
>> - output dev_info string if platform data missing
>> - expanded CC list of patch
>>
>>  Documentation/devicetree/bindings/pwm/pxa-pwm.txt | 31 ++++++++++++
>>  arch/arm/boot/dts/pxa27x.dtsi                     | 24 +++++++++
>>  drivers/pwm/pwm-pxa.c                             | 62
>> +++++++++++++++++++++++ 3 files changed, 117 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>> b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt new file mode 100644
>> index 0000000..6fcf90c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>> @@ -0,0 +1,31 @@
>> +Marvell PWM controller
>> +
>> +Required properties:
>> +- compatible: should be one of:
>> +  - "marvell,pxa250-pwm"
>> +  - "marvell,pxa270-pwm"
>> +  - "marvell,pxa168-pwm"
>> +  - "marvell,pxa910-pwm"
> 
> This really is something I dont quite understand. Why should the driver list 
> _every_ _single_ existing CPU that contains such PWM block? Is there any 
> agreement about that? For me, it'd make much more sense to list only the CPUs 
> where the IP block actually changed in some way, so that the differences can be 
> discerned that way.

I believe that this was Stephen's suggestion.

I actually don't object myself.  For the price of a few strings, it
- clearly shows which SoCs the driver supports
- ensures that any future differences are handled cleanly (e.g., if a hw bug in
one is discovered and a work-around is implemented)
- keeps thngs clean if support for another processor which does have pwm hw
differences is added

Especially the third point... otherwise, you have the case of a somewhat
confusing many-to-one mapping of processors to compatible strings.

Thanks,
Mike

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Marek Vasut]

Dear Mike Dunn,

> On 09/15/2013 07:07 AM, Marek Vasut wrote:
> > Dear Mike Dunn,
> > 
> >> This patch adds device tree support to the PXA's PWM driver.  Only an OF
> >> match table is added; nothing needs to be extracted from the device tree
> >> node.  The existing ID table is reused for the match table data.
> >> 
> >> Tested on a Palm Treo 680 (both platform data and DT cases).
> >> 
> >> Signed-off-by: Mike Dunn <mikedunn-kFrNdAxtuftBDgjK7y7TUQ@public.gmane.org>
> >> ---
> >> Changle log:
> >> v3:
> >> - remove support for the polarity flag
> >> - remove per-chip pwm index cell; define custom of_xlate()
> >> 
> >>    (now #pwm-cells = <1>)
> >> 
> >> - "compatible" strings for all devices added to OF match table
> >> - various stylistic changes recommended by reviewers
> >> 
> >> v2:
> >> - of_match_table contains only the "pxa250-pwm" compatible string;
> >> require one device instance per pwm
> >> - add Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> >> - add support for polarity flag in DT and implement set_polarity()
> >> method
> >> 
> >>   (the treo 680 inverts the signal between pwm out and backlight)
> >> 
> >> - return -EINVAL instead of -ENODEV if platform data or DT node not
> >> found - output dev_info string if platform data missing
> >> - expanded CC list of patch
> >> 
> >>  Documentation/devicetree/bindings/pwm/pxa-pwm.txt | 31 ++++++++++++
> >>  arch/arm/boot/dts/pxa27x.dtsi                     | 24 +++++++++
> >>  drivers/pwm/pwm-pxa.c                             | 62
> >> 
> >> +++++++++++++++++++++++ 3 files changed, 117 insertions(+)
> >> 
> >>  create mode 100644 Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> >> 
> >> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> >> b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt new file mode 100644
> >> index 0000000..6fcf90c
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> >> @@ -0,0 +1,31 @@
> >> +Marvell PWM controller
> >> +
> >> +Required properties:
> >> +- compatible: should be one of:
> >> +  - "marvell,pxa250-pwm"
> >> +  - "marvell,pxa270-pwm"
> >> +  - "marvell,pxa168-pwm"
> >> +  - "marvell,pxa910-pwm"
> > 
> > This really is something I dont quite understand. Why should the driver
> > list _every_ _single_ existing CPU that contains such PWM block? Is
> > there any agreement about that? For me, it'd make much more sense to
> > list only the CPUs where the IP block actually changed in some way, so
> > that the differences can be discerned that way.
> 
> I believe that this was Stephen's suggestion.
> 
> I actually don't object myself.  For the price of a few strings, it
> - clearly shows which SoCs the driver supports
> - ensures that any future differences are handled cleanly (e.g., if a hw
> bug in one is discovered and a work-around is implemented)
> - keeps thngs clean if support for another processor which does have pwm hw
> differences is added
> 
> Especially the third point... otherwise, you have the case of a somewhat
> confusing many-to-one mapping of processors to compatible strings.

OK, to push it ad-absurdum, shall we not start listing every single revision of 
the CPU as well ? Like pxa250a0 250a1 etc ? Yes , this many-to-one mapping is 
something I don't quite understand, that's why I'd love to see a good reasoning 
from Stephen.

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Marek Vasut]

Dear Mike Dunn,

> On 09/15/2013 07:07 AM, Marek Vasut wrote:
> > Dear Mike Dunn,
> > 
> >> This patch adds device tree support to the PXA's PWM driver.  Only an OF
> >> match table is added; nothing needs to be extracted from the device tree
> >> node.  The existing ID table is reused for the match table data.
> >> 
> >> Tested on a Palm Treo 680 (both platform data and DT cases).
> >> 
> >> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> >> ---
> >> Changle log:
> >> v3:
> >> - remove support for the polarity flag
> >> - remove per-chip pwm index cell; define custom of_xlate()
> >> 
> >>    (now #pwm-cells = <1>)
> >> 
> >> - "compatible" strings for all devices added to OF match table
> >> - various stylistic changes recommended by reviewers
> >> 
> >> v2:
> >> - of_match_table contains only the "pxa250-pwm" compatible string;
> >> require one device instance per pwm
> >> - add Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> >> - add support for polarity flag in DT and implement set_polarity()
> >> method
> >> 
> >>   (the treo 680 inverts the signal between pwm out and backlight)
> >> 
> >> - return -EINVAL instead of -ENODEV if platform data or DT node not
> >> found - output dev_info string if platform data missing
> >> - expanded CC list of patch
> >> 
> >>  Documentation/devicetree/bindings/pwm/pxa-pwm.txt | 31 ++++++++++++
> >>  arch/arm/boot/dts/pxa27x.dtsi                     | 24 +++++++++
> >>  drivers/pwm/pwm-pxa.c                             | 62
> >> 
> >> +++++++++++++++++++++++ 3 files changed, 117 insertions(+)
> >> 
> >>  create mode 100644 Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> >> 
> >> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> >> b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt new file mode 100644
> >> index 0000000..6fcf90c
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> >> @@ -0,0 +1,31 @@
> >> +Marvell PWM controller
> >> +
> >> +Required properties:
> >> +- compatible: should be one of:
> >> +  - "marvell,pxa250-pwm"
> >> +  - "marvell,pxa270-pwm"
> >> +  - "marvell,pxa168-pwm"
> >> +  - "marvell,pxa910-pwm"
> > 
> > This really is something I dont quite understand. Why should the driver
> > list _every_ _single_ existing CPU that contains such PWM block? Is
> > there any agreement about that? For me, it'd make much more sense to
> > list only the CPUs where the IP block actually changed in some way, so
> > that the differences can be discerned that way.
> 
> I believe that this was Stephen's suggestion.
> 
> I actually don't object myself.  For the price of a few strings, it
> - clearly shows which SoCs the driver supports
> - ensures that any future differences are handled cleanly (e.g., if a hw
> bug in one is discovered and a work-around is implemented)
> - keeps thngs clean if support for another processor which does have pwm hw
> differences is added
> 
> Especially the third point... otherwise, you have the case of a somewhat
> confusing many-to-one mapping of processors to compatible strings.

OK, to push it ad-absurdum, shall we not start listing every single revision of 
the CPU as well ? Like pxa250a0 250a1 etc ? Yes , this many-to-one mapping is 
something I don't quite understand, that's why I'd love to see a good reasoning 
from Stephen.

Best regards,
Marek Vasut

[PATCH v3] PWM: PXA: add device tree support to PWM driver

[Marek Vasut]

Dear Mike Dunn,

> On 09/15/2013 07:07 AM, Marek Vasut wrote:
> > Dear Mike Dunn,
> > 
> >> This patch adds device tree support to the PXA's PWM driver.  Only an OF
> >> match table is added; nothing needs to be extracted from the device tree
> >> node.  The existing ID table is reused for the match table data.
> >> 
> >> Tested on a Palm Treo 680 (both platform data and DT cases).
> >> 
> >> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> >> ---
> >> Changle log:
> >> v3:
> >> - remove support for the polarity flag
> >> - remove per-chip pwm index cell; define custom of_xlate()
> >> 
> >>    (now #pwm-cells = <1>)
> >> 
> >> - "compatible" strings for all devices added to OF match table
> >> - various stylistic changes recommended by reviewers
> >> 
> >> v2:
> >> - of_match_table contains only the "pxa250-pwm" compatible string;
> >> require one device instance per pwm
> >> - add Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> >> - add support for polarity flag in DT and implement set_polarity()
> >> method
> >> 
> >>   (the treo 680 inverts the signal between pwm out and backlight)
> >> 
> >> - return -EINVAL instead of -ENODEV if platform data or DT node not
> >> found - output dev_info string if platform data missing
> >> - expanded CC list of patch
> >> 
> >>  Documentation/devicetree/bindings/pwm/pxa-pwm.txt | 31 ++++++++++++
> >>  arch/arm/boot/dts/pxa27x.dtsi                     | 24 +++++++++
> >>  drivers/pwm/pwm-pxa.c                             | 62
> >> 
> >> +++++++++++++++++++++++ 3 files changed, 117 insertions(+)
> >> 
> >>  create mode 100644 Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> >> 
> >> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> >> b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt new file mode 100644
> >> index 0000000..6fcf90c
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> >> @@ -0,0 +1,31 @@
> >> +Marvell PWM controller
> >> +
> >> +Required properties:
> >> +- compatible: should be one of:
> >> +  - "marvell,pxa250-pwm"
> >> +  - "marvell,pxa270-pwm"
> >> +  - "marvell,pxa168-pwm"
> >> +  - "marvell,pxa910-pwm"
> > 
> > This really is something I dont quite understand. Why should the driver
> > list _every_ _single_ existing CPU that contains such PWM block? Is
> > there any agreement about that? For me, it'd make much more sense to
> > list only the CPUs where the IP block actually changed in some way, so
> > that the differences can be discerned that way.
> 
> I believe that this was Stephen's suggestion.
> 
> I actually don't object myself.  For the price of a few strings, it
> - clearly shows which SoCs the driver supports
> - ensures that any future differences are handled cleanly (e.g., if a hw
> bug in one is discovered and a work-around is implemented)
> - keeps thngs clean if support for another processor which does have pwm hw
> differences is added
> 
> Especially the third point... otherwise, you have the case of a somewhat
> confusing many-to-one mapping of processors to compatible strings.

OK, to push it ad-absurdum, shall we not start listing every single revision of 
the CPU as well ? Like pxa250a0 250a1 etc ? Yes , this many-to-one mapping is 
something I don't quite understand, that's why I'd love to see a good reasoning 
from Stephen.

Best regards,
Marek Vasut

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Stephen Warren]

On 09/13/2013 10:54 AM, Mike Dunn wrote:
> This patch adds device tree support to the PXA's PWM driver.  Only an OF match
> table is added; nothing needs to be extracted from the device tree node.  The
> existing ID table is reused for the match table data.
> 
> Tested on a Palm Treo 680 (both platform data and DT cases).

> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt

> +- compatible: should be one of:
> +  - "marvell,pxa250-pwm"
> +  - "marvell,pxa270-pwm"
> +  - "marvell,pxa168-pwm"
> +  - "marvell,pxa910-pwm"

Not just one of, but possible more than one...

> diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi

> +		pwm0: pwm@40b00000 {
> +			compatible = "marvell,pxa270-pwm";

I thought the assertion was that pax270 and pxa250 were both compatible?
If so, that should be:

		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Stephen Warren]

On 09/13/2013 10:54 AM, Mike Dunn wrote:
> This patch adds device tree support to the PXA's PWM driver.  Only an OF match
> table is added; nothing needs to be extracted from the device tree node.  The
> existing ID table is reused for the match table data.
> 
> Tested on a Palm Treo 680 (both platform data and DT cases).

> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt

> +- compatible: should be one of:
> +  - "marvell,pxa250-pwm"
> +  - "marvell,pxa270-pwm"
> +  - "marvell,pxa168-pwm"
> +  - "marvell,pxa910-pwm"

Not just one of, but possible more than one...

> diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi

> +		pwm0: pwm@40b00000 {
> +			compatible = "marvell,pxa270-pwm";

I thought the assertion was that pax270 and pxa250 were both compatible?
If so, that should be:

		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";

[PATCH v3] PWM: PXA: add device tree support to PWM driver

[Stephen Warren]

On 09/13/2013 10:54 AM, Mike Dunn wrote:
> This patch adds device tree support to the PXA's PWM driver.  Only an OF match
> table is added; nothing needs to be extracted from the device tree node.  The
> existing ID table is reused for the match table data.
> 
> Tested on a Palm Treo 680 (both platform data and DT cases).

> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt

> +- compatible: should be one of:
> +  - "marvell,pxa250-pwm"
> +  - "marvell,pxa270-pwm"
> +  - "marvell,pxa168-pwm"
> +  - "marvell,pxa910-pwm"

Not just one of, but possible more than one...

> diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi

> +		pwm0: pwm at 40b00000 {
> +			compatible = "marvell,pxa270-pwm";

I thought the assertion was that pax270 and pxa250 were both compatible?
If so, that should be:

		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Stephen Warren]

On 09/16/2013 09:10 AM, Marek Vasut wrote:
...
> OK, to push it ad-absurdum, shall we not start listing every single revision of 
> the CPU as well? Like pxa250a0 250a1 etc ? Yes , this many-to-one mapping is 

It's not about CPU revisions, it's about revisions of the SoC that
contain the IP block.

> something I don't quite understand, that's why I'd love to see a good reasoning 
> from Stephen.

Please see the earlier discussions.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Stephen Warren]

On 09/16/2013 09:10 AM, Marek Vasut wrote:
...
> OK, to push it ad-absurdum, shall we not start listing every single revision of 
> the CPU as well? Like pxa250a0 250a1 etc ? Yes , this many-to-one mapping is 

It's not about CPU revisions, it's about revisions of the SoC that
contain the IP block.

> something I don't quite understand, that's why I'd love to see a good reasoning 
> from Stephen.

Please see the earlier discussions.

[PATCH v3] PWM: PXA: add device tree support to PWM driver

[Stephen Warren]

On 09/16/2013 09:10 AM, Marek Vasut wrote:
...
> OK, to push it ad-absurdum, shall we not start listing every single revision of 
> the CPU as well? Like pxa250a0 250a1 etc ? Yes , this many-to-one mapping is 

It's not about CPU revisions, it's about revisions of the SoC that
contain the IP block.

> something I don't quite understand, that's why I'd love to see a good reasoning 
> from Stephen.

Please see the earlier discussions.

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Marek Vasut]

Dear Stephen Warren,

> On 09/16/2013 09:10 AM, Marek Vasut wrote:
> ...
> 
> > OK, to push it ad-absurdum, shall we not start listing every single
> > revision of the CPU as well? Like pxa250a0 250a1 etc ? Yes , this
> > many-to-one mapping is
> 
> It's not about CPU revisions, it's about revisions of the SoC that
> contain the IP block.

But that doesn't matter if the IP block is the same in all of them.

> > something I don't quite understand, that's why I'd love to see a good
> > reasoning from Stephen.
> 
> Please see the earlier discussions.

I did, so I must really be slow or something.

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Marek Vasut]

Dear Stephen Warren,

> On 09/16/2013 09:10 AM, Marek Vasut wrote:
> ...
> 
> > OK, to push it ad-absurdum, shall we not start listing every single
> > revision of the CPU as well? Like pxa250a0 250a1 etc ? Yes , this
> > many-to-one mapping is
> 
> It's not about CPU revisions, it's about revisions of the SoC that
> contain the IP block.

But that doesn't matter if the IP block is the same in all of them.

> > something I don't quite understand, that's why I'd love to see a good
> > reasoning from Stephen.
> 
> Please see the earlier discussions.

I did, so I must really be slow or something.

Best regards,
Marek Vasut

[PATCH v3] PWM: PXA: add device tree support to PWM driver

[Marek Vasut]

Dear Stephen Warren,

> On 09/16/2013 09:10 AM, Marek Vasut wrote:
> ...
> 
> > OK, to push it ad-absurdum, shall we not start listing every single
> > revision of the CPU as well? Like pxa250a0 250a1 etc ? Yes , this
> > many-to-one mapping is
> 
> It's not about CPU revisions, it's about revisions of the SoC that
> contain the IP block.

But that doesn't matter if the IP block is the same in all of them.

> > something I don't quite understand, that's why I'd love to see a good
> > reasoning from Stephen.
> 
> Please see the earlier discussions.

I did, so I must really be slow or something.

Best regards,
Marek Vasut

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Marek Vasut]

Dear Stephen Warren,

> On 09/13/2013 10:54 AM, Mike Dunn wrote:
> > This patch adds device tree support to the PXA's PWM driver.  Only an OF
> > match table is added; nothing needs to be extracted from the device tree
> > node.  The existing ID table is reused for the match table data.
> > 
> > Tested on a Palm Treo 680 (both platform data and DT cases).
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> > b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> > 
> > +- compatible: should be one of:
> > +  - "marvell,pxa250-pwm"
> > +  - "marvell,pxa270-pwm"
> > +  - "marvell,pxa168-pwm"
> > +  - "marvell,pxa910-pwm"
> 
> Not just one of, but possible more than one...
> 
> > diff --git a/arch/arm/boot/dts/pxa27x.dtsi
> > b/arch/arm/boot/dts/pxa27x.dtsi
> > 
> > +		pwm0: pwm@40b00000 {
> > +			compatible = "marvell,pxa270-pwm";
> 
> I thought the assertion was that pax270 and pxa250 were both compatible?
> If so, that should be:
> 
> 		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";

I see what you mean with the compatible strings ... but if we have this 
"fallback" compatiblity string, do we also need the table of all chips sporting 
the IP block in the driver itself?

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Marek Vasut]

Dear Stephen Warren,

> On 09/13/2013 10:54 AM, Mike Dunn wrote:
> > This patch adds device tree support to the PXA's PWM driver.  Only an OF
> > match table is added; nothing needs to be extracted from the device tree
> > node.  The existing ID table is reused for the match table data.
> > 
> > Tested on a Palm Treo 680 (both platform data and DT cases).
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> > b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> > 
> > +- compatible: should be one of:
> > +  - "marvell,pxa250-pwm"
> > +  - "marvell,pxa270-pwm"
> > +  - "marvell,pxa168-pwm"
> > +  - "marvell,pxa910-pwm"
> 
> Not just one of, but possible more than one...
> 
> > diff --git a/arch/arm/boot/dts/pxa27x.dtsi
> > b/arch/arm/boot/dts/pxa27x.dtsi
> > 
> > +		pwm0: pwm@40b00000 {
> > +			compatible = "marvell,pxa270-pwm";
> 
> I thought the assertion was that pax270 and pxa250 were both compatible?
> If so, that should be:
> 
> 		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";

I see what you mean with the compatible strings ... but if we have this 
"fallback" compatiblity string, do we also need the table of all chips sporting 
the IP block in the driver itself?

Best regards,
Marek Vasut

[PATCH v3] PWM: PXA: add device tree support to PWM driver

[Marek Vasut]

Dear Stephen Warren,

> On 09/13/2013 10:54 AM, Mike Dunn wrote:
> > This patch adds device tree support to the PXA's PWM driver.  Only an OF
> > match table is added; nothing needs to be extracted from the device tree
> > node.  The existing ID table is reused for the match table data.
> > 
> > Tested on a Palm Treo 680 (both platform data and DT cases).
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> > b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> > 
> > +- compatible: should be one of:
> > +  - "marvell,pxa250-pwm"
> > +  - "marvell,pxa270-pwm"
> > +  - "marvell,pxa168-pwm"
> > +  - "marvell,pxa910-pwm"
> 
> Not just one of, but possible more than one...
> 
> > diff --git a/arch/arm/boot/dts/pxa27x.dtsi
> > b/arch/arm/boot/dts/pxa27x.dtsi
> > 
> > +		pwm0: pwm at 40b00000 {
> > +			compatible = "marvell,pxa270-pwm";
> 
> I thought the assertion was that pax270 and pxa250 were both compatible?
> If so, that should be:
> 
> 		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";

I see what you mean with the compatible strings ... but if we have this 
"fallback" compatiblity string, do we also need the table of all chips sporting 
the IP block in the driver itself?

Best regards,
Marek Vasut

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Stephen Warren]

On 09/16/2013 05:01 PM, Marek Vasut wrote:
> Dear Stephen Warren,
> 
>> On 09/13/2013 10:54 AM, Mike Dunn wrote:
>>> This patch adds device tree support to the PXA's PWM driver.  Only an OF
>>> match table is added; nothing needs to be extracted from the device tree
>>> node.  The existing ID table is reused for the match table data.
>>>
>>> Tested on a Palm Treo 680 (both platform data and DT cases).
>>>
>>> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>>> b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>>>
>>> +- compatible: should be one of:
>>> +  - "marvell,pxa250-pwm"
>>> +  - "marvell,pxa270-pwm"
>>> +  - "marvell,pxa168-pwm"
>>> +  - "marvell,pxa910-pwm"
>>
>> Not just one of, but possible more than one...
>>
>>> diff --git a/arch/arm/boot/dts/pxa27x.dtsi
>>> b/arch/arm/boot/dts/pxa27x.dtsi
>>>
>>> +		pwm0: pwm@40b00000 {
>>> +			compatible = "marvell,pxa270-pwm";
>>
>> I thought the assertion was that pax270 and pxa250 were both compatible?
>> If so, that should be:
>>
>> 		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
> 
> I see what you mean with the compatible strings ... but if we have this 
> "fallback" compatiblity string, do we also need the table of all chips sporting 
> the IP block in the driver itself?

I would assume the driver can just support "marvell,pxa250-pwm" for now.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Stephen Warren]

On 09/16/2013 05:01 PM, Marek Vasut wrote:
> Dear Stephen Warren,
> 
>> On 09/13/2013 10:54 AM, Mike Dunn wrote:
>>> This patch adds device tree support to the PXA's PWM driver.  Only an OF
>>> match table is added; nothing needs to be extracted from the device tree
>>> node.  The existing ID table is reused for the match table data.
>>>
>>> Tested on a Palm Treo 680 (both platform data and DT cases).
>>>
>>> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>>> b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>>>
>>> +- compatible: should be one of:
>>> +  - "marvell,pxa250-pwm"
>>> +  - "marvell,pxa270-pwm"
>>> +  - "marvell,pxa168-pwm"
>>> +  - "marvell,pxa910-pwm"
>>
>> Not just one of, but possible more than one...
>>
>>> diff --git a/arch/arm/boot/dts/pxa27x.dtsi
>>> b/arch/arm/boot/dts/pxa27x.dtsi
>>>
>>> +		pwm0: pwm@40b00000 {
>>> +			compatible = "marvell,pxa270-pwm";
>>
>> I thought the assertion was that pax270 and pxa250 were both compatible?
>> If so, that should be:
>>
>> 		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
> 
> I see what you mean with the compatible strings ... but if we have this 
> "fallback" compatiblity string, do we also need the table of all chips sporting 
> the IP block in the driver itself?

I would assume the driver can just support "marvell,pxa250-pwm" for now.

[PATCH v3] PWM: PXA: add device tree support to PWM driver

[Stephen Warren]

On 09/16/2013 05:01 PM, Marek Vasut wrote:
> Dear Stephen Warren,
> 
>> On 09/13/2013 10:54 AM, Mike Dunn wrote:
>>> This patch adds device tree support to the PXA's PWM driver.  Only an OF
>>> match table is added; nothing needs to be extracted from the device tree
>>> node.  The existing ID table is reused for the match table data.
>>>
>>> Tested on a Palm Treo 680 (both platform data and DT cases).
>>>
>>> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>>> b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>>>
>>> +- compatible: should be one of:
>>> +  - "marvell,pxa250-pwm"
>>> +  - "marvell,pxa270-pwm"
>>> +  - "marvell,pxa168-pwm"
>>> +  - "marvell,pxa910-pwm"
>>
>> Not just one of, but possible more than one...
>>
>>> diff --git a/arch/arm/boot/dts/pxa27x.dtsi
>>> b/arch/arm/boot/dts/pxa27x.dtsi
>>>
>>> +		pwm0: pwm at 40b00000 {
>>> +			compatible = "marvell,pxa270-pwm";
>>
>> I thought the assertion was that pax270 and pxa250 were both compatible?
>> If so, that should be:
>>
>> 		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
> 
> I see what you mean with the compatible strings ... but if we have this 
> "fallback" compatiblity string, do we also need the table of all chips sporting 
> the IP block in the driver itself?

I would assume the driver can just support "marvell,pxa250-pwm" for now.

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Mike Dunn]

On 09/16/2013 12:44 PM, Stephen Warren wrote:
> On 09/13/2013 10:54 AM, Mike Dunn wrote:
>> This patch adds device tree support to the PXA's PWM driver.  Only an OF match
>> table is added; nothing needs to be extracted from the device tree node.  The
>> existing ID table is reused for the match table data.
>>
>> Tested on a Palm Treo 680 (both platform data and DT cases).
> 
>> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> 
>> +- compatible: should be one of:
>> +  - "marvell,pxa250-pwm"
>> +  - "marvell,pxa270-pwm"
>> +  - "marvell,pxa168-pwm"
>> +  - "marvell,pxa910-pwm"
> 
> Not just one of, but possible more than one...


So the phrasing should be "compatible: should be among:" ?


> 
>> diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
> 
>> +		pwm0: pwm@40b00000 {
>> +			compatible = "marvell,pxa270-pwm";
> 
> I thought the assertion was that pax270 and pxa250 were both compatible?
> If so, that should be:
> 
> 		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";


Indeed.  Currently, they are all the same.  By that logic, shouldn't it be

compatible = "marvell,pxa250-pwm", "marvell,pxa270-pwm", "marvell,pxa168-pwm",
"marvell,pxa910-pwm";

I am admittedly fuzzy on device tree, but this is starting to seem strange.

Anyway, I'll defer to your judgement and resubmit if you can give me precise
guidance on the above.  Thanks again for the review.

Thanks,
Mike

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Mike Dunn]

On 09/16/2013 12:44 PM, Stephen Warren wrote:
> On 09/13/2013 10:54 AM, Mike Dunn wrote:
>> This patch adds device tree support to the PXA's PWM driver.  Only an OF match
>> table is added; nothing needs to be extracted from the device tree node.  The
>> existing ID table is reused for the match table data.
>>
>> Tested on a Palm Treo 680 (both platform data and DT cases).
> 
>> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> 
>> +- compatible: should be one of:
>> +  - "marvell,pxa250-pwm"
>> +  - "marvell,pxa270-pwm"
>> +  - "marvell,pxa168-pwm"
>> +  - "marvell,pxa910-pwm"
> 
> Not just one of, but possible more than one...


So the phrasing should be "compatible: should be among:" ?


> 
>> diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
> 
>> +		pwm0: pwm@40b00000 {
>> +			compatible = "marvell,pxa270-pwm";
> 
> I thought the assertion was that pax270 and pxa250 were both compatible?
> If so, that should be:
> 
> 		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";


Indeed.  Currently, they are all the same.  By that logic, shouldn't it be

compatible = "marvell,pxa250-pwm", "marvell,pxa270-pwm", "marvell,pxa168-pwm",
"marvell,pxa910-pwm";

I am admittedly fuzzy on device tree, but this is starting to seem strange.

Anyway, I'll defer to your judgement and resubmit if you can give me precise
guidance on the above.  Thanks again for the review.

Thanks,
Mike

[PATCH v3] PWM: PXA: add device tree support to PWM driver

[Mike Dunn]

On 09/16/2013 12:44 PM, Stephen Warren wrote:
> On 09/13/2013 10:54 AM, Mike Dunn wrote:
>> This patch adds device tree support to the PXA's PWM driver.  Only an OF match
>> table is added; nothing needs to be extracted from the device tree node.  The
>> existing ID table is reused for the match table data.
>>
>> Tested on a Palm Treo 680 (both platform data and DT cases).
> 
>> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
> 
>> +- compatible: should be one of:
>> +  - "marvell,pxa250-pwm"
>> +  - "marvell,pxa270-pwm"
>> +  - "marvell,pxa168-pwm"
>> +  - "marvell,pxa910-pwm"
> 
> Not just one of, but possible more than one...


So the phrasing should be "compatible: should be among:" ?


> 
>> diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
> 
>> +		pwm0: pwm at 40b00000 {
>> +			compatible = "marvell,pxa270-pwm";
> 
> I thought the assertion was that pax270 and pxa250 were both compatible?
> If so, that should be:
> 
> 		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";


Indeed.  Currently, they are all the same.  By that logic, shouldn't it be

compatible = "marvell,pxa250-pwm", "marvell,pxa270-pwm", "marvell,pxa168-pwm",
"marvell,pxa910-pwm";

I am admittedly fuzzy on device tree, but this is starting to seem strange.

Anyway, I'll defer to your judgement and resubmit if you can give me precise
guidance on the above.  Thanks again for the review.

Thanks,
Mike

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Stephen Warren]

On 09/17/2013 08:07 AM, Mike Dunn wrote:
> On 09/16/2013 12:44 PM, Stephen Warren wrote:
>> On 09/13/2013 10:54 AM, Mike Dunn wrote:
>>> This patch adds device tree support to the PXA's PWM driver.  Only an OF match
>>> table is added; nothing needs to be extracted from the device tree node.  The
>>> existing ID table is reused for the match table data.
>>>
>>> Tested on a Palm Treo 680 (both platform data and DT cases).
>>
>>> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>>
>>> +- compatible: should be one of:
>>> +  - "marvell,pxa250-pwm"
>>> +  - "marvell,pxa270-pwm"
>>> +  - "marvell,pxa168-pwm"
>>> +  - "marvell,pxa910-pwm"
>>
>> Not just one of, but possible more than one...
> 
> So the phrasing should be "compatible: should be among:" ?

I think I've seen "One or more of".


>>> diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
>>
>>> +		pwm0: pwm@40b00000 {
>>> +			compatible = "marvell,pxa270-pwm";
>>
>> I thought the assertion was that pax270 and pxa250 were both compatible?
>> If so, that should be:
>>
>> 		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
> 
> 
> Indeed.  Currently, they are all the same.  By that logic, shouldn't it be
> 
> compatible = "marvell,pxa250-pwm", "marvell,pxa270-pwm", "marvell,pxa168-pwm",
> "marvell,pxa910-pwm";

compatible should include:

* The exact HW model (so the driver knows exactly which HW is present in
order to enable any bug quirks).
* The "first" HW model this HW is compatible with, since this is what
the driver will bind to (ignoring the possibility of bug quirks).

So, you don't need to include all the values, just two in this case.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Stephen Warren]

On 09/17/2013 08:07 AM, Mike Dunn wrote:
> On 09/16/2013 12:44 PM, Stephen Warren wrote:
>> On 09/13/2013 10:54 AM, Mike Dunn wrote:
>>> This patch adds device tree support to the PXA's PWM driver.  Only an OF match
>>> table is added; nothing needs to be extracted from the device tree node.  The
>>> existing ID table is reused for the match table data.
>>>
>>> Tested on a Palm Treo 680 (both platform data and DT cases).
>>
>>> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>>
>>> +- compatible: should be one of:
>>> +  - "marvell,pxa250-pwm"
>>> +  - "marvell,pxa270-pwm"
>>> +  - "marvell,pxa168-pwm"
>>> +  - "marvell,pxa910-pwm"
>>
>> Not just one of, but possible more than one...
> 
> So the phrasing should be "compatible: should be among:" ?

I think I've seen "One or more of".


>>> diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
>>
>>> +		pwm0: pwm@40b00000 {
>>> +			compatible = "marvell,pxa270-pwm";
>>
>> I thought the assertion was that pax270 and pxa250 were both compatible?
>> If so, that should be:
>>
>> 		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
> 
> 
> Indeed.  Currently, they are all the same.  By that logic, shouldn't it be
> 
> compatible = "marvell,pxa250-pwm", "marvell,pxa270-pwm", "marvell,pxa168-pwm",
> "marvell,pxa910-pwm";

compatible should include:

* The exact HW model (so the driver knows exactly which HW is present in
order to enable any bug quirks).
* The "first" HW model this HW is compatible with, since this is what
the driver will bind to (ignoring the possibility of bug quirks).

So, you don't need to include all the values, just two in this case.

[PATCH v3] PWM: PXA: add device tree support to PWM driver

[Stephen Warren]

On 09/17/2013 08:07 AM, Mike Dunn wrote:
> On 09/16/2013 12:44 PM, Stephen Warren wrote:
>> On 09/13/2013 10:54 AM, Mike Dunn wrote:
>>> This patch adds device tree support to the PXA's PWM driver.  Only an OF match
>>> table is added; nothing needs to be extracted from the device tree node.  The
>>> existing ID table is reused for the match table data.
>>>
>>> Tested on a Palm Treo 680 (both platform data and DT cases).
>>
>>> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>>
>>> +- compatible: should be one of:
>>> +  - "marvell,pxa250-pwm"
>>> +  - "marvell,pxa270-pwm"
>>> +  - "marvell,pxa168-pwm"
>>> +  - "marvell,pxa910-pwm"
>>
>> Not just one of, but possible more than one...
> 
> So the phrasing should be "compatible: should be among:" ?

I think I've seen "One or more of".


>>> diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
>>
>>> +		pwm0: pwm at 40b00000 {
>>> +			compatible = "marvell,pxa270-pwm";
>>
>> I thought the assertion was that pax270 and pxa250 were both compatible?
>> If so, that should be:
>>
>> 		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
> 
> 
> Indeed.  Currently, they are all the same.  By that logic, shouldn't it be
> 
> compatible = "marvell,pxa250-pwm", "marvell,pxa270-pwm", "marvell,pxa168-pwm",
> "marvell,pxa910-pwm";

compatible should include:

* The exact HW model (so the driver knows exactly which HW is present in
order to enable any bug quirks).
* The "first" HW model this HW is compatible with, since this is what
the driver will bind to (ignoring the possibility of bug quirks).

So, you don't need to include all the values, just two in this case.

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Mike Dunn]

On 09/17/2013 09:17 AM, Stephen Warren wrote:
> On 09/17/2013 08:07 AM, Mike Dunn wrote:
>> On 09/16/2013 12:44 PM, Stephen Warren wrote:
>>> On 09/13/2013 10:54 AM, Mike Dunn wrote:
>>>> This patch adds device tree support to the PXA's PWM driver.  Only an OF match
>>>> table is added; nothing needs to be extracted from the device tree node.  The
>>>> existing ID table is reused for the match table data.
>>>>
>>>> Tested on a Palm Treo 680 (both platform data and DT cases).
>>>
>>>> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>>>
>>>> +- compatible: should be one of:
>>>> +  - "marvell,pxa250-pwm"
>>>> +  - "marvell,pxa270-pwm"
>>>> +  - "marvell,pxa168-pwm"
>>>> +  - "marvell,pxa910-pwm"
>>>
>>> Not just one of, but possible more than one...
>>
>> So the phrasing should be "compatible: should be among:" ?
> 
> I think I've seen "One or more of".


OK.


> 
> 
>>>> diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
>>>
>>>> +		pwm0: pwm@40b00000 {
>>>> +			compatible = "marvell,pxa270-pwm";
>>>
>>> I thought the assertion was that pax270 and pxa250 were both compatible?
>>> If so, that should be:
>>>
>>> 		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
>>
>>
>> Indeed.  Currently, they are all the same.  By that logic, shouldn't it be
>>
>> compatible = "marvell,pxa250-pwm", "marvell,pxa270-pwm", "marvell,pxa168-pwm",
>> "marvell,pxa910-pwm";
> 
> compatible should include:
> 
> * The exact HW model (so the driver knows exactly which HW is present in
> order to enable any bug quirks).
> * The "first" HW model this HW is compatible with, since this is what
> the driver will bind to (ignoring the possibility of bug quirks).
> 
> So, you don't need to include all the values, just two in this case.


I think I see...  and hypothetically, if the the driver must know that it is a
pxa270 in order for it to function correctly due to (hypothetical) hardware
differences, then only the first compatible string should appear?

Anyway, thanks again!  I'll resubmit tomorrow.

Mike

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Mike Dunn]

On 09/17/2013 09:17 AM, Stephen Warren wrote:
> On 09/17/2013 08:07 AM, Mike Dunn wrote:
>> On 09/16/2013 12:44 PM, Stephen Warren wrote:
>>> On 09/13/2013 10:54 AM, Mike Dunn wrote:
>>>> This patch adds device tree support to the PXA's PWM driver.  Only an OF match
>>>> table is added; nothing needs to be extracted from the device tree node.  The
>>>> existing ID table is reused for the match table data.
>>>>
>>>> Tested on a Palm Treo 680 (both platform data and DT cases).
>>>
>>>> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>>>
>>>> +- compatible: should be one of:
>>>> +  - "marvell,pxa250-pwm"
>>>> +  - "marvell,pxa270-pwm"
>>>> +  - "marvell,pxa168-pwm"
>>>> +  - "marvell,pxa910-pwm"
>>>
>>> Not just one of, but possible more than one...
>>
>> So the phrasing should be "compatible: should be among:" ?
> 
> I think I've seen "One or more of".


OK.


> 
> 
>>>> diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
>>>
>>>> +		pwm0: pwm@40b00000 {
>>>> +			compatible = "marvell,pxa270-pwm";
>>>
>>> I thought the assertion was that pax270 and pxa250 were both compatible?
>>> If so, that should be:
>>>
>>> 		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
>>
>>
>> Indeed.  Currently, they are all the same.  By that logic, shouldn't it be
>>
>> compatible = "marvell,pxa250-pwm", "marvell,pxa270-pwm", "marvell,pxa168-pwm",
>> "marvell,pxa910-pwm";
> 
> compatible should include:
> 
> * The exact HW model (so the driver knows exactly which HW is present in
> order to enable any bug quirks).
> * The "first" HW model this HW is compatible with, since this is what
> the driver will bind to (ignoring the possibility of bug quirks).
> 
> So, you don't need to include all the values, just two in this case.


I think I see...  and hypothetically, if the the driver must know that it is a
pxa270 in order for it to function correctly due to (hypothetical) hardware
differences, then only the first compatible string should appear?

Anyway, thanks again!  I'll resubmit tomorrow.

Mike

[PATCH v3] PWM: PXA: add device tree support to PWM driver

[Mike Dunn]

On 09/17/2013 09:17 AM, Stephen Warren wrote:
> On 09/17/2013 08:07 AM, Mike Dunn wrote:
>> On 09/16/2013 12:44 PM, Stephen Warren wrote:
>>> On 09/13/2013 10:54 AM, Mike Dunn wrote:
>>>> This patch adds device tree support to the PXA's PWM driver.  Only an OF match
>>>> table is added; nothing needs to be extracted from the device tree node.  The
>>>> existing ID table is reused for the match table data.
>>>>
>>>> Tested on a Palm Treo 680 (both platform data and DT cases).
>>>
>>>> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
>>>
>>>> +- compatible: should be one of:
>>>> +  - "marvell,pxa250-pwm"
>>>> +  - "marvell,pxa270-pwm"
>>>> +  - "marvell,pxa168-pwm"
>>>> +  - "marvell,pxa910-pwm"
>>>
>>> Not just one of, but possible more than one...
>>
>> So the phrasing should be "compatible: should be among:" ?
> 
> I think I've seen "One or more of".


OK.


> 
> 
>>>> diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
>>>
>>>> +		pwm0: pwm at 40b00000 {
>>>> +			compatible = "marvell,pxa270-pwm";
>>>
>>> I thought the assertion was that pax270 and pxa250 were both compatible?
>>> If so, that should be:
>>>
>>> 		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
>>
>>
>> Indeed.  Currently, they are all the same.  By that logic, shouldn't it be
>>
>> compatible = "marvell,pxa250-pwm", "marvell,pxa270-pwm", "marvell,pxa168-pwm",
>> "marvell,pxa910-pwm";
> 
> compatible should include:
> 
> * The exact HW model (so the driver knows exactly which HW is present in
> order to enable any bug quirks).
> * The "first" HW model this HW is compatible with, since this is what
> the driver will bind to (ignoring the possibility of bug quirks).
> 
> So, you don't need to include all the values, just two in this case.


I think I see...  and hypothetically, if the the driver must know that it is a
pxa270 in order for it to function correctly due to (hypothetical) hardware
differences, then only the first compatible string should appear?

Anyway, thanks again!  I'll resubmit tomorrow.

Mike

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Stephen Warren]

On 09/17/2013 12:56 PM, Mike Dunn wrote:
> On 09/17/2013 09:17 AM, Stephen Warren wrote:
>> On 09/17/2013 08:07 AM, Mike Dunn wrote:
>>> On 09/16/2013 12:44 PM, Stephen Warren wrote:
>>>> On 09/13/2013 10:54 AM, Mike Dunn wrote:
>>>>> This patch adds device tree support to the PXA's PWM driver.  Only an OF match
>>>>> table is added; nothing needs to be extracted from the device tree node.  The
>>>>> existing ID table is reused for the match table data.
>>>>>
>>>>> Tested on a Palm Treo 680 (both platform data and DT cases).

>>>>> diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
>>>>
>>>>> +		pwm0: pwm@40b00000 {
>>>>> +			compatible = "marvell,pxa270-pwm";
>>>>
>>>> I thought the assertion was that pax270 and pxa250 were both compatible?
>>>> If so, that should be:
>>>>
>>>> 		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
>>>
>>>
>>> Indeed.  Currently, they are all the same.  By that logic, shouldn't it be
>>>
>>> compatible = "marvell,pxa250-pwm", "marvell,pxa270-pwm", "marvell,pxa168-pwm",
>>> "marvell,pxa910-pwm";
>>
>> compatible should include:
>>
>> * The exact HW model (so the driver knows exactly which HW is present in
>> order to enable any bug quirks).
>> * The "first" HW model this HW is compatible with, since this is what
>> the driver will bind to (ignoring the possibility of bug quirks).
>>
>> So, you don't need to include all the values, just two in this case.
> 
> I think I see...  and hypothetically, if the the driver must know that it is a
> pxa270 in order for it to function correctly due to (hypothetical) hardware
> differences, then only the first compatible string should appear?

Yes, if the 250 and 270 HW were actually different, then you'd only
include one or the other compatible values in the property.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] PWM: PXA: add device tree support to PWM driver

[Stephen Warren]

On 09/17/2013 12:56 PM, Mike Dunn wrote:
> On 09/17/2013 09:17 AM, Stephen Warren wrote:
>> On 09/17/2013 08:07 AM, Mike Dunn wrote:
>>> On 09/16/2013 12:44 PM, Stephen Warren wrote:
>>>> On 09/13/2013 10:54 AM, Mike Dunn wrote:
>>>>> This patch adds device tree support to the PXA's PWM driver.  Only an OF match
>>>>> table is added; nothing needs to be extracted from the device tree node.  The
>>>>> existing ID table is reused for the match table data.
>>>>>
>>>>> Tested on a Palm Treo 680 (both platform data and DT cases).

>>>>> diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
>>>>
>>>>> +		pwm0: pwm@40b00000 {
>>>>> +			compatible = "marvell,pxa270-pwm";
>>>>
>>>> I thought the assertion was that pax270 and pxa250 were both compatible?
>>>> If so, that should be:
>>>>
>>>> 		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
>>>
>>>
>>> Indeed.  Currently, they are all the same.  By that logic, shouldn't it be
>>>
>>> compatible = "marvell,pxa250-pwm", "marvell,pxa270-pwm", "marvell,pxa168-pwm",
>>> "marvell,pxa910-pwm";
>>
>> compatible should include:
>>
>> * The exact HW model (so the driver knows exactly which HW is present in
>> order to enable any bug quirks).
>> * The "first" HW model this HW is compatible with, since this is what
>> the driver will bind to (ignoring the possibility of bug quirks).
>>
>> So, you don't need to include all the values, just two in this case.
> 
> I think I see...  and hypothetically, if the the driver must know that it is a
> pxa270 in order for it to function correctly due to (hypothetical) hardware
> differences, then only the first compatible string should appear?

Yes, if the 250 and 270 HW were actually different, then you'd only
include one or the other compatible values in the property.

[PATCH v3] PWM: PXA: add device tree support to PWM driver

[Stephen Warren]

On 09/17/2013 12:56 PM, Mike Dunn wrote:
> On 09/17/2013 09:17 AM, Stephen Warren wrote:
>> On 09/17/2013 08:07 AM, Mike Dunn wrote:
>>> On 09/16/2013 12:44 PM, Stephen Warren wrote:
>>>> On 09/13/2013 10:54 AM, Mike Dunn wrote:
>>>>> This patch adds device tree support to the PXA's PWM driver.  Only an OF match
>>>>> table is added; nothing needs to be extracted from the device tree node.  The
>>>>> existing ID table is reused for the match table data.
>>>>>
>>>>> Tested on a Palm Treo 680 (both platform data and DT cases).

>>>>> diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
>>>>
>>>>> +		pwm0: pwm at 40b00000 {
>>>>> +			compatible = "marvell,pxa270-pwm";
>>>>
>>>> I thought the assertion was that pax270 and pxa250 were both compatible?
>>>> If so, that should be:
>>>>
>>>> 		compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
>>>
>>>
>>> Indeed.  Currently, they are all the same.  By that logic, shouldn't it be
>>>
>>> compatible = "marvell,pxa250-pwm", "marvell,pxa270-pwm", "marvell,pxa168-pwm",
>>> "marvell,pxa910-pwm";
>>
>> compatible should include:
>>
>> * The exact HW model (so the driver knows exactly which HW is present in
>> order to enable any bug quirks).
>> * The "first" HW model this HW is compatible with, since this is what
>> the driver will bind to (ignoring the possibility of bug quirks).
>>
>> So, you don't need to include all the values, just two in this case.
> 
> I think I see...  and hypothetically, if the the driver must know that it is a
> pxa270 in order for it to function correctly due to (hypothetical) hardware
> differences, then only the first compatible string should appear?

Yes, if the 250 and 270 HW were actually different, then you'd only
include one or the other compatible values in the property.