* [PATCH] pwm: pxa: add device tree support to pwm driver
@ 2013-09-03 19:23 Mike Dunn
2013-09-03 22:20 ` Marek Vasut
0 siblings, 1 reply; 11+ messages in thread
From: Mike Dunn @ 2013-09-03 19:23 UTC (permalink / raw)
To: linux-pwm
Cc: Marek Vasut, devicetree, Mike Dunn, Rob Herring, Thierry Reding,
Haojian Zhuang, Grant Likely, Robert Jarzmik, linux-arm-kernel
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 platform_device 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>
---
I don't have data sheets handy for the newer Marvell pxa's (and I'm confused
about the presence of a pwm unit on pxa25x), so only pxa27x.dtsi was updated.
Thanks for looking!
arch/arm/boot/dts/pxa27x.dtsi | 12 ++++++++++++
drivers/pwm/pwm-pxa.c | 31 +++++++++++++++++++++++++++++++
2 files changed, 43 insertions(+)
diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
index d7c5d72..4031dce 100644
--- a/arch/arm/boot/dts/pxa27x.dtsi
+++ b/arch/arm/boot/dts/pxa27x.dtsi
@@ -10,5 +10,17 @@
marvell,intc-priority;
marvell,intc-nr-irqs = <34>;
};
+
+ pwm0: pwm@40b00000 {
+ compatible = "marvell,pxa27x-pwm";
+ reg = <0x40b00000 0x100000>;
+ #pwm-cells = <2>;
+ };
+
+ pwm1: pwm@40c00000 {
+ compatible = "marvell,pxa27x-pwm";
+ reg = <0x40c00000 0x100000>;
+ #pwm-cells = <2>;
+ };
};
};
diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index a4d2164..be5f914 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,30 @@ static struct pwm_ops pxa_pwm_ops = {
.owner = THIS_MODULE,
};
+#ifdef CONFIG_OF
+/* use the platform_device id table for OF match table data */
+static struct of_device_id pwm_of_match[] = {
+ { .compatible = "marvell,pxa25x-pwm", .data = &pwm_id_table[0] },
+ { .compatible = "marvell,pxa27x-pwm", .data = &pwm_id_table[1] },
+ { .compatible = "marvell,pxa168-pwm", .data = &pwm_id_table[2] },
+ { .compatible = "marvell,pxa910-pwm", .data = &pwm_id_table[3] },
+ { }
+};
+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 *pwm_pxa_devid =
+ of_match_device(pwm_of_match, dev);
+ if (pwm_pxa_devid)
+ return (const struct platform_device_id *)pwm_pxa_devid->data;
+ else
+ return NULL;
+}
+#else /* !CONFIG_OF */
+#define pxa_pwm_get_id_dt(dev) ((const struct platform_device_id *)NULL)
+#endif
+
static int pwm_probe(struct platform_device *pdev)
{
const struct platform_device_id *id = platform_get_device_id(pdev);
@@ -131,6 +156,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 -ENODEV;
+
pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
if (pwm == NULL) {
dev_err(&pdev->dev, "failed to allocate memory\n");
@@ -176,6 +206,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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] pwm: pxa: add device tree support to pwm driver
2013-09-03 19:23 [PATCH] pwm: pxa: add device tree support to pwm driver Mike Dunn
@ 2013-09-03 22:20 ` Marek Vasut
2013-09-04 14:23 ` Mike Dunn
0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2013-09-03 22:20 UTC (permalink / raw)
To: linux-arm-kernel
Cc: linux-pwm, Mike Dunn, devicetree, Rob Herring, Thierry Reding,
Haojian Zhuang, Grant Likely, Robert Jarzmik
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 platform_device 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>
> ---
>
> I don't have data sheets handy for the newer Marvell pxa's (and I'm
> confused about the presence of a pwm unit on pxa25x), so only pxa27x.dtsi
> was updated. Thanks for looking!
>
> arch/arm/boot/dts/pxa27x.dtsi | 12 ++++++++++++
> drivers/pwm/pwm-pxa.c | 31 +++++++++++++++++++++++++++++++
> 2 files changed, 43 insertions(+)
>
> diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
> index d7c5d72..4031dce 100644
> --- a/arch/arm/boot/dts/pxa27x.dtsi
> +++ b/arch/arm/boot/dts/pxa27x.dtsi
> @@ -10,5 +10,17 @@
> marvell,intc-priority;
> marvell,intc-nr-irqs = <34>;
> };
> +
> + pwm0: pwm@40b00000 {
> + compatible = "marvell,pxa27x-pwm";
> + reg = <0x40b00000 0x100000>;
> + #pwm-cells = <2>;
> + };
> +
> + pwm1: pwm@40c00000 {
> + compatible = "marvell,pxa27x-pwm";
> + reg = <0x40c00000 0x100000>;
> + #pwm-cells = <2>;
> + };
> };
> };
> diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
> index a4d2164..be5f914 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,30 @@ static struct pwm_ops pxa_pwm_ops = {
> .owner = THIS_MODULE,
> };
>
> +#ifdef CONFIG_OF
> +/* use the platform_device id table for OF match table data */
> +static struct of_device_id pwm_of_match[] = {
> + { .compatible = "marvell,pxa25x-pwm", .data = &pwm_id_table[0] },
> + { .compatible = "marvell,pxa27x-pwm", .data = &pwm_id_table[1] },
> + { .compatible = "marvell,pxa168-pwm", .data = &pwm_id_table[2] },
> + { .compatible = "marvell,pxa910-pwm", .data = &pwm_id_table[3] },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, pwm_of_match);
Are PXA2xx and PXA3xx PWM impleemntations not all the same ? If so, why not just
stick with pxa25x-pwm only for all of the CPUs (aka. the lowest CPU model). Then
the table would have but a single entry.
[...]
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] pwm: pxa: add device tree support to pwm driver
2013-09-03 22:20 ` Marek Vasut
@ 2013-09-04 14:23 ` Mike Dunn
2013-09-04 14:35 ` Marek Vasut
2013-09-04 14:38 ` Sergei Shtylyov
0 siblings, 2 replies; 11+ messages in thread
From: Mike Dunn @ 2013-09-04 14:23 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-pwm, devicetree, Rob Herring, Thierry Reding,
Haojian Zhuang, Grant Likely, Robert Jarzmik, linux-arm-kernel
On 09/03/2013 03:20 PM, Marek Vasut wrote:
[...]
>>
>> +#ifdef CONFIG_OF
>> +/* use the platform_device id table for OF match table data */
>> +static struct of_device_id pwm_of_match[] = {
>> + { .compatible = "marvell,pxa25x-pwm", .data = &pwm_id_table[0] },
>> + { .compatible = "marvell,pxa27x-pwm", .data = &pwm_id_table[1] },
>> + { .compatible = "marvell,pxa168-pwm", .data = &pwm_id_table[2] },
>> + { .compatible = "marvell,pxa910-pwm", .data = &pwm_id_table[3] },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(of, pwm_of_match);
>
> Are PXA2xx and PXA3xx PWM impleemntations not all the same ? If so, why not just
> stick with pxa25x-pwm only for all of the CPUs (aka. the lowest CPU model). Then
> the table would have but a single entry.
I'm just echoing the existing platform_device_id table...
static const struct platform_device_id pwm_id_table[] = {
/* PWM has_secondary_pwm? */
{ "pxa25x-pwm", 0 },
{ "pxa27x-pwm", HAS_SECONDARY_PWM },
{ "pxa168-pwm", 0 },
{ "pxa910-pwm", 0 },
{ },
};
MODULE_DEVICE_TABLE(platform, pwm_id_table);
... so that my changes to the driver are minimal. Yes, apparently the only
difference is the existance of a "secondary" pwm for pxa27x.
BTW, the pxa27x actually has four pwms, which is why the addition I made to
pxa27x.dtsi has two nodes (the driver handles two pwms for each device instance
in the pxa27x case).
Thanks Marek.
Mike
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] pwm: pxa: add device tree support to pwm driver
2013-09-04 14:23 ` Mike Dunn
@ 2013-09-04 14:35 ` Marek Vasut
2013-09-04 15:41 ` Mike Dunn
2013-09-04 14:38 ` Sergei Shtylyov
1 sibling, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2013-09-04 14:35 UTC (permalink / raw)
To: Mike Dunn
Cc: linux-pwm, devicetree, Rob Herring, Thierry Reding,
Haojian Zhuang, Grant Likely, Robert Jarzmik, linux-arm-kernel
Dear Mike Dunn,
> On 09/03/2013 03:20 PM, Marek Vasut wrote:
>
> [...]
>
> >> +#ifdef CONFIG_OF
> >> +/* use the platform_device id table for OF match table data */
> >> +static struct of_device_id pwm_of_match[] = {
> >> + { .compatible = "marvell,pxa25x-pwm", .data = &pwm_id_table[0] },
> >> + { .compatible = "marvell,pxa27x-pwm", .data = &pwm_id_table[1] },
> >> + { .compatible = "marvell,pxa168-pwm", .data = &pwm_id_table[2] },
> >> + { .compatible = "marvell,pxa910-pwm", .data = &pwm_id_table[3] },
> >> + { }
> >> +};
> >> +MODULE_DEVICE_TABLE(of, pwm_of_match);
> >
> > Are PXA2xx and PXA3xx PWM impleemntations not all the same ? If so, why
> > not just stick with pxa25x-pwm only for all of the CPUs (aka. the lowest
> > CPU model). Then the table would have but a single entry.
>
> I'm just echoing the existing platform_device_id table...
>
> static const struct platform_device_id pwm_id_table[] = {
> /* PWM has_secondary_pwm? */
> { "pxa25x-pwm", 0 },
> { "pxa27x-pwm", HAS_SECONDARY_PWM },
> { "pxa168-pwm", 0 },
> { "pxa910-pwm", 0 },
> { },
> };
> MODULE_DEVICE_TABLE(platform, pwm_id_table);
>
> ... so that my changes to the driver are minimal. Yes, apparently the only
> difference is the existance of a "secondary" pwm for pxa27x.
>
> BTW, the pxa27x actually has four pwms, which is why the addition I made to
> pxa27x.dtsi has two nodes (the driver handles two pwms for each device
> instance in the pxa27x case).
>
What's that "secondary PWM" there? I no longer remember, sorry. The question
remains still, we can have two entries there (pxa25x and pxa27x) ORR have one
entry (pxa25x) + mrvl,has-secondary-pwm entry.
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] pwm: pxa: add device tree support to pwm driver
2013-09-04 14:35 ` Marek Vasut
@ 2013-09-04 15:41 ` Mike Dunn
2013-09-04 22:11 ` Marek Vasut
0 siblings, 1 reply; 11+ messages in thread
From: Mike Dunn @ 2013-09-04 15:41 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-pwm, devicetree, Rob Herring, Thierry Reding,
Haojian Zhuang, Grant Likely, Robert Jarzmik, linux-arm-kernel
On 09/04/2013 07:35 AM, Marek Vasut wrote:
> Dear Mike Dunn,
>
>> On 09/03/2013 03:20 PM, Marek Vasut wrote:
>>
>> [...]
>>
>>>> +#ifdef CONFIG_OF
>>>> +/* use the platform_device id table for OF match table data */
>>>> +static struct of_device_id pwm_of_match[] = {
>>>> + { .compatible = "marvell,pxa25x-pwm", .data = &pwm_id_table[0] },
>>>> + { .compatible = "marvell,pxa27x-pwm", .data = &pwm_id_table[1] },
>>>> + { .compatible = "marvell,pxa168-pwm", .data = &pwm_id_table[2] },
>>>> + { .compatible = "marvell,pxa910-pwm", .data = &pwm_id_table[3] },
>>>> + { }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, pwm_of_match);
>>>
>>> Are PXA2xx and PXA3xx PWM impleemntations not all the same ? If so, why
>>> not just stick with pxa25x-pwm only for all of the CPUs (aka. the lowest
>>> CPU model). Then the table would have but a single entry.
>>
>> I'm just echoing the existing platform_device_id table...
>>
>> static const struct platform_device_id pwm_id_table[] = {
>> /* PWM has_secondary_pwm? */
>> { "pxa25x-pwm", 0 },
>> { "pxa27x-pwm", HAS_SECONDARY_PWM },
>> { "pxa168-pwm", 0 },
>> { "pxa910-pwm", 0 },
>> { },
>> };
>> MODULE_DEVICE_TABLE(platform, pwm_id_table);
>>
>> ... so that my changes to the driver are minimal. Yes, apparently the only
>> difference is the existance of a "secondary" pwm for pxa27x.
>>
>> BTW, the pxa27x actually has four pwms, which is why the addition I made to
>> pxa27x.dtsi has two nodes (the driver handles two pwms for each device
>> instance in the pxa27x case).
>>
>
> What's that "secondary PWM" there? I no longer remember, sorry.
If pdev->id_entry->driver_data == HAS_SECONDARY_PWM, then pwm_chip->npwm=2 when
pwmchip_add() is called. Otherwise pwm_chip->npwm=1. The driver knows that the
second pwm's registers are at a fixed offset from the first. For compatibility,
the pxa27x maps the registers for the third pwm at a distant offset, and makes
the offset between 3 and 4 the same as between 1 and 2. Yes, the driver mkes
this unnecessarily complicated. There should just be one device instance per
pwm, and dispense with the whole driver_data thing. I guess there's some
history there.
> The question
> remains still, we can have two entries there (pxa25x and pxa27x) ORR have one
> entry (pxa25x) + mrvl,has-secondary-pwm entry.
It looks like defining "compatible" properties that mirror the old
platform_device_id names won't fly... wildcards are verboten (see Sergei's
comment). So your inclination to use one value for the "compatible" property is
correct. I think the way to go is to forget the whole HAS_SECONDARY_PWM in the
DT case, have one device instance per pwm, and use "compatible=marvell,pwm".
Other suggestions welcome.
Thanks,
Mike
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] pwm: pxa: add device tree support to pwm driver
2013-09-04 15:41 ` Mike Dunn
@ 2013-09-04 22:11 ` Marek Vasut
2013-09-05 15:24 ` Mike Dunn
0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2013-09-04 22:11 UTC (permalink / raw)
To: Mike Dunn
Cc: linux-pwm, devicetree, Rob Herring, Thierry Reding,
Haojian Zhuang, Grant Likely, Robert Jarzmik, linux-arm-kernel
Dear Mike Dunn,
> On 09/04/2013 07:35 AM, Marek Vasut wrote:
> > Dear Mike Dunn,
> >
> >> On 09/03/2013 03:20 PM, Marek Vasut wrote:
> >>
> >> [...]
> >>
> >>>> +#ifdef CONFIG_OF
> >>>> +/* use the platform_device id table for OF match table data */
> >>>> +static struct of_device_id pwm_of_match[] = {
> >>>> + { .compatible = "marvell,pxa25x-pwm", .data = &pwm_id_table[0]
},
> >>>> + { .compatible = "marvell,pxa27x-pwm", .data = &pwm_id_table[1]
},
> >>>> + { .compatible = "marvell,pxa168-pwm", .data = &pwm_id_table[2]
},
> >>>> + { .compatible = "marvell,pxa910-pwm", .data = &pwm_id_table[3]
},
> >>>> + { }
> >>>> +};
> >>>> +MODULE_DEVICE_TABLE(of, pwm_of_match);
> >>>
> >>> Are PXA2xx and PXA3xx PWM impleemntations not all the same ? If so, why
> >>> not just stick with pxa25x-pwm only for all of the CPUs (aka. the
> >>> lowest CPU model). Then the table would have but a single entry.
> >>
> >> I'm just echoing the existing platform_device_id table...
> >>
> >> static const struct platform_device_id pwm_id_table[] = {
> >>
> >> /* PWM has_secondary_pwm? */
> >> { "pxa25x-pwm", 0 },
> >> { "pxa27x-pwm", HAS_SECONDARY_PWM },
> >> { "pxa168-pwm", 0 },
> >> { "pxa910-pwm", 0 },
> >> { },
> >>
> >> };
> >> MODULE_DEVICE_TABLE(platform, pwm_id_table);
> >>
> >> ... so that my changes to the driver are minimal. Yes, apparently the
> >> only difference is the existance of a "secondary" pwm for pxa27x.
> >>
> >> BTW, the pxa27x actually has four pwms, which is why the addition I made
> >> to pxa27x.dtsi has two nodes (the driver handles two pwms for each
> >> device instance in the pxa27x case).
> >
> > What's that "secondary PWM" there? I no longer remember, sorry.
>
> If pdev->id_entry->driver_data == HAS_SECONDARY_PWM, then pwm_chip->npwm=2
> when pwmchip_add() is called. Otherwise pwm_chip->npwm=1. The driver
> knows that the second pwm's registers are at a fixed offset from the
> first. For compatibility, the pxa27x maps the registers for the third pwm
> at a distant offset, and makes the offset between 3 and 4 the same as
> between 1 and 2. Yes, the driver mkes this unnecessarily complicated.
> There should just be one device instance per pwm, and dispense with the
> whole driver_data thing. I guess there's some history there.
OK, I checked the datasheet. The register block for PWM<n + 2> is at offset of
0x10 from PWM<n> , for n in {0, 1} .
Why can we not just register four PWM blocks, each with 0x10 register window
size then? I know there's history (maybe), but then, with DT, this might go
away.
> > The question
> > remains still, we can have two entries there (pxa25x and pxa27x) ORR have
> > one entry (pxa25x) + mrvl,has-secondary-pwm entry.
>
> It looks like defining "compatible" properties that mirror the old
> platform_device_id names won't fly...
Yes of course, this won't work. I didn't know the layout exactly.
> wildcards are verboten (see Sergei's
> comment). So your inclination to use one value for the "compatible"
> property is correct. I think the way to go is to forget the whole
> HAS_SECONDARY_PWM in the DT case, have one device instance per pwm, and
> use "compatible=marvell,pwm". Other suggestions welcome.
compatbile=marvell,pxa25x-pwm , no ? The lowest CPU with the block.
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] pwm: pxa: add device tree support to pwm driver
2013-09-04 22:11 ` Marek Vasut
@ 2013-09-05 15:24 ` Mike Dunn
2013-09-05 15:34 ` Marek Vasut
0 siblings, 1 reply; 11+ messages in thread
From: Mike Dunn @ 2013-09-05 15:24 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-pwm, devicetree, Rob Herring, Thierry Reding,
Haojian Zhuang, Grant Likely, Robert Jarzmik, linux-arm-kernel
On 09/04/2013 03:11 PM, Marek Vasut wrote:
>>>
[...]
>>> What's that "secondary PWM" there? I no longer remember, sorry.
>>
>> If pdev->id_entry->driver_data == HAS_SECONDARY_PWM, then pwm_chip->npwm=2
>> when pwmchip_add() is called. Otherwise pwm_chip->npwm=1. The driver
>> knows that the second pwm's registers are at a fixed offset from the
>> first. For compatibility, the pxa27x maps the registers for the third pwm
>> at a distant offset, and makes the offset between 3 and 4 the same as
>> between 1 and 2. Yes, the driver mkes this unnecessarily complicated.
>> There should just be one device instance per pwm, and dispense with the
>> whole driver_data thing. I guess there's some history there.
>
> OK, I checked the datasheet. The register block for PWM<n + 2> is at offset of
> 0x10 from PWM<n> , for n in {0, 1} .
>
> Why can we not just register four PWM blocks, each with 0x10 register window
> size then? I know there's history (maybe), but then, with DT, this might go
> away.
Indeed. That is what I am also thinking.
>
>>> The question
>>> remains still, we can have two entries there (pxa25x and pxa27x) ORR have
>>> one entry (pxa25x) + mrvl,has-secondary-pwm entry.
>>
>> It looks like defining "compatible" properties that mirror the old
>> platform_device_id names won't fly...
>
> Yes of course, this won't work. I didn't know the layout exactly.
>
>> wildcards are verboten (see Sergei's
>> comment). So your inclination to use one value for the "compatible"
>> property is correct. I think the way to go is to forget the whole
>> HAS_SECONDARY_PWM in the DT case, have one device instance per pwm, and
>> use "compatible=marvell,pwm". Other suggestions welcome.
>
> compatbile=marvell,pxa25x-pwm , no ? The lowest CPU with the block.
Unless I am missing something, the compatible string does not need to replicate
any of the existing platform_device_id names, so wouldn't "marvell,pxa" be
better? Except for register mapping and the number of units present on a
particular pxa variant, the peripheral is software compatible across all pxa
processors. Plus there is the problem of the 'x' wildcard in "pxa25x-pwm".
Thanks,
Mike
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] pwm: pxa: add device tree support to pwm driver
2013-09-05 15:24 ` Mike Dunn
@ 2013-09-05 15:34 ` Marek Vasut
2013-09-05 16:07 ` Mike Dunn
0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2013-09-05 15:34 UTC (permalink / raw)
To: Mike Dunn
Cc: linux-pwm, devicetree, Rob Herring, Thierry Reding,
Haojian Zhuang, Grant Likely, Robert Jarzmik, linux-arm-kernel
Dear Mike Dunn,
> On 09/04/2013 03:11 PM, Marek Vasut wrote:
>
>
> [...]
>
> >>> What's that "secondary PWM" there? I no longer remember, sorry.
> >>
> >> If pdev->id_entry->driver_data == HAS_SECONDARY_PWM, then
> >> pwm_chip->npwm=2 when pwmchip_add() is called. Otherwise
> >> pwm_chip->npwm=1. The driver knows that the second pwm's registers are
> >> at a fixed offset from the first. For compatibility, the pxa27x maps
> >> the registers for the third pwm at a distant offset, and makes the
> >> offset between 3 and 4 the same as between 1 and 2. Yes, the driver
> >> mkes this unnecessarily complicated. There should just be one device
> >> instance per pwm, and dispense with the whole driver_data thing. I
> >> guess there's some history there.
> >
> > OK, I checked the datasheet. The register block for PWM<n + 2> is at
> > offset of 0x10 from PWM<n> , for n in {0, 1} .
> >
> > Why can we not just register four PWM blocks, each with 0x10 register
> > window size then? I know there's history (maybe), but then, with DT,
> > this might go away.
>
> Indeed. That is what I am also thinking.
>
> >>> The question
> >>> remains still, we can have two entries there (pxa25x and pxa27x) ORR
> >>> have one entry (pxa25x) + mrvl,has-secondary-pwm entry.
> >>
> >> It looks like defining "compatible" properties that mirror the old
> >> platform_device_id names won't fly...
> >
> > Yes of course, this won't work. I didn't know the layout exactly.
> >
> >> wildcards are verboten (see Sergei's
> >> comment). So your inclination to use one value for the "compatible"
> >> property is correct. I think the way to go is to forget the whole
> >> HAS_SECONDARY_PWM in the DT case, have one device instance per pwm, and
> >> use "compatible=marvell,pwm". Other suggestions welcome.
> >
> > compatbile=marvell,pxa25x-pwm , no ? The lowest CPU with the block.
>
> Unless I am missing something, the compatible string does not need to
> replicate any of the existing platform_device_id names, so wouldn't
> "marvell,pxa" be better? Except for register mapping and the number of
> units present on a particular pxa variant, the peripheral is software
> compatible across all pxa processors. Plus there is the problem of the
> 'x' wildcard in "pxa25x-pwm".
So use pxa250 ?
My concern is once marvell comes up with PXA1048576 which will have a different
PWM unit, then what will be the name for this new one?
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] pwm: pxa: add device tree support to pwm driver
2013-09-05 15:34 ` Marek Vasut
@ 2013-09-05 16:07 ` Mike Dunn
0 siblings, 0 replies; 11+ messages in thread
From: Mike Dunn @ 2013-09-05 16:07 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-pwm, devicetree, Rob Herring, Thierry Reding,
Haojian Zhuang, Grant Likely, Robert Jarzmik, linux-arm-kernel
On 09/05/2013 08:34 AM, Marek Vasut wrote:
>>>
[...]
>>> compatbile=marvell,pxa25x-pwm , no ? The lowest CPU with the block.
>>
>> Unless I am missing something, the compatible string does not need to
>> replicate any of the existing platform_device_id names, so wouldn't
>> "marvell,pxa" be better? Except for register mapping and the number of
>> units present on a particular pxa variant, the peripheral is software
>> compatible across all pxa processors. Plus there is the problem of the
>> 'x' wildcard in "pxa25x-pwm".
>
> So use pxa250 ?
>
> My concern is once marvell comes up with PXA1048576 which will have a different
> PWM unit, then what will be the name for this new one?
I see. OK then, pxa250 it is.
Thanks Marek,
Mike
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] pwm: pxa: add device tree support to pwm driver
2013-09-04 14:23 ` Mike Dunn
2013-09-04 14:35 ` Marek Vasut
@ 2013-09-04 14:38 ` Sergei Shtylyov
2013-09-04 15:44 ` Mike Dunn
1 sibling, 1 reply; 11+ messages in thread
From: Sergei Shtylyov @ 2013-09-04 14:38 UTC (permalink / raw)
To: Mike Dunn
Cc: Marek Vasut, linux-pwm, devicetree, Rob Herring, Thierry Reding,
Haojian Zhuang, Grant Likely, Robert Jarzmik, linux-arm-kernel
Hello.
On 04-09-2013 18:23, Mike Dunn wrote:
>>> +#ifdef CONFIG_OF
>>> +/* use the platform_device id table for OF match table data */
>>> +static struct of_device_id pwm_of_match[] = {
>>> + { .compatible = "marvell,pxa25x-pwm", .data = &pwm_id_table[0] },
>>> + { .compatible = "marvell,pxa27x-pwm", .data = &pwm_id_table[1] },
>>> + { .compatible = "marvell,pxa168-pwm", .data = &pwm_id_table[2] },
>>> + { .compatible = "marvell,pxa910-pwm", .data = &pwm_id_table[3] },
>>> + { }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, pwm_of_match);
>> Are PXA2xx and PXA3xx PWM impleemntations not all the same ? If so, why not just
>> stick with pxa25x-pwm only for all of the CPUs (aka. the lowest CPU model). Then
>> the table would have but a single entry.
> I'm just echoing the existing platform_device_id table...
> static const struct platform_device_id pwm_id_table[] = {
> /* PWM has_secondary_pwm? */
> { "pxa25x-pwm", 0 },
> { "pxa27x-pwm", HAS_SECONDARY_PWM },
> { "pxa168-pwm", 0 },
> { "pxa910-pwm", 0 },
> { },
> };
> MODULE_DEVICE_TABLE(platform, pwm_id_table);
Unlike 'struct platform_device_id', the "compatible" property can't have
wildcards such as "pxa25x-pwm". You should choose one model (which e.g. was
produced first in the PXA25x series) and use that.
> Mike
WBR, Sergei
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] pwm: pxa: add device tree support to pwm driver
2013-09-04 14:38 ` Sergei Shtylyov
@ 2013-09-04 15:44 ` Mike Dunn
0 siblings, 0 replies; 11+ messages in thread
From: Mike Dunn @ 2013-09-04 15:44 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Marek Vasut, linux-pwm, devicetree, Rob Herring, Thierry Reding,
Haojian Zhuang, Grant Likely, Robert Jarzmik, linux-arm-kernel
On 09/04/2013 07:38 AM, Sergei Shtylyov wrote:
> Hello.
>
> On 04-09-2013 18:23, Mike Dunn wrote:
>
>>>> +#ifdef CONFIG_OF
>>>> +/* use the platform_device id table for OF match table data */
>>>> +static struct of_device_id pwm_of_match[] = {
>>>> + { .compatible = "marvell,pxa25x-pwm", .data = &pwm_id_table[0] },
>>>> + { .compatible = "marvell,pxa27x-pwm", .data = &pwm_id_table[1] },
>>>> + { .compatible = "marvell,pxa168-pwm", .data = &pwm_id_table[2] },
>>>> + { .compatible = "marvell,pxa910-pwm", .data = &pwm_id_table[3] },
>>>> + { }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, pwm_of_match);
>
>>> Are PXA2xx and PXA3xx PWM impleemntations not all the same ? If so, why not just
>>> stick with pxa25x-pwm only for all of the CPUs (aka. the lowest CPU model). Then
>>> the table would have but a single entry.
>
>> I'm just echoing the existing platform_device_id table...
>
>> static const struct platform_device_id pwm_id_table[] = {
>> /* PWM has_secondary_pwm? */
>> { "pxa25x-pwm", 0 },
>> { "pxa27x-pwm", HAS_SECONDARY_PWM },
>> { "pxa168-pwm", 0 },
>> { "pxa910-pwm", 0 },
>> { },
>> };
>> MODULE_DEVICE_TABLE(platform, pwm_id_table);
>
> Unlike 'struct platform_device_id', the "compatible" property can't have
> wildcards such as "pxa25x-pwm". You should choose one model (which e.g. was
> produced first in the PXA25x series) and use that.
Thanks Sergei, I forgot about that. I also forgot to add a file to
Documentation/devicetree/bindings/pwm/ Looks like I have more work.
Mike
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-09-05 16:07 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-03 19:23 [PATCH] pwm: pxa: add device tree support to pwm driver Mike Dunn
2013-09-03 22:20 ` Marek Vasut
2013-09-04 14:23 ` Mike Dunn
2013-09-04 14:35 ` Marek Vasut
2013-09-04 15:41 ` Mike Dunn
2013-09-04 22:11 ` Marek Vasut
2013-09-05 15:24 ` Mike Dunn
2013-09-05 15:34 ` Marek Vasut
2013-09-05 16:07 ` Mike Dunn
2013-09-04 14:38 ` Sergei Shtylyov
2013-09-04 15:44 ` Mike Dunn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).