| 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
| ||
|
Date: Thu, 3 Mar 2022 11:31:26 +0800
From: Song Chen <chensong_2000@....cn>
To: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc: johan@...nel.org, elder@...nel.org, gregkh@...uxfoundation.org,
thierry.reding@...il.com, lee.jones@...aro.org,
greybus-dev@...ts.linaro.org, linux-staging@...ts.linux.dev,
linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org,
elder@...e.org
Subject: Re: [PATCH v3] staging: greybus: introduce pwm_ops::apply
Hello,
在 2022/3/2 05:56, Uwe Kleine-König 写道:
> Hello,
>
> On Fri, Feb 25, 2022 at 05:16:01PM +0800, Song Chen wrote:
>> Introduce apply in pwm_ops to replace legacy operations,
>> like enable, disable, config and set_polarity.
>>
>> Signed-off-by: Song Chen <chensong_2000@....cn>
>>
>> ---
>> v2:
>> 1, define duty_cycle and period as u64 in gb_pwm_config_operation.
>> 2, define duty and period as u64 in gb_pwm_config_request.
>> 3, disable before configuring duty and period if the eventual goal
>> is a disabled state.
>>
>> v3:
>> Regarding duty_cycle and period, I read more discussion in this thread,
>> min, warn or -EINVAL, seems no perfect way acceptable for everyone.
>> How about we limit their value to INT_MAX and throw a warning at the
>> same time when they are wrong?
>
> My position is that the driver should implement the biggest possible
> period not bigger than the requested period. That's how all new drivers
> behave since I care for reviewing PWM stuff. So capping to U32_MAX as is
> (nearly) done below is good in my book.
>
>> ---
>> drivers/staging/greybus/pwm.c | 66 +++++++++++++++++++++--------------
>> 1 file changed, 40 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c
>> index 891a6a672378..3ec5bc54d616 100644
>> --- a/drivers/staging/greybus/pwm.c
>> +++ b/drivers/staging/greybus/pwm.c
>> @@ -204,43 +204,57 @@ static void gb_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>> gb_pwm_deactivate_operation(pwmc, pwm->hwpwm);
>> }
>>
>> -static int gb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> - int duty_ns, int period_ns)
>> +static int gb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> + const struct pwm_state *state)
>> {
>> + int err;
>> + bool enabled = pwm->state.enabled;
>> + u64 period = state->period;
>> + u64 duty_cycle = state->duty_cycle;
>> struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
>>
>> - return gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_ns, period_ns);
>> -};
>> -
>> -static int gb_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>> - enum pwm_polarity polarity)
>> -{
>> - struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
>> -
>> - return gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, polarity);
>> -};
>> + /* set polarity */
>> + if (state->polarity != pwm->state.polarity) {
>> + if (enabled) {
>> + gb_pwm_disable_operation(pwmc, pwm->hwpwm);
>
> So polarity can only be switched with the PWM off?
I have no devices in my hand to get it tested, but i think it's
reasonable to turn off PWM before switching off its polarity.
What's more, it follows the implementation of pwm_apply_legacy, which is
the way how it works now.
>
>> + enabled = false;
>> + }
>> + err = gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, state->polarity);
>> + if (err)
>> + return err;
>> + }
>>
>> -static int gb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> -{
>> - struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
>> + if (!state->enabled) {
>> + if (enabled)
>> + gb_pwm_disable_operation(pwmc, pwm->hwpwm);
>> + return 0;
>> + }
>>
>> - return gb_pwm_enable_operation(pwmc, pwm->hwpwm);
>> -};
>> + /* set period and duty cycle*/
>> + if (period > INT_MAX) {
>
> Given that in gb_pwm_config_operation the parameters are u32, I suggest
> to use U32_MAX here instead of INT_MAX.
>
will do.
>> + period = INT_MAX;
>> + dev_warn(chip->dev, "period is %llu ns, out of range\n", state->period);
>
> Please drop this warning. That's a bad one because it can be triggered
> from userspace.
will do
>
>> + }
>> + if (duty_cycle > INT_MAX) {
>> + duty_cycle = INT_MAX;
>> + dev_warn(chip->dev,
>> + "duty cycle is %llu ns, out of range\n", state->duty_cycle);
>> + }
>> + err = gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_cycle, period);
>
> Is it clear how gb_pwm_config_operation rounds? If yes, please document
> this. I also wonder if you could implement (in a separate change)
> .get_state().
Not clear about gb_pwm_config_operation rounds.
For get_state, i will look into it.
>
>> + if (err)
>> + return err;
>>
>> -static void gb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>> -{
>> - struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
>> + /* enable/disable */
>> + if (!enabled)
>> + return gb_pwm_enable_operation(pwmc, pwm->hwpwm);
>>
>> - gb_pwm_disable_operation(pwmc, pwm->hwpwm);
>> -};
>> + return 0;
>> +}
>>
>> static const struct pwm_ops gb_pwm_ops = {
>> .request = gb_pwm_request,
>> .free = gb_pwm_free,
>> - .config = gb_pwm_config,
>> - .set_polarity = gb_pwm_set_polarity,
>> - .enable = gb_pwm_enable,
>> - .disable = gb_pwm_disable,
>> + .apply = gb_pwm_apply,
>> .owner = THIS_MODULE,
>
> Best regards
> Uwe
>
Best regards
Song
Powered by blists - more mailing lists