LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Thierry Reding <thierry.reding@gmail.com>
To: Guru Das Srinagesh <gurus@codeaurora.org>
Cc: linux-pwm@vger.kernel.org, kernel-team@android.com,
	Mark Salyzyn <salyzyn@google.com>,
	Sandeep Patil <sspatil@google.com>,
	Subbaraman Narayanamurthy <subbaram@codeaurora.org>,
	linux-kernel@vger.kernel.org,
	Fenglin Wu <fenglinw@codeaurora.org>
Subject: Re: [PATCH 1/2] pwm: Add different PWM output types support
Date: Mon, 16 Sep 2019 16:01:46 +0200
Message-ID: <20190916140146.GC7488@ulmo> (raw)
In-Reply-To: <1568415464-20267-1-git-send-email-gurus@codeaurora.org>


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

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  parent reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-13 22:57 Guru Das Srinagesh
2019-09-13 22:57 ` [PATCH 2/2] pwm: core: Add option to config PWM duty/period with u64 data length Guru Das Srinagesh
2019-09-16 13:59   ` kbuild test robot
2019-09-16 14:00   ` Thierry Reding
2019-09-16 14:07   ` kbuild test robot
2019-09-16 14:01 ` Thierry Reding [this message]
2019-09-24  5:43   ` [PATCH 1/2] pwm: Add different PWM output types support Guru Das Srinagesh
2019-09-24  6:39     ` Uwe Kleine-König
2019-09-24 10:46       ` Thierry Reding
2019-09-16 18:25 ` Uwe Kleine-König
2019-09-24  6:01   ` Guru Das Srinagesh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190916140146.GC7488@ulmo \
    --to=thierry.reding@gmail.com \
    --cc=fenglinw@codeaurora.org \
    --cc=gurus@codeaurora.org \
    --cc=kernel-team@android.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=salyzyn@google.com \
    --cc=sspatil@google.com \
    --cc=subbaram@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git