Re: [PATCH v8 1/6] pwm: extend PWM framework with PWM modes

[Uwe Kleine-König]

On Tue, Jan 08, 2019 at 09:21:34AM +0000, Claudiu.Beznea@xxxxxxxxxxxxx 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@xxxxxxxxxxxxx wrote:
> >> On 05.01.2019 23:05, Uwe Kleine-König wrote:
> >>> 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.
> > 
> > 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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |