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

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

 



On Wed, Mar 16, 2022 at 10:14:30AM -0500, Alex Elder wrote:
> On 3/15/22 9:21 PM, Song Chen wrote:
> > diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c
> > index 891a6a672378..3add3032678b 100644
> > --- a/drivers/staging/greybus/pwm.c
> > +++ b/drivers/staging/greybus/pwm.c
> > @@ -204,43 +204,54 @@ 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;
> 
> The use of local variables here is inconsistent, and that
> can be confusing.  Specifically, the "enabled" variable
> represents the *current* state, while the "period" and
> "duty_cycle" variables represent the *desired* state.  To
> avoid confusion, if you're going to use local variables
> like that, they should all represent *either* the current
> state *or* the new state.  Please update your patch to
> do one or the other.

IMHO that it overly picky. I'm ok with the usage as is.

> >   	struct gb_pwm_chip *pwmc = pwm_chip_to_gb_pwm_chip(chip);
> > -	return gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_ns, period_ns);
> > -};
> > +	/* 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_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);
> > +	if (!state->enabled) {
> > +		if (enabled)
> > +			gb_pwm_disable_operation(pwmc, pwm->hwpwm);
> > +		return 0;
> 
> If you are disabling the device, you return without updating the
> period and duty cycle.  But you *do* set polarity.  Is that
> required by the PWM API?  (I don't actually know.)  Or can the
> polarity setting be simply ignored as well if the new state is
> disabled?

All is well here. A disabled PWM is expected to emit the inactive level.
So polarity matters, duty and period don't.

> Also, if the polarity changed, the device will have already been
> disabled above, so there's no need to do so again (and perhaps
> it might be a bad thing to do twice?).

That won't happen, because if the device was disabled for the polarity
change, enabled = false. In fact that is the purpose of the local
variable.

> > +	}
> > -	return gb_pwm_set_polarity_operation(pwmc, pwm->hwpwm, polarity);
> > -};
> 
> Since you're clamping the values to 32 bits here, your comment
> should explain why (because Greybus uses 32-bit values here,
> while the API supports 64 bit values).  That would be a much
> more useful piece of information than "set period and duty cycle".
> 
> > +	/* set period and duty cycle*/
> 
> Include a space before "*/" in your comments.

ack

> > +	if (period > U32_MAX)
> > +		period = U32_MAX;
> > -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 (duty_cycle > period)
> > +		duty_cycle = period;
> > -	return gb_pwm_enable_operation(pwmc, pwm->hwpwm);
> > -};
> > +	err = gb_pwm_config_operation(pwmc, pwm->hwpwm, duty_cycle, period);
> > +	if (err)
> > +		return err;
> 
> What if the new state set usage_power to true?  It would
> be ignored here.  Is it OK to silently ignore it?  Even
> if it is, a comment about that would be good to see, so
> we know it's intentional.

ignoring usage_power is OK. All but a single driver do it that way.

Best regards
Uwe

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

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux