pwm: imx: support output polarity inversion
Subsystems (6)
v4 cover pwm: imx: support output polarity inversion
This patch series adds support for polarity inversion to the pwm-imx driver. The patches have been tested on i.MX6, i.MX53 and with the ti-ehrpwm.c driver.
Changes wrt. v2:
- make the use of '#pwm-cells = <3>' optional, so that the change does not break existing DT blobs as suggested by Arnd Bergmann and Sascha Hauer.
Changes wrt. v3:
- implemented the approach suggested by Sascha Hauer
- don't modify struct pwm_ops in the imx_pwm probe function
Changes wrt. v4:
- rebased onto current linux-next
- dropped cleanup patch which is not necessary any more
- resent with ACK from Shawn Guo and Reviewed-By tag from Sascha Hauer
PATCH v5 1/3 pwm: print error messages with pr_err() instead of pr_debug()
Make the messages that are printed in case of fatal errors actually visible to the user without having to recompile the driver with debugging enabled.
--- drivers/pwm/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/pwm/core.c +2 -2
@@ -525,13 +525,13 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id) err =of_parse_phandle_with_args (np,"pwms" ,"#pwm-cells" , index, &args);if (err) { -pr_debug ("%s(): can't parse \"pwms\" property\n" , __func__); +pr_err ("%s(): can't parse \"pwms\" property\n" , __func__);return ERR_PTR (err); } pc =of_node_to_pwmchip (args.np);if (IS_ERR (pc)) { -pr_debug ("%s(): PWM chip not found\n" , __func__); +pr_err ("%s(): PWM chip not found\n" , __func__); pwm =ERR_CAST (pc); goto put; }
Signature · 1 line
1.7.10.4
PATCH v5 2/3 pwm: make the PWM_POLARITY flag in DTB optional
In reply to Lothar WaßmannChange the pwm chip driver registration, so that a chip driver that supports polarity inversion can still be used with DTBs that don't provide the 'PWM_POLARITY' flag.
This is done to provide polarity inversion support for the pwm-imx driver without having to modify all existing DTS files.
--- drivers/pwm/core.c | 18 ++++++------------ drivers/pwm/pwm-atmel-tcb.c | 1 - drivers/pwm/pwm-atmel.c | 4 +--- drivers/pwm/pwm-pxa.c | 5 ++--- 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 - include/linux/pwm.h | 1 - 10 files changed, 9 insertions(+), 25 deletions(-)
drivers/pwm/core.c +6 -12
@@ -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,12 +162,15 @@ 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)return ERR_PTR (-EINVAL ); +if (args->args_count >2 ) +return of_pwm_xlate_with_flags (pc, args); + pwm =pwm_request_from_chip (pc, args->args[0 ],NULL );if (IS_ERR (pwm))return pwm;
@@ -182,10 +185,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 +537,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;
drivers/pwm/pwm-atmel-tcb.c +0 -1
@@ -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;
drivers/pwm/pwm-atmel.c +1 -3
@@ -348,10 +348,8 @@ static int atmel_pwm_probe(struct platform_device *pdev) atmel_pwm->chip.dev = &pdev->dev; atmel_pwm->chip.ops = &atmel_pwm_ops; -if (pdev->dev.of_node) { +if (pdev->dev.of_node) atmel_pwm->chip.of_xlate = of_pwm_xlate_with_flags; - atmel_pwm->chip.of_pwm_n_cells =3 ; - } atmel_pwm->chip.base =-1 ; atmel_pwm->chip.npwm =4 ;
drivers/pwm/pwm-pxa.c +2 -3
@@ -191,10 +191,9 @@ static int pwm_probe(struct platform_device *pdev) pwm->chip.base =-1 ; pwm->chip.npwm = (id->driver_data &HAS_SECONDARY_PWM ) ?2 :1 ; -if (IS_ENABLED (CONFIG_OF )) { +if (IS_ENABLED (CONFIG_OF )) 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);
drivers/pwm/pwm-renesas-tpu.c +0 -1
@@ -419,7 +419,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 ;
drivers/pwm/pwm-samsung.c +0 -1
@@ -484,7 +484,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" );
drivers/pwm/pwm-tiecap.c +0 -1
@@ -227,7 +227,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 ;
drivers/pwm/pwm-tiehrpwm.c +0 -1
@@ -458,7 +458,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 ;
drivers/pwm/pwm-vt8500.c +0 -1
@@ -217,7 +217,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 ;
include/linux/pwm.h +0 -1
@@ -174,7 +174,6 @@ struct pwm_chip {struct pwm_device * (*of_xlate)(struct pwm_chip *pc,const struct of_phandle_args *args); -unsigned int of_pwm_n_cells;bool can_sleep; };
Signature · 1 line
1.7.10.4
PATCH v5 2/3 pwm: make the PWM_POLARITY flag in DTB optional
Quoted 6 lines · Lothar Wa?mann
Change the pwm chip driver registration, so that a chip driver that supports polarity inversion can still be used with DTBs that don't provide the '
PWM_POLARITY' flag.This is done to provide polarity inversion support for the pwm-imx driver without having to modify all existing DTS files.
I don't like how this throws out the window the only sanity checking we have in place for the #pwm-cells property.
As I understand it, the problem that you're trying to solve is one of backwards-compatibility where existing device trees have #pwm-cells = <2>, but the driver is extended to support flags as well.
In that case, can we not simply make of_pwm_xlate_with_flags() support that case transparently? That is, if the driver sets .of_pwm_n_cells to 3, we can still support #pwm-cells = <2> and use the default (no) flags instead.
Something like the below should do that (compile-tested only).
Thierry
-------------- next part --------------
drivers/pwm/core.c +16 -11
@@ -136,9 +136,14 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args) {struct pwm_device *pwm; +/* check that the driver supports a third cell for flags */ if (pc->of_pwm_n_cells <3 )return ERR_PTR (-EINVAL ); +/* flags in the third cell are optional */ +if (args->args_count <2 ) +return ERR_PTR (-EINVAL ); +if (args->args[0 ] >= pc->npwm)return ERR_PTR (-EINVAL );
@@ -148,10 +153,12 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)pwm_set_period (pwm, args->args[1 ]); -if (args->args[2 ] &PWM_POLARITY_INVERTED ) -pwm_set_polarity (pwm,PWM_POLARITY_INVERSED ); -else -pwm_set_polarity (pwm,PWM_POLARITY_NORMAL ); +if (args->args_count >2 ) { +if (args->args[2 ] &PWM_POLARITY_INVERTED ) +pwm_set_polarity (pwm,PWM_POLARITY_INVERSED ); +else +pwm_set_polarity (pwm,PWM_POLARITY_NORMAL ); + }return pwm; }
@@ -162,9 +169,14 @@ of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) {struct pwm_device *pwm; +/* sanity check driver support */ if (pc->of_pwm_n_cells <2 )return ERR_PTR (-EINVAL ); +/* all cells are required */ +if (args->args_count != pc->of_pwm_n_cells) +return ERR_PTR (-EINVAL ); +if (args->args[0 ] >= pc->npwm)return ERR_PTR (-EINVAL );
@@ -536,13 +548,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;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141009/da3e1b34/attachment.sig>
v6 cover pwm: imx: support output polarity inversion
This patch series adds support for polarity inversion to the pwm-imx driver. The patches have been tested on i.MX6, i.MX53 and with the ti-ehrpwm.c driver.
Changes wrt. v2:
- make the use of '#pwm-cells = <3>' optional, so that the change does not break existing DT blobs as suggested by Arnd Bergmann and Sascha Hauer.
Changes wrt. v3:
- implemented the approach suggested by Sascha Hauer
- don't modify struct pwm_ops in the imx_pwm probe function
Changes wrt. v4:
- rebased onto current linux-next
- dropped cleanup patch which is not necessary any more
- resent with ACK from Shawn Guo and Reviewed-By tag from Sascha Hauer
Changes wrt. v5:
- don't remove of_pwm_n_cells to keep sanity checks for #pwm-cells as suggested by Thierry Reding
PATCH v6 1/3 pwm: print error messages with pr_err() instead of pr_debug()
Make the messages that are printed in case of fatal errors actually visible to the user without having to recompile the driver with debugging enabled.
--- drivers/pwm/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/pwm/core.c +2 -2
@@ -525,13 +525,13 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id) err =of_parse_phandle_with_args (np,"pwms" ,"#pwm-cells" , index, &args);if (err) { -pr_debug ("%s(): can't parse \"pwms\" property\n" , __func__); +pr_err ("%s(): can't parse \"pwms\" property\n" , __func__);return ERR_PTR (err); } pc =of_node_to_pwmchip (args.np);if (IS_ERR (pc)) { -pr_debug ("%s(): PWM chip not found\n" , __func__); +pr_err ("%s(): PWM chip not found\n" , __func__); pwm =ERR_CAST (pc); goto put; }
Signature · 1 line
1.7.10.4
PATCH v6 2/3 pwm: make the PWM_POLARITY flag in DTB optional
In reply to Lothar WaßmannChange the pwm chip driver registration, so that a chip driver that supports polarity inversion can still be used with DTBs that don't provide the 'PWM_POLARITY' flag.
This is done to provide polarity inversion support for the pwm-imx driver without having to modify all existing DTS files.
--- drivers/pwm/core.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-)
drivers/pwm/core.c +16 -11
@@ -136,9 +136,14 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args) {struct pwm_device *pwm; +/* check, whether the driver supports a third cell for flags */ if (pc->of_pwm_n_cells <3 )return ERR_PTR (-EINVAL ); +/* flags in the third cell are optional */ +if (args->args_count <2 ) +return ERR_PTR (-EINVAL ); +if (args->args[0 ] >= pc->npwm)return ERR_PTR (-EINVAL );
@@ -148,10 +153,12 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)pwm_set_period (pwm, args->args[1 ]); -if (args->args[2 ] &PWM_POLARITY_INVERTED ) -pwm_set_polarity (pwm,PWM_POLARITY_INVERSED ); -else -pwm_set_polarity (pwm,PWM_POLARITY_NORMAL ); +if (args->args_count >2 ) { +if (args->args[2 ] &PWM_POLARITY_INVERTED ) +pwm_set_polarity (pwm,PWM_POLARITY_INVERSED ); +else +pwm_set_polarity (pwm,PWM_POLARITY_NORMAL ); + }return pwm; }
@@ -162,9 +169,14 @@ of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) {struct pwm_device *pwm; +/* sanity check driver support */ if (pc->of_pwm_n_cells <2 )return ERR_PTR (-EINVAL ); +/* all cells are required */ +if (args->args_count != pc->of_pwm_n_cells) +return ERR_PTR (-EINVAL ); +if (args->args[0 ] >= pc->npwm)return ERR_PTR (-EINVAL );
@@ -536,13 +548,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;
Signature · 1 line
1.7.10.4
PATCH v4 3/3 pwm: imx: support output polarity inversion
In reply to Lothar WaßmannThe i.MX pwm unit on i.MX27 and newer SoCs provides a configurable output polarity. This patch adds support to utilize this feature where available.
--- drivers/pwm/pwm-imx.c | 43 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 3 deletions(-)
drivers/pwm/pwm-imx.c +40 -3
@@ -38,6 +38,7 @@#define MX3_PWMCR_DOZEEN (1 << 24)#define MX3_PWMCR_WAITEN (1 << 23)#define MX3_PWMCR_DBGEN (1 << 22) +#define MX3_PWMCR_POUTC (1 << 18)#define MX3_PWMCR_CLKSRC_IPG_HIGH (2 << 16)#define MX3_PWMCR_CLKSRC_IPG (1 << 16)#define MX3_PWMCR_SWR (1 << 3)
@@ -179,6 +180,9 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,if (enable) cr |=MX3_PWMCR_EN ; +if (pwm->polarity ==PWM_POLARITY_INVERSED ) + cr |=MX3_PWMCR_POUTC ; +writel (cr, imx->mmio_base +MX3_PWMCR );return 0 ;
@@ -196,6 +200,11 @@ static void imx_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)else val &= ~MX3_PWMCR_EN ; +if (chip->pwms[0 ].polarity ==PWM_POLARITY_INVERSED ) + val |=MX3_PWMCR_POUTC ; +else + val &= ~MX3_PWMCR_POUTC ; +writel (val, imx->mmio_base +MX3_PWMCR ); }
@@ -239,27 +248,49 @@ static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)clk_disable_unprepare (imx->clk_per); } -static struct pwm_ops imx_pwm_ops = { +static int imx_pwm_set_polarity (struct pwm_chip *chip,struct pwm_device *pwm, +enum pwm_polarity polarity) +{ +struct imx_chip *imx =to_imx_chip (chip); + +dev_dbg (imx->chip.dev,"%s: polarity set to %s\n" , __func__, + polarity ==PWM_POLARITY_INVERSED ?"inverted" :"normal" ); + +return 0 ; +} + +static struct pwm_ops imx_pwm_ops_v1 = { .enable = imx_pwm_enable, .disable = imx_pwm_disable, .config = imx_pwm_config, .owner =THIS_MODULE , }; +static struct pwm_ops imx_pwm_ops_v2 = { + .enable = imx_pwm_enable, + .disable = imx_pwm_disable, + .set_polarity = imx_pwm_set_polarity, + .config = imx_pwm_config, + .owner =THIS_MODULE , +}; +struct imx_pwm_data {int (*config)(struct pwm_chip *chip,struct pwm_device *pwm,int duty_ns,int period_ns);void (*set_enable)(struct pwm_chip *chip,bool enable); +struct pwm_ops *pwm_ops; };static struct imx_pwm_data imx_pwm_data_v1 = { .config = imx_pwm_config_v1, .set_enable = imx_pwm_set_enable_v1, + .pwm_ops = &imx_pwm_ops_v1, };static struct imx_pwm_data imx_pwm_data_v2 = { .config = imx_pwm_config_v2, .set_enable = imx_pwm_set_enable_v2, + .pwm_ops = &imx_pwm_ops_v2, };static const struct of_device_id imx_pwm_dt_ids[] = {
@@ -281,6 +312,8 @@ static int imx_pwm_probe(struct platform_device *pdev)if (!of_id)return -ENODEV ; + data = of_id->data; + imx =devm_kzalloc (&pdev->dev,sizeof (*imx),GFP_KERNEL );if (imx ==NULL )return -ENOMEM ;
@@ -299,18 +332,22 @@ static int imx_pwm_probe(struct platform_device *pdev)return PTR_ERR (imx->clk_ipg); } - imx->chip.ops = &imx_pwm_ops; + imx->chip.ops = data->pwm_ops; imx->chip.dev = &pdev->dev; imx->chip.base =-1 ; imx->chip.npwm =1 ; imx->chip.can_sleep = true; +if (data->pwm_ops->set_polarity) { +dev_dbg (&pdev->dev,"PWM supports output inversion\n" ); + imx->chip.of_xlate = of_pwm_xlate_with_flags; + imx->chip.of_pwm_n_cells =3 ; + } r =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); imx->mmio_base =devm_ioremap_resource (&pdev->dev, r);if (IS_ERR (imx->mmio_base))return PTR_ERR (imx->mmio_base); - data = of_id->data; imx->config = data->config; imx->set_enable = data->set_enable;
Signature · 1 line
1.7.10.4
v6 cover pwm: imx: support output polarity inversion
In reply to Lothar WaßmannHi Lothar,
Quoted 3 lines · Lothar Wa?mann
This patch series adds support for polarity inversion to the pwm-imx driver. The patches have been tested on i.MX6, i.MX53 and with the
ti-ehrpwm.cdriver.
Do you know what prevented this patchset from getting merged?
We are looking for Polarity support in PWM for too, this is especially useful for backlight control.
--
Stefan
Quoted 24 lines
Changes wrt. v2:
- make the use of '#pwm-cells = <3>' optional, so that the change does not break existing DT blobs as suggested by Arnd Bergmann and Sascha Hauer.Changes wrt. v3:
- implemented the approach suggested by Sascha Hauer
- don't modify structpwm_opsin theimx_pwmprobe functionChanges wrt. v4:
- rebased onto current linux-next
- dropped cleanup patch which is not necessary any more
- resent with ACK from Shawn Guo and Reviewed-By tag from Sascha HauerChanges wrt. v5:
- don't removeof_pwm_n_cellsto keep sanity checks for #pwm-cells as suggested by Thierry Reding_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
v6 cover pwm: imx: support output polarity inversion
Hi,
Quoted 7 lines · On Thu, 08 Sep 2016 15:15:57 -0700 Stefan Agner wrote:
Quoted 3 lines · Lothar Wa?mann
This patch series adds support for polarity inversion to the pwm-imx driver. The patches have been tested on i.MX6, i.MX53 and with the
ti-ehrpwm.cdriver.Do you know what prevented this patchset from getting merged?
No idea.
Quoted 3 lines
We are looking for Polarity support in PWM for too, this is especially useful for backlight control.
Actually the PWM driver may be the wrong place to achieve this. When the backlight driver sets the brightness to 0 to switch the backlight off, it will disable the PWM. This will make the PWM pin go LOW and thus turn the backlight to full brightness rather than off (unless there is an additional GPIO that controls a backlight enable pin on the LCD).
Lothar Wa?mann
v6 cover pwm: imx: support output polarity inversion
Quoted 12 lines · Lothar Wa?mann
Hi,
Quoted 7 lines · On Thu, 08 Sep 2016 15:15:57 -0700 Stefan Agner wrote:
Quoted 3 lines · Lothar Wa?mann
This patch series adds support for polarity inversion to the pwm-imx driver. The patches have been tested on i.MX6, i.MX53 and with the
ti-ehrpwm.cdriver.Do you know what prevented this patchset from getting merged?
No idea.
Quoted 9 lines
Quoted 3 lines
We are looking for Polarity support in PWM for too, this is especially useful for backlight control.
Actually the PWM driver may be the wrong place to achieve this. When the backlight driver sets the brightness to 0 to switch the backlight off, it will disable the PWM. This will make the PWM pin go LOW and thus turn the backlight to full brightness rather than off (unless there is an additional GPIO that controls a backlight enable pin on the LCD).
Isn't a properly designed PWM putting a high level on its pin when disabled and configured with inversed polarity ?
If the HW is capable of it, the driver should be fixed.
Signature · 3 lines
Alexandre Belloni, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
v6 cover pwm: imx: support output polarity inversion
Hello,
Quoted 2 lines · Alexandre Belloni
Isn't a properly designed PWM putting a high level on its pin when disabled and configured with inversed polarity ?
it's not well defined. When trying several times over the years to properly define and document it, I didn't manage to agree with Thierry what is the right thing to define.
IMHO it would be sensible to make it explicitly undefined what happens when a PMW is disabled. This would simplify drivers from
pwm_config(mypwm, value, period); if (!value) pwm_disable(mypwm) else pwm_enable(led_dat->pwm);
to
pwm_config(mypwm, value, period);
and let the pwm driver disable it's clock (or whatever) when value is 0 and there are energy saving benefits that don't hurt the expected behaviour of the pin. So the hardware specific stuff is handled in the hardware specific driver and usage in pwm-consumers is simplified. Moreover this also simplifies some pwm drivers because they don't have to catch in software the cases where the hardware differs from the expectation[1].
Looking at drivers/leds/leds-pwm.c it doesn't ensure that each pwm_enable is paired by an pwm_disable (e.g. on .remove). Is this a bug? With my purposed semantics of .config and .disable this would be much easier to fix.
Regarding your question: Yeah, maybe all properly designed PWMs behave like you expect. But reality isn't only about properly designed hardware, so I wouldn't expect all hardware to behave. The inverse property might be software emulated and so on pwm_disable the pin might become 0.
The obvious downside of my suggestion is that this is a change in what most people expect (because it was "safe" to call pwm_enable before), but the resulting code is simpler and cleaner.
Today it's a (maybe small) bug, when a pwm consumer calls pwm_config with value=0 and doesn't disable it afterwards. IMHO that's a bug in the pwm API that pwm_config with value=0 doesn't imply (the wanted effects of) pwm_disable.
Best regards
Uwe
[1] This might even be impossible: Consider a PWM that gets 0 (or high-z) on hw-disable independent of configured duty or inversion. The driver now sees for an inverted pwm: pwm_config(this, 0, 100); pwm_disable(this); The driver cannot know if it should continue to drive the pin at 1, or if the pwm consumer stopped caring about the pwm and disabling the hardware is OK.
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
v6 cover pwm: imx: support output polarity inversion
Hi,
Thanks for that insight Uwe!
Quoted 23 lines · Uwe Kleine-K?nig
Hello,
Quoted 2 lines · Alexandre Belloni
Isn't a properly designed PWM putting a high level on its pin when disabled and configured with inversed polarity ?
it's not well defined. When trying several times over the years to properly define and document it, I didn't manage to agree with Thierry what is the right thing to define.
IMHO it would be sensible to make it explicitly undefined what happens when a PMW is disabled. This would simplify drivers from
pwm_config(mypwm, value, period); if (!value) pwm_disable(mypwm) else pwm_enable(led_dat->pwm);to
pwm_config(mypwm, value, period);
Quoted 7 lines
and let the pwm driver disable it's clock (or whatever) when value is 0 and there are energy saving benefits that don't hurt the expected behaviour of the pin. So the hardware specific stuff is handled in the hardware specific driver and usage in pwm-consumers is simplified. Moreover this also simplifies some pwm drivers because they don't have to catch in software the cases where the hardware differs from the expectation[1].
That sounds like a sane definition to me and what I would have expected from the PWM framework. That the pin is not defined after pwm_disable is totally understandable. It is usually a case which the board designer anyway needs to take care of (e.g. what is the state right after power on? If the designer cares about, he will put a pull-up/down in place).
And it seems also Sascha suggested that:
https://lkml.org/lkml/2013/1/4/139
I did not found where Thierry disagreed to that...?
Quoted 4 lines
Looking at
drivers/leds/leds-pwm.cit doesn't ensure that eachpwm_enableis paired by anpwm_disable(e.g. on .remove). Is this a bug? With my purposed semantics of .config and .disable this would be much easier to fix.
That looks like a bug to me.
Quoted 7 lines
Regarding your question: Yeah, maybe all properly designed PWMs behave like you expect. But reality isn't only about properly designed hardware, so I wouldn't expect all hardware to behave. The inverse property might be software emulated and so on
pwm_disablethe pin might become 0.
Quoted 8 lines
The obvious downside of my suggestion is that this is a change in what most people expect (because it was "safe" to call
pwm_enablebefore), but the resulting code is simpler and cleaner.Today it's a (maybe small) bug, when a pwm consumer calls
pwm_configwith value=0 and doesn't disable it afterwards. IMHO that's a bug in the pwm API thatpwm_configwith value=0 doesn't imply (the wanted effects of)pwm_disable.
I don't quite get what you are saying here. What wanted effects of pwm_disable would you like to move into pwm_config with value=0?
--
Stefan
Quoted 10 lines
Best regards
Uwe[1] This might even be impossible: Consider a PWM that gets 0 (or high-z) on hw-disable independent of configured duty or inversion. The driver now sees for an inverted pwm:
pwm_config(this, 0, 100);pwm_disable(this); The driver cannot know if it should continue to drive the pin at 1, or if the pwm consumer stopped caring about the pwm and disabling the hardware is OK.
v6 cover pwm: imx: support output polarity inversion
Hello Stefan,
Quoted 32 lines · Stefan Agner
Quoted 23 lines · Uwe Kleine-K?nig
Hello,
Quoted 2 lines · Alexandre Belloni
Isn't a properly designed PWM putting a high level on its pin when disabled and configured with inversed polarity ?
it's not well defined. When trying several times over the years to properly define and document it, I didn't manage to agree with Thierry what is the right thing to define.
IMHO it would be sensible to make it explicitly undefined what happens when a PMW is disabled. This would simplify drivers from
pwm_config(mypwm, value, period); if (!value) pwm_disable(mypwm) else pwm_enable(led_dat->pwm);to
pwm_config(mypwm, value, period);Quoted 7 lines
and let the pwm driver disable it's clock (or whatever) when value is 0 and there are energy saving benefits that don't hurt the expected behaviour of the pin. So the hardware specific stuff is handled in the hardware specific driver and usage in pwm-consumers is simplified. Moreover this also simplifies some pwm drivers because they don't have to catch in software the cases where the hardware differs from the expectation[1].
Quoted 8 lines
That sounds like a sane definition to me and what I would have expected from the PWM framework. That the pin is not defined after
pwm_disableis totally understandable. It is usually a case which the board designer anyway needs to take care of (e.g. what is the state right after power on? If the designer cares about, he will put a pull-up/down in place).And it seems also Sascha suggested that:
https://lkml.org/lkml/2013/1/4/139
actually I talked to Sascha in private before ranting :-)
Quoted 1 line
I did not found where Thierry disagreed to that...?
Hmm, I used gmane links before, most of them are dead today. :-|
http://www.spinics.net/lists/linux-leds/msg03237.html is one example.
Quoted 7 lines
Quoted 4 lines
Today it's a (maybe small) bug, when a pwm consumer calls
pwm_configwith value=0 and doesn't disable it afterwards. IMHO that's a bug in the pwm API thatpwm_configwith value=0 doesn't imply (the wanted effects of)pwm_disable.I don't quite get what you are saying here. What wanted effects of
pwm_disablewould you like to move intopwm_configwith value=0?
I want that the pwm driver disables its clock on pwm_config(mypwm, 0, someperiod) such that the consumer doesn't need to call pwm_disable(mypwm) to save power (assuming it's safe to do so, which only the pwm provider knows).
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
v6 cover pwm: imx: support output polarity inversion
Hi Uwe,
Quoted 4 lines · Uwe Kleine-K?nig
I want that the pwm driver disables its clock on
pwm_config(mypwm, 0, someperiod)such that the consumer doesn't need to callpwm_disable(mypwm)to save power (assuming it's safe to do so, which only the pwm provider knows).
I am not sure if this is such a good idea, because there are use cases where you want to keep the PWM driver enabled the whole time but still be able to change the duty cycle to 0 for some time without adding unnecessary delays when changing the duty cycle back to something else.
We have an application where we control fluid valves in a beer dispensing system through PWMs and these valves are pulsed with different PWM duty cycles for a short time. In-between the duty cycle is also 0. For example: Start at 0%, 100ms 90%, 200ms 70%, 300ms 0%, 100ms 90%, and so on..
There it is critical that the change from and to 0 duty cycle is not delayed by disabling and reenabling the clock.
The oscillator (if there is one) should be up and running, only the duty cycle should be 0 for a short time.
This would not be possible anymore with your API change, right?
Regards,
Clemens
v6 cover pwm: imx: support output polarity inversion
Hello Clemens,
Quoted 11 lines · Clemens Gruber
Quoted 4 lines · Uwe Kleine-K?nig
I want that the pwm driver disables its clock on
pwm_config(mypwm, 0, someperiod)such that the consumer doesn't need to callpwm_disable(mypwm)to save power (assuming it's safe to do so, which only the pwm provider knows).I am not sure if this is such a good idea, because there are use cases where you want to keep the PWM driver enabled the whole time but still be able to change the duty cycle to 0 for some time without adding unnecessary delays when changing the duty cycle back to something else.
Quoted 9 lines
We have an application where we control fluid valves in a beer dispensing system through PWMs and these valves are pulsed with different PWM duty cycles for a short time. In-between the duty cycle is also 0. For example: Start at 0%, 100ms 90%, 200ms 70%, 300ms 0%, 100ms 90%, and so on..
There it is critical that the change from and to 0 duty cycle is not delayed by disabling and reenabling the clock.
The oscillator (if there is one) should be up and running, only the duty cycle should be 0 for a short time.
I don't think it is sensible to map this requirement in the pwm api. The trade-off between performance and power saving is common between all types of devices and there are other mechanisms to handle this.
Also only some pwms are affected by this because disabling the clock doesn't introduce a measurable overhead for all of them.
With write(2) there is also no way to define if the hard disk should spin down after the request is completed. And this wouldn't make sense for an ssd.
So yes, there would be no way to prohibit stopping the pwm with the pwm API, but you could implement runtime pm for your device.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
v6 cover pwm: imx: support output polarity inversion
In reply to Lothar WaßmannHi Lothar,
Quoted 12 lines · Lothar Wa?mann
Hi,
Quoted 7 lines · On Thu, 08 Sep 2016 15:15:57 -0700 Stefan Agner wrote:
Quoted 3 lines · Lothar Wa?mann
This patch series adds support for polarity inversion to the pwm-imx driver. The patches have been tested on i.MX6, i.MX53 and with the
ti-ehrpwm.cdriver.Do you know what prevented this patchset from getting merged?
No idea.
Quoted 9 lines
Quoted 3 lines
We are looking for Polarity support in PWM for too, this is especially useful for backlight control.
Actually the PWM driver may be the wrong place to achieve this. When the backlight driver sets the brightness to 0 to switch the backlight off, it will disable the PWM. This will make the PWM pin go LOW and thus turn the backlight to full brightness rather than off (unless there is an additional GPIO that controls a backlight enable pin on the LCD).
I've just realized that I had submitted practically the same change (excluding iMX specifics) and about the same time in October 2014, but my v1 is one and a half hours later than yours preceding v6 :)
Since I've subscribed to the linux-pwm right before sending my changes, I don't have your changes in my mailbox. Would you mind to review my v3 "pwm: support backward compatibility of DTB extending PWM args":
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/303833.html
then incorporate anything you find useful into your series and resend v7? Or just resend the rebased v6 if nothing is found attracting?
In my turn I'll spend time to review the series and test it on iMX.
--
With best wishes,
Vladimir
PATCH v5 3/3 pwm: imx: support output polarity inversion
In reply to Lothar WaßmannThe i.MX pwm unit on i.MX27 and newer SoCs provides a configurable output polarity. This patch adds support to utilize this feature where available.
--- drivers/pwm/pwm-imx.c | 39 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-)
drivers/pwm/pwm-imx.c +36 -3
@@ -179,6 +179,9 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,if (enable) cr |=MX3_PWMCR_EN ; +if (pwm->polarity ==PWM_POLARITY_INVERSED ) + cr |=MX3_PWMCR_POUTC ; +writel (cr, imx->mmio_base +MX3_PWMCR );return 0 ;
@@ -196,6 +199,11 @@ static void imx_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)else val &= ~MX3_PWMCR_EN ; +if (chip->pwms[0 ].polarity ==PWM_POLARITY_INVERSED ) + val |=MX3_PWMCR_POUTC ; +else + val &= ~MX3_PWMCR_POUTC ; +writel (val, imx->mmio_base +MX3_PWMCR ); }
@@ -239,27 +247,49 @@ static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)clk_disable_unprepare (imx->clk_per); } -static struct pwm_ops imx_pwm_ops = { +static int imx_pwm_set_polarity (struct pwm_chip *chip,struct pwm_device *pwm, +enum pwm_polarity polarity) +{ +struct imx_chip *imx =to_imx_chip (chip); + +dev_dbg (imx->chip.dev,"%s: polarity set to %s\n" , __func__, + polarity ==PWM_POLARITY_INVERSED ?"inverted" :"normal" ); + +return 0 ; +} + +static struct pwm_ops imx_pwm_ops_v1 = { .enable = imx_pwm_enable, .disable = imx_pwm_disable, .config = imx_pwm_config, .owner =THIS_MODULE , }; +static struct pwm_ops imx_pwm_ops_v2 = { + .enable = imx_pwm_enable, + .disable = imx_pwm_disable, + .set_polarity = imx_pwm_set_polarity, + .config = imx_pwm_config, + .owner =THIS_MODULE , +}; +struct imx_pwm_data {int (*config)(struct pwm_chip *chip,struct pwm_device *pwm,int duty_ns,int period_ns);void (*set_enable)(struct pwm_chip *chip,bool enable); +struct pwm_ops *pwm_ops; };static struct imx_pwm_data imx_pwm_data_v1 = { .config = imx_pwm_config_v1, .set_enable = imx_pwm_set_enable_v1, + .pwm_ops = &imx_pwm_ops_v1, };static struct imx_pwm_data imx_pwm_data_v2 = { .config = imx_pwm_config_v2, .set_enable = imx_pwm_set_enable_v2, + .pwm_ops = &imx_pwm_ops_v2, };static const struct of_device_id imx_pwm_dt_ids[] = {
@@ -281,6 +311,10 @@ static int imx_pwm_probe(struct platform_device *pdev)if (!of_id)return -ENODEV ; + data = of_id->data; +if (data->pwm_ops->set_polarity) +dev_dbg (&pdev->dev,"PWM supports output inversion\n" ); + imx =devm_kzalloc (&pdev->dev,sizeof (*imx),GFP_KERNEL );if (imx ==NULL )return -ENOMEM ;
@@ -299,7 +333,7 @@ static int imx_pwm_probe(struct platform_device *pdev)return PTR_ERR (imx->clk_ipg); } - imx->chip.ops = &imx_pwm_ops; + imx->chip.ops = data->pwm_ops; imx->chip.dev = &pdev->dev; imx->chip.base =-1 ; imx->chip.npwm =1 ;
@@ -310,7 +344,6 @@ static int imx_pwm_probe(struct platform_device *pdev)if (IS_ERR (imx->mmio_base))return PTR_ERR (imx->mmio_base); - data = of_id->data; imx->config = data->config; imx->set_enable = data->set_enable;
Signature · 1 line
1.7.10.4