| Message ID | 20141009151605.GA8818@ulmo.nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lothar, On 2014-10-10 07:22, Lothar Waßmann wrote: > 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. 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 > > 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 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, 08 Sep 2016 15:15:57 -0700 Stefan Agner wrote: > On 2014-10-10 07:22, Lothar Waßmann wrote: > > 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. > > Do you know what prevented this patchset from getting merged? > No idea. > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/09/2016 at 09:18:57 +0200, Lothar Waßmann wrote : > Hi, > > On Thu, 08 Sep 2016 15:15:57 -0700 Stefan Agner wrote: > > On 2014-10-10 07:22, Lothar Waßmann wrote: > > > 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. > > > > Do you know what prevented this patchset from getting merged? > > > No idea. > > > 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.
Hi Lothar, On 09/09/2016 10:18 AM, Lothar Waßmann wrote: > Hi, > > On Thu, 08 Sep 2016 15:15:57 -0700 Stefan Agner wrote: >> On 2014-10-10 07:22, Lothar Waßmann wrote: >>> 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. >> >> Do you know what prevented this patchset from getting merged? >> > No idea. > >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Mon, Sep 12, 2016 at 02:45:53PM +0200, Alexandre Belloni wrote: > 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.
Hi, Thanks for that insight Uwe! On 2016-09-12 07:04, Uwe Kleine-König wrote: > Hello, > > On Mon, Sep 12, 2016 at 02:45:53PM +0200, Alexandre Belloni wrote: >> 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]. 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...? > 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. That looks like a bug to me. > > 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. 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 > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Stefan, On Mon, Sep 12, 2016 at 09:51:26AM -0700, Stefan Agner wrote: > On 2016-09-12 07:04, Uwe Kleine-König wrote: > > Hello, > > > > On Mon, Sep 12, 2016 at 02:45:53PM +0200, Alexandre Belloni wrote: > >> 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]. > > 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 actually I talked to Sascha in private before ranting :-) > 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. > > 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. > > 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? 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
Hi Uwe, On Mon, Sep 12, 2016 at 10:00:21PM +0200, Uwe Kleine-König wrote: > 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). 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Clemens, On Mon, Sep 12, 2016 at 11:12:54PM +0200, Clemens Gruber wrote: > On Mon, Sep 12, 2016 at 10:00:21PM +0200, Uwe Kleine-König wrote: > > 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). > > 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. 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
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 966497d10c6e..89a5e309b0a3 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -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;