| Message ID | 1390467898-9216-2-git-send-email-s.hauer@pengutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
Hi, Sascha Hauer wrote: > of_pwm_n_cells for the of_xlate handler is stored in struct pwm_chip, > but it is only ever used by the of_xlate handler itsel. Remove > of_pwm_n_cells from struct pwm_chip and let the handler do the argument > count checking to simplify the code. > This still does not make the PWM_POLARITY flag in the pwms node optional as was the goal because of_parse_phandle_with_args() requires at least #pwm-cells arguments in the node. So, with a DT configuration like: pwm0: pwm@0 { #pwm-cells = <3>; }; backlight { pwms = <&pwm0 0 100000>; }; the driver will bail out at of_parse_phandle_with_args() in of_pwm_get() with the error message: "/backlight: arguments longer than property" and never reach your clever xlate function. Thus you will still need to replace of_parse_phandle_with_args() with different code that copies most but not all of the functionality. Lothar Waßmann
On Thu, Jan 23, 2014 at 11:56:32AM +0100, Lothar Waßmann wrote: > Hi, > > Sascha Hauer wrote: > > of_pwm_n_cells for the of_xlate handler is stored in struct pwm_chip, > > but it is only ever used by the of_xlate handler itsel. Remove > > of_pwm_n_cells from struct pwm_chip and let the handler do the argument > > count checking to simplify the code. > > > This still does not make the PWM_POLARITY flag in the pwms node > optional as was the goal because of_parse_phandle_with_args() requires > at least #pwm-cells arguments in the node. > > So, with a DT configuration like: > pwm0: pwm@0 { > #pwm-cells = <3>; > }; > backlight { > pwms = <&pwm0 0 100000>; > }; We misunderstood each other. My goal was to allow the driver to also work with old devicetrees which specify #pwm-cells = <2>, not to allow inconsistent devicetrees like the snippet above. Sascha
On Thu, Jan 23, 2014 at 12:04:44PM +0100, Sascha Hauer wrote: > On Thu, Jan 23, 2014 at 11:56:32AM +0100, Lothar Waßmann wrote: > > Hi, > > > > Sascha Hauer wrote: > > > of_pwm_n_cells for the of_xlate handler is stored in struct pwm_chip, > > > but it is only ever used by the of_xlate handler itsel. Remove > > > of_pwm_n_cells from struct pwm_chip and let the handler do the argument > > > count checking to simplify the code. > > > > > This still does not make the PWM_POLARITY flag in the pwms node > > optional as was the goal because of_parse_phandle_with_args() requires > > at least #pwm-cells arguments in the node. > > > > So, with a DT configuration like: > > pwm0: pwm@0 { > > #pwm-cells = <3>; > > }; > > backlight { > > pwms = <&pwm0 0 100000>; > > }; > > We misunderstood each other. My goal was to allow the driver to also > work with old devicetrees which specify #pwm-cells = <2>, not to allow > inconsistent devicetrees like the snippet above. In which case, the patch I've posted seems to do that job too... I'm just about to test out the three-cell version.
On Thu, Jan 23, 2014 at 04:53:50PM +0000, Russell King - ARM Linux wrote: > On Thu, Jan 23, 2014 at 12:04:44PM +0100, Sascha Hauer wrote: > > On Thu, Jan 23, 2014 at 11:56:32AM +0100, Lothar Waßmann wrote: > > > Hi, > > > > > > Sascha Hauer wrote: > > > > of_pwm_n_cells for the of_xlate handler is stored in struct pwm_chip, > > > > but it is only ever used by the of_xlate handler itsel. Remove > > > > of_pwm_n_cells from struct pwm_chip and let the handler do the argument > > > > count checking to simplify the code. > > > > > > > This still does not make the PWM_POLARITY flag in the pwms node > > > optional as was the goal because of_parse_phandle_with_args() requires > > > at least #pwm-cells arguments in the node. > > > > > > So, with a DT configuration like: > > > pwm0: pwm@0 { > > > #pwm-cells = <3>; > > > }; > > > backlight { > > > pwms = <&pwm0 0 100000>; > > > }; > > > > We misunderstood each other. My goal was to allow the driver to also > > work with old devicetrees which specify #pwm-cells = <2>, not to allow > > inconsistent devicetrees like the snippet above. > > In which case, the patch I've posted seems to do that job too... I'm > just about to test out the three-cell version. Okay, this works, but there's a problem with pwm-leds. When the duty cycle is set to zero (when you set the brightness to zero) pwm-leds decides to disable the PWM after configuring it. This causes the PWM output to be driven low, causing the LED to go to maximum brightness. So, using the inversion at PWM level doesn't work. To make this work correctly, we really need pwm-leds to do the inversion rather than setting the inversion bit in hardware.
Hi, Russell King - ARM Linux wrote: > On Thu, Jan 23, 2014 at 04:53:50PM +0000, Russell King - ARM Linux wrote: > > On Thu, Jan 23, 2014 at 12:04:44PM +0100, Sascha Hauer wrote: > > > On Thu, Jan 23, 2014 at 11:56:32AM +0100, Lothar Waßmann wrote: > > > > Hi, > > > > > > > > Sascha Hauer wrote: > > > > > of_pwm_n_cells for the of_xlate handler is stored in struct pwm_chip, > > > > > but it is only ever used by the of_xlate handler itsel. Remove > > > > > of_pwm_n_cells from struct pwm_chip and let the handler do the argument > > > > > count checking to simplify the code. > > > > > > > > > This still does not make the PWM_POLARITY flag in the pwms node > > > > optional as was the goal because of_parse_phandle_with_args() requires > > > > at least #pwm-cells arguments in the node. > > > > > > > > So, with a DT configuration like: > > > > pwm0: pwm@0 { > > > > #pwm-cells = <3>; > > > > }; > > > > backlight { > > > > pwms = <&pwm0 0 100000>; > > > > }; > > > > > > We misunderstood each other. My goal was to allow the driver to also > > > work with old devicetrees which specify #pwm-cells = <2>, not to allow > > > inconsistent devicetrees like the snippet above. > > > > In which case, the patch I've posted seems to do that job too... I'm > > just about to test out the three-cell version. > > Okay, this works, but there's a problem with pwm-leds. > > When the duty cycle is set to zero (when you set the brightness to zero) > pwm-leds decides to disable the PWM after configuring it. This causes > the PWM output to be driven low, causing the LED to go to maximum > brightness. > > So, using the inversion at PWM level doesn't work. > The problem is that the driver calls pwm_disable() when the duty cycle is 0. This sets the PWM output low independent from the output polarity setting. > To make this work correctly, we really need pwm-leds to do the inversion > rather than setting the inversion bit in hardware. > The same holds for the pwm-backlight driver. The easiest fix would be not to call pwm_disable() even for a zero duty cycle. Lothar Waßmann
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 2ca9504..c882051 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -136,7 +136,7 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args) { struct pwm_device *pwm; - if (pc->of_pwm_n_cells < 3) + if (args->args_count != 3) return ERR_PTR(-EINVAL); if (args->args[0] >= pc->npwm) @@ -162,7 +162,7 @@ of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) { struct pwm_device *pwm; - if (pc->of_pwm_n_cells < 2) + if (args->args_count != 2) return ERR_PTR(-EINVAL); if (args->args[0] >= pc->npwm) @@ -182,10 +182,8 @@ static void of_pwmchip_add(struct pwm_chip *chip) if (!chip->dev || !chip->dev->of_node) return; - if (!chip->of_xlate) { + if (!chip->of_xlate) chip->of_xlate = of_pwm_simple_xlate; - chip->of_pwm_n_cells = 2; - } of_node_get(chip->dev->of_node); } @@ -536,13 +534,6 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id) goto put; } - if (args.args_count != pc->of_pwm_n_cells) { - pr_debug("%s: wrong #pwm-cells for %s\n", np->full_name, - args.np->full_name); - pwm = ERR_PTR(-EINVAL); - goto put; - } - pwm = pc->of_xlate(pc, &args); if (IS_ERR(pwm)) goto put; diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c index f3dcd02..55fabf8 100644 --- a/drivers/pwm/pwm-atmel-tcb.c +++ b/drivers/pwm/pwm-atmel-tcb.c @@ -395,7 +395,6 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev) tcbpwm->chip.dev = &pdev->dev; tcbpwm->chip.ops = &atmel_tcb_pwm_ops; tcbpwm->chip.of_xlate = of_pwm_xlate_with_flags; - tcbpwm->chip.of_pwm_n_cells = 3; tcbpwm->chip.base = -1; tcbpwm->chip.npwm = NPWM; tcbpwm->tc = tc; diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c index aff6ba9..0a8adb6 100644 --- a/drivers/pwm/pwm-renesas-tpu.c +++ b/drivers/pwm/pwm-renesas-tpu.c @@ -434,7 +434,6 @@ static int tpu_probe(struct platform_device *pdev) tpu->chip.dev = &pdev->dev; tpu->chip.ops = &tpu_pwm_ops; tpu->chip.of_xlate = of_pwm_xlate_with_flags; - tpu->chip.of_pwm_n_cells = 3; tpu->chip.base = -1; tpu->chip.npwm = TPU_CHANNEL_MAX; diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c index b59639e..8d8dced 100644 --- a/drivers/pwm/pwm-samsung.c +++ b/drivers/pwm/pwm-samsung.c @@ -487,7 +487,6 @@ static int pwm_samsung_probe(struct platform_device *pdev) return ret; chip->chip.of_xlate = of_pwm_xlate_with_flags; - chip->chip.of_pwm_n_cells = 3; } else { if (!pdev->dev.platform_data) { dev_err(&pdev->dev, "no platform data specified\n"); diff --git a/drivers/pwm/pwm-tiecap.c b/drivers/pwm/pwm-tiecap.c index 4e5c3d1..4d7a01a 100644 --- a/drivers/pwm/pwm-tiecap.c +++ b/drivers/pwm/pwm-tiecap.c @@ -229,7 +229,6 @@ static int ecap_pwm_probe(struct platform_device *pdev) pc->chip.dev = &pdev->dev; pc->chip.ops = &ecap_pwm_ops; pc->chip.of_xlate = of_pwm_xlate_with_flags; - pc->chip.of_pwm_n_cells = 3; pc->chip.base = -1; pc->chip.npwm = 1; diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c index a4d8f51..2c2621a 100644 --- a/drivers/pwm/pwm-tiehrpwm.c +++ b/drivers/pwm/pwm-tiehrpwm.c @@ -460,7 +460,6 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev) pc->chip.dev = &pdev->dev; pc->chip.ops = &ehrpwm_pwm_ops; pc->chip.of_xlate = of_pwm_xlate_with_flags; - pc->chip.of_pwm_n_cells = 3; pc->chip.base = -1; pc->chip.npwm = NUM_PWM_CHANNEL; diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c index 323125a..5472051 100644 --- a/drivers/pwm/pwm-vt8500.c +++ b/drivers/pwm/pwm-vt8500.c @@ -219,7 +219,6 @@ static int vt8500_pwm_probe(struct platform_device *pdev) chip->chip.dev = &pdev->dev; chip->chip.ops = &vt8500_pwm_ops; chip->chip.of_xlate = of_pwm_xlate_with_flags; - chip->chip.of_pwm_n_cells = 3; chip->chip.base = -1; chip->chip.npwm = VT8500_NR_PWMS;
of_pwm_n_cells for the of_xlate handler is stored in struct pwm_chip, but it is only ever used by the of_xlate handler itsel. Remove of_pwm_n_cells from struct pwm_chip and let the handler do the argument count checking to simplify the code. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/pwm/core.c | 15 +++------------ drivers/pwm/pwm-atmel-tcb.c | 1 - drivers/pwm/pwm-renesas-tpu.c | 1 - drivers/pwm/pwm-samsung.c | 1 - drivers/pwm/pwm-tiecap.c | 1 - drivers/pwm/pwm-tiehrpwm.c | 1 - drivers/pwm/pwm-vt8500.c | 1 - 7 files changed, 3 insertions(+), 18 deletions(-)