Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1376647 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RESEND 02/11] pwm: sysfs: Add PWM Capture support Thierry Reding <thierry.reding@gmail.com> - 2016-04-12 12:20 +0200
    Re: [RESEND 02/11] pwm: sysfs: Add PWM Capture support Lee Jones <lee.jones@linaro.org> - 2016-04-13 11:50 +0200

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

[Multipart message — attachments visible in raw view] — view raw

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?

Thierry

[toc] | [next] | [standalone]

#1377699

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web