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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]



在 2022/3/2 05:56, Uwe Kleine-König 写道:

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>

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.

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)

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

Best regards

greybus-dev mailing list -- greybus-dev@xxxxxxxxxxxxxxxx
To unsubscribe send an email to greybus-dev-leave@xxxxxxxxxxxxxxxx

[Index of Archives]     [Asterisk App Development]     [PJ SIP]     [Gnu Gatekeeper]     [IETF Sipping]     [Info Cyrus]     [ALSA User]     [Fedora Linux Users]     [Linux SCTP]     [DCCP]     [Gimp]     [Yosemite News]     [Deep Creek Hot Springs]     [Yosemite Campsites]     [ISDN Cause Codes]     [Asterisk Books]

  Powered by Linux