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

[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?

> +			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.

> +		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.

> +	}
> +	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().

> +	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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature