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

[<Claudiu.Beznea@microchip.com>]

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
>