| 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 | expand |
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 >
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
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.
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/ |
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
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
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 This is not needed an a upstream patch, checkpatch should have told you :( > 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 ++++++++++++++++++++++++++++++++++++++ You forgot a Documentation/ABI/ update for your new sysfs file :( thanks, greg k-h
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)