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

[PATCH v2] 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.
---
 drivers/staging/greybus/pwm.c             | 61 ++++++++++++-----------
 include/linux/greybus/greybus_protocols.h |  4 +-
 2 files changed, 34 insertions(+), 31 deletions(-)

diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c
index 891a6a672378..03c69db5b9be 100644
--- a/drivers/staging/greybus/pwm.c
+++ b/drivers/staging/greybus/pwm.c
@@ -89,7 +89,7 @@ static int gb_pwm_deactivate_operation(struct gb_pwm_chip *pwmc,
 }
 
 static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc,
-				   u8 which, u32 duty, u32 period)
+				   u8 which, u64 duty, u64 period)
 {
 	struct gb_pwm_config_request request;
 	struct gbphy_device *gbphy_dev;
@@ -99,8 +99,8 @@ static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc,
 		return -EINVAL;
 
 	request.which = which;
-	request.duty = cpu_to_le32(duty);
-	request.period = cpu_to_le32(period);
+	request.duty = duty;
+	request.period = period;
 
 	gbphy_dev = to_gbphy_dev(pwmc->chip.dev);
 	ret = gbphy_runtime_get_sync(gbphy_dev);
@@ -204,43 +204,46 @@ 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;
 	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*/
+	err = gb_pwm_config_operation(pwmc, pwm->hwpwm, state->duty_cycle, state->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,
 };
 
diff --git a/include/linux/greybus/greybus_protocols.h b/include/linux/greybus/greybus_protocols.h
index aeb8f9243545..81a6f16de098 100644
--- a/include/linux/greybus/greybus_protocols.h
+++ b/include/linux/greybus/greybus_protocols.h
@@ -812,8 +812,8 @@ struct gb_pwm_deactivate_request {
 
 struct gb_pwm_config_request {
 	__u8	which;
-	__le32	duty;
-	__le32	period;
+	__u64	duty;
+	__u64	period;
 } __packed;
 
 struct gb_pwm_polarity_request {
-- 
2.25.1

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

[Greg KH]

On Fri, Feb 11, 2022 at 08:02:27PM +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.
> ---
>  drivers/staging/greybus/pwm.c             | 61 ++++++++++++-----------
>  include/linux/greybus/greybus_protocols.h |  4 +-
>  2 files changed, 34 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c
> index 891a6a672378..03c69db5b9be 100644
> --- a/drivers/staging/greybus/pwm.c
> +++ b/drivers/staging/greybus/pwm.c
> @@ -89,7 +89,7 @@ static int gb_pwm_deactivate_operation(struct gb_pwm_chip *pwmc,
>  }
>  
>  static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc,
> -				   u8 which, u32 duty, u32 period)
> +				   u8 which, u64 duty, u64 period)
>  {
>  	struct gb_pwm_config_request request;
>  	struct gbphy_device *gbphy_dev;
> @@ -99,8 +99,8 @@ static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc,
>  		return -EINVAL;
>  
>  	request.which = which;
> -	request.duty = cpu_to_le32(duty);
> -	request.period = cpu_to_le32(period);
> +	request.duty = duty;
> +	request.period = period;
>  
>  	gbphy_dev = to_gbphy_dev(pwmc->chip.dev);
>  	ret = gbphy_runtime_get_sync(gbphy_dev);
> @@ -204,43 +204,46 @@ 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;
>  	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*/
> +	err = gb_pwm_config_operation(pwmc, pwm->hwpwm, state->duty_cycle, state->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,
>  };
>  
> diff --git a/include/linux/greybus/greybus_protocols.h b/include/linux/greybus/greybus_protocols.h
> index aeb8f9243545..81a6f16de098 100644
> --- a/include/linux/greybus/greybus_protocols.h
> +++ b/include/linux/greybus/greybus_protocols.h
> @@ -812,8 +812,8 @@ struct gb_pwm_deactivate_request {
>  
>  struct gb_pwm_config_request {
>  	__u8	which;
> -	__le32	duty;
> -	__le32	period;
> +	__u64	duty;
> +	__u64	period;
>  } __packed;

Did you just change a greybus protocol message that is sent over the
wire?  Why?  And why drop the endian marking of it?

Are you sure this is ok?

thanks,

greg k-h

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

[Song Chen]

Hi Greg,

在 2022/2/22 01:06, Greg KH 写道:
> On Fri, Feb 11, 2022 at 08:02:27PM +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.
>> ---
>>   drivers/staging/greybus/pwm.c             | 61 ++++++++++++-----------
>>   include/linux/greybus/greybus_protocols.h |  4 +-
>>   2 files changed, 34 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c
>> index 891a6a672378..03c69db5b9be 100644
>> --- a/drivers/staging/greybus/pwm.c
>> +++ b/drivers/staging/greybus/pwm.c
>> @@ -89,7 +89,7 @@ static int gb_pwm_deactivate_operation(struct gb_pwm_chip *pwmc,
>>   }
>>   
>>   static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc,
>> -				   u8 which, u32 duty, u32 period)
>> +				   u8 which, u64 duty, u64 period)
>>   {
>>   	struct gb_pwm_config_request request;
>>   	struct gbphy_device *gbphy_dev;
>> @@ -99,8 +99,8 @@ static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc,
>>   		return -EINVAL;
>>   
>>   	request.which = which;
>> -	request.duty = cpu_to_le32(duty);
>> -	request.period = cpu_to_le32(period);
>> +	request.duty = duty;
>> +	request.period = period;
>>   
>>   	gbphy_dev = to_gbphy_dev(pwmc->chip.dev);
>>   	ret = gbphy_runtime_get_sync(gbphy_dev);
>> @@ -204,43 +204,46 @@ 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;
>>   	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*/
>> +	err = gb_pwm_config_operation(pwmc, pwm->hwpwm, state->duty_cycle, state->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,
>>   };
>>   
>> diff --git a/include/linux/greybus/greybus_protocols.h b/include/linux/greybus/greybus_protocols.h
>> index aeb8f9243545..81a6f16de098 100644
>> --- a/include/linux/greybus/greybus_protocols.h
>> +++ b/include/linux/greybus/greybus_protocols.h
>> @@ -812,8 +812,8 @@ struct gb_pwm_deactivate_request {
>>   
>>   struct gb_pwm_config_request {
>>   	__u8	which;
>> -	__le32	duty;
>> -	__le32	period;
>> +	__u64	duty;
>> +	__u64	period;
>>   } __packed;
> 
> Did you just change a greybus protocol message that is sent over the
> wire?  Why?  And why drop the endian marking of it?

I discussed with Uwe about losing bit and found there is no perfect way 
to avoid, even in Uwe's patch[1], as a result, we decided to widen duty 
and period in gb_pwm_config_request, the other side of the wire is 
supposed to change accordingly to support 64bit duty and period too(this 
will introduce compatibility problem and there is no variable like 
version to address it), similar with ktime_t in y2038, below is the 
detail [2]

[1]:https://lore.kernel.org/all/20210312212119.1342666-1-u.kleine-koenig@pengutronix.de/
[2]:https://lore.kernel.org/all/20220211071601.4rpfbkit6c6dre2o@pengutronix.de/

endian is a problem, i shouldn't drop it.

BR

Song

> 
> Are you sure this is ok?
> 
> thanks,
> 
> greg k-h
> 

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

[Alex Elder]

On 2/22/22 12:19 AM, Song Chen wrote:
> Hi Greg,
> 
> 在 2022/2/22 01:06, Greg KH 写道:
>> On Fri, Feb 11, 2022 at 08:02:27PM +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>

. . .

>>> diff --git a/include/linux/greybus/greybus_protocols.h b/include/linux/greybus/greybus_protocols.h
>>> index aeb8f9243545..81a6f16de098 100644
>>> --- a/include/linux/greybus/greybus_protocols.h
>>> +++ b/include/linux/greybus/greybus_protocols.h
>>> @@ -812,8 +812,8 @@ struct gb_pwm_deactivate_request {
>>>   struct gb_pwm_config_request {
>>>       __u8    which;
>>> -    __le32    duty;
>>> -    __le32    period;
>>> +    __u64    duty;
>>> +    __u64    period;
>>>   } __packed;
>>
>> Did you just change a greybus protocol message that is sent over the
>> wire?  Why?  And why drop the endian marking of it?
> 
> I discussed with Uwe about losing bit and found there is no perfect
> way to avoid, even in Uwe's patch[1], as a result, we decided to

The patch you reference [1] does not change the size of the
duty cycle and period, it only ensures the value passed is
no more than can be represented in an integer.

> widen duty and period in gb_pwm_config_request, the other side of the
> wire is supposed to change accordingly to support 64bit duty and

This is what Greg meant about changing the over-the-wire protocol
message format.  You can't do that, or rather, if you do that, it
is a *lot* more work than just changing it as you have done here.

If it is really required (and that's not clear, at least not at
this time), then you need to modify the protocol version, and
then make sure the versioning logic works correctly, both on
the AP side and on the module side for all existing modules.

I would suggest that it is *not* required though, because the
existing module code was built with the assumption that it would
be provided a 32-bit unsigned value for its duty cycle and period.
So as long as you make sure the AP side doesn't send anything
nonsensical, they will continue to work as desired.

The correct fix in this case (assuming you don't want to change
the protocol) is to ensure that whatever value is passed in to
the pwm_ops->config callback (which is gb_pwm_config()) is
adjusted to be within a range usable by the existing protocol.
Since it's actually an unsigned value, you could double the
range supported without changing the protocol if you wanted to
do that.

> period too(this will introduce compatibility problem and there is no
> variable like version to address it), similar with ktime_t in y2038,
> below is the detail [2]

And similar to addressing the Y2038 issue, it is not an easy thing to do.

> [1]:https://lore.kernel.org/all/20210312212119.1342666-1-u.kleine-koenig@pengutronix.de/
> [2]:https://lore.kernel.org/all/20220211071601.4rpfbkit6c6dre2o@pengutronix.de/
> 
> endian is a problem, i shouldn't drop it.

This is absolutely correct.  Did you attempt to compile this?

					-Alex

> BR
> 
> Song
> 
>>
>> Are you sure this is ok?
>>
>> thanks,
>>
>> greg k-h
>>

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

[Greg KH]

On Tue, Feb 22, 2022 at 02:19:12PM +0800, Song Chen wrote:
> Hi Greg,
> 
> 在 2022/2/22 01:06, Greg KH 写道:
> > On Fri, Feb 11, 2022 at 08:02:27PM +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.
> > > ---
> > >   drivers/staging/greybus/pwm.c             | 61 ++++++++++++-----------
> > >   include/linux/greybus/greybus_protocols.h |  4 +-
> > >   2 files changed, 34 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c
> > > index 891a6a672378..03c69db5b9be 100644
> > > --- a/drivers/staging/greybus/pwm.c
> > > +++ b/drivers/staging/greybus/pwm.c
> > > @@ -89,7 +89,7 @@ static int gb_pwm_deactivate_operation(struct gb_pwm_chip *pwmc,
> > >   }
> > >   static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc,
> > > -				   u8 which, u32 duty, u32 period)
> > > +				   u8 which, u64 duty, u64 period)
> > >   {
> > >   	struct gb_pwm_config_request request;
> > >   	struct gbphy_device *gbphy_dev;
> > > @@ -99,8 +99,8 @@ static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc,
> > >   		return -EINVAL;
> > >   	request.which = which;
> > > -	request.duty = cpu_to_le32(duty);
> > > -	request.period = cpu_to_le32(period);
> > > +	request.duty = duty;
> > > +	request.period = period;
> > >   	gbphy_dev = to_gbphy_dev(pwmc->chip.dev);
> > >   	ret = gbphy_runtime_get_sync(gbphy_dev);
> > > @@ -204,43 +204,46 @@ 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;
> > >   	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*/
> > > +	err = gb_pwm_config_operation(pwmc, pwm->hwpwm, state->duty_cycle, state->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,
> > >   };
> > > diff --git a/include/linux/greybus/greybus_protocols.h b/include/linux/greybus/greybus_protocols.h
> > > index aeb8f9243545..81a6f16de098 100644
> > > --- a/include/linux/greybus/greybus_protocols.h
> > > +++ b/include/linux/greybus/greybus_protocols.h
> > > @@ -812,8 +812,8 @@ struct gb_pwm_deactivate_request {
> > >   struct gb_pwm_config_request {
> > >   	__u8	which;
> > > -	__le32	duty;
> > > -	__le32	period;
> > > +	__u64	duty;
> > > +	__u64	period;
> > >   } __packed;
> > 
> > Did you just change a greybus protocol message that is sent over the
> > wire?  Why?  And why drop the endian marking of it?
> 
> I discussed with Uwe about losing bit and found there is no perfect way to
> avoid, even in Uwe's patch[1], as a result, we decided to widen duty and
> period in gb_pwm_config_request, the other side of the wire is supposed to
> change accordingly to support 64bit duty and period too(this will introduce
> compatibility problem and there is no variable like version to address it),
> similar with ktime_t in y2038, below is the detail [2]

Where are you changing the "other side of the wire" here?  That wire is
in firmware, how are you going to coordinate that change?

This is a hardware API, you can not change it without a lot of very
careful work, and I still do not understand why it is needed.  What
"problem" is this solving that requires the protocol to change?

thanks,

greg k-h

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

[Uwe Kleine-König]

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

Hello,

orthogonal to the feedback you got for the protocol part, here some
thoughts to the PWM specific bits;

On Fri, Feb 11, 2022 at 08:02:27PM +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.
> ---
>  drivers/staging/greybus/pwm.c             | 61 ++++++++++++-----------
>  include/linux/greybus/greybus_protocols.h |  4 +-
>  2 files changed, 34 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c
> index 891a6a672378..03c69db5b9be 100644
> --- a/drivers/staging/greybus/pwm.c
> +++ b/drivers/staging/greybus/pwm.c
> @@ -89,7 +89,7 @@ static int gb_pwm_deactivate_operation(struct gb_pwm_chip *pwmc,
>  }
>  
>  static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc,
> -				   u8 which, u32 duty, u32 period)
> +				   u8 which, u64 duty, u64 period)
>  {
>  	struct gb_pwm_config_request request;
>  	struct gbphy_device *gbphy_dev;
> @@ -99,8 +99,8 @@ static int gb_pwm_config_operation(struct gb_pwm_chip *pwmc,
>  		return -EINVAL;
>  
>  	request.which = which;
> -	request.duty = cpu_to_le32(duty);
> -	request.period = cpu_to_le32(period);
> +	request.duty = duty;
> +	request.period = period;

If you don't change the wire protocol, you want something like:

	if (period > U32_MAX)
		period = U32_MAX;

	if (duty > period)
		duty = period;

To further improve the PWM driver it would be great if you added a
paragraph about the details of its behaviour; in the format the other
drivers do that (see e.g. drivers/pwm/pwm-sifive.c). It should describe
the following properties:

 - Is a period completed when period/duty changes?
 - Is a period completed when the hardware is disabled?
 - How does the output behave on disable?

In this specific case I think there is also some rounding behaviour in
the backend?! If so, describing this would be good, too.

>  	gbphy_dev = to_gbphy_dev(pwmc->chip.dev);
>  	ret = gbphy_runtime_get_sync(gbphy_dev);
> @@ -204,43 +204,46 @@ 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;
>  	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;
> +	}

This is copied from the legacy pwm handling in the pwm core. This is
good in principle. But if your hardware can change polarity without
stopping operation, that would be the thing to do. (The pwm core cannot
assume this, so it doesn't.)

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: 484 bytes --]