[1/2] pwm: Add different PWM output types support
diff mbox series

Message ID 1568415464-20267-1-git-send-email-gurus@codeaurora.org
State Changes Requested
Headers show
Series
  • [1/2] pwm: Add different PWM output types support
Related show

Commit Message

Guru Das Srinagesh Sept. 13, 2019, 10:57 p.m. UTC
From: Fenglin Wu <fenglinw@codeaurora.org>

Normally, PWM channel has fixed output until software request to change
its settings. There are some PWM devices which their outputs could be
changed autonomously according to a predefined pattern programmed in
hardware. Add pwm_output_type enum type to identify these two different
PWM types and add relevant helper functions to set and get PWM output
types and pattern.

Change-Id: Ia1f914a45ab4f4dd7be037a395eeb89d0e65a80e
Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
---
 drivers/pwm/core.c  | 26 ++++++++++++++++++++
 drivers/pwm/sysfs.c | 50 ++++++++++++++++++++++++++++++++++++++
 include/linux/pwm.h | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 146 insertions(+)

Comments

Thierry Reding Sept. 16, 2019, 2:01 p.m. UTC | #1
On Fri, Sep 13, 2019 at 03:57:43PM -0700, Guru Das Srinagesh wrote:
> From: Fenglin Wu <fenglinw@codeaurora.org>
> 
> Normally, PWM channel has fixed output until software request to change
> its settings. There are some PWM devices which their outputs could be
> changed autonomously according to a predefined pattern programmed in
> hardware. Add pwm_output_type enum type to identify these two different
> PWM types and add relevant helper functions to set and get PWM output
> types and pattern.
> 
> Change-Id: Ia1f914a45ab4f4dd7be037a395eeb89d0e65a80e
> Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
> Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> ---
>  drivers/pwm/core.c  | 26 ++++++++++++++++++++
>  drivers/pwm/sysfs.c | 50 ++++++++++++++++++++++++++++++++++++++
>  include/linux/pwm.h | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 146 insertions(+)

This doesn't seem right to me. Are you describing a PWM pin that's
actually driven in GPIO mode? We usually configure that using pinctrl.

Thierry

> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 8edfac1..960a451 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -282,6 +282,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
>  		pwm->pwm = chip->base + i;
>  		pwm->hwpwm = i;
>  		pwm->state.polarity = polarity;
> +		pwm->state.output_type = PWM_OUTPUT_FIXED;
>  
>  		if (chip->ops->get_state)
>  			chip->ops->get_state(chip, pwm, &pwm->state);
> @@ -498,6 +499,31 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
>  			pwm->state.polarity = state->polarity;
>  		}
>  
> +		if (state->output_type != pwm->state.output_type) {
> +			if (!pwm->chip->ops->set_output_type)
> +				return -ENOTSUPP;
> +
> +			err = pwm->chip->ops->set_output_type(pwm->chip, pwm,
> +						state->output_type);
> +			if (err)
> +				return err;
> +
> +			pwm->state.output_type = state->output_type;
> +		}
> +
> +		if (state->output_pattern != pwm->state.output_pattern &&
> +				state->output_pattern != NULL) {
> +			if (!pwm->chip->ops->set_output_pattern)
> +				return -ENOTSUPP;
> +
> +			err = pwm->chip->ops->set_output_pattern(pwm->chip,
> +					pwm, state->output_pattern);
> +			if (err)
> +				return err;
> +
> +			pwm->state.output_pattern = state->output_pattern;
> +		}
> +
>  		if (state->period != pwm->state.period ||
>  		    state->duty_cycle != pwm->state.duty_cycle) {
>  			err = pwm->chip->ops->config(pwm->chip, pwm,
> diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> index 2389b86..ab703f2 100644
> --- a/drivers/pwm/sysfs.c
> +++ b/drivers/pwm/sysfs.c
> @@ -215,11 +215,60 @@ static ssize_t capture_show(struct device *child,
>  	return sprintf(buf, "%u %u\n", result.period, result.duty_cycle);
>  }
>  
> +static ssize_t output_type_show(struct device *child,
> +			     struct device_attribute *attr,
> +			     char *buf)
> +{
> +	const struct pwm_device *pwm = child_to_pwm_device(child);
> +	const char *output_type = "unknown";
> +	struct pwm_state state;
> +
> +	pwm_get_state(pwm, &state);
> +	switch (state.output_type) {
> +	case PWM_OUTPUT_FIXED:
> +		output_type = "fixed";
> +		break;
> +	case PWM_OUTPUT_MODULATED:
> +		output_type = "modulated";
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n", output_type);
> +}
> +
> +static ssize_t output_type_store(struct device *child,
> +			      struct device_attribute *attr,
> +			      const char *buf, size_t size)
> +{
> +	struct pwm_export *export = child_to_pwm_export(child);
> +	struct pwm_device *pwm = export->pwm;
> +	struct pwm_state state;
> +	int ret = -EINVAL;
> +
> +	mutex_lock(&export->lock);
> +	pwm_get_state(pwm, &state);
> +	if (sysfs_streq(buf, "fixed"))
> +		state.output_type = PWM_OUTPUT_FIXED;
> +	else if (sysfs_streq(buf, "modulated"))
> +		state.output_type = PWM_OUTPUT_MODULATED;
> +	else
> +		goto unlock;
> +
> +	ret = pwm_apply_state(pwm, &state);
> +unlock:
> +	mutex_unlock(&export->lock);
> +
> +	return ret ? : size;
> +}
> +
>  static DEVICE_ATTR_RW(period);
>  static DEVICE_ATTR_RW(duty_cycle);
>  static DEVICE_ATTR_RW(enable);
>  static DEVICE_ATTR_RW(polarity);
>  static DEVICE_ATTR_RO(capture);
> +static DEVICE_ATTR_RW(output_type);
>  
>  static struct attribute *pwm_attrs[] = {
>  	&dev_attr_period.attr,
> @@ -227,6 +276,7 @@ static ssize_t capture_show(struct device *child,
>  	&dev_attr_enable.attr,
>  	&dev_attr_polarity.attr,
>  	&dev_attr_capture.attr,
> +	&dev_attr_output_type.attr,
>  	NULL
>  };
>  ATTRIBUTE_GROUPS(pwm);
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 24632a7..416f08e 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -48,6 +48,29 @@ enum {
>  	PWMF_EXPORTED = 1 << 1,
>  };
>  
> +/**
> + * enum pwm_output_type - output type of the PWM signal
> + * @PWM_OUTPUT_FIXED: PWM output is fixed until a change request
> + * @PWM_OUTPUT_MODULATED: PWM output is modulated in hardware
> + * autonomously with a predefined pattern
> + */
> +enum pwm_output_type {
> +	PWM_OUTPUT_FIXED = 1 << 0,
> +	PWM_OUTPUT_MODULATED = 1 << 1,
> +};
> +
> +/**
> + * struct pwm_output_pattern - PWM duty pattern for MODULATED duty type
> + * @duty_pattern: PWM duty cycles in the pattern for duty modulation
> + * @num_entries: number of entries in the pattern
> + * @cycles_per_duty: number of PWM period cycles an entry stays at
> + */
> +struct pwm_output_pattern {
> +	unsigned int *duty_pattern;
> +	unsigned int num_entries;
> +	unsigned int cycles_per_duty;
> +};
> +
>  /*
>   * struct pwm_state - state of a PWM channel
>   * @period: PWM period (in nanoseconds)
> @@ -59,6 +82,8 @@ struct pwm_state {
>  	unsigned int period;
>  	unsigned int duty_cycle;
>  	enum pwm_polarity polarity;
> +	enum pwm_output_type output_type;
> +	struct pwm_output_pattern *output_pattern;
>  	bool enabled;
>  };
>  
> @@ -144,6 +169,26 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
>  	return state.polarity;
>  }
>  
> +static inline enum pwm_output_type pwm_get_output_type(
> +		const struct pwm_device *pwm)
> +{
> +	struct pwm_state state;
> +
> +	pwm_get_state(pwm, &state);
> +
> +	return state.output_type;
> +}
> +
> +static inline struct pwm_output_pattern *pwm_get_output_pattern(
> +				struct pwm_device *pwm)
> +{
> +	struct pwm_state state;
> +
> +	pwm_get_state(pwm, &state);
> +
> +	return pwm->state.output_pattern ?: NULL;
> +}
> +
>  static inline void pwm_get_args(const struct pwm_device *pwm,
>  				struct pwm_args *args)
>  {
> @@ -250,6 +295,9 @@ static inline void pwm_init_state(const struct pwm_device *pwm,
>   * @get_state: get the current PWM state. This function is only
>   *	       called once per PWM device when the PWM chip is
>   *	       registered.
> + * @get_output_type_supported: get the supported output type
> + * @set_output_type: set PWM output type
> + * @set_output_pattern: set the pattern for the modulated output
>   * @owner: helps prevent removal of modules exporting active PWMs
>   * @config: configure duty cycles and period length for this PWM
>   * @set_polarity: configure the polarity of this PWM
> @@ -265,6 +313,13 @@ struct pwm_ops {
>  		     struct pwm_state *state);
>  	void (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,
>  			  struct pwm_state *state);
> +	int (*get_output_type_supported)(struct pwm_chip *chip,
> +			struct pwm_device *pwm);
> +	int (*set_output_type)(struct pwm_chip *chip, struct pwm_device *pwm,
> +			enum pwm_output_type output_type);
> +	int (*set_output_pattern)(struct pwm_chip *chip,
> +			struct pwm_device *pwm,
> +			struct pwm_output_pattern *output_pattern);
>  	struct module *owner;
>  
>  	/* Only used by legacy drivers */
> @@ -320,6 +375,21 @@ struct pwm_capture {
>  int pwm_adjust_config(struct pwm_device *pwm);
>  
>  /**
> + * pwm_output_type_support()
> + * @pwm: PWM device
> + *
> + * Returns:  output types supported by the PWM device
> + */
> +static inline int pwm_get_output_type_supported(struct pwm_device *pwm)
> +{
> +	if (pwm->chip->ops->get_output_type_supported != NULL)
> +		return pwm->chip->ops->get_output_type_supported(pwm->chip,
> +				pwm);
> +	else
> +		return PWM_OUTPUT_FIXED;
> +}
> +
> +/**
>   * pwm_config() - change a PWM device configuration
>   * @pwm: PWM device
>   * @duty_ns: "on" time (in nanoseconds)
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
Uwe Kleine-König Sept. 16, 2019, 6:25 p.m. UTC | #2
Hello,

On Fri, Sep 13, 2019 at 03:57:43PM -0700, Guru Das Srinagesh wrote:
> From: Fenglin Wu <fenglinw@codeaurora.org>
> 
> Normally, PWM channel has fixed output until software request to change
> its settings. There are some PWM devices which their outputs could be
> changed autonomously according to a predefined pattern programmed in
> hardware. Add pwm_output_type enum type to identify these two different
> PWM types and add relevant helper functions to set and get PWM output
> types and pattern.

I have problems to understand what your modulated mode does even after
reading your commit log and the patch.

Also you should note here what is the intended usage and add support for
it for at least one (preferably more) drivers to make this actually
usable.

> Change-Id: Ia1f914a45ab4f4dd7be037a395eeb89d0e65a80e

In Linux we don't use these. You're making it easier to apply your patch
if you drop the change-id lines.

> diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> index 2389b86..ab703f2 100644
> --- a/drivers/pwm/sysfs.c
> +++ b/drivers/pwm/sysfs.c
> @@ -215,11 +215,60 @@ static ssize_t capture_show(struct device *child,
>  	return sprintf(buf, "%u %u\n", result.period, result.duty_cycle);
>  }
>  
> +static ssize_t output_type_show(struct device *child,
> +			     struct device_attribute *attr,
> +			     char *buf)
> +{
> +	const struct pwm_device *pwm = child_to_pwm_device(child);
> +	const char *output_type = "unknown";
> +	struct pwm_state state;
> +
> +	pwm_get_state(pwm, &state);
> +	switch (state.output_type) {
> +	case PWM_OUTPUT_FIXED:
> +		output_type = "fixed";
> +		break;
> +	case PWM_OUTPUT_MODULATED:
> +		output_type = "modulated";
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n", output_type);
> +}
> +
> +static ssize_t output_type_store(struct device *child,
> +			      struct device_attribute *attr,
> +			      const char *buf, size_t size)
> +{
> +	struct pwm_export *export = child_to_pwm_export(child);
> +	struct pwm_device *pwm = export->pwm;
> +	struct pwm_state state;
> +	int ret = -EINVAL;
> +
> +	mutex_lock(&export->lock);
> +	pwm_get_state(pwm, &state);
> +	if (sysfs_streq(buf, "fixed"))
> +		state.output_type = PWM_OUTPUT_FIXED;
> +	else if (sysfs_streq(buf, "modulated"))
> +		state.output_type = PWM_OUTPUT_MODULATED;
> +	else
> +		goto unlock;
> +
> +	ret = pwm_apply_state(pwm, &state);
> +unlock:
> +	mutex_unlock(&export->lock);
> +
> +	return ret ? : size;
> +}

So in sysfs you cannot set a pattern. Doesn't that mean this makes using
modulated mode hardly useful?

Best regards
Uwe
Guru Das Srinagesh Sept. 24, 2019, 5:43 a.m. UTC | #3
On Mon, Sep 16, 2019 at 04:01:46PM +0200, Thierry Reding wrote:
> On Fri, Sep 13, 2019 at 03:57:43PM -0700, Guru Das Srinagesh wrote:
> > From: Fenglin Wu <fenglinw@codeaurora.org>
> > 
> > Normally, PWM channel has fixed output until software request to change
> > its settings. There are some PWM devices which their outputs could be
> > changed autonomously according to a predefined pattern programmed in
> > hardware. Add pwm_output_type enum type to identify these two different
> > PWM types and add relevant helper functions to set and get PWM output
> > types and pattern.
> > 
> > Change-Id: Ia1f914a45ab4f4dd7be037a395eeb89d0e65a80e
> > Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
> > Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> > ---
> >  drivers/pwm/core.c  | 26 ++++++++++++++++++++
> >  drivers/pwm/sysfs.c | 50 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/pwm.h | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 146 insertions(+)
> 
> This doesn't seem right to me. Are you describing a PWM pin that's
> actually driven in GPIO mode? We usually configure that using pinctrl.
> 
> Thierry

Sorry, let me clarify.

Some Qualcomm PMICs have a PWM block called the Light Pulse Generator (LPG).
This block allows for the generation of a HW-controlled PWM "pattern", i.e. a
sequential altering of duty cycle, in addition to the normal PWM "fixed" duty
cycle operation, which is what the framework does currently. This pattern is
user-configurable in the form of a look-up table in the devicetree. The LPG's
registers have to be configured with the data in the look up table in order to
start the generation of the pattern. An example of a pattern is the "breath"
pattern, which simply ramps up the duty cycle and then ramps it down.

This "pattern" mode is what has been defined as PWM_OUTPUT_MODULATED in this
patch. I see that the use of the term "modulated" is misleading - a more
accurate term would be PWM_OUTPUT_PATTERN perhaps.

This patch merely adds framework support to differentiate between the "fixed"
and "pattern" modes of operation. Actions such as configuring the LPG with the
devicetree pattern and setting it up for generating the pattern are performed
in the driver only if the output type is read as "pattern" and not otherwise.
Guru Das Srinagesh Sept. 24, 2019, 6:01 a.m. UTC | #4
On Mon, Sep 16, 2019 at 08:25:24PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Sep 13, 2019 at 03:57:43PM -0700, Guru Das Srinagesh wrote:
> > From: Fenglin Wu <fenglinw@codeaurora.org>
> > 
> > Normally, PWM channel has fixed output until software request to change
> > its settings. There are some PWM devices which their outputs could be
> > changed autonomously according to a predefined pattern programmed in
> > hardware. Add pwm_output_type enum type to identify these two different
> > PWM types and add relevant helper functions to set and get PWM output
> > types and pattern.
> 
> I have problems to understand what your modulated mode does even after
> reading your commit log and the patch.

Hi Uwe,

I have posted a reply to Thierry's email in which I provide some
background and details about this patch for your review. Hopefully that
clarifies things - I can provide some more clarifications if necessary.

> 
> Also you should note here what is the intended usage and add support for
> it for at least one (preferably more) drivers to make this actually
> usable.

The PWM_OUTPUT_MODULATED mode is useful for PWM peripherals like
Qualcomm's Light Pulse Generator (LPG) that can output a duty-cycle
pattern in hardware as well as output a fixed duty-cycle signal. All
this mode really does is to provide a way for drivers to carry out some
actions for the "pattern" mode of operation that are not required (or
relevant) for the "fixed" mode of operation.

> 
> > Change-Id: Ia1f914a45ab4f4dd7be037a395eeb89d0e65a80e
> 
> In Linux we don't use these. You're making it easier to apply your patch
> if you drop the change-id lines.

Sorry, forgot to strip these before sending out the patch.

> 
> > diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> > index 2389b86..ab703f2 100644
> > --- a/drivers/pwm/sysfs.c
> > +++ b/drivers/pwm/sysfs.c
> > @@ -215,11 +215,60 @@ static ssize_t capture_show(struct device *child,
> >  	return sprintf(buf, "%u %u\n", result.period, result.duty_cycle);
> >  }
> >  
> > +static ssize_t output_type_show(struct device *child,
> > +			     struct device_attribute *attr,
> > +			     char *buf)
> > +{
> > +	const struct pwm_device *pwm = child_to_pwm_device(child);
> > +	const char *output_type = "unknown";
> > +	struct pwm_state state;
> > +
> > +	pwm_get_state(pwm, &state);
> > +	switch (state.output_type) {
> > +	case PWM_OUTPUT_FIXED:
> > +		output_type = "fixed";
> > +		break;
> > +	case PWM_OUTPUT_MODULATED:
> > +		output_type = "modulated";
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return snprintf(buf, PAGE_SIZE, "%s\n", output_type);
> > +}
> > +
> > +static ssize_t output_type_store(struct device *child,
> > +			      struct device_attribute *attr,
> > +			      const char *buf, size_t size)
> > +{
> > +	struct pwm_export *export = child_to_pwm_export(child);
> > +	struct pwm_device *pwm = export->pwm;
> > +	struct pwm_state state;
> > +	int ret = -EINVAL;
> > +
> > +	mutex_lock(&export->lock);
> > +	pwm_get_state(pwm, &state);
> > +	if (sysfs_streq(buf, "fixed"))
> > +		state.output_type = PWM_OUTPUT_FIXED;
> > +	else if (sysfs_streq(buf, "modulated"))
> > +		state.output_type = PWM_OUTPUT_MODULATED;
> > +	else
> > +		goto unlock;
> > +
> > +	ret = pwm_apply_state(pwm, &state);
> > +unlock:
> > +	mutex_unlock(&export->lock);
> > +
> > +	return ret ? : size;
> > +}
> 
> So in sysfs you cannot set a pattern. Doesn't that mean this makes using
> modulated mode hardly useful?

The pattern has to be set through the devicetree and not sysfs. Will
change the output type sysfs parameter to read-only.

> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Uwe Kleine-König Sept. 24, 2019, 6:39 a.m. UTC | #5
On Mon, Sep 23, 2019 at 10:43:43PM -0700, Guru Das Srinagesh wrote:
> On Mon, Sep 16, 2019 at 04:01:46PM +0200, Thierry Reding wrote:
> > On Fri, Sep 13, 2019 at 03:57:43PM -0700, Guru Das Srinagesh wrote:
> > > From: Fenglin Wu <fenglinw@codeaurora.org>
> > > 
> > > Normally, PWM channel has fixed output until software request to change
> > > its settings. There are some PWM devices which their outputs could be
> > > changed autonomously according to a predefined pattern programmed in
> > > hardware. Add pwm_output_type enum type to identify these two different
> > > PWM types and add relevant helper functions to set and get PWM output
> > > types and pattern.
> > > 
> > > Change-Id: Ia1f914a45ab4f4dd7be037a395eeb89d0e65a80e
> > > Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
> > > Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> > > ---
> > >  drivers/pwm/core.c  | 26 ++++++++++++++++++++
> > >  drivers/pwm/sysfs.c | 50 ++++++++++++++++++++++++++++++++++++++
> > >  include/linux/pwm.h | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 146 insertions(+)
> > 
> > This doesn't seem right to me. Are you describing a PWM pin that's
> > actually driven in GPIO mode? We usually configure that using pinctrl.
> > 
> > Thierry
> 
> Sorry, let me clarify.
> 
> Some Qualcomm PMICs have a PWM block called the Light Pulse Generator (LPG).
> This block allows for the generation of a HW-controlled PWM "pattern", i.e. a
> sequential altering of duty cycle, in addition to the normal PWM "fixed" duty
> cycle operation, which is what the framework does currently. This pattern is
> user-configurable in the form of a look-up table in the devicetree. The LPG's
> registers have to be configured with the data in the look up table in order to
> start the generation of the pattern. An example of a pattern is the "breath"
> pattern, which simply ramps up the duty cycle and then ramps it down.

I'll try to describe it in my words to check if I got it right: So the
mode you want to add needs a sequence of PWM states and the hardware is
expected to apply them in turn, each for a configurable count of
periods. If I understand this right, this is expected to be cyclic?

> This "pattern" mode is what has been defined as PWM_OUTPUT_MODULATED in this
> patch. I see that the use of the term "modulated" is misleading - a more
> accurate term would be PWM_OUTPUT_PATTERN perhaps.

Not sure "pattern" is better. 

The PWM on the newer imx SoCs (using the imx27 driver) has a FIFO with
length 4 that allows to program changing settings. Only the duty cycle
can be modified and as repeat count only 1, 2, 4 and 8 are available. I
assume the FIFO can be fed by the dma engine.

Note I only know this feature from reading the reference manual and
never used it.

> This patch merely adds framework support to differentiate between the "fixed"
> and "pattern" modes of operation. Actions such as configuring the LPG with the
> devicetree pattern and setting it up for generating the pattern are performed
> in the driver only if the output type is read as "pattern" and not otherwise.

Up to now I'm not convinced that this extension is a good one that can
be supported by several PWM implementations. I'd say we should collect
first some details about different implementations and what these could
implement to get a picture what kind of API is sensible.

Best regards
Uwe
Thierry Reding Sept. 24, 2019, 10:46 a.m. UTC | #6
On Tue, Sep 24, 2019 at 08:39:26AM +0200, Uwe Kleine-König wrote:
> On Mon, Sep 23, 2019 at 10:43:43PM -0700, Guru Das Srinagesh wrote:
> > On Mon, Sep 16, 2019 at 04:01:46PM +0200, Thierry Reding wrote:
> > > On Fri, Sep 13, 2019 at 03:57:43PM -0700, Guru Das Srinagesh wrote:
> > > > From: Fenglin Wu <fenglinw@codeaurora.org>
> > > > 
> > > > Normally, PWM channel has fixed output until software request to change
> > > > its settings. There are some PWM devices which their outputs could be
> > > > changed autonomously according to a predefined pattern programmed in
> > > > hardware. Add pwm_output_type enum type to identify these two different
> > > > PWM types and add relevant helper functions to set and get PWM output
> > > > types and pattern.
> > > > 
> > > > Change-Id: Ia1f914a45ab4f4dd7be037a395eeb89d0e65a80e
> > > > Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
> > > > Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> > > > ---
> > > >  drivers/pwm/core.c  | 26 ++++++++++++++++++++
> > > >  drivers/pwm/sysfs.c | 50 ++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/pwm.h | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 146 insertions(+)
> > > 
> > > This doesn't seem right to me. Are you describing a PWM pin that's
> > > actually driven in GPIO mode? We usually configure that using pinctrl.
> > > 
> > > Thierry
> > 
> > Sorry, let me clarify.
> > 
> > Some Qualcomm PMICs have a PWM block called the Light Pulse Generator (LPG).
> > This block allows for the generation of a HW-controlled PWM "pattern", i.e. a
> > sequential altering of duty cycle, in addition to the normal PWM "fixed" duty
> > cycle operation, which is what the framework does currently. This pattern is
> > user-configurable in the form of a look-up table in the devicetree. The LPG's
> > registers have to be configured with the data in the look up table in order to
> > start the generation of the pattern. An example of a pattern is the "breath"
> > pattern, which simply ramps up the duty cycle and then ramps it down.
> 
> I'll try to describe it in my words to check if I got it right: So the
> mode you want to add needs a sequence of PWM states and the hardware is
> expected to apply them in turn, each for a configurable count of
> periods. If I understand this right, this is expected to be cyclic?
> 
> > This "pattern" mode is what has been defined as PWM_OUTPUT_MODULATED in this
> > patch. I see that the use of the term "modulated" is misleading - a more
> > accurate term would be PWM_OUTPUT_PATTERN perhaps.
> 
> Not sure "pattern" is better. 
> 
> The PWM on the newer imx SoCs (using the imx27 driver) has a FIFO with
> length 4 that allows to program changing settings. Only the duty cycle
> can be modified and as repeat count only 1, 2, 4 and 8 are available. I
> assume the FIFO can be fed by the dma engine.
> 
> Note I only know this feature from reading the reference manual and
> never used it.
> 
> > This patch merely adds framework support to differentiate between the "fixed"
> > and "pattern" modes of operation. Actions such as configuring the LPG with the
> > devicetree pattern and setting it up for generating the pattern are performed
> > in the driver only if the output type is read as "pattern" and not otherwise.
> 
> Up to now I'm not convinced that this extension is a good one that can
> be supported by several PWM implementations. I'd say we should collect
> first some details about different implementations and what these could
> implement to get a picture what kind of API is sensible.

I agree. This sounds to me like it's stretching the concept of a PWM a
bit too much. Sounds like drivers/leds might be a better fit for this.

Thierry

Patch
diff mbox series

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 8edfac1..960a451 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -282,6 +282,7 @@  int pwmchip_add_with_polarity(struct pwm_chip *chip,
 		pwm->pwm = chip->base + i;
 		pwm->hwpwm = i;
 		pwm->state.polarity = polarity;
+		pwm->state.output_type = PWM_OUTPUT_FIXED;
 
 		if (chip->ops->get_state)
 			chip->ops->get_state(chip, pwm, &pwm->state);
@@ -498,6 +499,31 @@  int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
 			pwm->state.polarity = state->polarity;
 		}
 
+		if (state->output_type != pwm->state.output_type) {
+			if (!pwm->chip->ops->set_output_type)
+				return -ENOTSUPP;
+
+			err = pwm->chip->ops->set_output_type(pwm->chip, pwm,
+						state->output_type);
+			if (err)
+				return err;
+
+			pwm->state.output_type = state->output_type;
+		}
+
+		if (state->output_pattern != pwm->state.output_pattern &&
+				state->output_pattern != NULL) {
+			if (!pwm->chip->ops->set_output_pattern)
+				return -ENOTSUPP;
+
+			err = pwm->chip->ops->set_output_pattern(pwm->chip,
+					pwm, state->output_pattern);
+			if (err)
+				return err;
+
+			pwm->state.output_pattern = state->output_pattern;
+		}
+
 		if (state->period != pwm->state.period ||
 		    state->duty_cycle != pwm->state.duty_cycle) {
 			err = pwm->chip->ops->config(pwm->chip, pwm,
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 2389b86..ab703f2 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -215,11 +215,60 @@  static ssize_t capture_show(struct device *child,
 	return sprintf(buf, "%u %u\n", result.period, result.duty_cycle);
 }
 
+static ssize_t output_type_show(struct device *child,
+			     struct device_attribute *attr,
+			     char *buf)
+{
+	const struct pwm_device *pwm = child_to_pwm_device(child);
+	const char *output_type = "unknown";
+	struct pwm_state state;
+
+	pwm_get_state(pwm, &state);
+	switch (state.output_type) {
+	case PWM_OUTPUT_FIXED:
+		output_type = "fixed";
+		break;
+	case PWM_OUTPUT_MODULATED:
+		output_type = "modulated";
+		break;
+	default:
+		break;
+	}
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", output_type);
+}
+
+static ssize_t output_type_store(struct device *child,
+			      struct device_attribute *attr,
+			      const char *buf, size_t size)
+{
+	struct pwm_export *export = child_to_pwm_export(child);
+	struct pwm_device *pwm = export->pwm;
+	struct pwm_state state;
+	int ret = -EINVAL;
+
+	mutex_lock(&export->lock);
+	pwm_get_state(pwm, &state);
+	if (sysfs_streq(buf, "fixed"))
+		state.output_type = PWM_OUTPUT_FIXED;
+	else if (sysfs_streq(buf, "modulated"))
+		state.output_type = PWM_OUTPUT_MODULATED;
+	else
+		goto unlock;
+
+	ret = pwm_apply_state(pwm, &state);
+unlock:
+	mutex_unlock(&export->lock);
+
+	return ret ? : size;
+}
+
 static DEVICE_ATTR_RW(period);
 static DEVICE_ATTR_RW(duty_cycle);
 static DEVICE_ATTR_RW(enable);
 static DEVICE_ATTR_RW(polarity);
 static DEVICE_ATTR_RO(capture);
+static DEVICE_ATTR_RW(output_type);
 
 static struct attribute *pwm_attrs[] = {
 	&dev_attr_period.attr,
@@ -227,6 +276,7 @@  static ssize_t capture_show(struct device *child,
 	&dev_attr_enable.attr,
 	&dev_attr_polarity.attr,
 	&dev_attr_capture.attr,
+	&dev_attr_output_type.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(pwm);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 24632a7..416f08e 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -48,6 +48,29 @@  enum {
 	PWMF_EXPORTED = 1 << 1,
 };
 
+/**
+ * enum pwm_output_type - output type of the PWM signal
+ * @PWM_OUTPUT_FIXED: PWM output is fixed until a change request
+ * @PWM_OUTPUT_MODULATED: PWM output is modulated in hardware
+ * autonomously with a predefined pattern
+ */
+enum pwm_output_type {
+	PWM_OUTPUT_FIXED = 1 << 0,
+	PWM_OUTPUT_MODULATED = 1 << 1,
+};
+
+/**
+ * struct pwm_output_pattern - PWM duty pattern for MODULATED duty type
+ * @duty_pattern: PWM duty cycles in the pattern for duty modulation
+ * @num_entries: number of entries in the pattern
+ * @cycles_per_duty: number of PWM period cycles an entry stays at
+ */
+struct pwm_output_pattern {
+	unsigned int *duty_pattern;
+	unsigned int num_entries;
+	unsigned int cycles_per_duty;
+};
+
 /*
  * struct pwm_state - state of a PWM channel
  * @period: PWM period (in nanoseconds)
@@ -59,6 +82,8 @@  struct pwm_state {
 	unsigned int period;
 	unsigned int duty_cycle;
 	enum pwm_polarity polarity;
+	enum pwm_output_type output_type;
+	struct pwm_output_pattern *output_pattern;
 	bool enabled;
 };
 
@@ -144,6 +169,26 @@  static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
 	return state.polarity;
 }
 
+static inline enum pwm_output_type pwm_get_output_type(
+		const struct pwm_device *pwm)
+{
+	struct pwm_state state;
+
+	pwm_get_state(pwm, &state);
+
+	return state.output_type;
+}
+
+static inline struct pwm_output_pattern *pwm_get_output_pattern(
+				struct pwm_device *pwm)
+{
+	struct pwm_state state;
+
+	pwm_get_state(pwm, &state);
+
+	return pwm->state.output_pattern ?: NULL;
+}
+
 static inline void pwm_get_args(const struct pwm_device *pwm,
 				struct pwm_args *args)
 {
@@ -250,6 +295,9 @@  static inline void pwm_init_state(const struct pwm_device *pwm,
  * @get_state: get the current PWM state. This function is only
  *	       called once per PWM device when the PWM chip is
  *	       registered.
+ * @get_output_type_supported: get the supported output type
+ * @set_output_type: set PWM output type
+ * @set_output_pattern: set the pattern for the modulated output
  * @owner: helps prevent removal of modules exporting active PWMs
  * @config: configure duty cycles and period length for this PWM
  * @set_polarity: configure the polarity of this PWM
@@ -265,6 +313,13 @@  struct pwm_ops {
 		     struct pwm_state *state);
 	void (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,
 			  struct pwm_state *state);
+	int (*get_output_type_supported)(struct pwm_chip *chip,
+			struct pwm_device *pwm);
+	int (*set_output_type)(struct pwm_chip *chip, struct pwm_device *pwm,
+			enum pwm_output_type output_type);
+	int (*set_output_pattern)(struct pwm_chip *chip,
+			struct pwm_device *pwm,
+			struct pwm_output_pattern *output_pattern);
 	struct module *owner;
 
 	/* Only used by legacy drivers */
@@ -320,6 +375,21 @@  struct pwm_capture {
 int pwm_adjust_config(struct pwm_device *pwm);
 
 /**
+ * pwm_output_type_support()
+ * @pwm: PWM device
+ *
+ * Returns:  output types supported by the PWM device
+ */
+static inline int pwm_get_output_type_supported(struct pwm_device *pwm)
+{
+	if (pwm->chip->ops->get_output_type_supported != NULL)
+		return pwm->chip->ops->get_output_type_supported(pwm->chip,
+				pwm);
+	else
+		return PWM_OUTPUT_FIXED;
+}
+
+/**
  * pwm_config() - change a PWM device configuration
  * @pwm: PWM device
  * @duty_ns: "on" time (in nanoseconds)