linux-kernel - Re: [PATCH 4/4] pwm: omap-dmtimer: Implement .apply callback

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:   Tue, 25 Feb 2020 10:31:45 +0530
From:   Lokesh Vutla <lokeshvutla@...com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
CC:     Thierry Reding <thierry.reding@...il.com>,
        Tony Lindgren <tony@...mide.com>,
        Linux OMAP Mailing List <linux-omap@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-pwm@...r.kernel.org>,
        Sekhar Nori <nsekhar@...com>
Subject: Re: [PATCH 4/4] pwm: omap-dmtimer: Implement .apply callback

Hi Uwe,

On 24/02/20 2:37 PM, Uwe Kleine-König wrote:
> On Mon, Feb 24, 2020 at 10:51:35AM +0530, Lokesh Vutla wrote:
>> Implement .apply callback and drop the legacy callbacks(enable, disable,
>> config, set_polarity).
>>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@...com>
>> ---
>>  drivers/pwm/pwm-omap-dmtimer.c | 141 +++++++++++++++++++--------------
>>  1 file changed, 80 insertions(+), 61 deletions(-)
>>

[..snip..]

>> -static int pwm_omap_dmtimer_set_polarity(struct pwm_chip *chip,
>> -					 struct pwm_device *pwm,
>> -					 enum pwm_polarity polarity)
>> +/**
>> + * pwm_omap_dmtimer_apply() - Changes the state of the pwm omap dm timer.
>> + * @chip:	Pointer to PWM controller
>> + * @pwm:	Pointer to PWM channel
>> + * @state:	New sate to apply
>> + *
>> + * Return 0 if successfully changed the state else appropriate error.
>> + */
>> +static int pwm_omap_dmtimer_apply(struct pwm_chip *chip,
>> +				  struct pwm_device *pwm,
>> +				  const struct pwm_state *state)
>>  {
>>  	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
>> +	int ret = 0;
>>  
>> -	/*
>> -	 * PWM core will not call set_polarity while PWM is enabled so it's
>> -	 * safe to reconfigure the timer here without stopping it first.
>> -	 */
>>  	mutex_lock(&omap->mutex);
>> -	omap->pdata->set_pwm(omap->dm_timer,
>> -			     polarity == PWM_POLARITY_INVERSED,
>> -			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
>> +
>> +	if (pwm_is_enabled(pwm) && !state->enabled) {
> 
> In my book calling PWM API functions (designed for PWM consumers) is not
> so nice. I would prefer you checking the hardware registers or cache the
> state locally instead of relying on the core here.

.start and .stop apis does read the hardware registers and check the state
before making any changes. Do you want to drop off the pwm_is_enabled(pwm) check
here?

> 
> It would be great to have a general description at the top of the driver
> (like for example drivers/pwm/pwm-sifive.c) that answers things like:
> 
>  - Does calling .stop completes the currently running period (it
>    should)?

Existing driver implementation abruptly stops the cycle. I can make changes such
that it completes the currently running period.

>  - Does changing polarity, duty_cycle and period complete the running
>    period?

- Polarity can be changed only when the pwm is not running. Ill add extra guards
to reflect this behavior.
- Changing duty_cycle and period does complete the running period and new values
gets reflected in next cycle.

>  - How does the hardware behave on disable? (i.e. does it output the
>    state the pin is at in that moment? Does it go High-Z?)

Now that I am making changes to complete the current period on disable, the pin
goes to Low after disabling(completing the cycle).

Ill add all these points as you mentioned in v2.

Thanks and regards,
Lokesh