| Message ID | 1535461286-12308-1-git-send-email-claudiu.beznea@microchip.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | extend PWM framework to support PWM modes | expand |
Thierry, On 28/08/2018 at 15:01, Claudiu Beznea wrote: > Hi, > > Please give feedback on these patches which extends the PWM framework in > order to support multiple PWM modes of operations. This series is a rework > of [1] and [2]. This series started with a RFC back on 5 April 2017 "extend PWM framework to support PWM modes". The continuous work starting with v2 of this series on January 12, 2018. Then Claudiu tried to address all comments up to v4 which didn't have any more reviews. He posted a v5 without comments since May 22, 2018. This series is basically a resent of the v5 (as said in the $subject). We would like to know what is preventing this series to be included in the PWM sub-system. Note that if some issue still remain with it, we are ready to help to solve them. Without feedback from you side, we fear that we would miss a merge window again for no obvious reason (DT part is Acked by Rob: patch 5/9). Best regards, Nicolas > The current patch series add the following PWM modes: > - PWM mode normal > - PWM mode complementary > - PWM mode push-pull > > Normal mode - for PWM channels with one output; output waveforms looks like > this: > __ __ __ __ > PWM __| |__| |__| |__| |__ > <--T--> > > Where T is the signal period > > Since PWMs with more than one output per channel could be used as one > output PWM the normal mode is the default mode for all PWMs (if not > specified otherwise). > > Complementary mode - for PWM channels with two outputs; output waveforms > for a PWM channel in complementary mode looks line this: > __ __ __ __ > PWMH1 __| |__| |__| |__| |__ > __ __ __ __ __ > PWML1 |__| |__| |__| |__| > <--T--> > > Where T is the signal period. > > Push-pull mode - for PWM channels with two outputs; output waveforms for a > PWM channel in push-pull mode with normal polarity looks like this: > __ __ > PWMH __| |________| |________ > __ __ > PWML ________| |________| |__ > <--T--> > > If polarity is inversed: > __ ________ ________ > PWMH |__| |__| > ________ ________ __ > PWML |__| |__| > <--T--> > > Where T is the signal period. > > The PWM working modes are per PWM channel registered as PWM's capabilities. > The driver registers itself to PWM core a get_caps() function, in > struct pwm_ops, that will be used by PWM core to retrieve PWM capabilities. > If this function is not registered in driver's probe, a default function > will be used to retrieve PWM capabilities. Currently, the default > capabilities includes only PWM normal mode. > > PWM state has been updated to keep PWM mode. PWM mode could be configured > via sysfs or via DT. pwm_apply_state() will do the preliminary validation > for PWM mode to be applied. > > In sysfs, user could get PWM modes by reading mode file of PWM device: > root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# ls -l > total 0 > -r--r--r-- 1 root root 4096 Oct 9 09:07 capture > lrwxrwxrwx 1 root root 0 Oct 9 09:07 device -> ../../pwmchip0 > -rw-r--r-- 1 root root 4096 Oct 9 08:42 duty_cycle > -rw-r--r-- 1 root root 4096 Oct 9 08:44 enable > --w------- 1 root root 4096 Oct 9 09:07 export > -rw-r--r-- 1 root root 4096 Oct 9 08:43 mode > -r--r--r-- 1 root root 4096 Oct 9 09:07 npwm > -rw-r--r-- 1 root root 4096 Oct 9 08:42 period > -rw-r--r-- 1 root root 4096 Oct 9 08:44 polarity > drwxr-xr-x 2 root root 0 Oct 9 09:07 power > lrwxrwxrwx 1 root root 0 Oct 9 09:07 subsystem -> ../../../../../../../../class/pwm > -rw-r--r-- 1 root root 4096 Oct 9 08:42 uevent > --w------- 1 root root 4096 Oct 9 09:07 unexport > root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# cat mode > normal complementary [push-pull] > > The mode enclosed in bracket is the currently active mode. > > The mode could be set, via sysfs, by writing to mode file one of the modes > displayed at read: > root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# echo normal > mode > root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# cat mode > [normal] complementary push-pull > > The PWM push-pull mode could be usefull in applications like half bridge > converters. > > This series also add PWM modes support for Atmel/Microchip SoCs. > > Thank you, > Claudiu Beznea > > [1] https://www.spinics.net/lists/arm-kernel/msg580275.html > [2] https://lkml.org/lkml/2018/1/12/359 > > Changes in v5: > - solved kbuild errors by removing dummy functions for case where > CONFIG_PWM is not defined; adopted this approach since the removed > function are used only when CONFIG_PWM is defined (in PWM core > and few drivers from drivers/pwm/ directory) > > Changes in v4: > - removed changes related to pwm_config() as per maintainer proposals > - added pwm_mode_get_valid() to retrieve a valid PWM mode fror PWM device > instead of using BIT(ffs(caps.mode) - 1) and changed drivers to use > pwm_mode_get_valid() instead of pwm_get_caps() + BIT(ffs(caps.mode) - 1) > (patches 2, 3, 4 from this series) > - renamed PWM_MODE() macro in PWMC_MODE() to avoid conflicts with > pwm-sun4i.c driver ('C' stands for capability) > - removed pwm_caps_valid() function > - renamed PWM_DTMODE_COMPLEMENTARY and PWM_DTMODE_PUSH_PULL macros in > PWM_MODE_COMPLEMENTARY and PWM_MODE_PUSH_PULL > > Changes in v3: > - removed changes related to only one of_xlate function for all PWM drivers > - switch to PWM capabilities per PWM channel nor per PWM chip > - squash documentation and bindings patches as requeted by reviewer > - introduced PWM_MODE(name) macro and used a bit enum for pwm modes > - related to DT bindings, used flags cell also for PWM modes > - updated of_xlate specific functions with "state->mode = mode;" > instructions to avoid pwm_apply_state() failures > - use available modes for PWM channel in pwm_config() by first calling > pwm_get_caps() to get caps.modes > - use loops through available modes in mode_store()/mode_show() and also in > of_pwm_xlate_with_flags() instead of "if else" instructions; in this way, > the addition of a new mode is independent of this code sections > - use DTLI=1, DTHI=0 register settings to obtain push-pull mode waveforms > for Atmel/Microchip PWM controller. > > Changes in v2: > - remove of_xlate and of_pwm_n_cells and use generic functions to pharse DT > inputs; this is done in patches 1, 2, 3, 4, 5, 6, 7 of this series; this will > make easy the addition of PWM mode support from DT > - add PWM mode normal > - register PWM modes as capabilities of PWM chips at driver probe and, in case > driver doesn't provide these capabilities use default ones > - change the way PWM mode is pharsed via DT by using a new input for pwms > binding property > > > Claudiu Beznea (9): > pwm: extend PWM framework with PWM modes > pwm: clps711x: populate PWM mode in of_xlate function > pwm: cros-ec: populate PWM mode in of_xlate function > pwm: pxa: populate PWM mode in of_xlate function > pwm: add PWM modes > pwm: atmel: add pwm capabilities > pwm: add push-pull mode support > pwm: add documentation for pwm push-pull mode > pwm: atmel: add push-pull mode support > > Documentation/devicetree/bindings/pwm/pwm.txt | 11 ++- > Documentation/pwm.txt | 42 ++++++++- > drivers/pwm/core.c | 125 +++++++++++++++++++++++++- > drivers/pwm/pwm-atmel.c | 118 +++++++++++++++++------- > drivers/pwm/pwm-clps711x.c | 10 ++- > drivers/pwm/pwm-cros-ec.c | 1 + > drivers/pwm/pwm-pxa.c | 1 + > drivers/pwm/sysfs.c | 61 +++++++++++++ > include/dt-bindings/pwm/pwm.h | 2 + > include/linux/pwm.h | 64 +++++++++++++ > 10 files changed, 395 insertions(+), 40 deletions(-) >
On 14/09/2018 at 18:20, Nicolas Ferre wrote: > Thierry, > > On 28/08/2018 at 15:01, Claudiu Beznea wrote: >> Hi, >> >> Please give feedback on these patches which extends the PWM framework in >> order to support multiple PWM modes of operations. This series is a rework >> of [1] and [2]. > > This series started with a RFC back on 5 April 2017 "extend PWM > framework to support PWM modes". The continuous work starting with v2 of > this series on January 12, 2018. > > Then Claudiu tried to address all comments up to v4 which didn't have > any more reviews. He posted a v5 without comments since May 22, 2018. > This series is basically a resent of the v5 (as said in the $subject). > > We would like to know what is preventing this series to be included in > the PWM sub-system. Note that if some issue still remain with it, we are > ready to help to solve them. > > Without feedback from you side, we fear that we would miss a merge > window again for no obvious reason (DT part is Acked by Rob: patch 5/9). 3 weeks no news about my previous ping... ping again! > Best regards, > Nicolas > > >> The current patch series add the following PWM modes: >> - PWM mode normal >> - PWM mode complementary >> - PWM mode push-pull >> >> Normal mode - for PWM channels with one output; output waveforms looks like >> this: >> __ __ __ __ >> PWM __| |__| |__| |__| |__ >> <--T--> >> >> Where T is the signal period >> >> Since PWMs with more than one output per channel could be used as one >> output PWM the normal mode is the default mode for all PWMs (if not >> specified otherwise). >> >> Complementary mode - for PWM channels with two outputs; output waveforms >> for a PWM channel in complementary mode looks line this: >> __ __ __ __ >> PWMH1 __| |__| |__| |__| |__ >> __ __ __ __ __ >> PWML1 |__| |__| |__| |__| >> <--T--> >> >> Where T is the signal period. >> >> Push-pull mode - for PWM channels with two outputs; output waveforms for a >> PWM channel in push-pull mode with normal polarity looks like this: >> __ __ >> PWMH __| |________| |________ >> __ __ >> PWML ________| |________| |__ >> <--T--> >> >> If polarity is inversed: >> __ ________ ________ >> PWMH |__| |__| >> ________ ________ __ >> PWML |__| |__| >> <--T--> >> >> Where T is the signal period. >> >> The PWM working modes are per PWM channel registered as PWM's capabilities. >> The driver registers itself to PWM core a get_caps() function, in >> struct pwm_ops, that will be used by PWM core to retrieve PWM capabilities. >> If this function is not registered in driver's probe, a default function >> will be used to retrieve PWM capabilities. Currently, the default >> capabilities includes only PWM normal mode. >> >> PWM state has been updated to keep PWM mode. PWM mode could be configured >> via sysfs or via DT. pwm_apply_state() will do the preliminary validation >> for PWM mode to be applied. >> >> In sysfs, user could get PWM modes by reading mode file of PWM device: >> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# ls -l >> total 0 >> -r--r--r-- 1 root root 4096 Oct 9 09:07 capture >> lrwxrwxrwx 1 root root 0 Oct 9 09:07 device -> ../../pwmchip0 >> -rw-r--r-- 1 root root 4096 Oct 9 08:42 duty_cycle >> -rw-r--r-- 1 root root 4096 Oct 9 08:44 enable >> --w------- 1 root root 4096 Oct 9 09:07 export >> -rw-r--r-- 1 root root 4096 Oct 9 08:43 mode >> -r--r--r-- 1 root root 4096 Oct 9 09:07 npwm >> -rw-r--r-- 1 root root 4096 Oct 9 08:42 period >> -rw-r--r-- 1 root root 4096 Oct 9 08:44 polarity >> drwxr-xr-x 2 root root 0 Oct 9 09:07 power >> lrwxrwxrwx 1 root root 0 Oct 9 09:07 subsystem -> ../../../../../../../../class/pwm >> -rw-r--r-- 1 root root 4096 Oct 9 08:42 uevent >> --w------- 1 root root 4096 Oct 9 09:07 unexport >> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# cat mode >> normal complementary [push-pull] >> >> The mode enclosed in bracket is the currently active mode. >> >> The mode could be set, via sysfs, by writing to mode file one of the modes >> displayed at read: >> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# echo normal > mode >> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# cat mode >> [normal] complementary push-pull >> >> The PWM push-pull mode could be usefull in applications like half bridge >> converters. >> >> This series also add PWM modes support for Atmel/Microchip SoCs. >> >> Thank you, >> Claudiu Beznea >> >> [1] https://www.spinics.net/lists/arm-kernel/msg580275.html >> [2] https://lkml.org/lkml/2018/1/12/359 >> >> Changes in v5: >> - solved kbuild errors by removing dummy functions for case where >> CONFIG_PWM is not defined; adopted this approach since the removed >> function are used only when CONFIG_PWM is defined (in PWM core >> and few drivers from drivers/pwm/ directory) >> >> Changes in v4: >> - removed changes related to pwm_config() as per maintainer proposals >> - added pwm_mode_get_valid() to retrieve a valid PWM mode fror PWM device >> instead of using BIT(ffs(caps.mode) - 1) and changed drivers to use >> pwm_mode_get_valid() instead of pwm_get_caps() + BIT(ffs(caps.mode) - 1) >> (patches 2, 3, 4 from this series) >> - renamed PWM_MODE() macro in PWMC_MODE() to avoid conflicts with >> pwm-sun4i.c driver ('C' stands for capability) >> - removed pwm_caps_valid() function >> - renamed PWM_DTMODE_COMPLEMENTARY and PWM_DTMODE_PUSH_PULL macros in >> PWM_MODE_COMPLEMENTARY and PWM_MODE_PUSH_PULL >> >> Changes in v3: >> - removed changes related to only one of_xlate function for all PWM drivers >> - switch to PWM capabilities per PWM channel nor per PWM chip >> - squash documentation and bindings patches as requeted by reviewer >> - introduced PWM_MODE(name) macro and used a bit enum for pwm modes >> - related to DT bindings, used flags cell also for PWM modes >> - updated of_xlate specific functions with "state->mode = mode;" >> instructions to avoid pwm_apply_state() failures >> - use available modes for PWM channel in pwm_config() by first calling >> pwm_get_caps() to get caps.modes >> - use loops through available modes in mode_store()/mode_show() and also in >> of_pwm_xlate_with_flags() instead of "if else" instructions; in this way, >> the addition of a new mode is independent of this code sections >> - use DTLI=1, DTHI=0 register settings to obtain push-pull mode waveforms >> for Atmel/Microchip PWM controller. >> >> Changes in v2: >> - remove of_xlate and of_pwm_n_cells and use generic functions to pharse DT >> inputs; this is done in patches 1, 2, 3, 4, 5, 6, 7 of this series; this will >> make easy the addition of PWM mode support from DT >> - add PWM mode normal >> - register PWM modes as capabilities of PWM chips at driver probe and, in case >> driver doesn't provide these capabilities use default ones >> - change the way PWM mode is pharsed via DT by using a new input for pwms >> binding property >> >> >> Claudiu Beznea (9): >> pwm: extend PWM framework with PWM modes >> pwm: clps711x: populate PWM mode in of_xlate function >> pwm: cros-ec: populate PWM mode in of_xlate function >> pwm: pxa: populate PWM mode in of_xlate function >> pwm: add PWM modes >> pwm: atmel: add pwm capabilities >> pwm: add push-pull mode support >> pwm: add documentation for pwm push-pull mode >> pwm: atmel: add push-pull mode support >> >> Documentation/devicetree/bindings/pwm/pwm.txt | 11 ++- >> Documentation/pwm.txt | 42 ++++++++- >> drivers/pwm/core.c | 125 +++++++++++++++++++++++++- >> drivers/pwm/pwm-atmel.c | 118 +++++++++++++++++------- >> drivers/pwm/pwm-clps711x.c | 10 ++- >> drivers/pwm/pwm-cros-ec.c | 1 + >> drivers/pwm/pwm-pxa.c | 1 + >> drivers/pwm/sysfs.c | 61 +++++++++++++ >> include/dt-bindings/pwm/pwm.h | 2 + >> include/linux/pwm.h | 64 +++++++++++++ >> 10 files changed, 395 insertions(+), 40 deletions(-) >> > >
On Fri, Sep 14, 2018 at 06:20:48PM +0200, Nicolas Ferre wrote: > Thierry, > > On 28/08/2018 at 15:01, Claudiu Beznea wrote: > > Hi, > > > > Please give feedback on these patches which extends the PWM framework in > > order to support multiple PWM modes of operations. This series is a rework > > of [1] and [2]. > > This series started with a RFC back on 5 April 2017 "extend PWM framework to > support PWM modes". The continuous work starting with v2 of this series on > January 12, 2018. > > Then Claudiu tried to address all comments up to v4 which didn't have any > more reviews. He posted a v5 without comments since May 22, 2018. This > series is basically a resent of the v5 (as said in the $subject). > > We would like to know what is preventing this series to be included in the > PWM sub-system. Note that if some issue still remain with it, we are ready > to help to solve them. > > Without feedback from you side, we fear that we would miss a merge window > again for no obvious reason (DT part is Acked by Rob: patch 5/9). First off, apologies for not getting around to this earlier. I think this series is mostly fine, but I still have doubts about the DT aspects of this. In particular, Rob raised a concern about this here: https://lkml.org/lkml/2018/1/22/655 and it seems like that particular question was never fully resolved as the discussion veered off that particular topic. I know that Rob acked the DT parts of this, but I suspect that this might have been glossed over. To restate the concern: these extended modes have special uses and none of the users in the kernel, other than sysfs, can use anything other than the normal mode. They may work fine with other modes, but only if they ignore the extras that come with them. Therefore I think it's safe to say that anyone who would want to use these modes would want to explicitly say so. For example the sysfs interface already does that by changing the mode only after the "mode" attribute is written. Any users for special use-cases would want to do the same thing, that is, drive a PWM in a specific mode, on purpose. You wouldn't have a "generic" user such as pwm-backlight or leds-pwm request anything other than the normal mode. So my question is, do we really need to represent these modes in DT? The series currently doesn't contain any patches that add users of these new modes. Are such patches available somewhere, or is the only user of this supposed to be sysfs? I'm hesitant to move forward with this as-is without seeing how it will be used. The PWM specifier flags are somewhat abused by adding modes to them. I think this hasn't been completely thought through, because the only reason to specify a mode is to actually set that mode. But then the DT ABI allows a bitmask of modes to be requested via DT. I know that only the first of those modes will end up being used, but then why even allow it in the first place? And again, even if we allow the mode to be specified in DT, how do the consumer drivers know that the correct mode was being set in DT. Let's say we have a consumer that requires the PWM to be driven in complementary mode. Should it rely on the DT to contain the correct specification for the mode? And if it knows that it needs complementary mode, why not just set that mode itself? That way there's no margin for error. Thierry
On 16.10.2018 15:03, Thierry Reding wrote: > On Fri, Sep 14, 2018 at 06:20:48PM +0200, Nicolas Ferre wrote: >> Thierry, >> >> On 28/08/2018 at 15:01, Claudiu Beznea wrote: >>> Hi, >>> >>> Please give feedback on these patches which extends the PWM framework in >>> order to support multiple PWM modes of operations. This series is a rework >>> of [1] and [2]. >> >> This series started with a RFC back on 5 April 2017 "extend PWM framework to >> support PWM modes". The continuous work starting with v2 of this series on >> January 12, 2018. >> >> Then Claudiu tried to address all comments up to v4 which didn't have any >> more reviews. He posted a v5 without comments since May 22, 2018. This >> series is basically a resent of the v5 (as said in the $subject). >> >> We would like to know what is preventing this series to be included in the >> PWM sub-system. Note that if some issue still remain with it, we are ready >> to help to solve them. >> >> Without feedback from you side, we fear that we would miss a merge window >> again for no obvious reason (DT part is Acked by Rob: patch 5/9). > > First off, apologies for not getting around to this earlier. > > I think this series is mostly fine, but I still have doubts about the DT > aspects of this. In particular, Rob raised a concern about this here: > > https://lkml.org/lkml/2018/1/22/655 > > and it seems like that particular question was never fully resolved as > the discussion veered off that particular topic. 1/ If you are talking about this sentence: "Yes, but you have to make "normal" be no bit set to be compatible with everything already out there." The current implementation consider that if no mode is provided then, the old approach is considered, meaning the normal mode will be used by every PWM in-kernel clients. In of_pwm_xlate_with_flags() the pmw->args.mode is initialized with what pwm_mode_get_valid() returns. In case of controllers which does not implement something special for PWM modes the PWM normal mode will be returned (pwmchip_get_default_caps() function has to be called in the end). Otherwise the pwm->args.mode will be populated with what user provided as input from DT, if what was provided from DT is valid for PWM channel. Please see that pwm_mode_valid() is used to validate user input, otherwise PWM normal mode will be used. + pwm->args.mode = pwm_mode_get_valid(pc, pwm); - if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED) - pwm->args.polarity = PWM_POLARITY_INVERSED; + if (args->args_count > 2) { + if (args->args[2] & PWM_POLARITY_INVERTED) + pwm->args.polarity = PWM_POLARITY_INVERSED; + + for (modebit = PWMC_MODE_COMPLEMENTARY_BIT; + modebit < PWMC_MODE_CNT; modebit++) { + unsigned long mode = BIT(modebit); + + if ((args->args[2] & mode) && + pwm_mode_valid(pwm, mode)) { + pwm->args.mode = mode; + break; + } + } + } 2/ If you are talking about this sentence: "Thinking about this some more, shouldn't the new modes just be implied? A client is going to require one of these modes or it won't work right." As explained at point 1, if there is no mode requested from DT the default mode for channel will be used, which, in case of PWM controller which are not implementing the new modes, will be PWM normal mode. 3/ If you are talking about: "Also complementary mode could be accomplished with a single pwm output and a board level inverter, right? How would that be handled when the PWM driver doesn't support that mode?" This complicates the things and I think it requires extra device tree bindings to describe extra per-pwm channel capabilities. For the moment, with this series I've stopped to only have the capabilities registered from driver. My understanding is that this could be accomplished with extra device tree bindings in PWM controller to describe PWM channels capabilities. If you also want me to look into this direction please let me know. And also, suggestions would be appreciated. I know that Rob acked > the DT parts of this, but I suspect that this might have been glossed > over. If this is about point 3 I've emphasize above I would like to have some inputs from your side, if you agree with a behavior like having extra DT bindings. The intention wasn't to left this over but to have a better understanding of the subject. I'm thinking if it is ok to have modules outside of the SoC that model a behavior that could not be controlled from software (it could be only hardware) but to behave in a single way. Take for instance this scenario: - new DT bindings are implemented to specify this behavior per channel - hardware modules are soldered outside of a PWM channel with one output - the new DT bindings are not specified for the soldered PWM channel - user enables this channel, it will have only normal mode available for it to configure (because the new DT bindings were not provided) - but the real output the user will see would be in complementary or even push-pull mode. > > To restate the concern: these extended modes have special uses and none > of the users in the kernel, other than sysfs, can use anything other > than the normal mode. They may work fine with other modes, but only if > they ignore the extras that come with them. Therefore I think it's safe > to say that anyone who would want to use these modes would want to > explicitly say so. For example the sysfs interface already does that by > changing the mode only after the "mode" attribute is written. Any users > for special use-cases would want to do the same thing, that is, drive a > PWM in a specific mode, on purpose. You wouldn't have a "generic" user > such as pwm-backlight or leds-pwm request anything other than the normal > mode. > > So my question is, do we really need to represent these modes in DT? The > series currently doesn't contain any patches that add users of these new > modes. Are such patches available somewhere, or is the only user of this > supposed to be sysfs? For the moment, no, there is no in-kernel user for this, only sysfs. I had in mind to adapt the use of these new mode for PWM regulator for scenarios described in [1] page 2126. Anyway, since these patches doesn't introduce any user other that sysfs it will not disturbed me to drop the changes. By the time I or someone else will do some other changes that requires this, the DT part should also be introduced. [1] http://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf > > I'm hesitant to move forward with this as-is without seeing how it will > be used. For the moment only sysfs is supposed to use this. The PWM specifier flags are somewhat abused by adding modes to > them. I see. I think this hasn't been completely thought through, because the > only reason to specify a mode is to actually set that mode. Maybe it wasn't clear understand, with the current implementation if no mode will be specified the default mode will be used. There is no impose to specify a mode. But then the > DT ABI allows a bitmask of modes to be requested via DT. I know that > only the first of those modes will end up being used, but then why even > allow it in the first place? I had in mind that I will change the PWM regulator driver to work in scenarios like the one specified in the link above. > > And again, even if we allow the mode to be specified in DT, how do the > consumer drivers know that the correct mode was being set in DT. PWM user will call at some point devm_pwm_get() which, in turn, will call of_pwm_get() which in turn will initialize PWM args with inputs from DT. After that PWM user will, at some point, apply a PWM state; if it is initialized with PWM args initialized when devm_pwm_get() was called then pwm_apply_state() would fail if no good mode is provided as input via DT. Same thing happens if a bad period is provided via DT. Let's > say we have a consumer that requires the PWM to be driven in > complementary mode. Should it rely on the DT to contain the correct > specification for the mode? And if it knows that it needs complementary > mode, why not just set that mode itself? I'm thinking it's the same way as is with PWM period which could also be provided from DT. In the end a bad period value could be provided from device tree. E.g. Atmel PWM controller could generate PWM signals who's periods could not be higher than ~0.6 seconds. If a bad value is provided the pwm_apply_state() will fail. Thank you, Claudiu Beznea That way there's no margin for > error. > > Thierry >
On Wed, Oct 17, 2018 at 12:41:53PM +0000, Claudiu.Beznea@microchip.com wrote: > > > On 16.10.2018 15:03, Thierry Reding wrote: > > On Fri, Sep 14, 2018 at 06:20:48PM +0200, Nicolas Ferre wrote: > >> Thierry, > >> > >> On 28/08/2018 at 15:01, Claudiu Beznea wrote: > >>> Hi, > >>> > >>> Please give feedback on these patches which extends the PWM framework in > >>> order to support multiple PWM modes of operations. This series is a rework > >>> of [1] and [2]. > >> > >> This series started with a RFC back on 5 April 2017 "extend PWM framework to > >> support PWM modes". The continuous work starting with v2 of this series on > >> January 12, 2018. > >> > >> Then Claudiu tried to address all comments up to v4 which didn't have any > >> more reviews. He posted a v5 without comments since May 22, 2018. This > >> series is basically a resent of the v5 (as said in the $subject). > >> > >> We would like to know what is preventing this series to be included in the > >> PWM sub-system. Note that if some issue still remain with it, we are ready > >> to help to solve them. > >> > >> Without feedback from you side, we fear that we would miss a merge window > >> again for no obvious reason (DT part is Acked by Rob: patch 5/9). > > > > First off, apologies for not getting around to this earlier. > > > > I think this series is mostly fine, but I still have doubts about the DT > > aspects of this. In particular, Rob raised a concern about this here: > > > > https://lkml.org/lkml/2018/1/22/655 > > > > and it seems like that particular question was never fully resolved as > > the discussion veered off that particular topic. > > 1/ If you are talking about this sentence: > "Yes, but you have to make "normal" be no bit set to be compatible with > everything already out there." > > The current implementation consider that if no mode is provided then, the > old approach is considered, meaning the normal mode will be used by every > PWM in-kernel clients. > > In of_pwm_xlate_with_flags() the pmw->args.mode is initialized with what > pwm_mode_get_valid() returns. In case of controllers which does not > implement something special for PWM modes the PWM normal mode will be > returned (pwmchip_get_default_caps() function has to be called in the end). > Otherwise the pwm->args.mode will be populated with what user provided as > input from DT, if what was provided from DT is valid for PWM channel. > Please see that pwm_mode_valid() is used to validate user input, otherwise > PWM normal mode will be used. No, that part looks fine. > > + pwm->args.mode = pwm_mode_get_valid(pc, pwm); > > - if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED) > - pwm->args.polarity = PWM_POLARITY_INVERSED; > + if (args->args_count > 2) { > + if (args->args[2] & PWM_POLARITY_INVERTED) > + pwm->args.polarity = PWM_POLARITY_INVERSED; > + > + for (modebit = PWMC_MODE_COMPLEMENTARY_BIT; > + modebit < PWMC_MODE_CNT; modebit++) { > + unsigned long mode = BIT(modebit); > + > + if ((args->args[2] & mode) && > + pwm_mode_valid(pwm, mode)) { > + pwm->args.mode = mode; > + break; > + } > + } > + } > > > 2/ If you are talking about this sentence: > "Thinking about this some more, shouldn't the new modes just be > implied? A client is going to require one of these modes or it won't > work right." > > As explained at point 1, if there is no mode requested from DT the default > mode for channel will be used, which, in case of PWM controller which are > not implementing the new modes, will be PWM normal mode. I don't think that's an issue. I think what Rob was referring to and which mirrors my concern is that these modes are a feature that doesn't extend to typical use-cases. So for all existing use-cases (like LED or backlight) we always assume a PWM running in normal mode. Now, if you write a driver for some particular piece of hardware that needs a mode that is not the normal mode, the question is: wouldn't that driver know that it wants exactly push-pull or complementary mode? Wouldn't it have to explicitly check that the PWM supports it and select it (i.e. in the driver code)? Say you have a driver that requires push-pull mode. It doesn't really make sense to require the mode to be encoded in DT, because the driver will only work with one specific mode anyway. So might as well require it and have the driver check for support and fail if the PWM is not compatible. This would likely never happen, because hardware engineers couldn't have validated the design in that case, but there's no reason for the mode to be specified in DT because it is fixed by the very use- case anyway. Also, leaving it out of DT simplifies things. If you allow the mode to be specified in DT you could end up with a situation where the driver required push-pull mode, but the DT specifies complementary mode. What do you do in such a situation? Warn about it and just go with push-pull anyway? Then why give the users a way of screwing things up in the first place? > 3/ If you are talking about: > "Also complementary mode could be accomplished with a single pwm output > and a board level inverter, right? How would that be handled when the > PWM driver doesn't support that mode?" > This complicates the things and I think it requires extra device tree > bindings to describe extra per-pwm channel capabilities. For the moment, > with this series I've stopped to only have the capabilities registered from > driver. My understanding is that this could be accomplished with extra > device tree bindings in PWM controller to describe PWM channels > capabilities. If you also want me to look into this direction please let me > know. And also, suggestions would be appreciated. I think this is very interesting, but I don't think this needs to be done as part of this series. > I know that Rob acked > > the DT parts of this, but I suspect that this might have been glossed > > over. > > If this is about point 3 I've emphasize above I would like to have some > inputs from your side, if you agree with a behavior like having extra DT > bindings. The intention wasn't to left this over but to have a better > understanding of the subject. I'm thinking if it is ok to have modules > outside of the SoC that model a behavior that could not be controlled from > software (it could be only hardware) but to behave in a single way. Take > for instance this scenario: > > - new DT bindings are implemented to specify this behavior per channel > - hardware modules are soldered outside of a PWM channel with one output > - the new DT bindings are not specified for the soldered PWM channel > - user enables this channel, it will have only normal mode available for it > to configure (because the new DT bindings were not provided) > - but the real output the user will see would be in complementary or even > push-pull mode. I think we could possible model this as a "logical" PWM in DT. We could for example have something like this: / { ... pwms { pwm@0 { compatible = "pwm-fixed"; /* or whatever */ pwms = <&pwm 0 40000>; /* input PWM */ mode = <PWM_MODE_COMPLEMENTARY>; }; ... }; ... }; The above would model a logical PWM that is driven by the specified PWM in normal mode but which is effectively complementary because of some additional circuitry on the board. > > To restate the concern: these extended modes have special uses and none > > of the users in the kernel, other than sysfs, can use anything other > > than the normal mode. They may work fine with other modes, but only if > > they ignore the extras that come with them. Therefore I think it's safe > > to say that anyone who would want to use these modes would want to > > explicitly say so. For example the sysfs interface already does that by > > changing the mode only after the "mode" attribute is written. Any users > > for special use-cases would want to do the same thing, that is, drive a > > PWM in a specific mode, on purpose. You wouldn't have a "generic" user > > such as pwm-backlight or leds-pwm request anything other than the normal > > mode. > > > > So my question is, do we really need to represent these modes in DT? The > > series currently doesn't contain any patches that add users of these new > > modes. Are such patches available somewhere, or is the only user of this > > supposed to be sysfs? > > For the moment, no, there is no in-kernel user for this, only sysfs. I had > in mind to adapt the use of these new mode for PWM regulator for scenarios > described in [1] page 2126. Anyway, since these patches doesn't introduce > any user other that sysfs it will not disturbed me to drop the changes. By > the time I or someone else will do some other changes that requires this, > the DT part should also be introduced. > > [1] http://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf Yes, I'd like that. The half-bridge converter certainly sounds like something that may be able to use the DT bindings that you specified, but I'd be less concerned about these changes if we had actual users. > > I'm hesitant to move forward with this as-is without seeing how it will > > be used. > > For the moment only sysfs is supposed to use this. > > The PWM specifier flags are somewhat abused by adding modes to > > them. > > I see. > > I think this hasn't been completely thought through, because the > > only reason to specify a mode is to actually set that mode. > > Maybe it wasn't clear understand, with the current implementation if no > mode will be specified the default mode will be used. There is no impose to > specify a mode. > > But then the > > DT ABI allows a bitmask of modes to be requested via DT. I know that > > only the first of those modes will end up being used, but then why even > > allow it in the first place? > > I had in mind that I will change the PWM regulator driver to work in > scenarios like the one specified in the link above. Yeah, that sounds like it would be reasonable from a quick look. However, what I don't quite understand yet is why the mode in the PWM specifier would need to be a bitmask. Take for example the pwm-regulator case for a half-bridge converter. If your board uses such a setup, then you absolutely must drive the PWM in push-pull mode, otherwise the converter will not work, right? So you want exactly one mode to be applied. Then why complicate matters by allowing the mode to be a bitmask? We could just as easily reserve, say, 8 bits (24-31) for the mode, which could then be identical to enum pwm_mode. > > And again, even if we allow the mode to be specified in DT, how do the > > consumer drivers know that the correct mode was being set in DT. > > PWM user will call at some point devm_pwm_get() which, in turn, will call > of_pwm_get() which in turn will initialize PWM args with inputs from DT. > After that PWM user will, at some point, apply a PWM state; if it is > initialized with PWM args initialized when devm_pwm_get() was called then > pwm_apply_state() would fail if no good mode is provided as input via DT. > > Same thing happens if a bad period is provided via DT. But that only checks that the DT specified a supported mode, it doesn't mean that it's the correct one. For cases like pwm-regulator this may be fine because the driver ultimately doesn't care about the exact mode. If you have a driver that only works with a specific mode, however, it can be problematic. > Let's > > say we have a consumer that requires the PWM to be driven in > > complementary mode. Should it rely on the DT to contain the correct > > specification for the mode? And if it knows that it needs complementary > > mode, why not just set that mode itself? > > I'm thinking it's the same way as is with PWM period which could also be > provided from DT. In the end a bad period value could be provided from > device tree. E.g. Atmel PWM controller could generate PWM signals who's > periods could not be higher than ~0.6 seconds. If a bad value is provided > the pwm_apply_state() will fail. I understand that. And it's good to validate these things in the driver. However, the PWM driver can only validate for the PWM that it is driving. It doesn't know if the consumer has any special requirements regarding the mode. So if the PWM supports push-pull mode and the DT contains PWM_MODE_PUSH_PULL, then everything is fine as far as the PWM driver is concerned. However, if the consumer driver strictly requires complementary mode, there's nothing the PWM driver can do about it. So we either need the consumer to be able to query the mode if it has any specific needs and refuse to use a PWM that has the wrong mode in the specifier, or the consumer needs to explicitly set a mode, in which case there's no need to have it in DT and the PWM driver needs to reject it if the PWM doesn't support it. Thierry
On 18.10.2018 19:00, Thierry Reding wrote: > On Wed, Oct 17, 2018 at 12:41:53PM +0000, Claudiu.Beznea@microchip.com wrote: >> >> >> On 16.10.2018 15:03, Thierry Reding wrote: >>> On Fri, Sep 14, 2018 at 06:20:48PM +0200, Nicolas Ferre wrote: >>>> Thierry, >>>> >>>> On 28/08/2018 at 15:01, Claudiu Beznea wrote: >>>>> Hi, >>>>> >>>>> Please give feedback on these patches which extends the PWM framework in >>>>> order to support multiple PWM modes of operations. This series is a rework >>>>> of [1] and [2]. >>>> >>>> This series started with a RFC back on 5 April 2017 "extend PWM framework to >>>> support PWM modes". The continuous work starting with v2 of this series on >>>> January 12, 2018. >>>> >>>> Then Claudiu tried to address all comments up to v4 which didn't have any >>>> more reviews. He posted a v5 without comments since May 22, 2018. This >>>> series is basically a resent of the v5 (as said in the $subject). >>>> >>>> We would like to know what is preventing this series to be included in the >>>> PWM sub-system. Note that if some issue still remain with it, we are ready >>>> to help to solve them. >>>> >>>> Without feedback from you side, we fear that we would miss a merge window >>>> again for no obvious reason (DT part is Acked by Rob: patch 5/9). >>> >>> First off, apologies for not getting around to this earlier. >>> >>> I think this series is mostly fine, but I still have doubts about the DT >>> aspects of this. In particular, Rob raised a concern about this here: >>> >>> https://lkml.org/lkml/2018/1/22/655 >>> >>> and it seems like that particular question was never fully resolved as >>> the discussion veered off that particular topic. >> >> 1/ If you are talking about this sentence: >> "Yes, but you have to make "normal" be no bit set to be compatible with >> everything already out there." >> >> The current implementation consider that if no mode is provided then, the >> old approach is considered, meaning the normal mode will be used by every >> PWM in-kernel clients. >> >> In of_pwm_xlate_with_flags() the pmw->args.mode is initialized with what >> pwm_mode_get_valid() returns. In case of controllers which does not >> implement something special for PWM modes the PWM normal mode will be >> returned (pwmchip_get_default_caps() function has to be called in the end). >> Otherwise the pwm->args.mode will be populated with what user provided as >> input from DT, if what was provided from DT is valid for PWM channel. >> Please see that pwm_mode_valid() is used to validate user input, otherwise >> PWM normal mode will be used. > > No, that part looks fine. > >> >> + pwm->args.mode = pwm_mode_get_valid(pc, pwm); >> >> - if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED) >> - pwm->args.polarity = PWM_POLARITY_INVERSED; >> + if (args->args_count > 2) { >> + if (args->args[2] & PWM_POLARITY_INVERTED) >> + pwm->args.polarity = PWM_POLARITY_INVERSED; >> + >> + for (modebit = PWMC_MODE_COMPLEMENTARY_BIT; >> + modebit < PWMC_MODE_CNT; modebit++) { >> + unsigned long mode = BIT(modebit); >> + >> + if ((args->args[2] & mode) && >> + pwm_mode_valid(pwm, mode)) { >> + pwm->args.mode = mode; >> + break; >> + } >> + } >> + } >> >> >> 2/ If you are talking about this sentence: >> "Thinking about this some more, shouldn't the new modes just be >> implied? A client is going to require one of these modes or it won't >> work right." >> >> As explained at point 1, if there is no mode requested from DT the default >> mode for channel will be used, which, in case of PWM controller which are >> not implementing the new modes, will be PWM normal mode. > > I don't think that's an issue. I think what Rob was referring to and > which mirrors my concern is that these modes are a feature that doesn't > extend to typical use-cases. So for all existing use-cases (like LED or > backlight) we always assume a PWM running in normal mode. Now, if you > write a driver for some particular piece of hardware that needs a mode > that is not the normal mode, the question is: wouldn't that driver know > that it wants exactly push-pull or complementary mode? It should, yes. Wouldn't it have > to explicitly check that the PWM supports it and select it (i.e. in the > driver code)? Yes, that should be the flow. I added the DT changes for cases where a driver could use more than one mode and to be able to choose one of the modes it may needs. > > Say you have a driver that requires push-pull mode. It doesn't really > make sense to require the mode to be encoded in DT, because the driver > will only work with one specific mode anyway. So might as well require > it and have the driver check for support and fail if the PWM is not > compatible. This would likely never happen, because hardware engineers > couldn't have validated the design in that case, but there's no reason > for the mode to be specified in DT because it is fixed by the very use- > case anyway. Yes, agree. > > Also, leaving it out of DT simplifies things. Agree. > If you allow the mode to > be specified in DT you could end up with a situation where the driver > required push-pull mode, but the DT specifies complementary mode. What > do you do in such a situation? Warn about it and just go with push-pull > anyway? Then why give the users a way of screwing things up in the first > place? I only introduce this because I had in mind the PWM regulator and I was thinking to make it work for either push-pull mode and normal mode. But since there is no code for this at the moment I totally agree with you to not introduce the DT part. My bad I push it here without a use case. > >> 3/ If you are talking about: >> "Also complementary mode could be accomplished with a single pwm output >> and a board level inverter, right? How would that be handled when the >> PWM driver doesn't support that mode?" >> This complicates the things and I think it requires extra device tree >> bindings to describe extra per-pwm channel capabilities. For the moment, >> with this series I've stopped to only have the capabilities registered from >> driver. My understanding is that this could be accomplished with extra >> device tree bindings in PWM controller to describe PWM channels >> capabilities. If you also want me to look into this direction please let me >> know. And also, suggestions would be appreciated. > > I think this is very interesting, but I don't think this needs to be > done as part of this series. > >> I know that Rob acked >>> the DT parts of this, but I suspect that this might have been glossed >>> over. >> >> If this is about point 3 I've emphasize above I would like to have some >> inputs from your side, if you agree with a behavior like having extra DT >> bindings. The intention wasn't to left this over but to have a better >> understanding of the subject. I'm thinking if it is ok to have modules >> outside of the SoC that model a behavior that could not be controlled from >> software (it could be only hardware) but to behave in a single way. Take >> for instance this scenario: >> >> - new DT bindings are implemented to specify this behavior per channel >> - hardware modules are soldered outside of a PWM channel with one output >> - the new DT bindings are not specified for the soldered PWM channel >> - user enables this channel, it will have only normal mode available for it >> to configure (because the new DT bindings were not provided) >> - but the real output the user will see would be in complementary or even >> push-pull mode. > > I think we could possible model this as a "logical" PWM in DT. We could > for example have something like this: > > / { > ... > > pwms { > pwm@0 { > compatible = "pwm-fixed"; /* or whatever */ > pwms = <&pwm 0 40000>; /* input PWM */ > mode = <PWM_MODE_COMPLEMENTARY>; > }; > > ... > }; > > ... > }; > > The above would model a logical PWM that is driven by the specified PWM > in normal mode but which is effectively complementary because of some > additional circuitry on the board. Ok, i see it. Sounds good to me. > >>> To restate the concern: these extended modes have special uses and none >>> of the users in the kernel, other than sysfs, can use anything other >>> than the normal mode. They may work fine with other modes, but only if >>> they ignore the extras that come with them. Therefore I think it's safe >>> to say that anyone who would want to use these modes would want to >>> explicitly say so. For example the sysfs interface already does that by >>> changing the mode only after the "mode" attribute is written. Any users >>> for special use-cases would want to do the same thing, that is, drive a >>> PWM in a specific mode, on purpose. You wouldn't have a "generic" user >>> such as pwm-backlight or leds-pwm request anything other than the normal >>> mode. >>> >>> So my question is, do we really need to represent these modes in DT? The >>> series currently doesn't contain any patches that add users of these new >>> modes. Are such patches available somewhere, or is the only user of this >>> supposed to be sysfs? >> >> For the moment, no, there is no in-kernel user for this, only sysfs. I had >> in mind to adapt the use of these new mode for PWM regulator for scenarios >> described in [1] page 2126. Anyway, since these patches doesn't introduce >> any user other that sysfs it will not disturbed me to drop the changes. By >> the time I or someone else will do some other changes that requires this, >> the DT part should also be introduced. >> >> [1] http://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf > > Yes, I'd like that. The half-bridge converter certainly sounds like > something that may be able to use the DT bindings that you specified, > but I'd be less concerned about these changes if we had actual users. I understand. Now, thinking again at what you proposed above with regards to logical PWM channels I'm wondering if, for future, if needed, would be good for PWM clients that could used a PWM channel in more than one PWM mode, to have specified in device tree, as a child of PWM controller, the mode that the client would use that channel. E.g. if DriverX wants to use PWM0 in complementary mode: pwm@00aabbcc { // ... pwm@0 { mode = <PWM_MODE_COMPLEMENTARY>; // this being the only mode that could be used for // PWM channel 0 }; } driverx@00ffffff { pwms=<pwm 0 50000>; } For future reference, do you find this feasible? > >>> I'm hesitant to move forward with this as-is without seeing how it will >>> be used. >> >> For the moment only sysfs is supposed to use this. >> >> The PWM specifier flags are somewhat abused by adding modes to >>> them. >> >> I see. >> >> I think this hasn't been completely thought through, because the >>> only reason to specify a mode is to actually set that mode. >> >> Maybe it wasn't clear understand, with the current implementation if no >> mode will be specified the default mode will be used. There is no impose to >> specify a mode. >> >> But then the >>> DT ABI allows a bitmask of modes to be requested via DT. I know that >>> only the first of those modes will end up being used, but then why even >>> allow it in the first place? >> >> I had in mind that I will change the PWM regulator driver to work in >> scenarios like the one specified in the link above. > > Yeah, that sounds like it would be reasonable from a quick look. > However, what I don't quite understand yet is why the mode in the PWM > specifier would need to be a bitmask. You are talking to have them as bitmask in pwm-flags cell right? I though to stick this to the current way to request the PWM mode. Take for example the pwm-regulator > case for a half-bridge converter. If your board uses such a setup, then > you absolutely must drive the PWM in push-pull mode, otherwise the > converter will not work, right? Right! So you want exactly one mode to be > applied. Then why complicate matters by allowing the mode to be a > bitmask? Just to have everything behaving almost in the same way as it was previously. I'm saying to request the PWM channel from a PWM client (via DT) as it was previously done but just adding the PWM mode (in pwm-flags cell as per this version). I also was not sure about this: in the 2nd version of this series I introduced a new cell for PWM modes but this new cell was after pwm-flags cell, and pwm-flags cell is optional, so to have simpler code, in scenarios with PWM modes user would have also specified the pwm-flags cell (although maybe it was not necessary). > We could just as easily reserve, say, 8 bits (24-31) for the > mode, which could then be identical to enum pwm_mode. In pwm-flags cell, right? > >>> And again, even if we allow the mode to be specified in DT, how do the >>> consumer drivers know that the correct mode was being set in DT. >> >> PWM user will call at some point devm_pwm_get() which, in turn, will call >> of_pwm_get() which in turn will initialize PWM args with inputs from DT. >> After that PWM user will, at some point, apply a PWM state; if it is >> initialized with PWM args initialized when devm_pwm_get() was called then >> pwm_apply_state() would fail if no good mode is provided as input via DT. >> >> Same thing happens if a bad period is provided via DT. > > But that only checks that the DT specified a supported mode, it doesn't > mean that it's the correct one. For cases like pwm-regulator this may be > fine because the driver ultimately doesn't care about the exact mode. If > you have a driver that only works with a specific mode, however, it can > be problematic. Yes, agree. > >> Let's >>> say we have a consumer that requires the PWM to be driven in >>> complementary mode. Should it rely on the DT to contain the correct >>> specification for the mode? And if it knows that it needs complementary >>> mode, why not just set that mode itself? >> >> I'm thinking it's the same way as is with PWM period which could also be >> provided from DT. In the end a bad period value could be provided from >> device tree. E.g. Atmel PWM controller could generate PWM signals who's >> periods could not be higher than ~0.6 seconds. If a bad value is provided >> the pwm_apply_state() will fail. > > I understand that. And it's good to validate these things in the driver. > However, the PWM driver can only validate for the PWM that it is > driving. It doesn't know if the consumer has any special requirements > regarding the mode. So if the PWM supports push-pull mode and the DT > contains PWM_MODE_PUSH_PULL, then everything is fine as far as the PWM > driver is concerned. However, if the consumer driver strictly requires > complementary mode, there's nothing the PWM driver can do about it. So > we either need the consumer to be able to query the mode if it has any > specific needs and refuse to use a PWM that has the wrong mode in the > specifier, or the consumer needs to explicitly set a mode, in which case > there's no need to have it in DT and the PWM driver needs to reject it > if the PWM doesn't support it. Ok, I see your point and understand that DT part may be risky and complicates the things. And I agree to remove it from this series since, anyway, there is no in-kernel user for that. Thank you, Claudiu Beznea > > Thierry >
Hello Claudiu, On Tue, Aug 28, 2018 at 04:01:17PM +0300, Claudiu Beznea wrote: > Please give feedback on these patches which extends the PWM framework in > order to support multiple PWM modes of operations. This series is a rework > of [1] and [2]. > > The current patch series add the following PWM modes: > - PWM mode normal > - PWM mode complementary > - PWM mode push-pull > > Normal mode - for PWM channels with one output; output waveforms looks like > this: > __ __ __ __ > PWM __| |__| |__| |__| |__ > <--T--> > > Where T is the signal period In my discussion about some suggested changes to the PWM framework I used slightly different way to show the wave-forms in ASCII which are IMHO slightly better. Also I think it is valuable to not use a 50% duty cycle in the examples to remove some ambiguity. With a duty cycle of 1/3 the normal mode looks as follows in "my" way: __ __ __ PWM __/ \_____/ \_____/ \_____/ ^ ^ ^ ^ The carets mark always the start of a period. And note the rising and falling edges are already part of the active and inactive phases respectively which matches reality. > Since PWMs with more than one output per channel could be used as one > output PWM the normal mode is the default mode for all PWMs (if not > specified otherwise). > > Complementary mode - for PWM channels with two outputs; output waveforms > for a PWM channel in complementary mode looks line this: > __ __ __ __ > PWMH1 __| |__| |__| |__| |__ > __ __ __ __ __ > PWML1 |__| |__| |__| |__| > <--T--> > > Where T is the signal period. So this translates to (I think): __ __ __ __ __ PWMH __/ \_____/ \_____/ \_____/ \_____/ \_____/ __ _____ _____ _____ _____ _____ PWML \__/ \__/ \__/ \__/ \__/ \ ^ ^ ^ ^ ^ ^ That is PWML always pulls in the opposite direction of PWMH. Maybe we could come up with better terms than PWMH and PWML (which might be specific for the Atmel implementation?). Maybe "normal" and "complement"? > Push-pull mode - for PWM channels with two outputs; output waveforms for a > PWM channel in push-pull mode with normal polarity looks like this: > __ __ > PWMH __| |________| |________ > __ __ > PWML ________| |________| |__ > <--T--> That again with the alternative display method and duty cycle 1/3: __ __ __ PWMA __/ \______________/ \______________/ \______ __ __ PWMB ___________/ \______________/ \______________/ ^ ^ ^ ^ ^ ^ That is PWMA and PWMB are active only every 2nd period taking alternate turns, right? > If polarity is inversed: > __ ________ ________ > PWMH |__| |__| > ________ ________ __ > PWML |__| |__| > <--T--> That's again with duty cycle 1/3: __ ______________ ______________ ______ PWMA \__/ \__/ \__/ ___________ ______________ ______________ PWMB \__/ \__/ \ ^ ^ ^ ^ ^ ^ Given that the start of period isn't externally visible this is equivalent to using a duty cycle 2/3 and not inverting which results in: __ ______________ ______________ ______ PWMA \__/ \__/ \__/ ___________ ______________ ______________ PWMB \__/ \__/ \ ^ ^ ^ ^ ^ I would really like if a more detailed description of the modes would be created as part of this series. Currently there are a few implied properties hidden in the PWM API (unrelated to this series) which I try to resolve together with Thierry. Getting this documented right from the start would be great here. I didn't look in detail into the driver implementation, but from the PWMs implemented in the STM32F4 family I would have chosen a different model which makes me wonder if we should stick to a more general way to describe two outputs from a single PWM channel. I would use four values with nanosecond resolution to describe these: .period .duty_cycle .alt_duty_cycle .alt_offset period and duty_cycle is as before for the primary output and then the alt_* values describe offset and duty cycle of the secondary output. What you called "normal mode" would then be specified using .period = $period .duty_cycle = $duty_cycle .alt_duty_cycle = 0 .alt_offset = dontcare Your "push pull mode" would be: .period = 2 * $period .duty_cycle = $duty_cycle .alt_duty_cycle = $duty_cycle .alt_offset = $period and complementary mode would be specified using: .period = $period .duty_cycle = $duty_cycle .alt_duty_cycle = $period - $duty_cycle .alt_offset = $duty_cycle With this abstraction stuff like "complementary output with dead-time insertion" (something like: __ __ __ PWMA __/ \______________/ \______________/ \______ __________ __________ __ PWMB _______/ \______/ \______/ ^ ^ ^ ) could be modelled. > The PWM working modes are per PWM channel registered as PWM's capabilities. > The driver registers itself to PWM core a get_caps() function, in > struct pwm_ops, that will be used by PWM core to retrieve PWM capabilities. > If this function is not registered in driver's probe, a default function > will be used to retrieve PWM capabilities. Currently, the default > capabilities includes only PWM normal mode. In the i2c framework this is a function, too, and I wonder if simplicity is better served when this is just a flags member in the pwm_ops structure. > PWM state has been updated to keep PWM mode. PWM mode could be configured > via sysfs or via DT. pwm_apply_state() will do the preliminary validation > for PWM mode to be applied. > > In sysfs, user could get PWM modes by reading mode file of PWM device: > root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# ls -l > total 0 > -r--r--r-- 1 root root 4096 Oct 9 09:07 capture > lrwxrwxrwx 1 root root 0 Oct 9 09:07 device -> ../../pwmchip0 > -rw-r--r-- 1 root root 4096 Oct 9 08:42 duty_cycle > -rw-r--r-- 1 root root 4096 Oct 9 08:44 enable > --w------- 1 root root 4096 Oct 9 09:07 export > -rw-r--r-- 1 root root 4096 Oct 9 08:43 mode > -r--r--r-- 1 root root 4096 Oct 9 09:07 npwm > -rw-r--r-- 1 root root 4096 Oct 9 08:42 period > -rw-r--r-- 1 root root 4096 Oct 9 08:44 polarity > drwxr-xr-x 2 root root 0 Oct 9 09:07 power > lrwxrwxrwx 1 root root 0 Oct 9 09:07 subsystem -> ../../../../../../../../class/pwm > -rw-r--r-- 1 root root 4096 Oct 9 08:42 uevent > --w------- 1 root root 4096 Oct 9 09:07 unexport > root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# cat mode > normal complementary [push-pull] > > The mode enclosed in bracket is the currently active mode. > > The mode could be set, via sysfs, by writing to mode file one of the modes > displayed at read: > root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# echo normal > mode > root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# cat mode > [normal] complementary push-pull Getting a simple user of this into the kernel would be beneficial, too. In my discussion with Thierry I'm faced with arguments like: You're simplifying stuff which might destroy a use-case for sysfs users. This is hard to argue against because it involves some guesswork to reconstruct or contradict such arguments. Best regards Uwe
Hi Uwe, Thank you for your inputs and sorry for the late response. Please see my answers inline. On 22.10.2018 11:29, Uwe Kleine-König wrote: > Hello Claudiu, > > On Tue, Aug 28, 2018 at 04:01:17PM +0300, Claudiu Beznea wrote: >> Please give feedback on these patches which extends the PWM framework in >> order to support multiple PWM modes of operations. This series is a rework >> of [1] and [2]. >> >> The current patch series add the following PWM modes: >> - PWM mode normal >> - PWM mode complementary >> - PWM mode push-pull >> >> Normal mode - for PWM channels with one output; output waveforms looks like >> this: >> __ __ __ __ >> PWM __| |__| |__| |__| |__ >> <--T--> >> >> Where T is the signal period > > In my discussion about some suggested changes to the PWM framework I > used slightly different way to show the wave-forms in ASCII which are > IMHO slightly better. It is true that the slope version is more like in real world but, on the other hand, the datasheets and documentations mostly uses the digital waveform formats (with no slopes) with regards to, at least PWM. > Also I think it is valuable to not use a 50% duty > cycle in the examples to remove some ambiguity. Ok, sure, I will use a 1/3 duty cycle on next version. > > With a duty cycle of 1/3 the normal mode looks as follows in "my" way: > > __ __ __ > PWM __/ \_____/ \_____/ \_____/ > ^ ^ ^ ^ > > The carets mark always the start of a period. And note the rising and > falling edges are already part of the active and inactive phases > respectively which matches reality. Ok, I'll use the carets on next version. > >> Since PWMs with more than one output per channel could be used as one >> output PWM the normal mode is the default mode for all PWMs (if not >> specified otherwise). >> >> Complementary mode - for PWM channels with two outputs; output waveforms >> for a PWM channel in complementary mode looks line this: >> __ __ __ __ >> PWMH1 __| |__| |__| |__| |__ >> __ __ __ __ __ >> PWML1 |__| |__| |__| |__| >> <--T--> >> >> Where T is the signal period. > > So this translates to (I think): > > __ __ __ __ __ > PWMH __/ \_____/ \_____/ \_____/ \_____/ \_____/ > __ _____ _____ _____ _____ _____ > PWML \__/ \__/ \__/ \__/ \__/ \ > ^ ^ ^ ^ ^ ^ > > That is PWML always pulls in the opposite direction of PWMH. Maybe we > could come up with better terms than PWMH and PWML (which might be > specific for the Atmel implementation?). Yes, this is Atmel implementation naming. > Maybe "normal" and > "complement"? I will think about it try to come with new naming. Normal and Complement may be confusing for users with regards to PWM modes. > >> Push-pull mode - for PWM channels with two outputs; output waveforms for a >> PWM channel in push-pull mode with normal polarity looks like this: >> __ __ >> PWMH __| |________| |________ >> __ __ >> PWML ________| |________| |__ >> <--T--> > > That again with the alternative display method and duty cycle 1/3: > > __ __ __ > PWMA __/ \______________/ \______________/ \______ > __ __ > PWMB ___________/ \______________/ \______________/ > ^ ^ ^ ^ ^ ^ Ok. > > That is PWMA and PWMB are active only every 2nd period taking alternate > turns, right? Yes. > > >> If polarity is inversed: >> __ ________ ________ >> PWMH |__| |__| >> ________ ________ __ >> PWML |__| |__| >> <--T--> > > That's again with duty cycle 1/3: > > __ ______________ ______________ ______ > PWMA \__/ \__/ \__/ > ___________ ______________ ______________ > PWMB \__/ \__/ \ > ^ ^ ^ ^ ^ ^ > Ok. > Given that the start of period isn't externally visible this is > equivalent to using a duty cycle 2/3 and not inverting which results in: > > __ ______________ ______________ ______ > PWMA \__/ \__/ \__/ > ___________ ______________ ______________ > PWMB \__/ \__/ \ > ^ ^ ^ ^ ^ > > I would really like if a more detailed description of the modes would be > created as part of this series. Sure, I will try to document it better. > Currently there are a few implied > properties hidden in the PWM API (unrelated to this series) which I try > to resolve together with Thierry. Getting this documented right from the > start would be great here. Could you tell me if you want something specific to be touch as part of documentation process for these PWM modes? > > I didn't look in detail into the driver implementation, but from the > PWMs implemented in the STM32F4 family I would have chosen a different > model which makes me wonder if we should stick to a more general way to > describe two outputs from a single PWM channel. > > I would use four values with nanosecond resolution to describe these: > > .period > .duty_cycle > .alt_duty_cycle > .alt_offset > > period and duty_cycle is as before for the primary output and then the > alt_* values describe offset and duty cycle of the secondary output. > > What you called "normal mode" would then be specified using > > .period = $period > .duty_cycle = $duty_cycle > .alt_duty_cycle = 0 > .alt_offset = dontcare > > Your "push pull mode" would be: > > .period = 2 * $period > .duty_cycle = $duty_cycle > .alt_duty_cycle = $duty_cycle > .alt_offset = $period > > and complementary mode would be specified using: > > .period = $period > .duty_cycle = $duty_cycle > .alt_duty_cycle = $period - $duty_cycle > .alt_offset = $duty_cycle > On Atmel PWM controller the push-pull mode is hardware generated based on period and duty cycles that are setup for only one channel. The hardware will take care of the synchronization b/w the outputs so that the push-pull characteristic to be generated. Having different configuration for every output part of the push-pull waveform will allow users to generate every kind of outputs. But for IPs that are capable of push-pull or complementary modes the generation of the 2 outputs are done in hardware (true in case of Atmel PWM controller). In case of STM32F4 as far as I can see from [1] "The advanced-control timers (TIM1 and TIM8 ) can output two complementary signals and manage the switching-off and the switching-on instants of the outputs." Maybe, in this case, if there are 2 hardware blocks that could be synced to work together, e.g. in complementary mode, the setting of these two timers should be done in driver so that the hardware blocks to be configured together, atomically, so that the complementary characteristics to be obtained. From my point of view it is better to implement in PWM core the concepts, e.g. push-pull, complementary, even dead-time (I had something in my queue for this) and the driver to do what it takes so that the IP to generate implemented concepts. Mostly, the applications of PWM will use the PWM in normal mode, push-pull, complementary or complementary with dead-time insertion. [1] https://www.st.com/content/ccc/resource/technical/document/reference_manual/3d/6d/5a/66/b4/99/40/d4/DM00031020.pdf/files/DM00031020.pdf/jcr:content/translations/en.DM00031020.pdf > With this abstraction stuff like "complementary output with dead-time > insertion" (something like: > > __ __ __ > PWMA __/ \______________/ \______________/ \______ > __________ __________ __ > PWMB _______/ \______/ \______/ > ^ ^ ^ > > ) could be modelled. Same for this, my opinion is that we should implement generic things in core and drivers should configure properly the IP so that it generates the proper signals. > >> The PWM working modes are per PWM channel registered as PWM's capabilities. >> The driver registers itself to PWM core a get_caps() function, in >> struct pwm_ops, that will be used by PWM core to retrieve PWM capabilities. >> If this function is not registered in driver's probe, a default function >> will be used to retrieve PWM capabilities. Currently, the default >> capabilities includes only PWM normal mode. > > In the i2c framework this is a function, too, and I wonder if simplicity > is better served when this is just a flags member in the pwm_ops > structure. Thierry proposed this so that we could retrieve capabilities per PWM channel. > >> PWM state has been updated to keep PWM mode. PWM mode could be configured >> via sysfs or via DT. pwm_apply_state() will do the preliminary validation >> for PWM mode to be applied. >> >> In sysfs, user could get PWM modes by reading mode file of PWM device: >> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# ls -l >> total 0 >> -r--r--r-- 1 root root 4096 Oct 9 09:07 capture >> lrwxrwxrwx 1 root root 0 Oct 9 09:07 device -> ../../pwmchip0 >> -rw-r--r-- 1 root root 4096 Oct 9 08:42 duty_cycle >> -rw-r--r-- 1 root root 4096 Oct 9 08:44 enable >> --w------- 1 root root 4096 Oct 9 09:07 export >> -rw-r--r-- 1 root root 4096 Oct 9 08:43 mode >> -r--r--r-- 1 root root 4096 Oct 9 09:07 npwm >> -rw-r--r-- 1 root root 4096 Oct 9 08:42 period >> -rw-r--r-- 1 root root 4096 Oct 9 08:44 polarity >> drwxr-xr-x 2 root root 0 Oct 9 09:07 power >> lrwxrwxrwx 1 root root 0 Oct 9 09:07 subsystem -> ../../../../../../../../class/pwm >> -rw-r--r-- 1 root root 4096 Oct 9 08:42 uevent >> --w------- 1 root root 4096 Oct 9 09:07 unexport >> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# cat mode >> normal complementary [push-pull] >> >> The mode enclosed in bracket is the currently active mode. >> >> The mode could be set, via sysfs, by writing to mode file one of the modes >> displayed at read: >> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# echo normal > mode >> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# cat mode >> [normal] complementary push-pull > > Getting a simple user of this into the kernel would be beneficial, too. > In my discussion with Thierry I'm faced with arguments like: You're > simplifying stuff which might destroy a use-case for sysfs users. This > is hard to argue against because it involves some guesswork to > reconstruct or contradict such arguments. Yep, currently only sysfs is using this. I will see what I can do about this to also have an in kernel user (I'll have to check, at least, how pwm-regulator is working with this). Thank you, Claudiu Beznea > > Best regards > Uwe >
Hello Claudiu, On Fri, Oct 26, 2018 at 10:44:43AM +0000, Claudiu.Beznea@microchip.com wrote: > Thank you for your inputs and sorry for the late response. Please see my > answers inline. No problems, I didn't held my breath :-) > On 22.10.2018 11:29, Uwe Kleine-König wrote: > > On Tue, Aug 28, 2018 at 04:01:17PM +0300, Claudiu Beznea wrote: > >> Since PWMs with more than one output per channel could be used as one > >> output PWM the normal mode is the default mode for all PWMs (if not > >> specified otherwise). > >> > >> Complementary mode - for PWM channels with two outputs; output waveforms > >> for a PWM channel in complementary mode looks line this: > >> __ __ __ __ > >> PWMH1 __| |__| |__| |__| |__ > >> __ __ __ __ __ > >> PWML1 |__| |__| |__| |__| > >> <--T--> > >> > >> Where T is the signal period. > > > > So this translates to (I think): > > > > __ __ __ __ __ > > PWMH __/ \_____/ \_____/ \_____/ \_____/ \_____/ > > __ _____ _____ _____ _____ _____ > > PWML \__/ \__/ \__/ \__/ \__/ \ > > ^ ^ ^ ^ ^ ^ > > > > That is PWML always pulls in the opposite direction of PWMH. Maybe we > > could come up with better terms than PWMH and PWML (which might be > > specific for the Atmel implementation?). > > Yes, this is Atmel implementation naming. > > > Maybe "normal" and > > "complement"? > > I will think about it try to come with new naming. Normal and Complement > may be confusing for users with regards to PWM modes. > > > > >> Push-pull mode - for PWM channels with two outputs; output waveforms for a > >> PWM channel in push-pull mode with normal polarity looks like this: > >> __ __ > >> PWMH __| |________| |________ > >> __ __ > >> PWML ________| |________| |__ > >> <--T--> > > > > That again with the alternative display method and duty cycle 1/3: > > > > __ __ __ > > PWMA __/ \______________/ \______________/ \______ > > __ __ > > PWMB ___________/ \______________/ \______________/ > > ^ ^ ^ ^ ^ ^ > Ok. > > > That is PWMA and PWMB are active only every 2nd period taking alternate > > turns, right? > > Yes. > > > > > > >> If polarity is inversed: > >> __ ________ ________ > >> PWMH |__| |__| > >> ________ ________ __ > >> PWML |__| |__| > >> <--T--> > > > > That's again with duty cycle 1/3: > > > > __ ______________ ______________ ______ > > PWMA \__/ \__/ \__/ > > ___________ ______________ ______________ > > PWMB \__/ \__/ \ > > ^ ^ ^ ^ ^ ^ > > > > Ok. > > > Given that the start of period isn't externally visible this is > > equivalent to using a duty cycle 2/3 and not inverting which results in: > > > > __ ______________ ______________ ______ > > PWMA \__/ \__/ \__/ > > ___________ ______________ ______________ > > PWMB \__/ \__/ \ > > ^ ^ ^ ^ ^ > > > > I would really like if a more detailed description of the modes would be > > created as part of this series. > > Sure, I will try to document it better. Note here I just leaked my belief that the PWM framework shouldn't necessarily expose inversion to users which is part of my discussion with Thierry. I think it would be sensible to drop it here. > > Currently there are a few implied > > properties hidden in the PWM API (unrelated to this series) which I try > > to resolve together with Thierry. Getting this documented right from the > > start would be great here. > > Could you tell me if you want something specific to be touch as part of > documentation process for these PWM modes? At least having something about the expectations in Documenation/ would be great. > > I didn't look in detail into the driver implementation, but from the > > PWMs implemented in the STM32F4 family I would have chosen a different > > model which makes me wonder if we should stick to a more general way to > > describe two outputs from a single PWM channel. > > > > I would use four values with nanosecond resolution to describe these: > > > > .period > > .duty_cycle > > .alt_duty_cycle > > .alt_offset > > > > period and duty_cycle is as before for the primary output and then the > > alt_* values describe offset and duty cycle of the secondary output. > > > > What you called "normal mode" would then be specified using > > > > .period = $period > > .duty_cycle = $duty_cycle > > .alt_duty_cycle = 0 > > .alt_offset = dontcare > > > > Your "push pull mode" would be: > > > > .period = 2 * $period > > .duty_cycle = $duty_cycle > > .alt_duty_cycle = $duty_cycle > > .alt_offset = $period > > > > and complementary mode would be specified using: > > > > .period = $period > > .duty_cycle = $duty_cycle > > .alt_duty_cycle = $period - $duty_cycle > > .alt_offset = $duty_cycle > > > > On Atmel PWM controller the push-pull mode is hardware generated based on > period and duty cycles that are setup for only one channel. The hardware > will take care of the synchronization b/w the outputs so that the push-pull > characteristic to be generated. > > Having different configuration for every output part of the push-pull > waveform will allow users to generate every kind of outputs. But for IPs > that are capable of push-pull or complementary modes the generation of the > 2 outputs are done in hardware (true in case of Atmel PWM controller). In > case of STM32F4 as far as I can see from [1] "The advanced-control timers > (TIM1 and TIM8 ) can output two complementary signals and > manage the switching-off and the switching-on instants of the outputs." > Maybe, in this case, if there are 2 hardware blocks that could be synced to > work together, e.g. in complementary mode, the setting of these two timers > should be done in driver so that the hardware blocks to be configured > together, atomically, so that the complementary characteristics to be obtained. The upside I see in my approach with .alt_duty_cycle and .alt_offset over your .mode is that it allows to describe more use-cases. If I wanted to support complementary with dead-time I'd need another member in pwm_state to specify the dead time. Then the next controller can only add dead time on one end of secondary output needing yet another mode enum. With my approach I think you can specify all sensible(TM) waveforms already now. Then a driver must not be adapted again when someone adds support for another mode. The downside is that if your PWM only supports complementary mode with no dead-time you have to find out from .alt_duty_cycle and .alt_offset that the specified wave form is indeed matching complementary mode. From from the framework's POV this is more elegant though (but YMMV). > > With this abstraction stuff like "complementary output with dead-time > > insertion" (something like: > > > > __ __ __ > > PWMA __/ \______________/ \______________/ \______ > > __________ __________ __ > > PWMB _______/ \______/ \______/ > > ^ ^ ^ > > > > ) could be modelled. > > Same for this, my opinion is that we should implement generic things in > core and drivers should configure properly the IP so that it generates the > proper signals. This is common to both our approaches. Just the way the PWM user specifies his/her wishes (and so the input for hw drivers) is different. > >> The PWM working modes are per PWM channel registered as PWM's capabilities. > >> The driver registers itself to PWM core a get_caps() function, in > >> struct pwm_ops, that will be used by PWM core to retrieve PWM capabilities. > >> If this function is not registered in driver's probe, a default function > >> will be used to retrieve PWM capabilities. Currently, the default > >> capabilities includes only PWM normal mode. > > > > In the i2c framework this is a function, too, and I wonder if simplicity > > is better served when this is just a flags member in the pwm_ops > > structure. > > Thierry proposed this so that we could retrieve capabilities per PWM channel. I don't have a complete overview over the different hardware implementations, but I'd expect that if two different implementations share the operations then the return value of .get_caps is shared by both. As long this is the case not introducing a callback for that is the easier path. Adding a callback later when (and if) this is required is trivial then. Best regards Uwe
Hi, Please give feedback on these patches which extends the PWM framework in order to support multiple PWM modes of operations. This series is a rework of [1] and [2]. The current patch series add the following PWM modes: - PWM mode normal - PWM mode complementary - PWM mode push-pull Normal mode - for PWM channels with one output; output waveforms looks like this: __ __ __ __ PWM __| |__| |__| |__| |__ <--T--> Where T is the signal period Since PWMs with more than one output per channel could be used as one output PWM the normal mode is the default mode for all PWMs (if not specified otherwise). Complementary mode - for PWM channels with two outputs; output waveforms for a PWM channel in complementary mode looks line this: __ __ __ __ PWMH1 __| |__| |__| |__| |__ __ __ __ __ __ PWML1 |__| |__| |__| |__| <--T--> Where T is the signal period. Push-pull mode - for PWM channels with two outputs; output waveforms for a PWM channel in push-pull mode with normal polarity looks like this: __ __ PWMH __| |________| |________ __ __ PWML ________| |________| |__ <--T--> If polarity is inversed: __ ________ ________ PWMH |__| |__| ________ ________ __ PWML |__| |__| <--T--> Where T is the signal period. The PWM working modes are per PWM channel registered as PWM's capabilities. The driver registers itself to PWM core a get_caps() function, in struct pwm_ops, that will be used by PWM core to retrieve PWM capabilities. If this function is not registered in driver's probe, a default function will be used to retrieve PWM capabilities. Currently, the default capabilities includes only PWM normal mode. PWM state has been updated to keep PWM mode. PWM mode could be configured via sysfs or via DT. pwm_apply_state() will do the preliminary validation for PWM mode to be applied. In sysfs, user could get PWM modes by reading mode file of PWM device: root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# ls -l total 0 -r--r--r-- 1 root root 4096 Oct 9 09:07 capture lrwxrwxrwx 1 root root 0 Oct 9 09:07 device -> ../../pwmchip0 -rw-r--r-- 1 root root 4096 Oct 9 08:42 duty_cycle -rw-r--r-- 1 root root 4096 Oct 9 08:44 enable --w------- 1 root root 4096 Oct 9 09:07 export -rw-r--r-- 1 root root 4096 Oct 9 08:43 mode -r--r--r-- 1 root root 4096 Oct 9 09:07 npwm -rw-r--r-- 1 root root 4096 Oct 9 08:42 period -rw-r--r-- 1 root root 4096 Oct 9 08:44 polarity drwxr-xr-x 2 root root 0 Oct 9 09:07 power lrwxrwxrwx 1 root root 0 Oct 9 09:07 subsystem -> ../../../../../../../../class/pwm -rw-r--r-- 1 root root 4096 Oct 9 08:42 uevent --w------- 1 root root 4096 Oct 9 09:07 unexport root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# cat mode normal complementary [push-pull] The mode enclosed in bracket is the currently active mode. The mode could be set, via sysfs, by writing to mode file one of the modes displayed at read: root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# echo normal > mode root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# cat mode [normal] complementary push-pull The PWM push-pull mode could be usefull in applications like half bridge converters. This series also add PWM modes support for Atmel/Microchip SoCs. Thank you, Claudiu Beznea [1] https://www.spinics.net/lists/arm-kernel/msg580275.html [2] https://lkml.org/lkml/2018/1/12/359 Changes in v5: - solved kbuild errors by removing dummy functions for case where CONFIG_PWM is not defined; adopted this approach since the removed function are used only when CONFIG_PWM is defined (in PWM core and few drivers from drivers/pwm/ directory) Changes in v4: - removed changes related to pwm_config() as per maintainer proposals - added pwm_mode_get_valid() to retrieve a valid PWM mode fror PWM device instead of using BIT(ffs(caps.mode) - 1) and changed drivers to use pwm_mode_get_valid() instead of pwm_get_caps() + BIT(ffs(caps.mode) - 1) (patches 2, 3, 4 from this series) - renamed PWM_MODE() macro in PWMC_MODE() to avoid conflicts with pwm-sun4i.c driver ('C' stands for capability) - removed pwm_caps_valid() function - renamed PWM_DTMODE_COMPLEMENTARY and PWM_DTMODE_PUSH_PULL macros in PWM_MODE_COMPLEMENTARY and PWM_MODE_PUSH_PULL Changes in v3: - removed changes related to only one of_xlate function for all PWM drivers - switch to PWM capabilities per PWM channel nor per PWM chip - squash documentation and bindings patches as requeted by reviewer - introduced PWM_MODE(name) macro and used a bit enum for pwm modes - related to DT bindings, used flags cell also for PWM modes - updated of_xlate specific functions with "state->mode = mode;" instructions to avoid pwm_apply_state() failures - use available modes for PWM channel in pwm_config() by first calling pwm_get_caps() to get caps.modes - use loops through available modes in mode_store()/mode_show() and also in of_pwm_xlate_with_flags() instead of "if else" instructions; in this way, the addition of a new mode is independent of this code sections - use DTLI=1, DTHI=0 register settings to obtain push-pull mode waveforms for Atmel/Microchip PWM controller. Changes in v2: - remove of_xlate and of_pwm_n_cells and use generic functions to pharse DT inputs; this is done in patches 1, 2, 3, 4, 5, 6, 7 of this series; this will make easy the addition of PWM mode support from DT - add PWM mode normal - register PWM modes as capabilities of PWM chips at driver probe and, in case driver doesn't provide these capabilities use default ones - change the way PWM mode is pharsed via DT by using a new input for pwms binding property Claudiu Beznea (9): pwm: extend PWM framework with PWM modes pwm: clps711x: populate PWM mode in of_xlate function pwm: cros-ec: populate PWM mode in of_xlate function pwm: pxa: populate PWM mode in of_xlate function pwm: add PWM modes pwm: atmel: add pwm capabilities pwm: add push-pull mode support pwm: add documentation for pwm push-pull mode pwm: atmel: add push-pull mode support Documentation/devicetree/bindings/pwm/pwm.txt | 11 ++- Documentation/pwm.txt | 42 ++++++++- drivers/pwm/core.c | 125 +++++++++++++++++++++++++- drivers/pwm/pwm-atmel.c | 118 +++++++++++++++++------- drivers/pwm/pwm-clps711x.c | 10 ++- drivers/pwm/pwm-cros-ec.c | 1 + drivers/pwm/pwm-pxa.c | 1 + drivers/pwm/sysfs.c | 61 +++++++++++++ include/dt-bindings/pwm/pwm.h | 2 + include/linux/pwm.h | 64 +++++++++++++ 10 files changed, 395 insertions(+), 40 deletions(-)