On 05.01.2019 23:05, Uwe Kleine-König wrote: > Hello, > > On Thu, Jan 03, 2019 at 01:29:44PM +0000, Claudiu.Beznea@xxxxxxxxxxxxx wrote: >> From: Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx> >> >> 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 >
