| Message ID | 1379519982-7618-1-git-send-email-mikedunn@newsguy.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 18, 2013 at 08:59:42AM -0700, Mike Dunn wrote: [...] > Only an OF match table is added; [...] That's no longer true. You also add a custom .of_xlate() function. > diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt [...] > +Required properties: > +- compatible: should be one or more 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 Nit: "NB:" looks like it starts a new property. Perhaps something like: "Note that one device node must be present for each PWM ..." would be less confusing to the eye. > + 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. So that again confuses me. If the controller really is one or two PWM channels, and four PWM channels, as given in the pxa27x.dtsi, are in fact two controllers (not four) then I wonder if this really is the correct binding for it. That said, Stephen seems to be okay with it, and I'll yield to his authority on the matter. > +- #pwm-cells: should be 1. This cell is used to specify the period in Nit: "Should be 1." It's a sentence and therefore should start with a capital letter. > + nanoseconds. (Because one device instance is created for each PWM output, > + the per-chip index is superflous and not used.) I'd omit the parentheses (and their content). The description of the cell is plenty specific about what it should contain. No need to list what it shouldn't contain. > diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi I think this belongs in a separate patch so it can go through the arm-soc tree. > diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c [...] > +#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; Is that cast really necessary? id->data is const void *, so shouldn't need a cast. > + else > + return NULL; Perhaps "return id ? id->data : NULL;"? > +struct pwm_device * > +pxa_pwm_of_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) Should be static. > +{ > + struct pwm_device *pwm; > + You probably want to check that pc->of_pwm_n_cells at least 1 here. > + 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 I prefer not to have these dummy implementations for static functions. See below... > 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); This could be replaced with: if (IS_ENABLED(CONFIG_OF) && id == NULL) id = pxa_pwm_get_id_dt(&pdev->dev); And pxa_pwm_get_id_dt() can be unconditionally defined. The compiler will automatically remove it for !OF because it is never used. Also the comment can then be dropped since the code is already explicit. > 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; Similarly if you write this as: if (IS_ENABLED(CONFIG_OF)) { pwm->chip.of_xlate = pxa_pwm_of_xlate; pwm->chip.of_pwm_n_cells = 1; } Then the only thing that needs #ifdef CONFIG_OF protection will be the OF match table. Thierry
On 09/19/2013 05:28 AM, Thierry Reding wrote: > On Wed, Sep 18, 2013 at 08:59:42AM -0700, Mike Dunn wrote: > [...] >> Only an OF match table is added; > [...] > > That's no longer true. You also add a custom .of_xlate() function. Is it acceptable to re-word the commit message in susbequent versions? > >> diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt > [...] >> +Required properties: >> +- compatible: should be one or more 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 > > Nit: "NB:" looks like it starts a new property. Perhaps something like: > > "Note that one device node must be present for each PWM ..." > > would be less confusing to the eye. Yes, good point. I like clarity myself. > >> + 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. > > So that again confuses me. If the controller really is one or two PWM > channels, and four PWM channels, as given in the pxa27x.dtsi, are in > fact two controllers (not four) then I wonder if this really is the > correct binding for it. > > That said, Stephen seems to be okay with it, and I'll yield to his > authority on the matter. > >> +- #pwm-cells: should be 1. This cell is used to specify the period in > > Nit: "Should be 1." It's a sentence and therefore should start with a > capital letter. OK > >> + nanoseconds. (Because one device instance is created for each PWM output, >> + the per-chip index is superflous and not used.) > > I'd omit the parentheses (and their content). The description of the > cell is plenty specific about what it should contain. No need to list > what it shouldn't contain. OK. > >> diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi > > I think this belongs in a separate patch so it can go through the > arm-soc tree. Is there anyone else or another list that should be CC'd in this case? Thanks. > >> diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c > [...] >> +#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; > > Is that cast really necessary? id->data is const void *, so shouldn't > need a cast. You're right. > >> + else >> + return NULL; > > Perhaps "return id ? id->data : NULL;"? OK > >> +struct pwm_device * >> +pxa_pwm_of_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) > > Should be static. Oops, yes. > >> +{ >> + struct pwm_device *pwm; >> + > > You probably want to check that pc->of_pwm_n_cells at least 1 here. Well OK, but I didn't bother because it's explicitly set to one elsewhere in this source file. > >> + 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 > > I prefer not to have these dummy implementations for static functions. > See below... > >> 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); > > This could be replaced with: > > if (IS_ENABLED(CONFIG_OF) && id == NULL) > id = pxa_pwm_get_id_dt(&pdev->dev); > > And pxa_pwm_get_id_dt() can be unconditionally defined. The compiler > will automatically remove it for !OF because it is never used. Also the > comment can then be dropped since the code is already explicit. > >> 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; > > Similarly if you write this as: > > if (IS_ENABLED(CONFIG_OF)) { > pwm->chip.of_xlate = pxa_pwm_of_xlate; > pwm->chip.of_pwm_n_cells = 1; > } > > Then the only thing that needs #ifdef CONFIG_OF protection will be the > OF match table. Yes, much better. Thanks again Thierry. Mike
On Thu, Sep 19, 2013 at 09:53:31AM -0700, Mike Dunn wrote: > On 09/19/2013 05:28 AM, Thierry Reding wrote: > > On Wed, Sep 18, 2013 at 08:59:42AM -0700, Mike Dunn wrote: > > [...] > >> Only an OF match table is added; > > [...] > > > > That's no longer true. You also add a custom .of_xlate() function. > > > Is it acceptable to re-word the commit message in susbequent versions? Yes, of course. The commit message should be an accurate description of the patch. If as a result of review the patch changes in ways that make the commit message wrong, then it must be updated as well. > >> diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi > > > > I think this belongs in a separate patch so it can go through the > > arm-soc tree. > > > Is there anyone else or another list that should be CC'd in this case? Thanks. Well, the PXA maintainers would be the most likely to take the dtsi change through their tree. Usually the mach trees are merged into arm-soc, which then goes into Linus' tree. > >> +{ > >> + struct pwm_device *pwm; > >> + > > > > You probably want to check that pc->of_pwm_n_cells at least 1 here. > > > Well OK, but I didn't bother because it's explicitly set to one elsewhere in > this source file. Okay, nevermind. It's probably fine as is. Thierry
diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt new file mode 100644 index 0000000..01aca2e --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt @@ -0,0 +1,31 @@ +Marvell PWM controller + +Required properties: +- compatible: should be one or more 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..a705469 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", "marvell,pxa250-pwm"; + reg = <0x40b00000 0x10>; + #pwm-cells = <1>; + }; + + pwm1: pwm@40b00010 { + compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm"; + reg = <0x40b00010 0x10>; + #pwm-cells = <1>; + }; + + pwm2: pwm@40c00000 { + compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm"; + reg = <0x40c00000 0x10>; + #pwm-cells = <1>; + }; + + pwm3: pwm@40c00010 { + compatible = "marvell,pxa270-pwm", "marvell,pxa250-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,
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> --- Thanks Stephen for the reviews and explanations. Changle log: v4: - add second "compatible" string to pxa27x.dtsi - change phrasing in binding doc pxa-pwm.txt to "one or more of" 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