Re: [RESEND 02/11] pwm: sysfs: Add PWM Capture support

[Lee Jones <lee.jones@linaro.org>]

On Tue, 12 Apr 2016, Thierry Reding wrote:

> On Wed, Mar 02, 2016 at 03:32:00PM +0000, Lee Jones wrote:
> > Allow a user to read PWM Capture results from /sysfs. First,
> > the user must tell PWM Capture which channel they wish to
> > read from:
> > 
> >   $ echo 2 > $PWMCHIP/capture
> > 
> > To start a capture and read the result, simply read the file:
> > 
> >   $ cat $PWMCHIP/capture
> > 
> > The output format is left to the device.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/pwm/sysfs.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> > index 9c90886..3572ef4 100644
> > --- a/drivers/pwm/sysfs.c
> > +++ b/drivers/pwm/sysfs.c
> > @@ -23,6 +23,8 @@
> >  #include <linux/kdev_t.h>
> >  #include <linux/pwm.h>
> >  
> > +static int capture_channel;
> 
> Can't do that, this is very racy because it isn't protected by any lock.
> Fortunately I don't think the global variable is at all necessary. See
> below.
> 
> > +
> >  struct pwm_export {
> >  	struct device child;
> >  	struct pwm_device *pwm;
> > @@ -167,16 +169,42 @@ static ssize_t polarity_store(struct device *child,
> >  	return ret ? : size;
> >  }
> >  
> > +static ssize_t capture_show(struct device *child,
> > +			    struct device_attribute *attr,
> > +			    char *buf)
> > +{
> > +	struct pwm_device *pwm = child_to_pwm_device(child);
> > +
> > +	return pwm_capture(pwm, capture_channel, buf);
> > +}
> > +
> > +static ssize_t capture_store(struct device *child,
> > +			     struct device_attribute *attr,
> > +			     const char *buf, size_t size)
> > +{
> > +	int val, ret;
> > +
> > +	ret = kstrtoint(buf, 0, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	capture_channel = val;
> > +
> > +	return 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_RW(capture);
> 
> These are all per-PWM attributes and the specific PWM device that they
> are associated with can be retrieved using child_to_pwm_device(child)
> (see the other attributes' implementation for examples). So I don't
> think the capture attribute needs to be writable at all. You already
> implement capture_show() in almost the right way, and if you drop the
> channel parameter from pwm_capture() as I suggested in my reply to patch
> 1/11 this should resolve itself automatically.
> 
> Of course capture_show() would become slightly more beefy if we return a
> standard result structure rather than leave it up to the drivers to fill
> out the sysfs string. The good thing is that it will be common code and
> therefore the sysfs interface would return the same format regardless of
> the driver.
> 
> Perhaps something like
> 
> 	struct pwm_device *pwm = child_to_pwm_device(child);
> 	struct pwm_capture result;
> 
> 	err = pwm_capture(pwm, &result);
> 	if (err < 0)
> 		return err;
> 
> 	return sprintf(buf, "%u %u\n", result.duty_cycle, result.period);
> 	
> would work?

Same reply as 1/11.  Now I know that we should be treating each of our
channels, as *completely* separate devices, I think this method seems
reasonable.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog