| Message ID | 1589723560-5734-2-git-send-email-vineetha.g.jaya.kumaran@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add PWM support for Intel Keem Bay SoC | expand |
On Sun, May 17, 2020 at 09:52:38PM +0800, vineetha.g.jaya.kumaran@intel.com wrote: > From: "Lai, Poey Seng" <poey.seng.lai@intel.com> > > In Keem Bay, the number of repetitions for the period/waveform > can be configured from userspace. This requires addition of a sysfs > attribute to get/set the repetition count. Setting this value to 0 > will result in continuous repetition of the waveform until the > channel is disabled or reconfigured. There is nothing specific for Keem Bay in this patch, the only special thing here is that this driver is the first implementor. IMHO all drivers that don't support count should be changed to fail if a count > 0 is passed and introducing support in the sysfs interface should be a separate change. Having said that I'm not convinced this is a good idea given that only very few driver can support this interface. Also this needs documentation, e.g. what is expected from .get_state(). You should also motivate what this functionality is used for in the commit log and I'd prefer to see an in-tree user (apart from sysfs which I don't count as such). Best regards Uwe
> -----Original Message----- > From: linux-pwm-owner@vger.kernel.org <linux-pwm-owner@vger.kernel.org> > On Behalf Of Uwe Kleine-König > Sent: Sunday, May 24, 2020 5:05 AM > To: G Jaya Kumaran, Vineetha <vineetha.g.jaya.kumaran@intel.com> > Cc: thierry.reding@gmail.com; robh+dt@kernel.org; linux- > pwm@vger.kernel.org; devicetree@vger.kernel.org; Wan Mohamad, Wan > Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; Shevchenko, > Andriy <andriy.shevchenko@intel.com> > Subject: Re: [PATCH 1/3] pwm: Add count attribute in sysfs for Intel Keem Bay > > On Sun, May 17, 2020 at 09:52:38PM +0800, > vineetha.g.jaya.kumaran@intel.com wrote: > > From: "Lai, Poey Seng" <poey.seng.lai@intel.com> > > > > In Keem Bay, the number of repetitions for the period/waveform can be > > configured from userspace. This requires addition of a sysfs attribute > > to get/set the repetition count. Setting this value to 0 will result > > in continuous repetition of the waveform until the channel is disabled > > or reconfigured. > > There is nothing specific for Keem Bay in this patch, the only special thing here is > that this driver is the first implementor. > > IMHO all drivers that don't support count should be changed to fail if a count > 0 > is passed and introducing support in the sysfs interface should be a separate > change. > > Having said that I'm not convinced this is a good idea given that only very few > driver can support this interface. Also this needs documentation, e.g. what is > expected from .get_state(). > > You should also motivate what this functionality is used for in the commit log > and I'd prefer to see an in-tree user (apart from sysfs which I don't count as > such). > Agreed, the wording used here is not accurate as this is not specific for Keem Bay. Before submitting v2, I will cross-check about the use-case for this functionality, and if it can be implemented in some other less intrusive way to the framework (perhaps via a DT property?) > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
diff --git a/Documentation/ABI/testing/sysfs-class-pwm b/Documentation/ABI/testing/sysfs-class-pwm index c20e613..87e219f 100644 --- a/Documentation/ABI/testing/sysfs-class-pwm +++ b/Documentation/ABI/testing/sysfs-class-pwm @@ -86,3 +86,12 @@ Description: Capture information about a PWM signal. The output format is a pair unsigned integers (period and duty cycle), separated by a single space. + +What: /sys/class/pwm/pwmchipN/pwmX/count +Date: May 2020 +KernelVersion: 5.6 +Contact: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> +Description: + Sets the repetition count of a PWM waveform. A value of 0 will + result in continuous repetition of the waveform until the + channel is disabled or reconfigured. diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index bca0496..fd42fb6 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -584,7 +584,8 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) if (state->period == pwm->state.period && state->duty_cycle == pwm->state.duty_cycle && state->polarity == pwm->state.polarity && - state->enabled == pwm->state.enabled) + state->enabled == pwm->state.enabled && + state->count == pwm->state.count) return 0; if (chip->ops->apply) { diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c index 2389b86..3c474fa 100644 --- a/drivers/pwm/sysfs.c +++ b/drivers/pwm/sysfs.c @@ -215,11 +215,47 @@ static ssize_t capture_show(struct device *child, return sprintf(buf, "%u %u\n", result.period, result.duty_cycle); } +static ssize_t count_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; + unsigned int val; + int ret; + + ret = kstrtouint(buf, 0, &val); + if (ret) + return ret; + + mutex_lock(&export->lock); + pwm_get_state(pwm, &state); + state.count = val; + ret = pwm_apply_state(pwm, &state); + mutex_unlock(&export->lock); + + return ret ? : size; +} + +static ssize_t count_show(struct device *child, + struct device_attribute *attr, + char *buf) +{ + const struct pwm_device *pwm = child_to_pwm_device(child); + struct pwm_state state; + + pwm_get_state(pwm, &state); + + return sprintf(buf, "%d\n", state.count); +} + 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(count); static struct attribute *pwm_attrs[] = { &dev_attr_period.attr, @@ -227,6 +263,7 @@ static ssize_t capture_show(struct device *child, &dev_attr_enable.attr, &dev_attr_polarity.attr, &dev_attr_capture.attr, + &dev_attr_count.attr, NULL }; ATTRIBUTE_GROUPS(pwm); diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 2635b2a..c874559 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -52,12 +52,14 @@ enum { * struct pwm_state - state of a PWM channel * @period: PWM period (in nanoseconds) * @duty_cycle: PWM duty cycle (in nanoseconds) + * @count: Repeat count of PWM waveforms. * @polarity: PWM polarity * @enabled: PWM enabled status */ struct pwm_state { unsigned int period; unsigned int duty_cycle; + unsigned int count; enum pwm_polarity polarity; bool enabled; };