| Message ID | 1378669218-10944-1-git-send-email-mikedunn@newsguy.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Support for inverted polarity is also added. > > Tested on a Palm Treo 680 (both platform data and DT cases). > > Signed-off-by: Mike Dunn <mikedunn@newsguy.com> > --- > Changle log: > 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 | 33 +++++++++++++ > arch/arm/boot/dts/pxa27x.dtsi | 24 ++++++++++ > drivers/pwm/pwm-pxa.c | 57 > +++++++++++++++++++++++ 3 files changed, 114 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..7b09bfa > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt > @@ -0,0 +1,33 @@ > +Marvell pwm controller > + > +Required properties: > +- compatible: > + for pxa25x, pxa27x, pxa168, pxa910: must be compatible = > "marvell,pxa250-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 3. > + cell 1: the per-chip index of the PWM to use, > + cell 2: the period in nanoseconds, Is there no generic OF prop for this? > + cell 3: flags; set bit 0 to specify inverse polarity. Do we not have some generic flag to indicate inverted PWM polarity? [...] > +#ifdef CONFIG_OF > +/* > + * Device tree users should 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. > + */ Above ... pxa250-pwm , no ? > +static struct of_device_id pwm_of_match[] = { > + { .compatible = "marvell,pxa250-pwm", .data = &pwm_id_table[0]}, Surely, data can be NULL, no ? [...] > @@ -145,6 +199,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 = of_pwm_xlate_with_flags; > + pwm->chip.of_pwm_n_cells = 3; Are these two settings needed ? [...] Best regards, Marek Vasut
On Sun, Sep 08, 2013 at 12:40:18PM -0700, Mike Dunn wrote: > This patch adds device tree support to the pxa's pwm driver. Only an OF match "PXA" and "PWM" > 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. Support "ID table" > Changle log: > 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 I think this needs to be reviewed by the device tree bindings maintainers (as listed in MAINTAINERS) as well. Would you mind resending the patch with all of them on Cc so that they have full context? Thanks. > Documentation/devicetree/bindings/pwm/pxa-pwm.txt | 33 +++++++++++++ > arch/arm/boot/dts/pxa27x.dtsi | 24 ++++++++++ > drivers/pwm/pwm-pxa.c | 57 +++++++++++++++++++++++ > 3 files changed, 114 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..7b09bfa > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt > @@ -0,0 +1,33 @@ > +Marvell pwm controller > + > +Required properties: > +- compatible: > + for pxa25x, pxa27x, pxa168, pxa910: must be compatible = "marvell,pxa250-pwm"; > +- reg: physical base address and length of the registers used by the pwm channel "pwm" -> "PWM". There are a few more occurrences that I haven't explicitly pointed out. > + 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. I'm not sure I like that very much. It seems a wrong representation of the hardware for the sake of modifying a smaller number of lines in the driver. > +- #pwm-cells: should be 3. > + cell 1: the per-chip index of the PWM to use, > + cell 2: the period in nanoseconds, > + cell 3: flags; set bit 0 to specify inverse polarity. The pwm controller This is the standard binding, so you can just refer to the standard binding documentation instead, like so: - #pwm-cells: Should be 2. See pwm.txt in this directory for a description of the cells format. > + does not implement reverse polarity, but some boards may pass the pwm output > + through an external inverter, which can cause problems if a client device > + assumes that an increase in the duty cycle results in an increase in output > + power. The pwm-backlight is an example of such a client. Please don't do that. Reverse polarity support should not be emulated. Either the hardware supports it or it doesn't. In the above case you should be able to achieve the same effect by adding the correct values in the pwm-backlight device node's brightness-levels property. > +Example pwm client node: > + > +backlight { > + compatible = "pwm-backlight"; > + pwms = <&pwm0 0 5000000 1>; /* pwm output is inverted */ > + ... The indentation here is funky. You should be using only tabs to indent. > +#ifdef CONFIG_OF > +/* > + * Device tree users should 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. > + */ > +static struct of_device_id pwm_of_match[] = { > + { .compatible = "marvell,pxa250-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 *pwm_pxa_devid = > + of_match_device(pwm_of_match, dev); If you name this variable something much shorter, like "id", this will fit on a single line, and the code below will be slightly more readable as well. > + if (pwm_pxa_devid) > + return (const struct platform_device_id *)pwm_pxa_devid->data; > + else > + return NULL; > +} Thierry
On 09/08/2013 05:49 PM, 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 platform_device id table is reused for the match table >> data. Support for inverted polarity is also added. >> >> Tested on a Palm Treo 680 (both platform data and DT cases). >> >> Signed-off-by: Mike Dunn <mikedunn@newsguy.com> >> --- >> Changle log: >> 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 | 33 +++++++++++++ >> arch/arm/boot/dts/pxa27x.dtsi | 24 ++++++++++ >> drivers/pwm/pwm-pxa.c | 57 >> +++++++++++++++++++++++ 3 files changed, 114 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..7b09bfa >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt >> @@ -0,0 +1,33 @@ >> +Marvell pwm controller >> + >> +Required properties: >> +- compatible: >> + for pxa25x, pxa27x, pxa168, pxa910: must be compatible = >> "marvell,pxa250-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 3. >> + cell 1: the per-chip index of the PWM to use, >> + cell 2: the period in nanoseconds, > > Is there no generic OF prop for this? Yes, all the pwm drivers with DT support have the same definitions for cell 1 and 2. Many have #pwm-cells=2, but for those with #pwm-cells=3, all define cell 3 the same way also. > >> + cell 3: flags; set bit 0 to specify inverse polarity. > > Do we not have some generic flag to indicate inverted PWM polarity? Yes, enum pwm_polarity, defined in include/linux/pwm.h. The pwm drivers with polarity support all define cell 3 the same way. The wording in the bindings documentation is a little different in each case, but all essentially say the same thing. In Documentation/devicetree/bindings/pwm/ see for example atmel-tcb-pwm.txt and pwm-tiecap.txt. I'm new to device tree, so maybe I am not understanding correctly this and your previous question... All DT properties are described as integers lacking any type (aside from their bit width), no? > > [...] > >> +#ifdef CONFIG_OF >> +/* >> + * Device tree users should 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. >> + */ > > Above ... pxa250-pwm , no ? > >> +static struct of_device_id pwm_of_match[] = { >> + { .compatible = "marvell,pxa250-pwm", .data = &pwm_id_table[0]}, > > Surely, data can be NULL, no ? It could, in which case pxa_pwm_get_id_dt() would explicitly return &pwm_id_table[0] instead of the .data element of the of_device_id. Not sure which way is better and why. That dumb platform_device_id table is causing all kinds of nuisance :) > > [...] > >> @@ -145,6 +199,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 = of_pwm_xlate_with_flags; >> + pwm->chip.of_pwm_n_cells = 3; > > Are these two settings needed ? Yes. See drivers/pwm/core.c:of_pwmchip_add(), where they are set to default values of of_pwm_simple_xlate and 2 if left uninitialized. Thanks again Marek, Mike
Dear Mike Dunn, [...] > >> +static struct of_device_id pwm_of_match[] = { > >> + { .compatible = "marvell,pxa250-pwm", .data = &pwm_id_table[0]}, > > > > Surely, data can be NULL, no ? > > It could, in which case pxa_pwm_get_id_dt() would explicitly return > &pwm_id_table[0] instead of the .data element of the of_device_id. Not > sure which way is better and why. That dumb platform_device_id table is > causing all kinds of nuisance :) Is the pwm_id_table needed at all anymore? > > [...] > > > >> @@ -145,6 +199,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 = of_pwm_xlate_with_flags; > >> + pwm->chip.of_pwm_n_cells = 3; > > > > Are these two settings needed ? > > Yes. See drivers/pwm/core.c:of_pwmchip_add(), where they are set to > default values of of_pwm_simple_xlate and 2 if left uninitialized. OK Best regards, Marek Vasut
On 09/09/2013 05:08 AM, Thierry Reding wrote: > On Sun, Sep 08, 2013 at 12:40:18PM -0700, Mike Dunn wrote: >> This patch adds device tree support to the pxa's pwm driver. Only an OF match > > "PXA" and "PWM" OK. And thank you for the review! > >> 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. Support > > "ID table" OK > >> Changle log: >> 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 > > I think this needs to be reviewed by the device tree bindings > maintainers (as listed in MAINTAINERS) as well. Would you mind resending > the patch with all of them on Cc so that they have full context? Thanks. You bet. Sorry... should have checked that. Adding them to this reply, and will resend original patch. > >> Documentation/devicetree/bindings/pwm/pxa-pwm.txt | 33 +++++++++++++ >> arch/arm/boot/dts/pxa27x.dtsi | 24 ++++++++++ >> drivers/pwm/pwm-pxa.c | 57 +++++++++++++++++++++++ >> 3 files changed, 114 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..7b09bfa >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt >> @@ -0,0 +1,33 @@ >> +Marvell pwm controller >> + >> +Required properties: >> +- compatible: >> + for pxa25x, pxa27x, pxa168, pxa910: must be compatible = "marvell,pxa250-pwm"; >> +- reg: physical base address and length of the registers used by the pwm channel > > "pwm" -> "PWM". There are a few more occurrences that I haven't > explicitly pointed out. > >> + 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. > > I'm not sure I like that very much. It seems a wrong representation of > the hardware for the sake of modifying a smaller number of lines in the > driver. I don't like it either, but this is an existing driver defect that will need to be corrected. To do so, I will need to to survey all the supported processors to determine how many pwm outputs is posessed by each. And there may be some confusion in that regard; the driver says "pxa25x" has one, but my pxa255 developer's manual makes no mention of a pwm. The pxa270 I am testing on has four, but the driver says "pxa27x" has two, so currently (using platform_data with existing driver) two devices are instantiated, each with two pwms. It seems that there are many variations within a single generation of the processor family, so to correctly identify the number of pwms, processor identification will have to be made on a finer granularity than the current "pxa25x", for example. I'm guessing that the hardware designers had this one-device-instance-per-pwm approach in mind when they made the decision to segregate the register sets of each pwm. I really hope that this sin can be forgiven :) > >> +- #pwm-cells: should be 3. >> + cell 1: the per-chip index of the PWM to use, >> + cell 2: the period in nanoseconds, >> + cell 3: flags; set bit 0 to specify inverse polarity. The pwm controller > > This is the standard binding, so you can just refer to the standard > binding documentation instead, like so: > > - #pwm-cells: Should be 2. See pwm.txt in this directory for a description of > the cells format. > >> + does not implement reverse polarity, but some boards may pass the pwm output >> + through an external inverter, which can cause problems if a client device >> + assumes that an increase in the duty cycle results in an increase in output >> + power. The pwm-backlight is an example of such a client. > > Please don't do that. Reverse polarity support should not be emulated. > Either the hardware supports it or it doesn't. In the above case you > should be able to achieve the same effect by adding the correct values > in the pwm-backlight device node's brightness-levels property. That was my initial thought too. But sadly, that is not the case. The pwm-backlight driver assumes that the brightness-levels increase when parsed from left to right (more specifically, it assumes the last one is max). If emulated polarity is unacceptable, the pwm-backlight driver will need some rework to be usable in my case. The change looks minor and straightforward at first glance, though. > >> +Example pwm client node: >> + >> +backlight { >> + compatible = "pwm-backlight"; >> + pwms = <&pwm0 0 5000000 1>; /* pwm output is inverted */ >> + ... > > The indentation here is funky. You should be using only tabs to indent. OK > >> +#ifdef CONFIG_OF >> +/* >> + * Device tree users should 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. >> + */ >> +static struct of_device_id pwm_of_match[] = { >> + { .compatible = "marvell,pxa250-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 *pwm_pxa_devid = >> + of_match_device(pwm_of_match, dev); > > If you name this variable something much shorter, like "id", this will > fit on a single line, and the code below will be slightly more readable > as well. OK. I struggle not to be too verbose :) Thanks again, Mike
On 09/09/2013 08:56 AM, Marek Vasut wrote: > Dear Mike Dunn, > > [...] > >>>> +static struct of_device_id pwm_of_match[] = { >>>> + { .compatible = "marvell,pxa250-pwm", .data = &pwm_id_table[0]}, >>> >>> Surely, data can be NULL, no ? >> >> It could, in which case pxa_pwm_get_id_dt() would explicitly return >> &pwm_id_table[0] instead of the .data element of the of_device_id. Not >> sure which way is better and why. That dumb platform_device_id table is >> causing all kinds of nuisance :) > > Is the pwm_id_table needed at all anymore? If the original driver is modified to switch to the one-device-instance-per-pwm approach, it can be removed. But changing the original driver will require changes to all the client device instantiations. Ac case can be made for the change, though. It purports to identify the number of pwms on the processor, but in fact fails at it. > >>> [...] >>> >>>> @@ -145,6 +199,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 = of_pwm_xlate_with_flags; >>>> + pwm->chip.of_pwm_n_cells = 3; >>> >>> Are these two settings needed ? >> >> Yes. See drivers/pwm/core.c:of_pwmchip_add(), where they are set to >> default values of of_pwm_simple_xlate and 2 if left uninitialized. > > OK > Thanks, Mike
On 09/09/2013 12:03 PM, Mike Dunn wrote: > On 09/09/2013 05:08 AM, Thierry Reding wrote: >> On Sun, Sep 08, 2013 at 12:40:18PM -0700, Mike Dunn wrote: >>> This patch adds device tree support to the pxa's pwm driver. Only an OF match >>> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt >>> +Marvell pwm controller >>> + >>> +Required properties: >>> +- compatible: >>> + for pxa25x, pxa27x, pxa168, pxa910: must be compatible = "marvell,pxa250-pwm"; >>> +- reg: physical base address and length of the registers used by the pwm channel >> >> "pwm" -> "PWM". There are a few more occurrences that I haven't >> explicitly pointed out. >> >>> + 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. >> >> I'm not sure I like that very much. It seems a wrong representation of >> the hardware for the sake of modifying a smaller number of lines in the >> driver. > > I don't like it either, but this is an existing driver defect that will need to > be corrected. To do so, I will need to to survey all the supported processors > to determine how many pwm outputs is posessed by each. And there may be some > confusion in that regard; the driver says "pxa25x" has one, but my pxa255 > developer's manual makes no mention of a pwm. The pxa270 I am testing on has > four, but the driver says "pxa27x" has two, so currently (using platform_data > with existing driver) two devices are instantiated, each with two pwms. It > seems that there are many variations within a single generation of the processor > family, so to correctly identify the number of pwms, processor identification > will have to be made on a finer granularity than the current "pxa25x", for > example. I'm guessing that the hardware designers had this > one-device-instance-per-pwm approach in mind when they made the decision to > segregate the register sets of each pwm. I really hope that this sin can be > forgiven :) The DT binding is an ABI, so it needs to correctly represent the HW. As such, I'd say that if the binding is wrong, we need to fix it even if it means a lot of work. That said, if each PWM truly is a *completely* independent block, with non-overlapping register spaces, there's certainly an argument that it's perfectly acceptable to represent each PWM as a separate DT node...
On 09/09/2013 11:35 AM, Stephen Warren wrote: > On 09/09/2013 12:03 PM, Mike Dunn wrote: >> On 09/09/2013 05:08 AM, Thierry Reding wrote: >>> On Sun, Sep 08, 2013 at 12:40:18PM -0700, Mike Dunn wrote: >>>> This patch adds device tree support to the pxa's pwm driver. Only an OF match > >>>> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt > >>>> +Marvell pwm controller >>>> + >>>> +Required properties: >>>> +- compatible: >>>> + for pxa25x, pxa27x, pxa168, pxa910: must be compatible = "marvell,pxa250-pwm"; >>>> +- reg: physical base address and length of the registers used by the pwm channel >>> >>> "pwm" -> "PWM". There are a few more occurrences that I haven't >>> explicitly pointed out. >>> >>>> + 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. >>> >>> I'm not sure I like that very much. It seems a wrong representation of >>> the hardware for the sake of modifying a smaller number of lines in the >>> driver. >> >> I don't like it either, but this is an existing driver defect that will need to >> be corrected. To do so, I will need to to survey all the supported processors >> to determine how many pwm outputs is posessed by each. And there may be some >> confusion in that regard; the driver says "pxa25x" has one, but my pxa255 >> developer's manual makes no mention of a pwm. The pxa270 I am testing on has >> four, but the driver says "pxa27x" has two, so currently (using platform_data >> with existing driver) two devices are instantiated, each with two pwms. It >> seems that there are many variations within a single generation of the processor >> family, so to correctly identify the number of pwms, processor identification >> will have to be made on a finer granularity than the current "pxa25x", for >> example. I'm guessing that the hardware designers had this >> one-device-instance-per-pwm approach in mind when they made the decision to >> segregate the register sets of each pwm. I really hope that this sin can be >> forgiven :) > > The DT binding is an ABI, so it needs to correctly represent the HW. As > such, I'd say that if the binding is wrong, we need to fix it even if it > means a lot of work. > > That said, if each PWM truly is a *completely* independent block, with > non-overlapping register spaces, there's certainly an argument that it's > perfectly acceptable to represent each PWM as a separate DT node... Thanks Stephen. Yes, each pwm output has its own registers with no common control registers. The only commonality is that they all share a clock in the clock unit. But this is handled in the clock manager. Thanks, Mike
diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt new file mode 100644 index 0000000..7b09bfa --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt @@ -0,0 +1,33 @@ +Marvell pwm controller + +Required properties: +- compatible: + for pxa25x, pxa27x, pxa168, pxa910: must be compatible = "marvell,pxa250-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 3. + cell 1: the per-chip index of the PWM to use, + cell 2: the period in nanoseconds, + cell 3: flags; set bit 0 to specify inverse polarity. The pwm controller + does not implement reverse polarity, but some boards may pass the pwm output + through an external inverter, which can cause problems if a client device + assumes that an increase in the duty cycle results in an increase in output + power. The pwm-backlight is an example of such a client. + +Example pwm device node: + +pwm0: pwm@40b00000 { + compatible = "marvell,pxa250-pwm"; + reg = <0x40b00000 0x10>; + #pwm-cells = <3>; +}; + +Example pwm client node: + +backlight { + compatible = "pwm-backlight"; + pwms = <&pwm0 0 5000000 1>; /* pwm output is inverted */ + ... +} diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi index d7c5d72..a12fe8e 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,pxa250-pwm"; + reg = <0x40b00000 0x10>; + #pwm-cells = <3>; + }; + + pwm1: pwm@40b00010 { + compatible = "marvell,pxa250-pwm"; + reg = <0x40b00010 0x10>; + #pwm-cells = <3>; + }; + + pwm2: pwm@40c00000 { + compatible = "marvell,pxa250-pwm"; + reg = <0x40c00000 0x10>; + #pwm-cells = <3>; + }; + + pwm3: pwm@40c00010 { + compatible = "marvell,pxa250-pwm"; + reg = <0x40c00010 0x10>; + #pwm-cells = <3>; + }; }; }; diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c index a4d2164..33e6a7d 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> @@ -48,6 +49,7 @@ struct pxa_pwm_chip { struct clk *clk; void __iomem *mmio_base; + enum pwm_polarity polarity; }; static inline struct pxa_pwm_chip *to_pxa_pwm_chip(struct pwm_chip *chip) @@ -88,6 +90,15 @@ static int pxa_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, else dc = (pv + 1) * duty_ns / period_ns; + if (pc->polarity == PWM_POLARITY_INVERSED) { + if (dc & PWMDCR_FD) + dc = 0; /* continuously low */ + else if (dc == 0) + dc = PWMDCR_FD; /* continuously high */ + else + dc = 1023 - dc; /* complement duty cycle */ + } + /* NOTE: the clock to PWM has to be enabled first * before writing to the registers */ @@ -117,13 +128,51 @@ static void pxa_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) clk_disable_unprepare(pc->clk); } +static int pxa_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm, + enum pwm_polarity polarity) +{ + struct pxa_pwm_chip *pc = to_pxa_pwm_chip(chip); + pc->polarity = polarity; + return 0; +} + static struct pwm_ops pxa_pwm_ops = { .config = pxa_pwm_config, .enable = pxa_pwm_enable, .disable = pxa_pwm_disable, + .set_polarity = pxa_pwm_set_polarity, .owner = THIS_MODULE, }; +#ifdef CONFIG_OF +/* + * Device tree users should 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. + */ +static struct of_device_id pwm_of_match[] = { + { .compatible = "marvell,pxa250-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 *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 */ +static const struct platform_device_id *pxa_pwm_get_id_dt(struct device *dev) +{ + dev_info(dev, "missing platform data\n"); + return NULL; +} +#endif + static int pwm_probe(struct platform_device *pdev) { const struct platform_device_id *id = platform_get_device_id(pdev); @@ -131,6 +180,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 +199,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 = of_pwm_xlate_with_flags; + pwm->chip.of_pwm_n_cells = 3; r = platform_get_resource(pdev, IORESOURCE_MEM, 0); pwm->mmio_base = devm_ioremap_resource(&pdev->dev, r); @@ -176,6 +232,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,
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. Support for inverted polarity is also added. Tested on a Palm Treo 680 (both platform data and DT cases). Signed-off-by: Mike Dunn <mikedunn@newsguy.com> --- Changle log: 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 | 33 +++++++++++++ arch/arm/boot/dts/pxa27x.dtsi | 24 ++++++++++ drivers/pwm/pwm-pxa.c | 57 +++++++++++++++++++++++ 3 files changed, 114 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/pxa-pwm.txt