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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 7 Jan 2019 09:30:55 +0000
From:   <Claudiu.Beznea@...rochip.com>
To:     <u.kleine-koenig@...gutronix.de>, <thierry.reding@...il.com>
CC:     <corbet@....net>, <Nicolas.Ferre@...rochip.com>,
        <alexandre.belloni@...tlin.com>, <Ludovic.Desroches@...rochip.com>,
        <linux-pwm@...r.kernel.org>, <linux-doc@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v8 1/6] pwm: extend PWM framework with PWM modes



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