Date:   Thu, 17 Mar 2022 13:41:15 +0800
From:   Song Chen <chensong_2000@....cn>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
        Alex Elder <elder@...aro.org>
Cc:     Alex Elder <elder@...e.org>, johan@...nel.org, elder@...nel.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
Subject: Re: [greybus-dev] Re: [PATCH v5] staging: greybus: introduce
 pwm_ops::apply

hi Alex & Uwe,

Thanks for the advices and clarifications.

在 2022/3/17 04:05, Uwe Kleine-König 写道:
> On Wed, Mar 16, 2022 at 12:20:11PM -0500, Alex Elder wrote:
>> On 3/16/22 11:29 AM, Uwe Kleine-König wrote:
>>> On Wed, Mar 16, 2022 at 10:14:30AM -0500, Alex Elder wrote:
>>>> On 3/15/22 9:21 PM, Song Chen wrote:
>>>>> diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c
>>>>> index 891a6a672378..3add3032678b 100644
>>>>> --- a/drivers/staging/greybus/pwm.c
>>>>> +++ b/drivers/staging/greybus/pwm.c
>>>>> @@ -204,43 +204,54 @@ 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;
>>>>
>>>> The use of local variables here is inconsistent, and that
>>>> can be confusing.  Specifically, the "enabled" variable
>>>> represents the *current* state, while the "period" and
>>>> "duty_cycle" variables represent the *desired* state.  To
>>>> avoid confusion, if you're going to use local variables
>>>> like that, they should all represent *either* the current
>>>> state *or* the new state.  Please update your patch to
>>>> do one or the other.
>>>
>>> IMHO that it overly picky. I'm ok with the usage as is.
>>
>> I see the "enabled" flag is used in a way that I didn't
>> notice before.  Changing its name to "disabled" (to mean
>> "we have disabled the device within this function already")
>> would allow it to be used in the same way, but would make
>> it more obvious it's not just a copy of "old" device state.

Some of drivers in driver/pwn use pwm->state.enabled directly in their 
apply and others name it "enabled", see pwm-tiecap.c, pwm-berlin.c, 
pwm-vt8500.c and pwm-stm32.c.

I prefer keeping consist with those drivers, "disabled" confuses me.

>>
>>>>>     	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
>>>>> -	return gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_ns, period_ns);
>>>>> -};
>>>>> +	/* set polarity */
>>>>> +	if (state->polarity != pwm->state.polarity) {
>>>>> +		if (enabled) {
>>>>> +			gb_pwm_disable_operation(pwmc, pwm->hwpwm);
>>>>> +			enabled = false;
>>>>> +		}
>>>>> +		err = gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, state->polarity);
>>>>> +		if (err)
>>>>> +			return err;
>>>>> +	}
>>>>> -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);
>>>>> +	if (!state->enabled) {
>>>>> +		if (enabled)
>>>>> +			gb_pwm_disable_operation(pwmc, pwm->hwpwm);
>>>>> +		return 0;
>>>>
>>>> If you are disabling the device, you return without updating the
>>>> period and duty cycle.  But you *do* set polarity.  Is that
>>>> required by the PWM API?  (I don't actually know.)  Or can the
>>>> polarity setting be simply ignored as well if the new state is
>>>> disabled?
>>>
>>> All is well here. A disabled PWM is expected to emit the inactive level.
>>> So polarity matters, duty and period don't.
>>
>> Thanks for clarifying that.  I did not know what was expected.
>>
>>>> Also, if the polarity changed, the device will have already been
>>>> disabled above, so there's no need to do so again (and perhaps
>>>> it might be a bad thing to do twice?).
>>>
>>> That won't happen, because if the device was disabled for the polarity
>>> change, enabled = false. In fact that is the purpose of the local
>>> variable.
>>
>> Now I see, yes, the local variable gets changed when the
>> disable occurred above.
>>
>>>>> +	}
>>>>> -	return gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, polarity);
>>>>> -};
>>>>
>>>> Since you're clamping the values to 32 bits here, your comment
>>>> should explain why (because Greybus uses 32-bit values here,
>>>> while the API supports 64 bit values).  That would be a much
>>>> more useful piece of information than "set period and duty cycle".
>>>>

done, thanks.

>>>>> +	/* set period and duty cycle*/
>>>>
>>>> Include a space before "*/" in your comments.
>>>
>>> ack
>>>

done, thanks.

>>>>> +	if (period > U32_MAX)
>>>>> +		period = U32_MAX;
>>>>> -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 (duty_cycle > period)
>>>>> +		duty_cycle = period;
>>>>> -	return gb_pwm_enable_operation(pwmc, pwm->hwpwm);
>>>>> -};
>>>>> +	err = gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_cycle, period);
>>>>> +	if (err)
>>>>> +		return err;
>>>>
>>>> What if the new state set usage_power to true?  It would
>>>> be ignored here.  Is it OK to silently ignore it?  Even
>>>> if it is, a comment about that would be good to see, so
>>>> we know it's intentional.
>>>
>>> ignoring usage_power is OK. All but a single driver do it that way.
>>
>> I don't actually see anything that sets usage_power to true,
>> although "pwm-pca9685.c" tests its value.
>>
>> I guess it's an advisory parameter that's passed to the apply
>> callback function.  It's described as optional, but--not being
>> a "PWM person"--this isn't obvious to me.  Maybe the comments
>> describing the field or the apply callback could define the
>> semantics a little better at some point.
> 
> One of the problems I see with usage_power is that it's not well
> defined. The idea is that when usage_power is true, the driver is free
> to implement any setting that just matches the relative duty_cycle of
> the request. So if you call pwm_apply with
> 
> 	.duty_cycle = 2000
> 	.period = 10000
> 	.usage_power = true
> 	.polarity = PWM_POLARITY_NORMAL
> 
> you can program the hardware to implement
> 
> 	.duty_cycle = 200
> 	.period = 1000
> 	.polarity = PWM_POLARITY_NORMAL
> 
> or
> 
> 	.duty_cycle = 8000000
> 	.period = 10000000
> 	.polarity = PWM_POLARITY_INVERTED
> 
> The expectation is however to only deviate in a sensible manner from the
> request, whatever that might mean.
> 
> I don't see much value in that field, there is only one implementing
> driver and no mainline user. If you're interested you can reread the
> discussions about it in the archives.
> 
> Best regards
> Uwe
> 

what's more, "like" in commit message, removed.

Thanks

BR

Song

Powered by blists - more mailing lists