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

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

[Song Chen]

Introduce apply in pwm_ops to replace legacy operations,
like enable, disable, config and set_polarity.

Signed-off-by: Song Chen <chensong_2000@189.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?
---
 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);
+			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) {
+		period = INT_MAX;
+		dev_warn(chip->dev, "period is %llu ns, out of range\n", state->period);
+	}
+	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);
+	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,
 };
 
-- 
2.25.1

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

[Greg KH]

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.

Why?  You are saying what you are doing here, but nothing about why this
is needed, or what it will help with, or what it will fix.

You need to explain why a change is needed.

thanks,

greg k-h

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4615 bytes --]

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@189.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?

> +			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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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@189.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