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

[Claudiu.Beznea]

On 09.01.2019 00:08, Uwe Kleine-KÃnig wrote:
> 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?

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
>