Re: [PATCH v6 2/4] pwm: make it possible to apply pwm changes in atomic context

[Sean Young]

Hi Thierry,


On Fri, Dec 08, 2023 at 05:19:52PM +0100, Thierry Reding wrote:
> On Wed, Nov 29, 2023 at 09:13:35AM +0000, Sean Young wrote:
> > Some pwm devices require sleeping, for example if the pwm device is
> > connected over i2c. However, many pwm devices could be used from atomic
> > context, e.g. memmory mapped pwm. This is useful for, for example, the
> > pwm-ir-tx driver which requires precise timing. Sleeping causes havoc
> > with the generated IR signal.
> > 
> > Since not all pmw devices can support atomic context, we also add a
> > pwm_is_atomic() function to check if it is supported.
> 
> s/i2c/I2C/ and s/pwm/PWM/ in the above. Also, s/memmory/memory/

Thanks for your detailed review. I agree with all your points, they are
all nice improvements. Just a question at the bottom:

> 
> > 
> > Signed-off-by: Sean Young <sean@xxxxxxxx>
> > ---
> >  Documentation/driver-api/pwm.rst |  9 +++++
> >  drivers/pwm/core.c               | 63 ++++++++++++++++++++++++++------
> >  drivers/pwm/pwm-renesas-tpu.c    |  1 -
> >  include/linux/pwm.h              | 29 ++++++++++++++-
> >  4 files changed, 87 insertions(+), 15 deletions(-)
> > 
> > diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst
> > index f1d8197c8c43..1d4536fdf47c 100644
> > --- a/Documentation/driver-api/pwm.rst
> > +++ b/Documentation/driver-api/pwm.rst
> > @@ -43,6 +43,15 @@ After being requested, a PWM has to be configured using::
> >  
> >  	int pwm_apply_might_sleep(struct pwm_device *pwm, struct pwm_state *state);
> >  
> > +Some PWM devices can be used from atomic context. You can check if this is
> > +supported with::
> > +
> > +        bool pwm_is_atomic(struct pwm_device *pwm);
> 
> This is now going to look a bit odd. I think it'd be best to change this
> to pwm_might_sleep() for consistency with the pwm_apply_might_sleep()
> function. Fine to keep the actual member variable as atomic, though, so
> we don't have to change the default for all drivers.

Agreed, I was struggling with finding good name and yours is much better,
thanks.

 > +{
> > +	return pwm->chip->atomic;
> > +}
> > +
> >  /* PWM provider APIs */
> >  int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
> >  		unsigned long timeout);
> > @@ -406,16 +420,27 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
> >  				       struct fwnode_handle *fwnode,
> >  				       const char *con_id);
> >  #else
> > +static inline bool pwm_is_atomic(struct pwm_device *pwm)
> > +{
> > +	return false;
> > +}
> > +
> >  static inline int pwm_apply_might_sleep(struct pwm_device *pwm,
> >  					const struct pwm_state *state)
> >  {
> >  	might_sleep();
> > -	return -ENOTSUPP;
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static inline int pwm_apply_atomic(struct pwm_device *pwm,
> > +				   const struct pwm_state *state)
> > +{
> > +	return -EOPNOTSUPP;
> >  }
> >  
> >  static inline int pwm_adjust_config(struct pwm_device *pwm)
> >  {
> > -	return -ENOTSUPP;
> > +	return -EOPNOTSUPP;
> >  }
> 
> What's wrong with ENOTSUPP?

This was found by checkpatch, see

https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl#L4891-L4892

# ENOTSUPP is not a standard error code and should be avoided in new patches.
# Folks usually mean EOPNOTSUPP (also called ENOTSUP), when they type ENOTSUPP.

ENOTSUPP is not really widely used in the tree.

So it was really done to keep checkpatch happy, please let me know what you
would like me to do here.

Thanks,

Sean