Re: [PATCH v3] staging: greybus: introduce pwm_ops::apply

[Song Chen]

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@xxxxxx>
> >
> > ---
> > 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