| Message ID | 1546522081-23659-2-git-send-email-claudiu.beznea@microchip.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | extend PWM framework to support PWM modes | expand |
Hello, On Thu, Jan 03, 2019 at 01:29:44PM +0000, Claudiu.Beznea@microchip.com wrote: > From: Claudiu Beznea <claudiu.beznea@microchip.com> > > Add basic PWM modes: normal and complementary. These modes should > differentiate the single output PWM channels from two outputs PWM > channels. These modes could be set as follow: > 1. PWM channels with one output per channel: > - normal mode > 2. PWM channels with two outputs per channel: > - normal mode > - complementary mode > Since users could use a PWM channel with two output as one output PWM > channel, the PWM normal mode is allowed to be set for PWM channels with > two outputs; in fact PWM normal mode should be supported by all PWMs. I still think that my suggestion that I sent in reply to your v5 using .alt_duty_cycle and .alt_offset is the better one as it is more generic. A word about that from Thierry before putting the mode into the pwm API would be great. I don't repeat what I wrote there assuming you still remember or are willing to look it up at e.g. https://www.spinics.net/lists/linux-pwm/msg08174.html (in the 2nd half of my mail). Also I think that if the capabilities function is the way forward adding support to detect availability of polarity inversion should be considered. This would also be an opportunity to split the introduction of the capabilities function and the introduction of complementary mode. (But my personal preference would be to just let .apply fail when an unsupported configuration is requested.) > +static int pwm_get_default_caps(struct pwm_caps *caps) > +{ > + static const struct pwm_caps default_caps = { > + .modes_msk = PWM_MODE_BIT(NORMAL), > + }; > + > + if (!caps) > + return -EINVAL; > + > + *caps = default_caps; > + > + return 0; > +} > + > +/** > + * pwm_get_caps() - get PWM capabilities of a PWM device > + * @pwm: PWM device to get the capabilities for > + * @caps: returned capabilities > + * > + * Returns: 0 on success or a negative error code on failure > + */ > +int pwm_get_caps(const struct pwm_device *pwm, struct pwm_caps *caps) > +{ > + if (!pwm || !caps) > + return -EINVAL; > + > + if (pwm->chip->ops->get_caps) > + return pwm->chip->ops->get_caps(pwm->chip, pwm, caps); > + > + return pwm_get_default_caps(caps); I'd drop pwm_get_default_caps (unless you introduce some more callers later) and fold its implementation into pwm_get_caps. > +} > +EXPORT_SYMBOL_GPL(pwm_get_caps); > [...] > @@ -53,12 +75,14 @@ enum { > * @period: PWM period (in nanoseconds) > * @duty_cycle: PWM duty cycle (in nanoseconds) > * @polarity: PWM polarity > + * @modebit: PWM mode bit > * @enabled: PWM enabled status > */ > struct pwm_state { > unsigned int period; > unsigned int duty_cycle; > enum pwm_polarity polarity; > + unsigned long modebit; I fail to see the upside of storing the mode as 2^mode instead of a plain enum pwm_mode. Given that struct pwm_state is visible for pwm users a plain pwm_mode would at least be more intuitive. Best regards Uwe
On 05.01.2019 23:05, Uwe Kleine-König wrote: > Hello, > > On Thu, Jan 03, 2019 at 01:29:44PM +0000, Claudiu.Beznea@microchip.com wrote: >> From: Claudiu Beznea <claudiu.beznea@microchip.com> >> >> Add basic PWM modes: normal and complementary. These modes should >> differentiate the single output PWM channels from two outputs PWM >> channels. These modes could be set as follow: >> 1. PWM channels with one output per channel: >> - normal mode >> 2. PWM channels with two outputs per channel: >> - normal mode >> - complementary mode >> Since users could use a PWM channel with two output as one output PWM >> channel, the PWM normal mode is allowed to be set for PWM channels with >> two outputs; in fact PWM normal mode should be supported by all PWMs. > > I still think that my suggestion that I sent in reply to your v5 using > .alt_duty_cycle and .alt_offset is the better one as it is more generic. I like it better my way, I explained myself why. > A word about that from Thierry before putting the mode into the pwm API > would be great. Yes, let's wait Thierry inputs on this. > > I don't repeat what I wrote there assuming you still remember or are > willing to look it up at > e.g. https://www.spinics.net/lists/linux-pwm/msg08174.html (in the 2nd half > of my mail). Yes, I remember it. > > Also I think that if the capabilities function is the way forward adding > support to detect availability of polarity inversion should be > considered. Yep, why not. But it should be done in a different patch. It is not related to this series. This would also be an opportunity to split the introduction > of the capabilities function and the introduction of complementary mode. > (But my personal preference would be to just let .apply fail when an > unsupported configuration is requested.) .apply fails when something wrong is requested. > >> +static int pwm_get_default_caps(struct pwm_caps *caps) >> +{ >> + static const struct pwm_caps default_caps = { >> + .modes_msk = PWM_MODE_BIT(NORMAL), >> + }; >> + >> + if (!caps) >> + return -EINVAL; >> + >> + *caps = default_caps; >> + >> + return 0; >> +} >> + >> +/** >> + * pwm_get_caps() - get PWM capabilities of a PWM device >> + * @pwm: PWM device to get the capabilities for >> + * @caps: returned capabilities >> + * >> + * Returns: 0 on success or a negative error code on failure >> + */ >> +int pwm_get_caps(const struct pwm_device *pwm, struct pwm_caps *caps) >> +{ >> + if (!pwm || !caps) >> + return -EINVAL; >> + >> + if (pwm->chip->ops->get_caps) >> + return pwm->chip->ops->get_caps(pwm->chip, pwm, caps); >> + >> + return pwm_get_default_caps(caps); > > I'd drop pwm_get_default_caps (unless you introduce some more callers > later) and fold its implementation into pwm_get_caps. I did it as Thierry proposed. > >> +} >> +EXPORT_SYMBOL_GPL(pwm_get_caps); >> [...] >> @@ -53,12 +75,14 @@ enum { >> * @period: PWM period (in nanoseconds) >> * @duty_cycle: PWM duty cycle (in nanoseconds) >> * @polarity: PWM polarity >> + * @modebit: PWM mode bit >> * @enabled: PWM enabled status >> */ >> struct pwm_state { >> unsigned int period; >> unsigned int duty_cycle; >> enum pwm_polarity polarity; >> + unsigned long modebit; > > I fail to see the upside of storing the mode as 2^mode instead of a > plain enum pwm_mode. Given that struct pwm_state is visible for pwm > users a plain pwm_mode would at least be more intuitive. To have all modes supported by a controller grouped in pwm_caps::modes_msk. > > Best regards > Uwe >
Hello Claudiu, On Mon, Jan 07, 2019 at 09:30:55AM +0000, Claudiu.Beznea@microchip.com wrote: > On 05.01.2019 23:05, Uwe Kleine-König wrote: > > On Thu, Jan 03, 2019 at 01:29:44PM +0000, Claudiu.Beznea@microchip.com wrote: > >> From: Claudiu Beznea <claudiu.beznea@microchip.com> > >> > >> Add basic PWM modes: normal and complementary. These modes should > >> differentiate the single output PWM channels from two outputs PWM > >> channels. These modes could be set as follow: > >> 1. PWM channels with one output per channel: > >> - normal mode > >> 2. PWM channels with two outputs per channel: > >> - normal mode > >> - complementary mode > >> Since users could use a PWM channel with two output as one output PWM > >> channel, the PWM normal mode is allowed to be set for PWM channels with > >> two outputs; in fact PWM normal mode should be supported by all PWMs. > > > > I still think that my suggestion that I sent in reply to your v5 using > > .alt_duty_cycle and .alt_offset is the better one as it is more generic. > > I like it better my way, I explained myself why. I couldn't really follow your argument though. You seemed to acknowledge that using .alt_duty_cycle and .alt_offset is more generic. Then you wrote that the push-pull mode is hardware generated on Atmel with some implementation details. IMHO these implementation details shouldn't be part of the PWM API and atmel's .apply should look as follows: if (state->alt_duty_cycle == 0) { ... configure for normal mode ... } else if (state->duty_cycle == state->alt_duty_cycle && state->alt_offset == state->period / 2) { ... configure for push pull mode ... } else if (state->duty_cycle + state->alt_duty_cycle == state->period && state->alt_offset == state->duty_cycle) { ... configure for complementary mode ... } else { return -EINVAL; } If it turns out to be a common pattern, we can add helper functions à la pwm_is_complementary_mode(state) and pwm_set_complementary_mode(state, period, duty_cycle). This allows to have a generic way to describe a wide range of wave forms in a uniform way in the API (which is good) and each driver implements the parts of this range that it can support. > > I don't repeat what I wrote there assuming you still remember or are > > willing to look it up at > > e.g. https://www.spinics.net/lists/linux-pwm/msg08174.html (in the 2nd half > > of my mail). > > Yes, I remember it. I expected that, my words were more directed to Thierry than you. > > Also I think that if the capabilities function is the way forward adding > > support to detect availability of polarity inversion should be > > considered. > > Yep, why not. But it should be done in a different patch. It is not related > to this series. Yes, given that polarity already exists, this would be a good opportunity to introduce the capability function for that and only afterwards add the new use case with modes. (But having said this, read further as I think that this capability function is a bad idea.) > > This would also be an opportunity to split the introduction > > of the capabilities function and the introduction of complementary mode. > > (But my personal preference would be to just let .apply fail when an > > unsupported configuration is requested.) > > .apply fails when something wrong is requested. If my controller doesn't support a second output is it "wrong" to request complementary mode? I'd say yes. So you have to catch that in .apply anyhow and there is little benefit to be able to ask the controller if it supports it beforehand. I don't have a provable statistic at hand, but my feeling is that quite some users of the i2c frame work get it wrong to first check the capabilities and only then try to use them. This is at least error prone and harder to use than the apply function returning an error code. And on the driver side the upside is to have all stuff related to which wave form can be generated and which cannot is a single place. (Just consider "inverted complementary mode". Theoretically this should work if your controller supports complementary mode and inverted mode. If you now have a driver for a controller that can do both, but not at the same time, the separation gets ugly. OK, this is a constructed example, but in my experience something like that happens earlier or later.) > >> [...] > >> @@ -53,12 +75,14 @@ enum { > >> * @period: PWM period (in nanoseconds) > >> * @duty_cycle: PWM duty cycle (in nanoseconds) > >> * @polarity: PWM polarity > >> + * @modebit: PWM mode bit > >> * @enabled: PWM enabled status > >> */ > >> struct pwm_state { > >> unsigned int period; > >> unsigned int duty_cycle; > >> enum pwm_polarity polarity; > >> + unsigned long modebit; > > > > I fail to see the upside of storing the mode as 2^mode instead of a > > plain enum pwm_mode. Given that struct pwm_state is visible for pwm > > users a plain pwm_mode would at least be more intuitive. > > To have all modes supported by a controller grouped in pwm_caps::modes_msk. My question was not about struct pwm_caps::modes_msk but about struct pwm_state::modebit. As struct pwm_state has visibility even outside of the pwm API (i.e. it is used by consumers) it is beneficial to keep that simple. Letting a consumer pass in the mode he wants is easier to explain than setting a single bit. Also error checking with a plain enum is easier because you just do: if (mode >= MODE_CNT) error() which is easy to grasp. Compare that to if (!is_power_of_two(modebit) || modebit >= PWM_MODE_BIT(CNT)) error() (modulo syntactical correctness). Best regards Uwe
Hi Uwe, On 08.01.2019 00:10, Uwe Kleine-König wrote: > Hello Claudiu, > > On Mon, Jan 07, 2019 at 09:30:55AM +0000, Claudiu.Beznea@microchip.com wrote: >> On 05.01.2019 23:05, Uwe Kleine-König wrote: >>> On Thu, Jan 03, 2019 at 01:29:44PM +0000, Claudiu.Beznea@microchip.com wrote: >>>> From: Claudiu Beznea <claudiu.beznea@microchip.com> >>>> >>>> Add basic PWM modes: normal and complementary. These modes should >>>> differentiate the single output PWM channels from two outputs PWM >>>> channels. These modes could be set as follow: >>>> 1. PWM channels with one output per channel: >>>> - normal mode >>>> 2. PWM channels with two outputs per channel: >>>> - normal mode >>>> - complementary mode >>>> Since users could use a PWM channel with two output as one output PWM >>>> channel, the PWM normal mode is allowed to be set for PWM channels with >>>> two outputs; in fact PWM normal mode should be supported by all PWMs. >>> >>> I still think that my suggestion that I sent in reply to your v5 using >>> .alt_duty_cycle and .alt_offset is the better one as it is more generic. >> >> I like it better my way, I explained myself why. > > I couldn't really follow your argument though. You seemed to acknowledge > that using .alt_duty_cycle and .alt_offset is more generic. True it is more generic in the way that it gives the possibility to configure all kind of waveforms. But not all controllers supports this. The use case of this would be to have dead-times with any values, right? > Then you > wrote that the push-pull mode is hardware generated on Atmel with some > implementation details. Yes, I wanted to say that Atmel PWM generates only a set of waveforms. > IMHO these implementation details shouldn't be > part of the PWM API and atmel's .apply should look as follows: > > if (state->alt_duty_cycle == 0) { > > ... configure for normal mode ... > > } else if (state->duty_cycle == state->alt_duty_cycle && > state->alt_offset == state->period / 2) { > > ... configure for push pull mode ... > > } else if (state->duty_cycle + state->alt_duty_cycle == state->period && > state->alt_offset == state->duty_cycle) { > > ... configure for complementary mode ... > > } else { > return -EINVAL; > } If so, the user should have been taken care of the relations b/w these values in order to configure a proper working mode. It would be good to have Thierry's inputs on these. > > If it turns out to be a common pattern, we can add helper functions à la > pwm_is_complementary_mode(state) and > pwm_set_complementary_mode(state, period, duty_cycle). This allows to > have a generic way to describe a wide range of wave forms in a uniform > way in the API (which is good) and each driver implements the parts of > this range that it can support. > >>> I don't repeat what I wrote there assuming you still remember or are >>> willing to look it up at >>> e.g. https://www.spinics.net/lists/linux-pwm/msg08174.html (in the 2nd half >>> of my mail). >> >> Yes, I remember it. > > I expected that, my words were more directed to Thierry than you. > >>> Also I think that if the capabilities function is the way forward adding >>> support to detect availability of polarity inversion should be >>> considered. >> >> Yep, why not. But it should be done in a different patch. It is not related >> to this series. > > Yes, given that polarity already exists, this would be a good > opportunity to introduce the capability function for that and only > afterwards add the new use case with modes. (But having said this, read > further as I think that this capability function is a bad idea.) > >>> This would also be an opportunity to split the introduction >>> of the capabilities function and the introduction of complementary mode. >>> (But my personal preference would be to just let .apply fail when an >>> unsupported configuration is requested.) >> >> .apply fails when something wrong is requested. > > If my controller doesn't support a second output is it "wrong" to > request complementary mode? I'd say yes. So you have to catch that in > .apply anyhow and there is little benefit to be able to ask the > controller if it supports it beforehand. This is checked by pwm_supports_mode() called in pwm_apply_state(). If controller doesn't support the mode, the apply will fail. > > I don't have a provable statistic at hand, but my feeling is that quite > some users of the i2c frame work get it wrong to first check the > capabilities and only then try to use them. This is at least error prone > and harder to use than the apply function returning an error code. > And on the driver side the upside is to have all stuff related to which > wave form can be generated and which cannot is a single place. (Just > consider "inverted complementary mode". Theoretically this should work > if your controller supports complementary mode and inverted mode. If you > now have a driver for a controller that can do both, but not at the same > time, the separation gets ugly. OK, this is a constructed example, but > in my experience something like that happens earlier or later.) > >>>> [...] >>>> @@ -53,12 +75,14 @@ enum { >>>> * @period: PWM period (in nanoseconds) >>>> * @duty_cycle: PWM duty cycle (in nanoseconds) >>>> * @polarity: PWM polarity >>>> + * @modebit: PWM mode bit >>>> * @enabled: PWM enabled status >>>> */ >>>> struct pwm_state { >>>> unsigned int period; >>>> unsigned int duty_cycle; >>>> enum pwm_polarity polarity; >>>> + unsigned long modebit; >>> >>> I fail to see the upside of storing the mode as 2^mode instead of a >>> plain enum pwm_mode. Given that struct pwm_state is visible for pwm >>> users a plain pwm_mode would at least be more intuitive. >> >> To have all modes supported by a controller grouped in pwm_caps::modes_msk. > > My question was not about struct pwm_caps::modes_msk but about > struct pwm_state::modebit. As struct pwm_state has visibility even > outside of the pwm API (i.e. it is used by consumers) it is beneficial > to keep that simple. Letting a consumer pass in the mode he wants is > easier to explain than setting a single bit. Also error checking with a > plain enum is easier because you just do: > > if (mode >= MODE_CNT) > error() > > which is easy to grasp. Compare that to > > if (!is_power_of_two(modebit) || modebit >= PWM_MODE_BIT(CNT)) > error() > > (modulo syntactical correctness). The reason I choose to have it as bit was the memcmp() at the beginning of pwm_apply_state() and to avoid starting enum pwm_mode from 1 and to avoid having bit 0 of pwm_caps::modes_msk unused (in the driver I'm using PWM_MODE_BIT() macro to fill in the driver's supported modes). Thank you, Claudiu Beznea > > Best regards > Uwe >
On Tue, Jan 08, 2019 at 09:21:34AM +0000, Claudiu.Beznea@microchip.com wrote: > Hi Uwe, > > On 08.01.2019 00:10, Uwe Kleine-König wrote: > > Hello Claudiu, > > > > On Mon, Jan 07, 2019 at 09:30:55AM +0000, Claudiu.Beznea@microchip.com wrote: > >> On 05.01.2019 23:05, Uwe Kleine-König wrote: > >>> On Thu, Jan 03, 2019 at 01:29:44PM +0000, Claudiu.Beznea@microchip.com wrote: > >>>> From: Claudiu Beznea <claudiu.beznea@microchip.com> > >>>> > >>>> Add basic PWM modes: normal and complementary. These modes should > >>>> differentiate the single output PWM channels from two outputs PWM > >>>> channels. These modes could be set as follow: > >>>> 1. PWM channels with one output per channel: > >>>> - normal mode > >>>> 2. PWM channels with two outputs per channel: > >>>> - normal mode > >>>> - complementary mode > >>>> Since users could use a PWM channel with two output as one output PWM > >>>> channel, the PWM normal mode is allowed to be set for PWM channels with > >>>> two outputs; in fact PWM normal mode should be supported by all PWMs. > >>> > >>> I still think that my suggestion that I sent in reply to your v5 using > >>> .alt_duty_cycle and .alt_offset is the better one as it is more generic. > >> > >> I like it better my way, I explained myself why. > > > > I couldn't really follow your argument though. You seemed to acknowledge > > that using .alt_duty_cycle and .alt_offset is more generic. > > True it is more generic in the way that it gives the possibility to > configure all kind of waveforms. But not all controllers supports this. > The use case of this would be to have dead-times with any values, right? Well, I didn't target that. The model I suggested is just a generic set of parameters that are able to describe the wave forms for all three modes you suggested. That it allows to express dead-times is just a nice by-product. > >>> I fail to see the upside of storing the mode as 2^mode instead of a > >>> plain enum pwm_mode. Given that struct pwm_state is visible for pwm > >>> users a plain pwm_mode would at least be more intuitive. > >> > >> To have all modes supported by a controller grouped in pwm_caps::modes_msk. > > > > My question was not about struct pwm_caps::modes_msk but about > > struct pwm_state::modebit. As struct pwm_state has visibility even > > outside of the pwm API (i.e. it is used by consumers) it is beneficial > > to keep that simple. Letting a consumer pass in the mode he wants is > > easier to explain than setting a single bit. Also error checking with a > > plain enum is easier because you just do: > > > > if (mode >= MODE_CNT) > > error() > > > > which is easy to grasp. Compare that to > > > > if (!is_power_of_two(modebit) || modebit >= PWM_MODE_BIT(CNT)) > > error() > > > > (modulo syntactical correctness). > > The reason I choose to have it as bit was the memcmp() at the beginning of > pwm_apply_state() and to avoid starting enum pwm_mode from 1 and to avoid > having bit 0 of pwm_caps::modes_msk unused (in the driver I'm using > PWM_MODE_BIT() macro to fill in the driver's supported modes). Does that mean you are convinced by my argument? Best regards Uwe
On 09.01.2019 00:08, Uwe Kleine-König wrote: > On Tue, Jan 08, 2019 at 09:21:34AM +0000, Claudiu.Beznea@microchip.com wrote: >> Hi Uwe, >> >> On 08.01.2019 00:10, Uwe Kleine-König wrote: >>> Hello Claudiu, >>> >>> On Mon, Jan 07, 2019 at 09:30:55AM +0000, Claudiu.Beznea@microchip.com wrote: >>>> On 05.01.2019 23:05, Uwe Kleine-König wrote: >>>>> On Thu, Jan 03, 2019 at 01:29:44PM +0000, Claudiu.Beznea@microchip.com wrote: >>>>>> From: Claudiu Beznea <claudiu.beznea@microchip.com> >>>>>> >>>>>> Add basic PWM modes: normal and complementary. These modes should >>>>>> differentiate the single output PWM channels from two outputs PWM >>>>>> channels. These modes could be set as follow: >>>>>> 1. PWM channels with one output per channel: >>>>>> - normal mode >>>>>> 2. PWM channels with two outputs per channel: >>>>>> - normal mode >>>>>> - complementary mode >>>>>> Since users could use a PWM channel with two output as one output PWM >>>>>> channel, the PWM normal mode is allowed to be set for PWM channels with >>>>>> two outputs; in fact PWM normal mode should be supported by all PWMs. >>>>> >>>>> I still think that my suggestion that I sent in reply to your v5 using >>>>> .alt_duty_cycle and .alt_offset is the better one as it is more generic. >>>> >>>> I like it better my way, I explained myself why. >>> >>> I couldn't really follow your argument though. You seemed to acknowledge >>> that using .alt_duty_cycle and .alt_offset is more generic. >> >> True it is more generic in the way that it gives the possibility to >> configure all kind of waveforms. But not all controllers supports this. >> The use case of this would be to have dead-times with any values, right? > > Well, I didn't target that. The model I suggested is just a generic set > of parameters that are able to describe the wave forms for all three > modes you suggested. That it allows to express dead-times is just a nice > by-product. > >>>>> I fail to see the upside of storing the mode as 2^mode instead of a >>>>> plain enum pwm_mode. Given that struct pwm_state is visible for pwm >>>>> users a plain pwm_mode would at least be more intuitive. >>>> >>>> To have all modes supported by a controller grouped in pwm_caps::modes_msk. >>> >>> My question was not about struct pwm_caps::modes_msk but about >>> struct pwm_state::modebit. As struct pwm_state has visibility even >>> outside of the pwm API (i.e. it is used by consumers) it is beneficial >>> to keep that simple. Letting a consumer pass in the mode he wants is >>> easier to explain than setting a single bit. Also error checking with a >>> plain enum is easier because you just do: >>> >>> if (mode >= MODE_CNT) >>> error() >>> >>> which is easy to grasp. Compare that to >>> >>> if (!is_power_of_two(modebit) || modebit >= PWM_MODE_BIT(CNT)) >>> error() >>> >>> (modulo syntactical correctness). >> >> The reason I choose to have it as bit was the memcmp() at the beginning of >> pwm_apply_state() and to avoid starting enum pwm_mode from 1 and to avoid >> having bit 0 of pwm_caps::modes_msk unused (in the driver I'm using >> PWM_MODE_BIT() macro to fill in the driver's supported modes). > > Does that mean you are convinced by my argument? I know what you are talking about. I balanced in between these two paths while I wrote the code. The approach I chose looked to me to be more easy to read. Now, since there is another person (you) thinking the other approach is best, I'm thinking to change it in the next version. Thank you, Claudiu Beznea > > Best regards > Uwe >
On Sat, Jan 05, 2019 at 10:05:22PM +0100, Uwe Kleine-König wrote: > Hello, > > On Thu, Jan 03, 2019 at 01:29:44PM +0000, Claudiu.Beznea@microchip.com wrote: > > From: Claudiu Beznea <claudiu.beznea@microchip.com> > > > > Add basic PWM modes: normal and complementary. These modes should > > differentiate the single output PWM channels from two outputs PWM > > channels. These modes could be set as follow: > > 1. PWM channels with one output per channel: > > - normal mode > > 2. PWM channels with two outputs per channel: > > - normal mode > > - complementary mode > > Since users could use a PWM channel with two output as one output PWM > > channel, the PWM normal mode is allowed to be set for PWM channels with > > two outputs; in fact PWM normal mode should be supported by all PWMs. > > I still think that my suggestion that I sent in reply to your v5 using > .alt_duty_cycle and .alt_offset is the better one as it is more generic. > A word about that from Thierry before putting the mode into the pwm API > would be great. > > I don't repeat what I wrote there assuming you still remember or are > willing to look it up at > e.g. https://www.spinics.net/lists/linux-pwm/msg08174.html (in the 2nd half > of my mail). The problem I see with .alt_duty_cycle and .alt_offset is that they provide too much flexibility in my opinion. There may be some variance in how the values are computed for the different modes and that just leads to additional code required in drivers to figure out what exactly the user wanted. If we only provide the user with a means to say which mode they want to operate the PWM in they can just tell us and the driver doesn't have to guess which one was meant. It also makes validation easier. We can check for capabilites upfront by just comparing a list of supported modes. With .alt_duty_cycle and .alt_offset we'd actually need to run code to figure out whether or not the given set of values corresponds to a supported configuration. > Also I think that if the capabilities function is the way forward adding > support to detect availability of polarity inversion should be > considered. This would also be an opportunity to split the introduction > of the capabilities function and the introduction of complementary mode. > (But my personal preference would be to just let .apply fail when an > unsupported configuration is requested.) > > > +static int pwm_get_default_caps(struct pwm_caps *caps) > > +{ > > + static const struct pwm_caps default_caps = { > > + .modes_msk = PWM_MODE_BIT(NORMAL), > > + }; > > + > > + if (!caps) > > + return -EINVAL; > > + > > + *caps = default_caps; > > + > > + return 0; > > +} > > + > > +/** > > + * pwm_get_caps() - get PWM capabilities of a PWM device > > + * @pwm: PWM device to get the capabilities for > > + * @caps: returned capabilities > > + * > > + * Returns: 0 on success or a negative error code on failure > > + */ > > +int pwm_get_caps(const struct pwm_device *pwm, struct pwm_caps *caps) > > +{ > > + if (!pwm || !caps) > > + return -EINVAL; > > + > > + if (pwm->chip->ops->get_caps) > > + return pwm->chip->ops->get_caps(pwm->chip, pwm, caps); > > + > > + return pwm_get_default_caps(caps); > > I'd drop pwm_get_default_caps (unless you introduce some more callers > later) and fold its implementation into pwm_get_caps. I think Claudiu and I may have talked past one another here. What I was suggesting was to make pwm_get_default_caps() an exported symbol so that drivers could use it if they didn't have extra capabilities. However, it seems like just handling that in pwm_get_caps() if ops->get_caps == NULL works just as well. I don't think it needs to be a different function in this case, but I don't mind if it is. > > +} > > +EXPORT_SYMBOL_GPL(pwm_get_caps); > > [...] > > @@ -53,12 +75,14 @@ enum { > > * @period: PWM period (in nanoseconds) > > * @duty_cycle: PWM duty cycle (in nanoseconds) > > * @polarity: PWM polarity > > + * @modebit: PWM mode bit > > * @enabled: PWM enabled status > > */ > > struct pwm_state { > > unsigned int period; > > unsigned int duty_cycle; > > enum pwm_polarity polarity; > > + unsigned long modebit; > > I fail to see the upside of storing the mode as 2^mode instead of a > plain enum pwm_mode. Given that struct pwm_state is visible for pwm > users a plain pwm_mode would at least be more intuitive. Agreed, we should have only the specific mode in the state and let the core deal with masks where necessary. Thierry
On Mon, Jan 07, 2019 at 11:10:40PM +0100, Uwe Kleine-König wrote: > Hello Claudiu, > > On Mon, Jan 07, 2019 at 09:30:55AM +0000, Claudiu.Beznea@microchip.com wrote: > > On 05.01.2019 23:05, Uwe Kleine-König wrote: > > > On Thu, Jan 03, 2019 at 01:29:44PM +0000, Claudiu.Beznea@microchip.com wrote: > > >> From: Claudiu Beznea <claudiu.beznea@microchip.com> > > >> > > >> Add basic PWM modes: normal and complementary. These modes should > > >> differentiate the single output PWM channels from two outputs PWM > > >> channels. These modes could be set as follow: > > >> 1. PWM channels with one output per channel: > > >> - normal mode > > >> 2. PWM channels with two outputs per channel: > > >> - normal mode > > >> - complementary mode > > >> Since users could use a PWM channel with two output as one output PWM > > >> channel, the PWM normal mode is allowed to be set for PWM channels with > > >> two outputs; in fact PWM normal mode should be supported by all PWMs. > > > > > > I still think that my suggestion that I sent in reply to your v5 using > > > .alt_duty_cycle and .alt_offset is the better one as it is more generic. > > > > I like it better my way, I explained myself why. > > I couldn't really follow your argument though. You seemed to acknowledge > that using .alt_duty_cycle and .alt_offset is more generic. Then you > wrote that the push-pull mode is hardware generated on Atmel with some > implementation details. IMHO these implementation details shouldn't be > part of the PWM API and atmel's .apply should look as follows: > > if (state->alt_duty_cycle == 0) { > > ... configure for normal mode ... > > } else if (state->duty_cycle == state->alt_duty_cycle && > state->alt_offset == state->period / 2) { > > ... configure for push pull mode ... > > } else if (state->duty_cycle + state->alt_duty_cycle == state->period && > state->alt_offset == state->duty_cycle) { > > ... configure for complementary mode ... > > } else { > return -EINVAL; > } > > If it turns out to be a common pattern, we can add helper functions à la > pwm_is_complementary_mode(state) and > pwm_set_complementary_mode(state, period, duty_cycle). This allows to > have a generic way to describe a wide range of wave forms in a uniform > way in the API (which is good) and each driver implements the parts of > this range that it can support. I think this is going to be the rule rather than the exception, so I'd expect we'll see these helpers used in pretty much all drivers that support more than just the normal mode. But I really don't see the point in having consumers jump through hoops to set one of the standard modes just to have the driver jump through more hoops to determine which mode was meant. There are only so many modes and I have never seen hardware that actually implements the kind of fine-grained control that would be possible with your proposal. The goal of an API is to abstract, but .alt_duty_cycle and .alt_offset would be an inversion of API abstraction. That is, we'd be requiring the drivers to abstract the inputs of the API, which is the wrong way around. > > > I don't repeat what I wrote there assuming you still remember or are > > > willing to look it up at > > > e.g. https://www.spinics.net/lists/linux-pwm/msg08174.html (in the 2nd half > > > of my mail). > > > > Yes, I remember it. > > I expected that, my words were more directed to Thierry than you. > > > > Also I think that if the capabilities function is the way forward adding > > > support to detect availability of polarity inversion should be > > > considered. > > > > Yep, why not. But it should be done in a different patch. It is not related > > to this series. > > Yes, given that polarity already exists, this would be a good > opportunity to introduce the capability function for that and only > afterwards add the new use case with modes. (But having said this, read > further as I think that this capability function is a bad idea.) I don't think we need to require this. The series is already big enough as it is and has been in the works for long enough. There's no harm in integrating polarity support into the capability function later on. > > > This would also be an opportunity to split the introduction > > > of the capabilities function and the introduction of complementary mode. > > > (But my personal preference would be to just let .apply fail when an > > > unsupported configuration is requested.) > > > > .apply fails when something wrong is requested. > > If my controller doesn't support a second output is it "wrong" to > request complementary mode? I'd say yes. So you have to catch that in > .apply anyhow and there is little benefit to be able to ask the > controller if it supports it beforehand. > > I don't have a provable statistic at hand, but my feeling is that quite > some users of the i2c frame work get it wrong to first check the > capabilities and only then try to use them. This is at least error prone > and harder to use than the apply function returning an error code. > And on the driver side the upside is to have all stuff related to which > wave form can be generated and which cannot is a single place. (Just > consider "inverted complementary mode". Theoretically this should work > if your controller supports complementary mode and inverted mode. If you > now have a driver for a controller that can do both, but not at the same > time, the separation gets ugly. OK, this is a constructed example, but > in my experience something like that happens earlier or later.) I think capabilities are useful in order to be able to implement fallbacks in consumer drivers. Sure the same thing could be implemented by trying to apply one state first and then downgrade and retry on failure and so on, but sometimes it's more convenient to know what's possible and determine what's the correct solution upfront. Thierry
Hello Thierry, On Wed, Feb 06, 2019 at 12:01:26AM +0100, Thierry Reding wrote: > On Mon, Jan 07, 2019 at 11:10:40PM +0100, Uwe Kleine-König wrote: > > On Mon, Jan 07, 2019 at 09:30:55AM +0000, Claudiu.Beznea@microchip.com wrote: > > > On 05.01.2019 23:05, Uwe Kleine-König wrote: > > > > On Thu, Jan 03, 2019 at 01:29:44PM +0000, Claudiu.Beznea@microchip.com wrote: > > > >> From: Claudiu Beznea <claudiu.beznea@microchip.com> > > > >> > > > >> Add basic PWM modes: normal and complementary. These modes should > > > >> differentiate the single output PWM channels from two outputs PWM > > > >> channels. These modes could be set as follow: > > > >> 1. PWM channels with one output per channel: > > > >> - normal mode > > > >> 2. PWM channels with two outputs per channel: > > > >> - normal mode > > > >> - complementary mode > > > >> Since users could use a PWM channel with two output as one output PWM > > > >> channel, the PWM normal mode is allowed to be set for PWM channels with > > > >> two outputs; in fact PWM normal mode should be supported by all PWMs. > > > > > > > > I still think that my suggestion that I sent in reply to your v5 using > > > > .alt_duty_cycle and .alt_offset is the better one as it is more generic. > > > > > > I like it better my way, I explained myself why. > > > > I couldn't really follow your argument though. You seemed to acknowledge > > that using .alt_duty_cycle and .alt_offset is more generic. Then you > > wrote that the push-pull mode is hardware generated on Atmel with some > > implementation details. IMHO these implementation details shouldn't be > > part of the PWM API and atmel's .apply should look as follows: > > > > if (state->alt_duty_cycle == 0) { > > > > ... configure for normal mode ... > > > > } else if (state->duty_cycle == state->alt_duty_cycle && > > state->alt_offset == state->period / 2) { > > > > ... configure for push pull mode ... > > > > } else if (state->duty_cycle + state->alt_duty_cycle == state->period && > > state->alt_offset == state->duty_cycle) { > > > > ... configure for complementary mode ... > > > > } else { > > return -EINVAL; > > } > > > > If it turns out to be a common pattern, we can add helper functions à la > > pwm_is_complementary_mode(state) and > > pwm_set_complementary_mode(state, period, duty_cycle). This allows to > > have a generic way to describe a wide range of wave forms in a uniform > > way in the API (which is good) and each driver implements the parts of > > this range that it can support. > > I think this is going to be the rule rather than the exception, so I'd > expect we'll see these helpers used in pretty much all drivers that > support more than just the normal mode. If you intended to contradict me here: You didn't. I have the same expectation. > But I really don't see the point in having consumers jump through hoops > to set one of the standard modes just to have the driver jump through > more hoops to determine which mode was meant. I think my approach is more natural and not more complicated at all. In all modes where this secondary output makes sense both outputs share the period length. In all modes both outputs have a falling and a raising edge each. Let's assume we support - normal mode (one output, secondary (if available) inactive) - complementary mode (secondary output is the inverse of primary output) - push-pull mode (primary output only does every second active phase, the secondy output does the ones that are skiped by the primary one) - complementary mode with deadtime (like above but there is a pause where both signals are inactive at the switch points, so the active phase of the secondary output is $deadtime_pre + $deadtime_post shorter than the primary output's inactive phase). To describe these modes we need with the approach suggested by Claudiu the following defines: enum mode { NORMAL, COMPLEMENTARY, PUSH_PULL PUSH_PULL_WITH_DEADTIME } struct pwm_state { unsigned int period; unsigned int duty_cycle; enum pwm_polarity polarity; bool enabled; enum mode mode; unsigned int deadtime_pre; unsigned int deadtime_post; } This has the following downsides: - The period in push-pull mode is somehow wrong because the signal combination repeats only every 2x $period ns. (I guess this is an implementation detail of the atmel hardware that leaks into the API here.) - There is redundancy in the description: { .period = U, .duty_cycle = V, .polarity = W, .enabled = true, .mode = PUSH_PULL, .deadtime_pre = DC, .deadtime_post = DC } is the same as { .period = U, .duty_cycle = V, .polarity = W, .enabled = true, .mode = PUSH_PULL_WITH_DEADTIME, .deadtime_pre = 0, .deadtime_post = 0 } . This is all more sane with my suggestion, and pwm_state is smaller with my approach. .period has always the same meaning and for a device that supports secondary mode .alt_offset and .alt_period always have the same semantic. (Opposed to .deadtime_X that only matter sometimes.) Also I don't see hoops for the implementing pwm driver: Assume it only supports normal and complementary mode. The difference is: - With Claudiu's approach: switch (state->mode) { case NORMAL: ... do normal ... break; case COMPLEMENTARY: ... do complementary ... break; default: return -ESOMETHING; break; } - with my approach: if (pwm_is_normal_mode(state) { ... do normal ... } else if (pwm_is_complementary_mode(state) { .. do complementary ... } else { return -ESOMETHING; } So I don't see a hoop apart from needed some pwm_is_XX_mode helpers in the core. Moreover for a flexible hardware that supports the full range (e.g. the hifive one where a driver is currently under discussion if only one pwm cell is implemented as I suggested in my review) the implementation is simpler with my approach it just looks as follows: configure(period, duty_cycle, alt_offset, alt_period) instead of switch (state->mode) { case NORMAL: configure(period, duty_cycle, 0, 0); break; case COMPLEMENTARY: configure(period, duty_cycle, duty_cycle, period - duty_cycle); break; case PUSH_PULL: configure(2 * period, duty_cycle, period, duty_cycle); break; case PUSH_PULL_WITH_DEADTIME: configure(2 * period, duty_cycle, period + deadtime_pre, duty_cycle - deadtime_pre - deadtime_post); break; default: return -ESOMETHING; break; } > There are only so many modes and I have never seen hardware that > actually implements the kind of fine-grained control that would be > possible with your proposal. That there is hardware that actually implements all the flexibility that is available is second-order. (But as said above, the hifive implementation can do it. And I think the ST implementation I saw some time ago can do it, too; I didn't recheck though.) The key here is to have a natural description of the intended waveform. And describing it using a mode and additional parameters depending on the mode is more complex than two additional parameters that can cover all waveforms. That not all drivers can implement all waveforms that consumers might request is common to both approaches. > The goal of an API is to abstract, but .alt_duty_cycle and .alt_offset > would be an inversion of API abstraction. No, the goal of an API is to give a way that is easy and natural to let consumers request all the stuff they might want. And if there is a single set of parameters that describes a broad subset of waveforms with parameters that you can measure in the wave form that is better than to separate waveforms into categories (modes) and implement each of these with their own parameter set. And then your categorisation might not match the capabilities of some hardware. Consider a device that can implement PUSH_PULL_WITH_DEADTIME but only with .deadtime_pre = .deadtime_post. That the API has to abstract is actually bad because it limits users. If a consumer wants push-pull mode with dead time and the hardware supports that but the API has abstracted that away, that's bad. If a consumer doesn't care if the configured duty cycle is already active when pwm_apply_state() returns or is only scheduled in the hardware for after the current period ends, this forces a delay on the consumer because the abstraction is that the configured wave form is already on the wire on return. Abstraction is necessary to cover different hardware implementations and allow users to handle these in a uniform way. So from a consumer's POV an abstraction that doesn't limit the accessible capabilities of the hardware is optimal. Using .alt_offset and .alt_period is an abstraction that limits less and so gives more possibilities to the consumers. > That is, we'd be requiring the drivers to abstract the inputs of the > API, which is the wrong way around. That is a normal "problem" for drivers. The driver gets a request and it has to determine if it can implement that. And if this is done using a comparison of .mode to known "good" values or by using a helper function that compares .alt_offset to .period is an implementation detail that doesn't matter much. > > > > I don't repeat what I wrote there assuming you still remember or are > > > > willing to look it up at > > > > e.g. https://www.spinics.net/lists/linux-pwm/msg08174.html (in the 2nd half > > > > of my mail). > > > > > > Yes, I remember it. > > > > I expected that, my words were more directed to Thierry than you. > > > > > > Also I think that if the capabilities function is the way forward adding > > > > support to detect availability of polarity inversion should be > > > > considered. > > > > > > Yep, why not. But it should be done in a different patch. It is not related > > > to this series. > > > > Yes, given that polarity already exists, this would be a good > > opportunity to introduce the capability function for that and only > > afterwards add the new use case with modes. (But having said this, read > > further as I think that this capability function is a bad idea.) > > I don't think we need to require this. The series is already big enough > as it is and has been in the works for long enough. There's no harm in > integrating polarity support into the capability function later on. I think "the series is long enough in the works" is not an argument to stop pointing out weaknesses. The harm that is done in not adding polarity support now is that it adds another thing to the todo list of things that are started a bit and need to be completed in the future. And the harm in adding underdone stuff to an API if there are known weaknesses is more work later. > > > > This would also be an opportunity to split the introduction > > > > of the capabilities function and the introduction of complementary mode. > > > > (But my personal preference would be to just let .apply fail when an > > > > unsupported configuration is requested.) > > > > > > .apply fails when something wrong is requested. > > > > If my controller doesn't support a second output is it "wrong" to > > request complementary mode? I'd say yes. So you have to catch that in > > .apply anyhow and there is little benefit to be able to ask the > > controller if it supports it beforehand. > > > > I don't have a provable statistic at hand, but my feeling is that quite > > some users of the i2c frame work get it wrong to first check the > > capabilities and only then try to use them. This is at least error prone > > and harder to use than the apply function returning an error code. > > And on the driver side the upside is to have all stuff related to which > > wave form can be generated and which cannot is a single place. (Just > > consider "inverted complementary mode". Theoretically this should work > > if your controller supports complementary mode and inverted mode. If you > > now have a driver for a controller that can do both, but not at the same > > time, the separation gets ugly. OK, this is a constructed example, but > > in my experience something like that happens earlier or later.) > > I think capabilities are useful in order to be able to implement > fallbacks in consumer drivers. Sure the same thing could be implemented > by trying to apply one state first and then downgrade and retry on > failure and so on, but sometimes it's more convenient to know what's > possible and determine what's the correct solution upfront. For me there is no big difference between: Oh, the driver cannot do inversed polarity, I have to come up with something else. and Oh, the driver can only implement periods that are powers of two of the input clk, I have to come up with something else. and Oh, I requested a duty cycle of 89 ns, but the hardware can only do 87 or 90 ns so I have to come up with something else. With capabilities you can only cover the first of these. With an approach similar to clk_round_rate you can easily cover all. Best regards Uwe
On 06.02.2019 00:49, Thierry Reding wrote: > On Sat, Jan 05, 2019 at 10:05:22PM +0100, Uwe Kleine-König wrote: >> Hello, >> >> On Thu, Jan 03, 2019 at 01:29:44PM +0000, Claudiu.Beznea@microchip.com wrote: >>> From: Claudiu Beznea <claudiu.beznea@microchip.com> >>> >>> Add basic PWM modes: normal and complementary. These modes should >>> differentiate the single output PWM channels from two outputs PWM >>> channels. These modes could be set as follow: >>> 1. PWM channels with one output per channel: >>> - normal mode >>> 2. PWM channels with two outputs per channel: >>> - normal mode >>> - complementary mode >>> Since users could use a PWM channel with two output as one output PWM >>> channel, the PWM normal mode is allowed to be set for PWM channels with >>> two outputs; in fact PWM normal mode should be supported by all PWMs. >> >> I still think that my suggestion that I sent in reply to your v5 using >> .alt_duty_cycle and .alt_offset is the better one as it is more generic. >> A word about that from Thierry before putting the mode into the pwm API >> would be great. >> >> I don't repeat what I wrote there assuming you still remember or are >> willing to look it up at >> e.g. https://www.spinics.net/lists/linux-pwm/msg08174.html (in the 2nd half >> of my mail). > > The problem I see with .alt_duty_cycle and .alt_offset is that they > provide too much flexibility in my opinion. There may be some variance > in how the values are computed for the different modes and that just > leads to additional code required in drivers to figure out what exactly > the user wanted. This is also my feeling. And I don't know how many controllers support having this. E.g. on the IP I'm working on (the Atmel one) the dead times have limited values, so that it cannot model all the possible delays b/w outputs. > > If we only provide the user with a means to say which mode they want to > operate the PWM in they can just tell us and the driver doesn't have to > guess which one was meant. > > It also makes validation easier. We can check for capabilites upfront by > just comparing a list of supported modes. With .alt_duty_cycle and > .alt_offset we'd actually need to run code to figure out whether or not > the given set of values corresponds to a supported configuration. > >> Also I think that if the capabilities function is the way forward adding >> support to detect availability of polarity inversion should be >> considered. This would also be an opportunity to split the introduction >> of the capabilities function and the introduction of complementary mode. >> (But my personal preference would be to just let .apply fail when an >> unsupported configuration is requested.) >> >>> +static int pwm_get_default_caps(struct pwm_caps *caps) >>> +{ >>> + static const struct pwm_caps default_caps = { >>> + .modes_msk = PWM_MODE_BIT(NORMAL), >>> + }; >>> + >>> + if (!caps) >>> + return -EINVAL; >>> + >>> + *caps = default_caps; >>> + >>> + return 0; >>> +} >>> + >>> +/** >>> + * pwm_get_caps() - get PWM capabilities of a PWM device >>> + * @pwm: PWM device to get the capabilities for >>> + * @caps: returned capabilities >>> + * >>> + * Returns: 0 on success or a negative error code on failure >>> + */ >>> +int pwm_get_caps(const struct pwm_device *pwm, struct pwm_caps *caps) >>> +{ >>> + if (!pwm || !caps) >>> + return -EINVAL; >>> + >>> + if (pwm->chip->ops->get_caps) >>> + return pwm->chip->ops->get_caps(pwm->chip, pwm, caps); >>> + >>> + return pwm_get_default_caps(caps); >> >> I'd drop pwm_get_default_caps (unless you introduce some more callers >> later) and fold its implementation into pwm_get_caps. > > I think Claudiu and I may have talked past one another here. What I was > suggesting was to make pwm_get_default_caps() an exported symbol so that > drivers could use it if they didn't have extra capabilities. However, it > seems like just handling that in pwm_get_caps() if ops->get_caps == NULL > works just as well. I don't think it needs to be a different function in > this case, but I don't mind if it is. > >>> +} >>> +EXPORT_SYMBOL_GPL(pwm_get_caps); >>> [...] >>> @@ -53,12 +75,14 @@ enum { >>> * @period: PWM period (in nanoseconds) >>> * @duty_cycle: PWM duty cycle (in nanoseconds) >>> * @polarity: PWM polarity >>> + * @modebit: PWM mode bit >>> * @enabled: PWM enabled status >>> */ >>> struct pwm_state { >>> unsigned int period; >>> unsigned int duty_cycle; >>> enum pwm_polarity polarity; >>> + unsigned long modebit; >> >> I fail to see the upside of storing the mode as 2^mode instead of a >> plain enum pwm_mode. Given that struct pwm_state is visible for pwm >> users a plain pwm_mode would at least be more intuitive. > > Agreed, we should have only the specific mode in the state and let the > core deal with masks where necessary. Good, I will rework this. Thank you, Claudiu Beznea > > Thierry > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On 06.02.2019 10:24, Uwe Kleine-König wrote: > Hello Thierry, > > On Wed, Feb 06, 2019 at 12:01:26AM +0100, Thierry Reding wrote: >> On Mon, Jan 07, 2019 at 11:10:40PM +0100, Uwe Kleine-König wrote: >>> On Mon, Jan 07, 2019 at 09:30:55AM +0000, Claudiu.Beznea@microchip.com wrote: >>>> On 05.01.2019 23:05, Uwe Kleine-König wrote: >>>>> On Thu, Jan 03, 2019 at 01:29:44PM +0000, Claudiu.Beznea@microchip.com wrote: >>>>>> From: Claudiu Beznea <claudiu.beznea@microchip.com> >>>>>> >>>>>> Add basic PWM modes: normal and complementary. These modes should >>>>>> differentiate the single output PWM channels from two outputs PWM >>>>>> channels. These modes could be set as follow: >>>>>> 1. PWM channels with one output per channel: >>>>>> - normal mode >>>>>> 2. PWM channels with two outputs per channel: >>>>>> - normal mode >>>>>> - complementary mode >>>>>> Since users could use a PWM channel with two output as one output PWM >>>>>> channel, the PWM normal mode is allowed to be set for PWM channels with >>>>>> two outputs; in fact PWM normal mode should be supported by all PWMs. >>>>> >>>>> I still think that my suggestion that I sent in reply to your v5 using >>>>> .alt_duty_cycle and .alt_offset is the better one as it is more generic. >>>> >>>> I like it better my way, I explained myself why. >>> >>> I couldn't really follow your argument though. You seemed to acknowledge >>> that using .alt_duty_cycle and .alt_offset is more generic. Then you >>> wrote that the push-pull mode is hardware generated on Atmel with some >>> implementation details. IMHO these implementation details shouldn't be >>> part of the PWM API and atmel's .apply should look as follows: >>> >>> if (state->alt_duty_cycle == 0) { >>> >>> ... configure for normal mode ... >>> >>> } else if (state->duty_cycle == state->alt_duty_cycle && >>> state->alt_offset == state->period / 2) { >>> >>> ... configure for push pull mode ... >>> >>> } else if (state->duty_cycle + state->alt_duty_cycle == state->period && >>> state->alt_offset == state->duty_cycle) { >>> >>> ... configure for complementary mode ... >>> >>> } else { >>> return -EINVAL; >>> } >>> >>> If it turns out to be a common pattern, we can add helper functions à la >>> pwm_is_complementary_mode(state) and >>> pwm_set_complementary_mode(state, period, duty_cycle). This allows to >>> have a generic way to describe a wide range of wave forms in a uniform >>> way in the API (which is good) and each driver implements the parts of >>> this range that it can support. >> >> I think this is going to be the rule rather than the exception, so I'd >> expect we'll see these helpers used in pretty much all drivers that >> support more than just the normal mode. > > If you intended to contradict me here: You didn't. I have the same > expectation. > >> But I really don't see the point in having consumers jump through hoops >> to set one of the standard modes just to have the driver jump through >> more hoops to determine which mode was meant. > > I think my approach is more natural and not more complicated at all. In > all modes where this secondary output makes sense both outputs share the > period length. In all modes both outputs have a falling and a raising > edge each. Let's assume we support > > - normal mode (one output, secondary (if available) inactive) > - complementary mode (secondary output is the inverse of primary > output) > - push-pull mode (primary output only does every second active phase, > the secondy output does the ones that are skiped by the primary one) > - complementary mode with deadtime (like above but there is a pause > where both signals are inactive at the switch points, so the active > phase of the secondary output is $deadtime_pre + $deadtime_post > shorter than the primary output's inactive phase). > > To describe these modes we need with the approach suggested by Claudiu > the following defines: > > enum mode { > NORMAL, > COMPLEMENTARY, > PUSH_PULL > PUSH_PULL_WITH_DEADTIME I see push-pull mode as a particular mode of complementary mode with dead-times equal to a half of a period. I don't get the meaning of push-pull with dead-times, there is already there a deadtime pre, post value equal with 1/2 of period. > } > > struct pwm_state { > unsigned int period; > unsigned int duty_cycle; > enum pwm_polarity polarity; > bool enabled; > enum mode mode; > unsigned int deadtime_pre; > unsigned int deadtime_post; > } > > This has the following downsides: > > - The period in push-pull mode is somehow wrong because the signal > combination repeats only every 2x $period ns. (I guess this is an > implementation detail of the atmel hardware that leaks into the API > here.) As far as I'm concern of the PWM push-pull mode it is a specific PWM working mode, not related to Atmel, which can be used to drive half-bridge converters. > > - There is redundancy in the description: > > { .period = U, .duty_cycle = V, .polarity = W, .enabled = true, .mode = PUSH_PULL, .deadtime_pre = DC, .deadtime_post = DC } > > is the same as > > { .period = U, .duty_cycle = V, .polarity = W, .enabled = true, .mode = PUSH_PULL_WITH_DEADTIME, .deadtime_pre = 0, .deadtime_post = 0 } > > . > > This is all more sane with my suggestion, and pwm_state is smaller with > my approach. .period has always the same meaning and for a device that > supports secondary mode .alt_offset and .alt_period always have the same > semantic. (Opposed to .deadtime_X that only matter sometimes.) > > Also I don't see hoops for the implementing pwm driver: Assume it only > supports normal and complementary mode. The difference is: > > - With Claudiu's approach: > > switch (state->mode) { > case NORMAL: > ... do normal ... > break; > case COMPLEMENTARY: > ... do complementary ... > break; > default: > return -ESOMETHING; > break; > } > > - with my approach: > > if (pwm_is_normal_mode(state) { > ... do normal ... > } else if (pwm_is_complementary_mode(state) { > .. do complementary ... > } else { > return -ESOMETHING; > } > > So I don't see a hoop apart from needed some pwm_is_XX_mode helpers in > the core. Moreover for a flexible hardware that supports the full range > (e.g. the hifive one where a driver is currently under discussion if > only one pwm cell is implemented as I suggested in my review) the > implementation is simpler with my approach it just looks as follows: > > configure(period, duty_cycle, alt_offset, alt_period) > > instead of > > switch (state->mode) { > case NORMAL: > configure(period, duty_cycle, 0, 0); > break; > case COMPLEMENTARY: > configure(period, duty_cycle, duty_cycle, period - duty_cycle); > break; > case PUSH_PULL: > configure(2 * period, duty_cycle, period, duty_cycle); > break; > case PUSH_PULL_WITH_DEADTIME: > configure(2 * period, duty_cycle, period + deadtime_pre, > duty_cycle - deadtime_pre - deadtime_post); > break; > default: > return -ESOMETHING; > break; > } > >> There are only so many modes and I have never seen hardware that >> actually implements the kind of fine-grained control that would be >> possible with your proposal. > > That there is hardware that actually implements all the flexibility that > is available is second-order. (But as said above, the hifive > implementation can do it. And I think the ST implementation I saw some > time ago can do it, too; I didn't recheck though.) The key here is to > have a natural description of the intended waveform. And describing it > using a mode and additional parameters depending on the mode is more > complex than two additional parameters that can cover all waveforms. > > That not all drivers can implement all waveforms that consumers might > request is common to both approaches. > >> The goal of an API is to abstract, but .alt_duty_cycle and .alt_offset >> would be an inversion of API abstraction. > > No, the goal of an API is to give a way that is easy and natural to let > consumers request all the stuff they might want. And if there is a > single set of parameters that describes a broad subset of waveforms with > parameters that you can measure in the wave form that is better than to > separate waveforms into categories (modes) and implement each of these > with their own parameter set. And then your categorisation might not > match the capabilities of some hardware. Consider a device that can > implement PUSH_PULL_WITH_DEADTIME but only with .deadtime_pre = > .deadtime_post. > > That the API has to abstract is actually bad because it limits users. > If a consumer wants push-pull mode with dead time and the hardware > supports that but the API has abstracted that away, that's bad. > If a consumer doesn't care if the configured duty cycle is already > active when pwm_apply_state() returns or is only scheduled in the > hardware for after the current period ends, this forces a delay on the > consumer because the abstraction is that the configured wave form is > already on the wire on return. > > Abstraction is necessary to cover different hardware implementations and > allow users to handle these in a uniform way. So from a consumer's POV > an abstraction that doesn't limit the accessible capabilities of the > hardware is optimal. > > Using .alt_offset and .alt_period is an abstraction that limits less and > so gives more possibilities to the consumers. > >> That is, we'd be requiring the drivers to abstract the inputs of the >> API, which is the wrong way around. > > That is a normal "problem" for drivers. The driver gets a request and it > has to determine if it can implement that. And if this is done using a > comparison of .mode to known "good" values or by using a helper function > that compares .alt_offset to .period is an implementation detail that > doesn't matter much. > >>>>> I don't repeat what I wrote there assuming you still remember or are >>>>> willing to look it up at >>>>> e.g. https://www.spinics.net/lists/linux-pwm/msg08174.html (in the 2nd half >>>>> of my mail). >>>> >>>> Yes, I remember it. >>> >>> I expected that, my words were more directed to Thierry than you. >>> >>>>> Also I think that if the capabilities function is the way forward adding >>>>> support to detect availability of polarity inversion should be >>>>> considered. >>>> >>>> Yep, why not. But it should be done in a different patch. It is not related >>>> to this series. >>> >>> Yes, given that polarity already exists, this would be a good >>> opportunity to introduce the capability function for that and only >>> afterwards add the new use case with modes. (But having said this, read >>> further as I think that this capability function is a bad idea.) >> >> I don't think we need to require this. The series is already big enough >> as it is and has been in the works for long enough. There's no harm in >> integrating polarity support into the capability function later on. > > I think "the series is long enough in the works" is not an argument to > stop pointing out weaknesses. The harm that is done in not adding > polarity support now is that it adds another thing to the todo list of > things that are started a bit and need to be completed in the future. > > And the harm in adding underdone stuff to an API if there are known > weaknesses is more work later. > >>>>> This would also be an opportunity to split the introduction >>>>> of the capabilities function and the introduction of complementary mode. >>>>> (But my personal preference would be to just let .apply fail when an >>>>> unsupported configuration is requested.) >>>> >>>> .apply fails when something wrong is requested. >>> >>> If my controller doesn't support a second output is it "wrong" to >>> request complementary mode? I'd say yes. So you have to catch that in >>> .apply anyhow and there is little benefit to be able to ask the >>> controller if it supports it beforehand. >>> >>> I don't have a provable statistic at hand, but my feeling is that quite >>> some users of the i2c frame work get it wrong to first check the >>> capabilities and only then try to use them. This is at least error prone >>> and harder to use than the apply function returning an error code. >>> And on the driver side the upside is to have all stuff related to which >>> wave form can be generated and which cannot is a single place. (Just >>> consider "inverted complementary mode". Theoretically this should work >>> if your controller supports complementary mode and inverted mode. If you >>> now have a driver for a controller that can do both, but not at the same >>> time, the separation gets ugly. OK, this is a constructed example, but >>> in my experience something like that happens earlier or later.) >> >> I think capabilities are useful in order to be able to implement >> fallbacks in consumer drivers. Sure the same thing could be implemented >> by trying to apply one state first and then downgrade and retry on >> failure and so on, but sometimes it's more convenient to know what's >> possible and determine what's the correct solution upfront. > > For me there is no big difference between: > > Oh, the driver cannot do inversed polarity, I have to come up > with something else. > > and > > Oh, the driver can only implement periods that are powers of two > of the input clk, I have to come up with something else. > > and > > Oh, I requested a duty cycle of 89 ns, but the hardware can only > do 87 or 90 ns so I have to come up with something else. > > With capabilities you can only cover the first of these. With an > approach similar to clk_round_rate you can easily cover all. > > Best regards > Uwe >
Hello, On Wed, Feb 13, 2019 at 03:38:46PM +0000, Claudiu.Beznea@microchip.com wrote: > On 06.02.2019 10:24, Uwe Kleine-König wrote: > > On Wed, Feb 06, 2019 at 12:01:26AM +0100, Thierry Reding wrote: > >> But I really don't see the point in having consumers jump through hoops > >> to set one of the standard modes just to have the driver jump through > >> more hoops to determine which mode was meant. > > > > I think my approach is more natural and not more complicated at all. In > > all modes where this secondary output makes sense both outputs share the > > period length. In all modes both outputs have a falling and a raising > > edge each. Let's assume we support > > > > - normal mode (one output, secondary (if available) inactive) > > - complementary mode (secondary output is the inverse of primary > > output) > > - push-pull mode (primary output only does every second active phase, > > the secondy output does the ones that are skiped by the primary one) > > - complementary mode with deadtime (like above but there is a pause > > where both signals are inactive at the switch points, so the active > > phase of the secondary output is $deadtime_pre + $deadtime_post > > shorter than the primary output's inactive phase). > > > > To describe these modes we need with the approach suggested by Claudiu > > the following defines: > > > > enum mode { > > NORMAL, > > COMPLEMENTARY, > > PUSH_PULL > > PUSH_PULL_WITH_DEADTIME Here I made a mistake, I should have written about COMPLEMENTARY_WITH_DEADTIME, not PUSH_PULL_WITH_DEADTIME. The argumentation is analogous however. PUSH_PULL_WITH_DEADTIME is completely unintuitive when thinking about the needed parameters, I'll skip talking about this. > I see push-pull mode as a particular mode of complementary mode with > dead-times equal to a half of a period. <pedantic>It's not half a period that you need to use as dead-time, but period/2 - duty_cycle.</pedantic> This means that we could model "complementary" and "push pull" (and even "push pull with deadtime") if we only had "complementary with deadtime". So the latter seems to be the universal we should implement, right? (Otherwise a driver for a hardware that is flexible enough to actually do it has to implement three or so different ways to yield requested waveforms when a single one would be enough.) The parameters for that are: - period length - duty_cycle - pre_deadtime - post_deadtime . The expressiveness is identical to my suggestion (with alt_duty_cycle and alt_offset), just that this uses more artificial and unintuitive characteristics of the wave to describe it. (An indication is that you got it wrong above, see my pedantic note.) I'd say only abstract using "modes" if they are really orthonal which you seem to agree isn't the case here. In fact we only need a single mode that driver authors need to grasp and if there are two or more different ways to describe the same waves we're only make it harder for them. The argument "But not all hardware has the flexibility to implement all wave forms that are decribable with any of these abstractions." isn't really a problem. We already have now the situation with only .period, .duty_cycle and .polarity that not all drivers can implement everything. So there has to be a mechanism to handle inability to implement a given request anyhow. > I don't get the meaning of push-pull with dead-times, there is already > there a deadtime pre, post value equal with 1/2 of period. FTR: in my argumentation COMPLEMENTARY is just what you defined it to be. I.e. it doesn't have deadtimes. But as shown above this doesn't really matter. Both COMPLEMENTARY with deadtimes and my suggestion have the same expressive power, just use different characteristics to describe a wave form. To summarize, requesting the following wave: ______ ______ secondary __________/ \______________/ \____ __ __ primary __/ \__________________/ \________________ ^ ^ A `--' .duty_cycle B `-------' .alt_offset C `------' .alt_duty_cycle D `----' .pre_dead_time E `------' .post_dead_time You need apart from the period length which is needed for all: - With my suggestion you need to pass A, B and C; and - with COMPLEMENTARY and deadtimes you need to provide A, D and E. Isn't A, B and C the most intuitive set, given that when describing only only the primary output you need A and for the secondary output you need only C? Also note that for PUSH_PULL with deadtime the deadtimes might be negative. The boundary checks for my approach are: 0 <= .duty_cycle <= .period (as we already have today) 0 <= .alt_duty_cycle <= .period (which is analogous to the above line) 0 <= .alt_offset < .period With COMPLEMENTARY and deadtimes it is harder, as there are conditions that have to speak about both .pre_dead_time and .post_dead_time. > > struct pwm_state { > > unsigned int period; > > unsigned int duty_cycle; > > enum pwm_polarity polarity; > > bool enabled; > > enum mode mode; > > unsigned int deadtime_pre; > > unsigned int deadtime_post; > > } > > > > This has the following downsides: > > > > - The period in push-pull mode is somehow wrong because the signal > > combination repeats only every 2x $period ns. (I guess this is an > > implementation detail of the atmel hardware that leaks into the API > > here.) > > As far as I'm concern of the PWM push-pull mode it is a specific PWM > working mode, not related to Atmel, which can be used to drive half-bridge > converters. This is not what I meant. The point I wanted to make here is, that I would consider it more natural to define the period length of the following wave form pair as the time between the two carets, not half of it as you suggested in your patch. __ __ _____________/ \__________________/ \_____ __ __ __/ \__________________/ \________________ ^ ^ That's because then for all wave forms the period of both outputs matches the period variable. Best regards Uwe
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 1581f6ab1b1f..eb444ee8d486 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -249,6 +249,88 @@ static bool pwm_ops_check(const struct pwm_ops *ops) return false; } +static int pwm_get_default_caps(struct pwm_caps *caps) +{ + static const struct pwm_caps default_caps = { + .modes_msk = PWM_MODE_BIT(NORMAL), + }; + + if (!caps) + return -EINVAL; + + *caps = default_caps; + + return 0; +} + +/** + * pwm_get_caps() - get PWM capabilities of a PWM device + * @pwm: PWM device to get the capabilities for + * @caps: returned capabilities + * + * Returns: 0 on success or a negative error code on failure + */ +int pwm_get_caps(const struct pwm_device *pwm, struct pwm_caps *caps) +{ + if (!pwm || !caps) + return -EINVAL; + + if (pwm->chip->ops->get_caps) + return pwm->chip->ops->get_caps(pwm->chip, pwm, caps); + + return pwm_get_default_caps(caps); +} +EXPORT_SYMBOL_GPL(pwm_get_caps); + +/** + * pwm_get_default_modebit() - get the default mode for PWM (as a bit mask) + * @pwm: PWM device to get the default mode for + * + * Returns: the default PWM mode (as a bit mask) for PWM device + */ +unsigned long pwm_get_default_modebit(const struct pwm_device *pwm) +{ + struct pwm_caps caps; + + if (pwm_get_caps(pwm, &caps)) + return PWM_MODE_BIT(NORMAL); + + return BIT(ffs(caps.modes_msk) - 1); +} +EXPORT_SYMBOL_GPL(pwm_get_default_modebit); + +/** + * pwm_supports_mode() - check if PWM mode is supported by PWM device + * @pwm: PWM device + * @modebit: PWM mode bit mask to be checked (see PWM_MODE_BIT()) + * + * Returns: true if PWM mode is supported, false otherwise + */ +bool pwm_supports_mode(const struct pwm_device *pwm, unsigned long modebit) +{ + struct pwm_caps caps; + + if (!pwm || !modebit) + return false; + + if (hweight_long(modebit) != 1 || ffs(modebit) - 1 >= PWM_MODE_CNT) + return false; + + if (pwm_get_caps(pwm, &caps)) + return false; + + return !!(caps.modes_msk & modebit); +} +EXPORT_SYMBOL_GPL(pwm_supports_mode); + +const char *pwm_get_mode_name(unsigned long modebit) +{ + if (modebit == PWM_MODE_BIT(COMPLEMENTARY)) + return "complementary"; + + return "normal"; +} + /** * pwmchip_add_with_polarity() - register a new PWM chip * @chip: the PWM chip to add @@ -294,6 +376,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip, pwm->pwm = chip->base + i; pwm->hwpwm = i; pwm->state.polarity = polarity; + pwm->state.modebit = pwm_get_default_modebit(pwm); if (chip->ops->get_state) chip->ops->get_state(chip, pwm, &pwm->state); @@ -469,7 +552,8 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) int err; if (!pwm || !state || !state->period || - state->duty_cycle > state->period) + state->duty_cycle > state->period || + !pwm_supports_mode(pwm, state->modebit)) return -EINVAL; if (!memcmp(state, &pwm->state, sizeof(*state))) @@ -530,6 +614,8 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) pwm->state.enabled = state->enabled; } + + pwm->state.modebit = state->modebit; } return 0; @@ -579,6 +665,8 @@ int pwm_adjust_config(struct pwm_device *pwm) pwm_get_args(pwm, &pargs); pwm_get_state(pwm, &state); + state.modebit = pwm_get_default_modebit(pwm); + /* * If the current period is zero it means that either the PWM driver * does not support initial state retrieval or the PWM has not yet @@ -999,6 +1087,7 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s) seq_printf(s, " duty: %u ns", state.duty_cycle); seq_printf(s, " polarity: %s", state.polarity ? "inverse" : "normal"); + seq_printf(s, " mode: %s", pwm_get_mode_name(state.modebit)); seq_puts(s, "\n"); } diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c index ceb233dd6048..7865fbafbeb4 100644 --- a/drivers/pwm/sysfs.c +++ b/drivers/pwm/sysfs.c @@ -223,11 +223,71 @@ static ssize_t capture_show(struct device *child, return sprintf(buf, "%u %u\n", result.period, result.duty_cycle); } +static ssize_t mode_show(struct device *child, + struct device_attribute *attr, + char *buf) +{ + struct pwm_device *pwm = child_to_pwm_device(child); + struct pwm_state state; + unsigned long modebit; + enum pwm_mode mode; + int len = 0; + + pwm_get_state(pwm, &state); + + for (mode = PWM_MODE_NORMAL; mode < PWM_MODE_CNT; mode++) { + modebit = BIT(mode); + if (pwm_supports_mode(pwm, modebit)) { + if (state.modebit == modebit) + len += scnprintf(buf + len, + PAGE_SIZE - len, "[%s] ", + pwm_get_mode_name(modebit)); + else + len += scnprintf(buf + len, + PAGE_SIZE - len, "%s ", + pwm_get_mode_name(modebit)); + } + } + + len += scnprintf(buf + len, PAGE_SIZE - len, "\n"); + return len; +} + +static ssize_t mode_store(struct device *child, + struct device_attribute *attr, + const char *buf, size_t size) +{ + struct pwm_export *export = child_to_pwm_export(child); + struct pwm_device *pwm = export->pwm; + struct pwm_state state; + unsigned long modebit; + enum pwm_mode mode; + int ret; + + for (mode = PWM_MODE_NORMAL; mode < PWM_MODE_CNT; mode++) { + modebit = BIT(mode); + if (sysfs_streq(buf, pwm_get_mode_name(modebit))) + break; + } + + if (mode == PWM_MODE_CNT) + return -EINVAL; + + mutex_lock(&export->lock); + pwm_get_state(pwm, &state); + state.modebit = modebit; + ret = pwm_apply_state(pwm, &state); + mutex_unlock(&export->lock); + + return ret ? : size; +} + static DEVICE_ATTR_RW(period); static DEVICE_ATTR_RW(duty_cycle); static DEVICE_ATTR_RW(enable); static DEVICE_ATTR_RW(polarity); static DEVICE_ATTR_RO(capture); +static DEVICE_ATTR_RW(mode); static struct attribute *pwm_attrs[] = { &dev_attr_period.attr, @@ -235,6 +295,7 @@ static struct attribute *pwm_attrs[] = { &dev_attr_enable.attr, &dev_attr_polarity.attr, &dev_attr_capture.attr, + &dev_attr_mode.attr, NULL }; ATTRIBUTE_GROUPS(pwm); diff --git a/include/linux/pwm.h b/include/linux/pwm.h index d5199b507d79..3d89343bc405 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -26,6 +26,28 @@ enum pwm_polarity { }; /** + * PWM modes capabilities + * @PWM_MODE_NORMAL: PWM has one output + * @PWM_MODE_COMPLEMENTARY: PWM has 2 outputs with opposite polarities + * @PWM_MODE_CNT: PWM modes count + */ +enum pwm_mode { + PWM_MODE_NORMAL, + PWM_MODE_COMPLEMENTARY, + PWM_MODE_CNT, +}; + +#define PWM_MODE_BIT(name) BIT(PWM_MODE_##name) + +/** + * struct pwm_caps - PWM capabilities + * @modes_msk: bitmask of supported modes (see PWM_MODE_*) + */ +struct pwm_caps { + unsigned long modes_msk; +}; + +/** * struct pwm_args - board-dependent PWM arguments * @period: reference period * @polarity: reference polarity @@ -53,12 +75,14 @@ enum { * @period: PWM period (in nanoseconds) * @duty_cycle: PWM duty cycle (in nanoseconds) * @polarity: PWM polarity + * @modebit: PWM mode bit * @enabled: PWM enabled status */ struct pwm_state { unsigned int period; unsigned int duty_cycle; enum pwm_polarity polarity; + unsigned long modebit; bool enabled; }; @@ -151,39 +175,6 @@ static inline void pwm_get_args(const struct pwm_device *pwm, } /** - * pwm_init_state() - prepare a new state to be applied with pwm_apply_state() - * @pwm: PWM device - * @state: state to fill with the prepared PWM state - * - * This functions prepares a state that can later be tweaked and applied - * to the PWM device with pwm_apply_state(). This is a convenient function - * that first retrieves the current PWM state and the replaces the period - * and polarity fields with the reference values defined in pwm->args. - * Once the function returns, you can adjust the ->enabled and ->duty_cycle - * fields according to your needs before calling pwm_apply_state(). - * - * ->duty_cycle is initially set to zero to avoid cases where the current - * ->duty_cycle value exceed the pwm_args->period one, which would trigger - * an error if the user calls pwm_apply_state() without adjusting ->duty_cycle - * first. - */ -static inline void pwm_init_state(const struct pwm_device *pwm, - struct pwm_state *state) -{ - struct pwm_args args; - - /* First get the current state. */ - pwm_get_state(pwm, state); - - /* Then fill it with the reference config */ - pwm_get_args(pwm, &args); - - state->period = args.period; - state->polarity = args.polarity; - state->duty_cycle = 0; -} - -/** * pwm_get_relative_duty_cycle() - Get a relative duty cycle value * @state: PWM state to extract the duty cycle from * @scale: target scale of the relative duty cycle @@ -254,6 +245,7 @@ pwm_set_relative_duty_cycle(struct pwm_state *state, unsigned int duty_cycle, * @get_state: get the current PWM state. This function is only * called once per PWM device when the PWM chip is * registered. + * @get_caps: get PWM capabilities. * @dbg_show: optional routine to show contents in debugfs * @owner: helps prevent removal of modules exporting active PWMs */ @@ -272,6 +264,8 @@ struct pwm_ops { struct pwm_state *state); void (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_state *state); + int (*get_caps)(const struct pwm_chip *chip, + const struct pwm_device *pwm, struct pwm_caps *caps); #ifdef CONFIG_DEBUG_FS void (*dbg_show)(struct pwm_chip *chip, struct seq_file *s); #endif @@ -402,6 +396,10 @@ struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip, unsigned int index, const char *label); +int pwm_get_caps(const struct pwm_device *pwm, struct pwm_caps *caps); +bool pwm_supports_mode(const struct pwm_device *pwm, unsigned long modebit); +unsigned long pwm_get_default_modebit(const struct pwm_device *pwm); +const char *pwm_get_mode_name(unsigned long modebit); struct pwm_device *of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args); @@ -488,6 +486,11 @@ static inline struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip, return ERR_PTR(-ENODEV); } +static inline unsigned long pwm_get_default_modebit(const struct pwm_device *pwm) +{ + return 0; +} + static inline struct pwm_device *pwm_get(struct device *dev, const char *consumer) { @@ -522,6 +525,40 @@ static inline void devm_pwm_put(struct device *dev, struct pwm_device *pwm) } #endif +/** + * pwm_init_state() - prepare a new state to be applied with pwm_apply_state() + * @pwm: PWM device + * @state: state to fill with the prepared PWM state + * + * This functions prepares a state that can later be tweaked and applied + * to the PWM device with pwm_apply_state(). This is a convenient function + * that first retrieves the current PWM state and the replaces the period + * and polarity fields with the reference values defined in pwm->args. + * Once the function returns, you can adjust the ->enabled and ->duty_cycle + * fields according to your needs before calling pwm_apply_state(). + * + * ->duty_cycle is initially set to zero to avoid cases where the current + * ->duty_cycle value exceed the pwm_args->period one, which would trigger + * an error if the user calls pwm_apply_state() without adjusting ->duty_cycle + * first. + */ +static inline void pwm_init_state(const struct pwm_device *pwm, + struct pwm_state *state) +{ + struct pwm_args args; + + /* First get the current state. */ + pwm_get_state(pwm, state); + + /* Then fill it with the reference config */ + pwm_get_args(pwm, &args); + + state->period = args.period; + state->polarity = args.polarity; + state->duty_cycle = 0; + state->modebit = pwm_get_default_modebit(pwm); +} + static inline void pwm_apply_args(struct pwm_device *pwm) { struct pwm_state state = { }; @@ -550,6 +587,7 @@ static inline void pwm_apply_args(struct pwm_device *pwm) state.enabled = false; state.polarity = pwm->args.polarity; state.period = pwm->args.period; + state.modebit = pwm_get_default_modebit(pwm); pwm_apply_state(pwm, &state); }