[RESEND 00/11] pwm: Add support for PWM Capture

[RESEND 00/11] pwm: Add support for PWM Capture

[Lee Jones]

The first part of this set extends the current PWM API to allow external
code to request a PWM Capture.  Subsequent patches then make use of the
new API by providing a userspace offering via /sysfs.  The final part of
the set supplies PWM Capture functionality into the already existing STi
PWM driver.
 
This patch-set has been tested end to end via /sysfs.

Lee Jones (11):
  pwm: Add PWM Capture support
  pwm: sysfs: Add PWM Capture support
  pwm: sti: Reorganise register names in preparation for new
    functionality
  pwm: sti: Only request clock rate when you need to
  pwm: sti: Supply PWM Capture register addresses and bit locations
  pwm: sti: Supply PWM Capture clock handling
  pwm: sti: Initialise PWM Capture channel data
  pwm: sti: Add support for PWM Capture IRQs
  pwm: sti: Add PWM Capture call-back
  pwm: sti: Enable PWM Capture
  pwm: sti: Take the opportunity to conduct a little house keeping

 drivers/pwm/core.c    |  26 ++++
 drivers/pwm/pwm-sti.c | 384 ++++++++++++++++++++++++++++++++++++++++++--------
 drivers/pwm/sysfs.c   |  28 ++++
 include/linux/pwm.h   |  13 ++
 4 files changed, 392 insertions(+), 59 deletions(-)

-- 
1.9.1

[RESEND 01/11] pwm: Add PWM Capture support

[Lee Jones]

Supply a PWM Capture call-back Op in order to pass back
information obtained by running analysis on PWM a signal.
This would normally (at least during testing) be called from
the Sysfs routines with a view to printing out PWM Capture
data which has been encoded into a string.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/core.c  | 26 ++++++++++++++++++++++++++
 include/linux/pwm.h | 13 +++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index d24ca5f..8f4a8a9 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -494,6 +494,32 @@ unlock:
 EXPORT_SYMBOL_GPL(pwm_set_polarity);
 
 /**
+ * pwm_capture() - capture and report a PWM signal
+ * @pwm: PWM device
+ * @channel: PWM capture channel to use
+ * @buf: buffer to place output message into
+ *
+ * Returns: 0 on success or a negative error code on failure.
+ */
+int pwm_capture(struct pwm_device *pwm, int channel, char *buf)
+{
+	int err;
+
+	if (!pwm || !pwm->chip->ops)
+		return -EINVAL;
+
+	if (!pwm->chip->ops->capture)
+		return -ENOSYS;
+
+	mutex_lock(&pwm->lock);
+	err = pwm->chip->ops->capture(pwm->chip, pwm, channel, buf);
+	mutex_unlock(&pwm->lock);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(pwm_capture);
+
+/**
  * pwm_enable() - start a PWM output toggling
  * @pwm: PWM device
  *
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index cfc3ed4..7bcff6b 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -33,6 +33,11 @@ int pwm_enable(struct pwm_device *pwm);
  * pwm_disable - stop a PWM output toggling
  */
 void pwm_disable(struct pwm_device *pwm);
+
+/*
+ * pwm_capture - capture and report a PWM signal
+ */
+int pwm_capture(struct pwm_device *pwm, int channel, char *buf);
 #else
 static inline struct pwm_device *pwm_request(int pwm_id, const char *label)
 {
@@ -56,6 +61,11 @@ static inline int pwm_enable(struct pwm_device *pwm)
 static inline void pwm_disable(struct pwm_device *pwm)
 {
 }
+
+static inline int pwm_capture(struct pwm_device *pwm, int channel, char *buf)
+{
+	return -EINVAL;
+}
 #endif
 
 struct pwm_chip;
@@ -150,6 +160,7 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
  * @free: optional hook for freeing a PWM
  * @config: configure duty cycles and period length for this PWM
  * @set_polarity: configure the polarity of this PWM
+ * @capture: capture and report PWM signal
  * @enable: enable PWM output toggling
  * @disable: disable PWM output toggling
  * @dbg_show: optional routine to show contents in debugfs
@@ -162,6 +173,8 @@ struct pwm_ops {
 		      int duty_ns, int period_ns);
 	int (*set_polarity)(struct pwm_chip *chip, struct pwm_device *pwm,
 			    enum pwm_polarity polarity);
+	int (*capture)(struct pwm_chip *chip, struct pwm_device *pwm,
+		       int channel, char *buf);
 	int (*enable)(struct pwm_chip *chip, struct pwm_device *pwm);
 	void (*disable)(struct pwm_chip *chip, struct pwm_device *pwm);
 #ifdef CONFIG_DEBUG_FS
-- 
1.9.1

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

[Thierry Reding]

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

On Wed, Mar 02, 2016 at 03:31:59PM +0000, Lee Jones wrote:
> Supply a PWM Capture call-back Op in order to pass back
> information obtained by running analysis on PWM a signal.
> This would normally (at least during testing) be called from
> the Sysfs routines with a view to printing out PWM Capture
> data which has been encoded into a string.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/pwm/core.c  | 26 ++++++++++++++++++++++++++
>  include/linux/pwm.h | 13 +++++++++++++
>  2 files changed, 39 insertions(+)

Overall I like the concept of introducing this capture functionality.

However I have a couple of questions, see below.

> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index d24ca5f..8f4a8a9 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -494,6 +494,32 @@ unlock:
>  EXPORT_SYMBOL_GPL(pwm_set_polarity);
>  
>  /**
> + * pwm_capture() - capture and report a PWM signal
> + * @pwm: PWM device
> + * @channel: PWM capture channel to use
> + * @buf: buffer to place output message into
> + *
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +int pwm_capture(struct pwm_device *pwm, int channel, char *buf)

This public interface seems to be targetted specifically at sysfs. As
such I'm not sure if there is reason to make it public, since the code
is unlikely to ever be called by other users in the kernel.

Do you think it would be possible to make the interface more generic by
passing back some form of structure containing the capture result? That
way users within the kernel could use the result without having to go
and parse a string filled in by the driver. It would also be easy to
implement sysfs support on top of that. Another advantage is that there
would be a standard result structure rather than a free-form string
filled by drivers that can't be controlled.

What kind of result does the STi hardware return? Looking at the driver
later in the series it seems to support triggering interrupts on rising
and falling edges and capture some running counter at these events. If
the frequency of the counter increment is known, these numbers should
allow us to determine both the period and duty cycle of the PWM signal
in nanoseconds. Would it be possible to rewrite this function and the
driver patch to something like this:

	int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result);

Where

	struct pwm_capture {
		unsigned int period;
		unsigned int duty_cycle;
	};

?

Another thing I noticed is that the code here seems to be confusing
channels and devices. In the PWM subsystem a struct pwm_device
represents a single channel. Allowing the channel to be specified is
redundant at best, and confusing at worst.

Thierry

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

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

[Lee Jones]

On Tue, 12 Apr 2016, Thierry Reding wrote:

> On Wed, Mar 02, 2016 at 03:31:59PM +0000, Lee Jones wrote:
> > Supply a PWM Capture call-back Op in order to pass back
> > information obtained by running analysis on PWM a signal.
> > This would normally (at least during testing) be called from
> > the Sysfs routines with a view to printing out PWM Capture
> > data which has been encoded into a string.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/pwm/core.c  | 26 ++++++++++++++++++++++++++
> >  include/linux/pwm.h | 13 +++++++++++++
> >  2 files changed, 39 insertions(+)
> 
> Overall I like the concept of introducing this capture functionality.
> 
> However I have a couple of questions, see below.
> 
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index d24ca5f..8f4a8a9 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -494,6 +494,32 @@ unlock:
> >  EXPORT_SYMBOL_GPL(pwm_set_polarity);
> >  
> >  /**
> > + * pwm_capture() - capture and report a PWM signal
> > + * @pwm: PWM device
> > + * @channel: PWM capture channel to use
> > + * @buf: buffer to place output message into
> > + *
> > + * Returns: 0 on success or a negative error code on failure.
> > + */
> > +int pwm_capture(struct pwm_device *pwm, int channel, char *buf)
> 
> This public interface seems to be targetted specifically at sysfs. As
> such I'm not sure if there is reason to make it public, since the code
> is unlikely to ever be called by other users in the kernel.
> 
> Do you think it would be possible to make the interface more generic by
> passing back some form of structure containing the capture result? That
> way users within the kernel could use the result without having to go
> and parse a string filled in by the driver. It would also be easy to
> implement sysfs support on top of that. Another advantage is that there
> would be a standard result structure rather than a free-form string
> filled by drivers that can't be controlled.
> 
> What kind of result does the STi hardware return? Looking at the driver
> later in the series it seems to support triggering interrupts on rising
> and falling edges and capture some running counter at these events. If
> the frequency of the counter increment is known, these numbers should
> allow us to determine both the period and duty cycle of the PWM signal
> in nanoseconds. Would it be possible to rewrite this function and the
> driver patch to something like this:
> 
> 	int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result);
> 
> Where
> 
> 	struct pwm_capture {
> 		unsigned int period;
> 		unsigned int duty_cycle;
> 	};
> 
> ?

Yes, I think that sounds feasible.

> Another thing I noticed is that the code here seems to be confusing
> channels and devices. In the PWM subsystem a struct pwm_device
> represents a single channel. Allowing the channel to be specified is
> redundant at best, and confusing at worst.

On the STi platform I'm working on, we have 2 devices PWM{0,1} and
each device has 4 separate channels [0..3].  Not all of them support
PWM capture, but the channels are 'a thing'.  I'd need to look into it
further, but I guess you'd like the driver to pretend we have 8
devices?  If that's the case, what's the point in the core 'npwm'
parameter?  Surely that's "channels per device"?

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

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

[Thierry Reding]

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

On Wed, Apr 13, 2016 at 10:36:05AM +0100, Lee Jones wrote:
> On Tue, 12 Apr 2016, Thierry Reding wrote:
> 
> > On Wed, Mar 02, 2016 at 03:31:59PM +0000, Lee Jones wrote:
> > > Supply a PWM Capture call-back Op in order to pass back
> > > information obtained by running analysis on PWM a signal.
> > > This would normally (at least during testing) be called from
> > > the Sysfs routines with a view to printing out PWM Capture
> > > data which has been encoded into a string.
> > > 
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/pwm/core.c  | 26 ++++++++++++++++++++++++++
> > >  include/linux/pwm.h | 13 +++++++++++++
> > >  2 files changed, 39 insertions(+)
> > 
> > Overall I like the concept of introducing this capture functionality.
> > 
> > However I have a couple of questions, see below.
> > 
> > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > index d24ca5f..8f4a8a9 100644
> > > --- a/drivers/pwm/core.c
> > > +++ b/drivers/pwm/core.c
> > > @@ -494,6 +494,32 @@ unlock:
> > >  EXPORT_SYMBOL_GPL(pwm_set_polarity);
> > >  
> > >  /**
> > > + * pwm_capture() - capture and report a PWM signal
> > > + * @pwm: PWM device
> > > + * @channel: PWM capture channel to use
> > > + * @buf: buffer to place output message into
> > > + *
> > > + * Returns: 0 on success or a negative error code on failure.
> > > + */
> > > +int pwm_capture(struct pwm_device *pwm, int channel, char *buf)
> > 
> > This public interface seems to be targetted specifically at sysfs. As
> > such I'm not sure if there is reason to make it public, since the code
> > is unlikely to ever be called by other users in the kernel.
> > 
> > Do you think it would be possible to make the interface more generic by
> > passing back some form of structure containing the capture result? That
> > way users within the kernel could use the result without having to go
> > and parse a string filled in by the driver. It would also be easy to
> > implement sysfs support on top of that. Another advantage is that there
> > would be a standard result structure rather than a free-form string
> > filled by drivers that can't be controlled.
> > 
> > What kind of result does the STi hardware return? Looking at the driver
> > later in the series it seems to support triggering interrupts on rising
> > and falling edges and capture some running counter at these events. If
> > the frequency of the counter increment is known, these numbers should
> > allow us to determine both the period and duty cycle of the PWM signal
> > in nanoseconds. Would it be possible to rewrite this function and the
> > driver patch to something like this:
> > 
> > 	int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result);
> > 
> > Where
> > 
> > 	struct pwm_capture {
> > 		unsigned int period;
> > 		unsigned int duty_cycle;
> > 	};
> > 
> > ?
> 
> Yes, I think that sounds feasible.
> 
> > Another thing I noticed is that the code here seems to be confusing
> > channels and devices. In the PWM subsystem a struct pwm_device
> > represents a single channel. Allowing the channel to be specified is
> > redundant at best, and confusing at worst.
> 
> On the STi platform I'm working on, we have 2 devices PWM{0,1} and
> each device has 4 separate channels [0..3].  Not all of them support
> PWM capture, but the channels are 'a thing'.  I'd need to look into it
> further, but I guess you'd like the driver to pretend we have 8
> devices?  If that's the case, what's the point in the core 'npwm'
> parameter?  Surely that's "channels per device"?

Well, it's technically "channels per _chip_". Perhaps the confusion is
with the historical naming: a PWM channel is represented by a struct
pwm_device, whereas what I think you're referring to as device (as in
"channels per device") is represented as a struct pwm_chip.

Thierry

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

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

[Lee Jones]

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;
+
 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);
 
 static struct attribute *pwm_attrs[] = {
 	&dev_attr_period.attr,
 	&dev_attr_duty_cycle.attr,
 	&dev_attr_enable.attr,
 	&dev_attr_polarity.attr,
+	&dev_attr_capture.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(pwm);
-- 
1.9.1

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

[Thierry Reding]

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

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

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

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

[Lee Jones]

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

[RESEND 03/11] pwm: sti: Reorganise register names in preparation for new functionality

[Lee Jones]

Exciting functionality is on the way to this device.  But
before we can add it, we need to do some basic housekeeping
so the additions can be added cleanly.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/pwm-sti.c | 76 +++++++++++++++++++++++++++------------------------
 1 file changed, 41 insertions(+), 35 deletions(-)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 92abbd5..4e06c4d 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -21,18 +21,22 @@
 #include <linux/slab.h>
 #include <linux/time.h>
 
-#define STI_DS_REG(ch)	(4 * (ch))	/* Channel's Duty Cycle register */
-#define STI_PWMCR	0x50		/* Control/Config register */
-#define STI_INTEN	0x54		/* Interrupt Enable/Disable register */
+#define PWM_OUT_VAL(x)	(0x00 + (4 * (x))) /* Channel's Duty Cycle register */
+
+#define STI_PWM_CTRL	0x50		/* Control/Config register */
+#define STI_INT_EN	0x54		/* Interrupt Enable/Disable register */
 #define PWM_PRESCALE_LOW_MASK		0x0f
 #define PWM_PRESCALE_HIGH_MASK		0xf0
 
 /* Regfield IDs */
 enum {
+	/* Bits in PWM_CTRL*/
 	PWMCLK_PRESCALE_LOW,
 	PWMCLK_PRESCALE_HIGH,
-	PWM_EN,
-	PWM_INT_EN,
+
+	PWM_OUT_EN,
+
+	PWM_CPT_INT_EN,
 
 	/* Keep last */
 	MAX_REGFIELDS
@@ -47,14 +51,14 @@ struct sti_pwm_compat_data {
 
 struct sti_pwm_chip {
 	struct device *dev;
-	struct clk *clk;
 	unsigned long clk_rate;
+	struct clk *pwm_clk;
 	struct regmap *regmap;
 	struct sti_pwm_compat_data *cdata;
 	struct regmap_field *prescale_low;
 	struct regmap_field *prescale_high;
-	struct regmap_field *pwm_en;
-	struct regmap_field *pwm_int_en;
+	struct regmap_field *pwm_out_en;
+	struct regmap_field *pwm_cpt_int_en;
 	struct pwm_chip chip;
 	struct pwm_device *cur;
 	unsigned long configured;
@@ -64,10 +68,10 @@ struct sti_pwm_chip {
 };
 
 static const struct reg_field sti_pwm_regfields[MAX_REGFIELDS] = {
-	[PWMCLK_PRESCALE_LOW]	= REG_FIELD(STI_PWMCR, 0, 3),
-	[PWMCLK_PRESCALE_HIGH]	= REG_FIELD(STI_PWMCR, 11, 14),
-	[PWM_EN]		= REG_FIELD(STI_PWMCR, 9, 9),
-	[PWM_INT_EN]		= REG_FIELD(STI_INTEN, 0, 0),
+	[PWMCLK_PRESCALE_LOW]	= REG_FIELD(STI_PWM_CTRL, 0, 3),
+	[PWMCLK_PRESCALE_HIGH]	= REG_FIELD(STI_PWM_CTRL, 11, 14),
+	[PWM_OUT_EN]		= REG_FIELD(STI_PWM_CTRL, 9, 9),
+	[PWM_CPT_INT_EN]	= REG_FIELD(STI_INT_EN, 1, 4),
 };
 
 static inline struct sti_pwm_chip *to_sti_pwmchip(struct pwm_chip *chip)
@@ -144,7 +148,7 @@ static int sti_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	    ((ncfg == 1) && (pwm->hwpwm != cur->hwpwm) && period_same) ||
 	    ((ncfg > 1) && period_same)) {
 		/* Enable clock before writing to PWM registers. */
-		ret = clk_enable(pc->clk);
+		ret = clk_enable(pc->pwm_clk);
 		if (ret)
 			return ret;
 
@@ -174,11 +178,12 @@ static int sti_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		 */
 		pwmvalx = cdata->max_pwm_cnt * duty_ns / period_ns;
 
-		ret = regmap_write(pc->regmap, STI_DS_REG(pwm->hwpwm), pwmvalx);
+		ret = regmap_write(pc->regmap,
+				   PWM_OUT_VAL(pwm->hwpwm), pwmvalx);
 		if (ret)
 			goto clk_dis;
 
-		ret = regmap_field_write(pc->pwm_int_en, 0);
+		ret = regmap_field_write(pc->pwm_cpt_int_en, 0);
 
 		set_bit(pwm->hwpwm, &pc->configured);
 		pc->cur = pwm;
@@ -190,7 +195,7 @@ static int sti_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	}
 
 clk_dis:
-	clk_disable(pc->clk);
+	clk_disable(pc->pwm_clk);
 	return ret;
 }
 
@@ -206,11 +211,11 @@ static int sti_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	 */
 	mutex_lock(&pc->sti_pwm_lock);
 	if (!pc->en_count) {
-		ret = clk_enable(pc->clk);
+		ret = clk_enable(pc->pwm_clk);
 		if (ret)
 			goto out;
 
-		ret = regmap_field_write(pc->pwm_en, 1);
+		ret = regmap_field_write(pc->pwm_out_en, 1);
 		if (ret) {
 			dev_err(dev, "failed to enable PWM device:%d\n",
 				pwm->hwpwm);
@@ -232,9 +237,9 @@ static void sti_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 		mutex_unlock(&pc->sti_pwm_lock);
 		return;
 	}
-	regmap_field_write(pc->pwm_en, 0);
+	regmap_field_write(pc->pwm_out_en, 0);
 
-	clk_disable(pc->clk);
+	clk_disable(pc->pwm_clk);
 	mutex_unlock(&pc->sti_pwm_lock);
 }
 
@@ -277,15 +282,16 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
 	if (IS_ERR(pc->prescale_high))
 		return PTR_ERR(pc->prescale_high);
 
-	pc->pwm_en = devm_regmap_field_alloc(dev, pc->regmap,
-					     reg_fields[PWM_EN]);
-	if (IS_ERR(pc->pwm_en))
-		return PTR_ERR(pc->pwm_en);
 
-	pc->pwm_int_en = devm_regmap_field_alloc(dev, pc->regmap,
-						 reg_fields[PWM_INT_EN]);
-	if (IS_ERR(pc->pwm_int_en))
-		return PTR_ERR(pc->pwm_int_en);
+	pc->pwm_out_en = devm_regmap_field_alloc(dev, pc->regmap,
+						 reg_fields[PWM_OUT_EN]);
+	if (IS_ERR(pc->pwm_out_en))
+		return PTR_ERR(pc->pwm_out_en);
+
+	pc->pwm_cpt_int_en = devm_regmap_field_alloc(dev, pc->regmap,
+						 reg_fields[PWM_CPT_INT_EN]);
+	if (IS_ERR(pc->pwm_cpt_int_en))
+		return PTR_ERR(pc->pwm_cpt_int_en);
 
 	return 0;
 }
@@ -341,19 +347,19 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	pc->clk = of_clk_get_by_name(dev->of_node, "pwm");
-	if (IS_ERR(pc->clk)) {
+	pc->pwm_clk = of_clk_get_by_name(dev->of_node, "pwm");
+	if (IS_ERR(pc->pwm_clk)) {
 		dev_err(dev, "failed to get PWM clock\n");
-		return PTR_ERR(pc->clk);
+		return PTR_ERR(pc->pwm_clk);
 	}
 
-	pc->clk_rate = clk_get_rate(pc->clk);
+	pc->clk_rate = clk_get_rate(pc->pwm_clk);
 	if (!pc->clk_rate) {
 		dev_err(dev, "failed to get clock rate\n");
 		return -EINVAL;
 	}
 
-	ret = clk_prepare(pc->clk);
+	ret = clk_prepare(pc->pwm_clk);
 	if (ret) {
 		dev_err(dev, "failed to prepare clock\n");
 		return ret;
@@ -367,7 +373,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
 
 	ret = pwmchip_add(&pc->chip);
 	if (ret < 0) {
-		clk_unprepare(pc->clk);
+		clk_unprepare(pc->pwm_clk);
 		return ret;
 	}
 
@@ -384,7 +390,7 @@ static int sti_pwm_remove(struct platform_device *pdev)
 	for (i = 0; i < pc->cdata->num_chan; i++)
 		pwm_disable(&pc->chip.pwms[i]);
 
-	clk_unprepare(pc->clk);
+	clk_unprepare(pc->pwm_clk);
 
 	return pwmchip_remove(&pc->chip);
 }
-- 
1.9.1

[RESEND 04/11] pwm: sti: Only request clock rate when you need to

[Lee Jones]

In the original code the clock rate was only obtained during
initialisation; however, the rate may change between then and
its use.  This patch ensures the correct rate is acquired just
before use.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/pwm-sti.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 4e06c4d..aa217e2 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -51,7 +51,6 @@ struct sti_pwm_compat_data {
 
 struct sti_pwm_chip {
 	struct device *dev;
-	unsigned long clk_rate;
 	struct clk *pwm_clk;
 	struct regmap *regmap;
 	struct sti_pwm_compat_data *cdata;
@@ -86,13 +85,20 @@ static int sti_pwm_get_prescale(struct sti_pwm_chip *pc, unsigned long period,
 				unsigned int *prescale)
 {
 	struct sti_pwm_compat_data *cdata = pc->cdata;
+	unsigned long clk_rate;
 	unsigned long val;
 	unsigned int ps;
 
+	clk_rate = clk_get_rate(pc->pwm_clk);
+	if (!clk_rate) {
+		dev_err(pc->dev, "failed to get clock rate\n");
+		return -EINVAL;
+	}
+
 	/*
 	 * prescale = ((period_ns * clk_rate) / (10^9 * (max_pwm_count + 1)) - 1
 	 */
-	val = NSEC_PER_SEC / pc->clk_rate;
+	val = NSEC_PER_SEC / clk_rate;
 	val *= cdata->max_pwm_cnt + 1;
 
 	if (period % val) {
@@ -353,12 +359,6 @@ static int sti_pwm_probe(struct platform_device *pdev)
 		return PTR_ERR(pc->pwm_clk);
 	}
 
-	pc->clk_rate = clk_get_rate(pc->pwm_clk);
-	if (!pc->clk_rate) {
-		dev_err(dev, "failed to get clock rate\n");
-		return -EINVAL;
-	}
-
 	ret = clk_prepare(pc->pwm_clk);
 	if (ret) {
 		dev_err(dev, "failed to prepare clock\n");
-- 
1.9.1

[RESEND 05/11] pwm: sti: Supply PWM Capture register addresses and bit locations

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/pwm-sti.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index aa217e2..2336bac 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -22,26 +22,48 @@
 #include <linux/time.h>
 
 #define PWM_OUT_VAL(x)	(0x00 + (4 * (x))) /* Channel's Duty Cycle register */
+#define PWM_CPT_VAL(x)	(0x10 + (4 * (x))) /* Capture value */
+#define PWM_CPT_EDGE(x) (0x30 + (4 * (x))) /* Edge to capture on */
 
 #define STI_PWM_CTRL	0x50		/* Control/Config register */
 #define STI_INT_EN	0x54		/* Interrupt Enable/Disable register */
+#define STI_INT_STA	0x58		/* Interrupt Status register */
+#define PWM_INT_ACK			0x5c
 #define PWM_PRESCALE_LOW_MASK		0x0f
 #define PWM_PRESCALE_HIGH_MASK		0xf0
+#define PWM_CPT_EDGE_MASK		0x03
+#define PWM_INT_ACK_MASK		0x1ff
+
+#define STI_MAX_CPT_CHANS		4
+#define CPT_DC_MAX			0xff
 
 /* Regfield IDs */
 enum {
 	/* Bits in PWM_CTRL*/
 	PWMCLK_PRESCALE_LOW,
 	PWMCLK_PRESCALE_HIGH,
+	CPTCLK_PRESCALE,
 
 	PWM_OUT_EN,
+	PWM_CPT_EN,
 
 	PWM_CPT_INT_EN,
+	PWM_CPT_INT_STAT,
 
 	/* Keep last */
 	MAX_REGFIELDS
 };
 
+/* Each capture input can be programmed to detect rising-edge, falling-edge,
+ * either edge or neither egde
+ */
+enum sti_cpt_edge {
+	CPT_EDGE_DISABLED,
+	CPT_EDGE_RISING,
+	CPT_EDGE_FALLING,
+	CPT_EDGE_BOTH,
+};
+
 struct sti_pwm_compat_data {
 	const struct reg_field *reg_fields;
 	unsigned int num_chan;
@@ -69,8 +91,11 @@ struct sti_pwm_chip {
 static const struct reg_field sti_pwm_regfields[MAX_REGFIELDS] = {
 	[PWMCLK_PRESCALE_LOW]	= REG_FIELD(STI_PWM_CTRL, 0, 3),
 	[PWMCLK_PRESCALE_HIGH]	= REG_FIELD(STI_PWM_CTRL, 11, 14),
+	[CPTCLK_PRESCALE]	= REG_FIELD(STI_PWM_CTRL, 4, 8),
 	[PWM_OUT_EN]		= REG_FIELD(STI_PWM_CTRL, 9, 9),
+	[PWM_CPT_EN]		= REG_FIELD(STI_PWM_CTRL, 10, 10),
 	[PWM_CPT_INT_EN]	= REG_FIELD(STI_INT_EN, 1, 4),
+	[PWM_CPT_INT_STAT]	= REG_FIELD(STI_INT_STA, 1, 4),
 };
 
 static inline struct sti_pwm_chip *to_sti_pwmchip(struct pwm_chip *chip)
-- 
1.9.1

[RESEND 06/11] pwm: sti: Supply PWM Capture clock handling

[Lee Jones]

ST's PWM IP is supplied by 2 different clocks.  One for PWM
Output and the other for Capture.  This patch provides clock
handling for the latter.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/pwm-sti.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 2336bac..9e6b1c8 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -74,6 +74,7 @@ struct sti_pwm_compat_data {
 struct sti_pwm_chip {
 	struct device *dev;
 	struct clk *pwm_clk;
+	struct clk *cpt_clk;
 	struct regmap *regmap;
 	struct sti_pwm_compat_data *cdata;
 	struct regmap_field *prescale_low;
@@ -183,6 +184,10 @@ static int sti_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		if (ret)
 			return ret;
 
+		ret = clk_enable(pc->cpt_clk);
+		if (ret)
+			return ret;
+
 		if (!period_same) {
 			ret = sti_pwm_get_prescale(pc, period_ns, &prescale);
 			if (ret)
@@ -227,6 +232,7 @@ static int sti_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 clk_dis:
 	clk_disable(pc->pwm_clk);
+	clk_disable(pc->cpt_clk);
 	return ret;
 }
 
@@ -246,6 +252,10 @@ static int sti_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 		if (ret)
 			goto out;
 
+		ret = clk_enable(pc->cpt_clk);
+		if (ret)
+			goto out;
+
 		ret = regmap_field_write(pc->pwm_out_en, 1);
 		if (ret) {
 			dev_err(dev, "failed to enable PWM device:%d\n",
@@ -271,6 +281,7 @@ static void sti_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	regmap_field_write(pc->pwm_out_en, 0);
 
 	clk_disable(pc->pwm_clk);
+	clk_disable(pc->cpt_clk);
 	mutex_unlock(&pc->sti_pwm_lock);
 }
 
@@ -390,6 +401,18 @@ static int sti_pwm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	pc->cpt_clk = of_clk_get_by_name(dev->of_node, "capture");
+	if (IS_ERR(pc->cpt_clk)) {
+		dev_err(dev, "failed to get PWM capture clock\n");
+		return PTR_ERR(pc->cpt_clk);
+	}
+
+	ret = clk_prepare(pc->cpt_clk);
+	if (ret) {
+		dev_err(dev, "failed to prepare clock\n");
+		return ret;
+	}
+
 	pc->chip.dev = dev;
 	pc->chip.ops = &sti_pwm_ops;
 	pc->chip.base = -1;
@@ -399,6 +422,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	ret = pwmchip_add(&pc->chip);
 	if (ret < 0) {
 		clk_unprepare(pc->pwm_clk);
+		clk_unprepare(pc->cpt_clk);
 		return ret;
 	}
 
@@ -416,6 +440,7 @@ static int sti_pwm_remove(struct platform_device *pdev)
 		pwm_disable(&pc->chip.pwms[i]);
 
 	clk_unprepare(pc->pwm_clk);
+	clk_unprepare(pc->cpt_clk);
 
 	return pwmchip_remove(&pc->chip);
 }
-- 
1.9.1

[RESEND 07/11] pwm: sti: Initialise PWM Capture channel data

[Lee Jones]

Each PWM Capture channel is allocated a structure to hold its own
state.  During a capture the channel may be partaking in one of 3
phases.  Initial (rising) phase change, a subsequent (falling)
phase change indicating end of the duty-cycle phase and finally
a final (rising) phase change indicating the end of the period.
The timer value snapshot each event is held in a variable of the
same name, and the phase number (0, 1, 2) is contained in the
index variable.  Other channel specific information, such as GPIO
pin, the IRQ wait queue and locking is also contained in the
structure.  This patch initialises this structure for each of
the available channels.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/pwm-sti.c | 49 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 42 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 9e6b1c8..fca692a 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -11,15 +11,19 @@
  */
 
 #include <linux/clk.h>
+#include <linux/gpio.h>
 #include <linux/math64.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
 #include <linux/regmap.h>
+#include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/time.h>
+#include <linux/wait.h>
 
 #define PWM_OUT_VAL(x)	(0x00 + (4 * (x))) /* Channel's Duty Cycle register */
 #define PWM_CPT_VAL(x)	(0x10 + (4 * (x))) /* Capture value */
@@ -64,9 +68,18 @@ enum sti_cpt_edge {
 	CPT_EDGE_BOTH,
 };
 
+struct sti_cpt_data {
+	u32 snapshot[3];
+	int index;
+	int gpio;
+	struct mutex lock;
+	wait_queue_head_t wait;
+};
+
 struct sti_pwm_compat_data {
 	const struct reg_field *reg_fields;
-	unsigned int num_chan;
+	unsigned int pwm_num_chan;
+	unsigned int cpt_num_chan;
 	unsigned int max_pwm_cnt;
 	unsigned int max_prescale;
 };
@@ -77,6 +90,7 @@ struct sti_pwm_chip {
 	struct clk *cpt_clk;
 	struct regmap *regmap;
 	struct sti_pwm_compat_data *cdata;
+	struct sti_cpt_data *cpt_data[STI_MAX_CPT_CHANS];
 	struct regmap_field *prescale_low;
 	struct regmap_field *prescale_high;
 	struct regmap_field *pwm_out_en;
@@ -307,10 +321,15 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
 	struct device_node *np = dev->of_node;
 	struct sti_pwm_compat_data *cdata = pc->cdata;
 	u32 num_chan;
+	int ret;
 
-	of_property_read_u32(np, "st,pwm-num-chan", &num_chan);
-	if (num_chan)
-		cdata->num_chan = num_chan;
+	ret = of_property_read_u32(np, "st,pwm-num-chan", &num_chan);
+	if (!ret)
+		cdata->pwm_num_chan = num_chan;
+
+	ret = of_property_read_u32(np, "st,capture-num-chan", &num_chan);
+	if (!ret)
+		cdata->cpt_num_chan = num_chan;
 
 	reg_fields = cdata->reg_fields;
 
@@ -347,9 +366,11 @@ static const struct regmap_config sti_pwm_regmap_config = {
 static int sti_pwm_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
 	struct sti_pwm_compat_data *cdata;
 	struct sti_pwm_chip *pc;
 	struct resource *res;
+	unsigned int chan;
 	int ret;
 
 	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
@@ -378,7 +399,8 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	cdata->reg_fields   = &sti_pwm_regfields[0];
 	cdata->max_prescale = 0xff;
 	cdata->max_pwm_cnt  = 255;
-	cdata->num_chan     = 1;
+	cdata->pwm_num_chan     = 1;
+	cdata->cpt_num_chan	= 0;
 
 	pc->cdata = cdata;
 	pc->dev = dev;
@@ -389,6 +411,19 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	for (chan = 0; chan < cdata->cpt_num_chan; chan++) {
+		struct sti_cpt_data *data;
+
+		data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+		if (!data)
+			return -ENOMEM;
+
+		init_waitqueue_head(&data->wait);
+		mutex_init(&data->lock);
+		data->gpio = of_get_named_gpio(np, "capture-gpios", chan);
+		pc->cpt_data[chan] = data;
+	}
+
 	pc->pwm_clk = of_clk_get_by_name(dev->of_node, "pwm");
 	if (IS_ERR(pc->pwm_clk)) {
 		dev_err(dev, "failed to get PWM clock\n");
@@ -416,7 +451,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	pc->chip.dev = dev;
 	pc->chip.ops = &sti_pwm_ops;
 	pc->chip.base = -1;
-	pc->chip.npwm = pc->cdata->num_chan;
+	pc->chip.npwm = pc->cdata->pwm_num_chan;
 	pc->chip.can_sleep = true;
 
 	ret = pwmchip_add(&pc->chip);
@@ -436,7 +471,7 @@ static int sti_pwm_remove(struct platform_device *pdev)
 	struct sti_pwm_chip *pc = platform_get_drvdata(pdev);
 	unsigned int i;
 
-	for (i = 0; i < pc->cdata->num_chan; i++)
+	for (i = 0; i < pc->cdata->pwm_num_chan; i++)
 		pwm_disable(&pc->chip.pwms[i]);
 
 	clk_unprepare(pc->pwm_clk);
-- 
1.9.1

Re: [RESEND 07/11] pwm: sti: Initialise PWM Capture channel data

[Thierry Reding]

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

On Wed, Mar 02, 2016 at 03:32:05PM +0000, Lee Jones wrote:
[...]
> +struct sti_cpt_data {
> +	u32 snapshot[3];
> +	int index;
> +	int gpio;

On a side-note, this should probably use struct gpio_desc * instead of
an integer along with the gpiod_*() APIs for the GPIO handling.

> +	struct mutex lock;
> +	wait_queue_head_t wait;
> +};
> +
>  struct sti_pwm_compat_data {
>  	const struct reg_field *reg_fields;
> -	unsigned int num_chan;
> +	unsigned int pwm_num_chan;
> +	unsigned int cpt_num_chan;
>  	unsigned int max_pwm_cnt;
>  	unsigned int max_prescale;
>  };
> @@ -77,6 +90,7 @@ struct sti_pwm_chip {
>  	struct clk *cpt_clk;
>  	struct regmap *regmap;
>  	struct sti_pwm_compat_data *cdata;
> +	struct sti_cpt_data *cpt_data[STI_MAX_CPT_CHANS];

The PWM subsystem allows chip-specific data to be associated with each
PWM device. I'd prefer if the driver used it rather than homebrew some-
thing similar. See pwm_set_chip_data() and pwm_get_chip_data().

> @@ -389,6 +411,19 @@ static int sti_pwm_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	for (chan = 0; chan < cdata->cpt_num_chan; chan++) {
> +		struct sti_cpt_data *data;
> +
> +		data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +		if (!data)
> +			return -ENOMEM;
> +
> +		init_waitqueue_head(&data->wait);
> +		mutex_init(&data->lock);
> +		data->gpio = of_get_named_gpio(np, "capture-gpios", chan);
> +		pc->cpt_data[chan] = data;

Converting to per-PWM data should be as simple as turning this last line
into:

	pwm_set_chip_data(pc->chip.pwms[chan], data);

Also I don't see any cleanup for this data in the driver. The memory for
the per-PWM data should be freed by devm_*() infrastructure, but how
will the GPIO be released?

Thierry

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

Re: [RESEND 07/11] pwm: sti: Initialise PWM Capture channel data

[Lee Jones]

On Tue, 12 Apr 2016, Thierry Reding wrote:

> On Wed, Mar 02, 2016 at 03:32:05PM +0000, Lee Jones wrote:
> [...]
> > +struct sti_cpt_data {
> > +	u32 snapshot[3];
> > +	int index;
> > +	int gpio;
> 
> On a side-note, this should probably use struct gpio_desc * instead of
> an integer along with the gpiod_*() APIs for the GPIO handling.

Why would you need to do that?

of_get_named_gpio() does all that for you.

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

Re: [RESEND 07/11] pwm: sti: Initialise PWM Capture channel data

[Thierry Reding]

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

On Fri, Apr 15, 2016 at 01:39:41PM +0100, Lee Jones wrote:
> On Tue, 12 Apr 2016, Thierry Reding wrote:
> 
> > On Wed, Mar 02, 2016 at 03:32:05PM +0000, Lee Jones wrote:
> > [...]
> > > +struct sti_cpt_data {
> > > +	u32 snapshot[3];
> > > +	int index;
> > > +	int gpio;
> > 
> > On a side-note, this should probably use struct gpio_desc * instead of
> > an integer along with the gpiod_*() APIs for the GPIO handling.
> 
> Why would you need to do that?
> 
> of_get_named_gpio() does all that for you.

Use the of_get_named_gpio*d*() function instead. My understanding is
that referring to GPIOs by integer is deprecated and should not be used
in new code.

Thierry

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

Re: [RESEND 07/11] pwm: sti: Initialise PWM Capture channel data

[Lee Jones]

On Fri, 15 Apr 2016, Thierry Reding wrote:

> On Fri, Apr 15, 2016 at 01:39:41PM +0100, Lee Jones wrote:
> > On Tue, 12 Apr 2016, Thierry Reding wrote:
> > 
> > > On Wed, Mar 02, 2016 at 03:32:05PM +0000, Lee Jones wrote:
> > > [...]
> > > > +struct sti_cpt_data {
> > > > +	u32 snapshot[3];
> > > > +	int index;
> > > > +	int gpio;
> > > 
> > > On a side-note, this should probably use struct gpio_desc * instead of
> > > an integer along with the gpiod_*() APIs for the GPIO handling.
> > 
> > Why would you need to do that?
> > 
> > of_get_named_gpio() does all that for you.
> 
> Use the of_get_named_gpio*d*() function instead. My understanding is
> that referring to GPIOs by integer is deprecated and should not be used
> in new code.

I've since conducted some research and have now converted over to
gpiod.  Thanks for the pointer.

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

Re: [RESEND 07/11] pwm: sti: Initialise PWM Capture channel data

[Lee Jones]

On Tue, 12 Apr 2016, Thierry Reding wrote:
> On Wed, Mar 02, 2016 at 03:32:05PM +0000, Lee Jones wrote:
> [...]
> > +struct sti_cpt_data {
> > +	u32 snapshot[3];
> > +	int index;
> > +	int gpio;

[...]

> > +
> > +		init_waitqueue_head(&data->wait);
> > +		mutex_init(&data->lock);
> > +		data->gpio = of_get_named_gpio(np, "capture-gpios", chan);
> > +		pc->cpt_data[chan] = data;
> 
> Converting to per-PWM data should be as simple as turning this last line
> into:
> 
> 	pwm_set_chip_data(pc->chip.pwms[chan], data);
> 
> Also I don't see any cleanup for this data in the driver. The memory for
> the per-PWM data should be freed by devm_*() infrastructure, but how
> will the GPIO be released?

There is no reason to release a GPIO.

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

Re: [RESEND 07/11] pwm: sti: Initialise PWM Capture channel data

[Thierry Reding]

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

On Fri, Apr 15, 2016 at 02:11:46PM +0100, Lee Jones wrote:
> On Tue, 12 Apr 2016, Thierry Reding wrote:
> > On Wed, Mar 02, 2016 at 03:32:05PM +0000, Lee Jones wrote:
> > [...]
> > > +struct sti_cpt_data {
> > > +	u32 snapshot[3];
> > > +	int index;
> > > +	int gpio;
> 
> [...]
> 
> > > +
> > > +		init_waitqueue_head(&data->wait);
> > > +		mutex_init(&data->lock);
> > > +		data->gpio = of_get_named_gpio(np, "capture-gpios", chan);
> > > +		pc->cpt_data[chan] = data;
> > 
> > Converting to per-PWM data should be as simple as turning this last line
> > into:
> > 
> > 	pwm_set_chip_data(pc->chip.pwms[chan], data);
> > 
> > Also I don't see any cleanup for this data in the driver. The memory for
> > the per-PWM data should be freed by devm_*() infrastructure, but how
> > will the GPIO be released?
> 
> There is no reason to release a GPIO.

Why? If you request it why would you not need to release it?

Thierry

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

[RESEND 08/11] pwm: sti: Add support for PWM Capture IRQs

[Lee Jones]

Here we're requesting the PWM Capture IRQ and supplying the
handler which will be called in the event of an IRQ fire to
handle it.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/pwm-sti.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 90 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index fca692a..82a69e4 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -12,6 +12,7 @@
 
 #include <linux/clk.h>
 #include <linux/gpio.h>
+#include <linux/interrupt.h>
 #include <linux/math64.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
@@ -94,7 +95,9 @@ struct sti_pwm_chip {
 	struct regmap_field *prescale_low;
 	struct regmap_field *prescale_high;
 	struct regmap_field *pwm_out_en;
+	struct regmap_field *pwm_cpt_en;
 	struct regmap_field *pwm_cpt_int_en;
+	struct regmap_field *pwm_cpt_int_stat;
 	struct pwm_chip chip;
 	struct pwm_device *cur;
 	unsigned long configured;
@@ -314,6 +317,74 @@ static const struct pwm_ops sti_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
+static irqreturn_t sti_pwm_interrupt(int irq, void *data)
+{
+	struct sti_pwm_chip *pc = data;
+	struct device *dev = pc->dev;
+	struct sti_cpt_data *d;
+	int channel;
+	int cpt_int_stat;
+	int reg;
+	int ret = IRQ_NONE;
+
+	ret = regmap_field_read(pc->pwm_cpt_int_stat, &cpt_int_stat);
+	if (ret)
+		return ret;
+
+	while (cpt_int_stat) {
+		channel = ffs(cpt_int_stat) - 1;
+
+		d = pc->cpt_data[channel];
+
+		/*
+		 * Capture input:
+		 *    _______                   _______
+		 *   |       |                 |       |
+		 * __|       |_________________|       |________
+		 *   ^0      ^1                ^2
+		 *
+		 * Capture start by the first available rising edge
+		 * When a capture event occurs, capture value (CPT_VALx)
+		 * is stored, index incremented, capture edge changed.
+		 *
+		 * After the capture, if the index > 1, we have collected
+		 * the necessary data so we signal the thread waiting for it
+		 * and disable the capture by setting capture edge to none
+		 *
+		 */
+
+		regmap_read(pc->regmap,
+			    PWM_CPT_VAL(channel), &d->snapshot[d->index]);
+
+		switch (d->index) {
+		case 0:
+		case 1:
+			regmap_read(pc->regmap, PWM_CPT_EDGE(channel), &reg);
+			reg ^= PWM_CPT_EDGE_MASK;
+			regmap_write(pc->regmap, PWM_CPT_EDGE(channel), reg);
+
+			d->index++;
+			break;
+		case 2:
+			regmap_write(pc->regmap,
+				     PWM_CPT_EDGE(channel), CPT_EDGE_DISABLED);
+			wake_up(&d->wait);
+			break;
+		default:
+			dev_err(dev, "Internal error\n");
+		}
+
+		clear_bit(channel, (unsigned long int *)&cpt_int_stat);
+
+		ret = IRQ_HANDLED;
+	}
+
+	/* Just ACK everything */
+	regmap_write(pc->regmap, PWM_INT_ACK, PWM_INT_ACK_MASK);
+
+	return ret;
+}
+
 static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
 {
 	struct device *dev = pc->dev;
@@ -354,6 +425,11 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
 	if (IS_ERR(pc->pwm_cpt_int_en))
 		return PTR_ERR(pc->pwm_cpt_int_en);
 
+	pc->pwm_cpt_int_stat = devm_regmap_field_alloc(dev, pc->regmap,
+						reg_fields[PWM_CPT_INT_STAT]);
+	if (IS_ERR(pc->pwm_cpt_int_stat))
+		return PTR_ERR(pc->pwm_cpt_int_stat);
+
 	return 0;
 }
 
@@ -371,7 +447,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	struct sti_pwm_chip *pc;
 	struct resource *res;
 	unsigned int chan;
-	int ret;
+	int ret, irq;
 
 	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
 	if (!pc)
@@ -392,6 +468,19 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(pc->regmap))
 		return PTR_ERR(pc->regmap);
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "Failed to obtain IRQ\n");
+		return -ENODEV;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq, sti_pwm_interrupt,
+			       0, pdev->name, (void *) pc);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to request IRQ\n");
+		return ret;
+	}
+
 	/*
 	 * Setup PWM data with default values: some values could be replaced
 	 * with specific ones provided from Device Tree.
-- 
1.9.1

Re: [RESEND 08/11] pwm: sti: Add support for PWM Capture IRQs

[Thierry Reding]

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

On Wed, Mar 02, 2016 at 03:32:06PM +0000, Lee Jones wrote:
[...]
> diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
[...]
> +static irqreturn_t sti_pwm_interrupt(int irq, void *data)
> +{
> +	struct sti_pwm_chip *pc = data;
> +	struct device *dev = pc->dev;
> +	struct sti_cpt_data *d;
> +	int channel;
> +	int cpt_int_stat;
> +	int reg;
> +	int ret = IRQ_NONE;
> +
> +	ret = regmap_field_read(pc->pwm_cpt_int_stat, &cpt_int_stat);
> +	if (ret)
> +		return ret;
> +
> +	while (cpt_int_stat) {
> +		channel = ffs(cpt_int_stat) - 1;
> +
> +		d = pc->cpt_data[channel];
> +
> +		/*
> +		 * Capture input:
> +		 *    _______                   _______
> +		 *   |       |                 |       |
> +		 * __|       |_________________|       |________
> +		 *   ^0      ^1                ^2
> +		 *
> +		 * Capture start by the first available rising edge
> +		 * When a capture event occurs, capture value (CPT_VALx)
> +		 * is stored, index incremented, capture edge changed.
> +		 *
> +		 * After the capture, if the index > 1, we have collected
> +		 * the necessary data so we signal the thread waiting for it
> +		 * and disable the capture by setting capture edge to none
> +		 *
> +		 */

How do you deal with the situation where someone will stop the PWM
signal half-way in? That is, suppose you've got events for the first and
second snapshots (0 and 1) and then someone stops the PWM and the event
for snapshot 2 never happens, how does the code recover?

> +
> +		regmap_read(pc->regmap,
> +			    PWM_CPT_VAL(channel), &d->snapshot[d->index]);
> +
> +		switch (d->index) {
> +		case 0:
> +		case 1:
> +			regmap_read(pc->regmap, PWM_CPT_EDGE(channel), &reg);
> +			reg ^= PWM_CPT_EDGE_MASK;
> +			regmap_write(pc->regmap, PWM_CPT_EDGE(channel), reg);
> +
> +			d->index++;
> +			break;
> +		case 2:
> +			regmap_write(pc->regmap,
> +				     PWM_CPT_EDGE(channel), CPT_EDGE_DISABLED);
> +			wake_up(&d->wait);
> +			break;
> +		default:
> +			dev_err(dev, "Internal error\n");
> +		}
> +
> +		clear_bit(channel, (unsigned long int *)&cpt_int_stat);

clear_bit() is a little unusual to use on regular data types, as
evidenced by the need for the goofy cast here.

> +
> +		ret = IRQ_HANDLED;
> +	}
> +
> +	/* Just ACK everything */
> +	regmap_write(pc->regmap, PWM_INT_ACK, PWM_INT_ACK_MASK);
> +
> +	return ret;
> +}
> +
>  static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
>  {
>  	struct device *dev = pc->dev;
> @@ -354,6 +425,11 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
>  	if (IS_ERR(pc->pwm_cpt_int_en))
>  		return PTR_ERR(pc->pwm_cpt_int_en);
>  
> +	pc->pwm_cpt_int_stat = devm_regmap_field_alloc(dev, pc->regmap,
> +						reg_fields[PWM_CPT_INT_STAT]);
> +	if (IS_ERR(pc->pwm_cpt_int_stat))
> +		return PTR_ERR(pc->pwm_cpt_int_stat);
> +
>  	return 0;
>  }
>  
> @@ -371,7 +447,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
>  	struct sti_pwm_chip *pc;
>  	struct resource *res;
>  	unsigned int chan;
> -	int ret;
> +	int ret, irq;
>  
>  	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
>  	if (!pc)
> @@ -392,6 +468,19 @@ static int sti_pwm_probe(struct platform_device *pdev)
>  	if (IS_ERR(pc->regmap))
>  		return PTR_ERR(pc->regmap);
>  
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "Failed to obtain IRQ\n");
> +		return -ENODEV;
> +	}

I think you need to propagate the return value of platform_get_irq()
here.

> +
> +	ret = devm_request_irq(&pdev->dev, irq, sti_pwm_interrupt,
> +			       0, pdev->name, (void *) pc);

No need for the explicit cast to void *.

Thierry

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

Re: [RESEND 08/11] pwm: sti: Add support for PWM Capture IRQs

[Lee Jones]

On Tue, 12 Apr 2016, Thierry Reding wrote:

> On Wed, Mar 02, 2016 at 03:32:06PM +0000, Lee Jones wrote:
> [...]
> > diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> [...]
> > +static irqreturn_t sti_pwm_interrupt(int irq, void *data)
> > +{
> > +	struct sti_pwm_chip *pc = data;
> > +	struct device *dev = pc->dev;
> > +	struct sti_cpt_data *d;
> > +	int channel;
> > +	int cpt_int_stat;
> > +	int reg;
> > +	int ret = IRQ_NONE;
> > +
> > +	ret = regmap_field_read(pc->pwm_cpt_int_stat, &cpt_int_stat);
> > +	if (ret)
> > +		return ret;
> > +
> > +	while (cpt_int_stat) {
> > +		channel = ffs(cpt_int_stat) - 1;
> > +
> > +		d = pc->cpt_data[channel];
> > +
> > +		/*
> > +		 * Capture input:
> > +		 *    _______                   _______
> > +		 *   |       |                 |       |
> > +		 * __|       |_________________|       |________
> > +		 *   ^0      ^1                ^2
> > +		 *
> > +		 * Capture start by the first available rising edge
> > +		 * When a capture event occurs, capture value (CPT_VALx)
> > +		 * is stored, index incremented, capture edge changed.
> > +		 *
> > +		 * After the capture, if the index > 1, we have collected
> > +		 * the necessary data so we signal the thread waiting for it
> > +		 * and disable the capture by setting capture edge to none
> > +		 *
> > +		 */
> 
> How do you deal with the situation where someone will stop the PWM
> signal half-way in? That is, suppose you've got events for the first and
> second snapshots (0 and 1) and then someone stops the PWM and the event
> for snapshot 2 never happens, how does the code recover?

The 'wait' will timeout and the cycle will be reset.

> > +
> > +		regmap_read(pc->regmap,
> > +			    PWM_CPT_VAL(channel), &d->snapshot[d->index]);
> > +
> > +		switch (d->index) {
> > +		case 0:
> > +		case 1:
> > +			regmap_read(pc->regmap, PWM_CPT_EDGE(channel), &reg);
> > +			reg ^= PWM_CPT_EDGE_MASK;
> > +			regmap_write(pc->regmap, PWM_CPT_EDGE(channel), reg);
> > +
> > +			d->index++;
> > +			break;
> > +		case 2:
> > +			regmap_write(pc->regmap,
> > +				     PWM_CPT_EDGE(channel), CPT_EDGE_DISABLED);
> > +			wake_up(&d->wait);
> > +			break;
> > +		default:
> > +			dev_err(dev, "Internal error\n");
> > +		}
> > +
> > +		clear_bit(channel, (unsigned long int *)&cpt_int_stat);
> 
> clear_bit() is a little unusual to use on regular data types, as
> evidenced by the need for the goofy cast here.

It's just a bit neater (and provides locking) than manually bit
twiddling using bitwise operators.  What do you suggest?

> > +
> > +		ret = IRQ_HANDLED;
> > +	}
> > +
> > +	/* Just ACK everything */
> > +	regmap_write(pc->regmap, PWM_INT_ACK, PWM_INT_ACK_MASK);
> > +
> > +	return ret;
> > +}
> > +
> >  static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
> >  {
> >  	struct device *dev = pc->dev;
> > @@ -354,6 +425,11 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
> >  	if (IS_ERR(pc->pwm_cpt_int_en))
> >  		return PTR_ERR(pc->pwm_cpt_int_en);
> >  
> > +	pc->pwm_cpt_int_stat = devm_regmap_field_alloc(dev, pc->regmap,
> > +						reg_fields[PWM_CPT_INT_STAT]);
> > +	if (IS_ERR(pc->pwm_cpt_int_stat))
> > +		return PTR_ERR(pc->pwm_cpt_int_stat);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -371,7 +447,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
> >  	struct sti_pwm_chip *pc;
> >  	struct resource *res;
> >  	unsigned int chan;
> > -	int ret;
> > +	int ret, irq;
> >  
> >  	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
> >  	if (!pc)
> > @@ -392,6 +468,19 @@ static int sti_pwm_probe(struct platform_device *pdev)
> >  	if (IS_ERR(pc->regmap))
> >  		return PTR_ERR(pc->regmap);
> >  
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0) {
> > +		dev_err(&pdev->dev, "Failed to obtain IRQ\n");
> > +		return -ENODEV;
> > +	}
> 
> I think you need to propagate the return value of platform_get_irq()
> here.

Yes, could do.  Although, I think we could go either way:

$ git grep -A5 platform_get_irq | grep "return ret\|return irq" | wc -l
176
$ git grep -A5 platform_get_irq | grep "return -EINVAL\|-ENODEV\|-ENXIO" | wc -l
256

Happy to change it though.

> > +
> > +	ret = devm_request_irq(&pdev->dev, irq, sti_pwm_interrupt,
> > +			       0, pdev->name, (void *) pc);
> 
> No need for the explicit cast to void *.

You're right.  Will drop it.

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

Re: [RESEND 08/11] pwm: sti: Add support for PWM Capture IRQs

[Thierry Reding]

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

On Wed, Apr 13, 2016 at 11:05:21AM +0100, Lee Jones wrote:
> On Tue, 12 Apr 2016, Thierry Reding wrote:
> 
> > On Wed, Mar 02, 2016 at 03:32:06PM +0000, Lee Jones wrote:
> > [...]
> > > diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
[...]
> > > +
> > > +		regmap_read(pc->regmap,
> > > +			    PWM_CPT_VAL(channel), &d->snapshot[d->index]);
> > > +
> > > +		switch (d->index) {
> > > +		case 0:
> > > +		case 1:
> > > +			regmap_read(pc->regmap, PWM_CPT_EDGE(channel), &reg);
> > > +			reg ^= PWM_CPT_EDGE_MASK;
> > > +			regmap_write(pc->regmap, PWM_CPT_EDGE(channel), reg);
> > > +
> > > +			d->index++;
> > > +			break;
> > > +		case 2:
> > > +			regmap_write(pc->regmap,
> > > +				     PWM_CPT_EDGE(channel), CPT_EDGE_DISABLED);
> > > +			wake_up(&d->wait);
> > > +			break;
> > > +		default:
> > > +			dev_err(dev, "Internal error\n");
> > > +		}
> > > +
> > > +		clear_bit(channel, (unsigned long int *)&cpt_int_stat);
> > 
> > clear_bit() is a little unusual to use on regular data types, as
> > evidenced by the need for the goofy cast here.
> 
> It's just a bit neater (and provides locking) than manually bit
> twiddling using bitwise operators.  What do you suggest?

I think the cast indicates that you're mixing unrelated interfaces. Also
looking at patch 9/11 it seems like you'll need extra locking to protect
against concurrent accesses in the interrupt handler and the ->capture()
implementation anyway, so might as well use that lock for this access.

> > > @@ -371,7 +447,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
> > >  	struct sti_pwm_chip *pc;
> > >  	struct resource *res;
> > >  	unsigned int chan;
> > > -	int ret;
> > > +	int ret, irq;
> > >  
> > >  	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
> > >  	if (!pc)
> > > @@ -392,6 +468,19 @@ static int sti_pwm_probe(struct platform_device *pdev)
> > >  	if (IS_ERR(pc->regmap))
> > >  		return PTR_ERR(pc->regmap);
> > >  
> > > +	irq = platform_get_irq(pdev, 0);
> > > +	if (irq < 0) {
> > > +		dev_err(&pdev->dev, "Failed to obtain IRQ\n");
> > > +		return -ENODEV;
> > > +	}
> > 
> > I think you need to propagate the return value of platform_get_irq()
> > here.
> 
> Yes, could do.  Although, I think we could go either way:
> 
> $ git grep -A5 platform_get_irq | grep "return ret\|return irq" | wc -l
> 176
> $ git grep -A5 platform_get_irq | grep "return -EINVAL\|-ENODEV\|-ENXIO" | wc -l
> 256
> 
> Happy to change it though.

I think all the latter are really wrong. Looking at the implementation
of platform_get_irq() there are a number of error codes that it can
return (-EPROBE_DEFER amongst them), so collapsing them all into an
-EINVAL, -ENODEV or -ENXIO is very wrong.

Thierry

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

[RESEND 09/11] pwm: sti: Add PWM Capture call-back

[Lee Jones]

Once a PWM Capture has been initiated, the capture call
enables a rising edge detection IRQ, then waits.  Once each
of the 3 phase changes have been recorded the thread then
wakes.  The remaining part of the call carries out the
relevant calculations and passes back a formatted string to
the caller.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/pwm-sti.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 82a69e4..8de9b4a 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -309,7 +309,79 @@ static void sti_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 	clear_bit(pwm->hwpwm, &pc->configured);
 }
 
+static int sti_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
+			   int channel, char *buf)
+{
+	struct sti_pwm_chip *pc = to_sti_pwmchip(chip);
+	struct sti_pwm_compat_data *cdata = pc->cdata;
+	struct sti_cpt_data *d = pc->cpt_data[channel];
+	struct device *dev = pc->dev;
+	unsigned int f, dc;
+	unsigned int high, low;
+	bool level;
+	int ret;
+
+	if (channel > cdata->cpt_num_chan - 1) {
+		dev_err(dev, "Channel %d is not valid\n", channel);
+		return -EINVAL;
+	}
+
+	mutex_lock(&d->lock);
+
+	/* Prepare capture measurement */
+	d->index = 0;
+	regmap_write(pc->regmap, PWM_CPT_EDGE(channel), CPT_EDGE_RISING);
+	regmap_field_write(pc->pwm_cpt_int_en, BIT(channel));
+	ret = wait_event_interruptible_timeout(d->wait, d->index > 1, HZ);
+
+	/*
+	 * In case we woke up for another reason than completion
+	 * make sure to disable the capture.
+	 */
+	regmap_write(pc->regmap, PWM_CPT_EDGE(channel), CPT_EDGE_DISABLED);
+
+	if (ret == -ERESTARTSYS)
+		goto out;
+
+	switch (d->index) {
+	case 0:
+	case 1:
+		/*
+		 * Getting here could mean :
+		 *  - input signal is constant of less than 1Hz
+		 *  - there is no input signal at all
+		 *
+		 * In such case the frequency is rounded down to 0
+		 * level of the supposed constant signal is reported
+		 * using duty cycle min and max values.
+		 */
+		level = gpio_get_value(d->gpio);
+
+		ret = sprintf(buf, "0:%u\n", level ? CPT_DC_MAX : 0);
+		break;
+	case 2:
+		/* We have evertying we need */
+		high = d->snapshot[1] - d->snapshot[0];
+		low  = d->snapshot[2] - d->snapshot[1];
+
+		/* Calculate frequency in Hz */
+		f = clk_get_rate(pc->cpt_clk) / (1 * (high + low));
+
+		/* Calculate the duty cycle */
+		dc = CPT_DC_MAX * high / (high + low);
+
+		ret = sprintf(buf, "%u:%u\n", f, dc);
+	default:
+		dev_err(dev, "Internal error\n");
+	}
+
+out:
+	mutex_unlock(&d->lock);
+	return ret;
+}
+
 static const struct pwm_ops sti_pwm_ops = {
+	.capture = sti_pwm_capture,
 	.config = sti_pwm_config,
 	.enable = sti_pwm_enable,
 	.disable = sti_pwm_disable,
-- 
1.9.1

Re: [RESEND 09/11] pwm: sti: Add PWM Capture call-back

[Thierry Reding]

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

On Wed, Mar 02, 2016 at 03:32:07PM +0000, Lee Jones wrote:
> Once a PWM Capture has been initiated, the capture call
> enables a rising edge detection IRQ, then waits.  Once each
> of the 3 phase changes have been recorded the thread then
> wakes.  The remaining part of the call carries out the
> relevant calculations and passes back a formatted string to
> the caller.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/pwm/pwm-sti.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> index 82a69e4..8de9b4a 100644
> --- a/drivers/pwm/pwm-sti.c
> +++ b/drivers/pwm/pwm-sti.c
> @@ -309,7 +309,79 @@ static void sti_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>  	clear_bit(pwm->hwpwm, &pc->configured);
>  }
>  
> +static int sti_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
> +			   int channel, char *buf)
> +{
> +	struct sti_pwm_chip *pc = to_sti_pwmchip(chip);
> +	struct sti_pwm_compat_data *cdata = pc->cdata;
> +	struct sti_cpt_data *d = pc->cpt_data[channel];
> +	struct device *dev = pc->dev;
> +	unsigned int f, dc;
> +	unsigned int high, low;
> +	bool level;
> +	int ret;
> +
> +	if (channel > cdata->cpt_num_chan - 1) {
> +		dev_err(dev, "Channel %d is not valid\n", channel);
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&d->lock);

Should this perhaps reuse the struct pwm_device's ->lock?

> +
> +	/* Prepare capture measurement */
> +	d->index = 0;
> +	regmap_write(pc->regmap, PWM_CPT_EDGE(channel), CPT_EDGE_RISING);
> +	regmap_field_write(pc->pwm_cpt_int_en, BIT(channel));
> +	ret = wait_event_interruptible_timeout(d->wait, d->index > 1, HZ);

The timeout here should make sure callers don't hang forever. But maybe
you can still make sure that when the PWM gets disabled the wait queue
is woken and perhaps return an appropriate error code to let users know
that the operation was interrupted.

Also, how about letting callers choose the value of the timeout? In some
cases they may be interested in long-running signals. In other cases the
whole second timeout may be much too long.

> +
> +	/*
> +	 * In case we woke up for another reason than completion
> +	 * make sure to disable the capture.
> +	 */
> +	regmap_write(pc->regmap, PWM_CPT_EDGE(channel), CPT_EDGE_DISABLED);

The comment here is slightly confusing because it implies that disabling
the capture should be done conditionally, whereas it is always disabled.

> +
> +	if (ret == -ERESTARTSYS)
> +		goto out;
> +
> +	switch (d->index) {
> +	case 0:
> +	case 1:
> +		/*
> +		 * Getting here could mean :
> +		 *  - input signal is constant of less than 1Hz
> +		 *  - there is no input signal at all
> +		 *
> +		 * In such case the frequency is rounded down to 0
> +		 * level of the supposed constant signal is reported
> +		 * using duty cycle min and max values.
> +		 */
> +		level = gpio_get_value(d->gpio);
> +
> +		ret = sprintf(buf, "0:%u\n", level ? CPT_DC_MAX : 0);
> +		break;
> +	case 2:
> +		/* We have evertying we need */
> +		high = d->snapshot[1] - d->snapshot[0];
> +		low  = d->snapshot[2] - d->snapshot[1];
> +
> +		/* Calculate frequency in Hz */
> +		f = clk_get_rate(pc->cpt_clk) / (1 * (high + low));

The multiplication by 1 is unnecessary.

Thierry

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

Re: [RESEND 09/11] pwm: sti: Add PWM Capture call-back

[Lee Jones]

On Tue, 12 Apr 2016, Thierry Reding wrote:

> On Wed, Mar 02, 2016 at 03:32:07PM +0000, Lee Jones wrote:
> > Once a PWM Capture has been initiated, the capture call
> > enables a rising edge detection IRQ, then waits.  Once each
> > of the 3 phase changes have been recorded the thread then
> > wakes.  The remaining part of the call carries out the
> > relevant calculations and passes back a formatted string to
> > the caller.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/pwm/pwm-sti.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 72 insertions(+)
> > 
> > diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> > index 82a69e4..8de9b4a 100644
> > --- a/drivers/pwm/pwm-sti.c
> > +++ b/drivers/pwm/pwm-sti.c
> > @@ -309,7 +309,79 @@ static void sti_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> >  	clear_bit(pwm->hwpwm, &pc->configured);
> >  }
> >  
> > +static int sti_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			   int channel, char *buf)
> > +{
> > +	struct sti_pwm_chip *pc = to_sti_pwmchip(chip);
> > +	struct sti_pwm_compat_data *cdata = pc->cdata;
> > +	struct sti_cpt_data *d = pc->cpt_data[channel];
> > +	struct device *dev = pc->dev;
> > +	unsigned int f, dc;
> > +	unsigned int high, low;
> > +	bool level;
> > +	int ret;
> > +
> > +	if (channel > cdata->cpt_num_chan - 1) {
> > +		dev_err(dev, "Channel %d is not valid\n", channel);
> > +		return -EINVAL;
> > +	}
> > +
> > +	mutex_lock(&d->lock);
> 
> Should this perhaps reuse the struct pwm_device's ->lock?
> 
> > +
> > +	/* Prepare capture measurement */
> > +	d->index = 0;
> > +	regmap_write(pc->regmap, PWM_CPT_EDGE(channel), CPT_EDGE_RISING);
> > +	regmap_field_write(pc->pwm_cpt_int_en, BIT(channel));
> > +	ret = wait_event_interruptible_timeout(d->wait, d->index > 1, HZ);
> 
> The timeout here should make sure callers don't hang forever. But maybe
> you can still make sure that when the PWM gets disabled the wait queue
> is woken and perhaps return an appropriate error code to let users know
> that the operation was interrupted.

Sure.  I'll look into that.

> Also, how about letting callers choose the value of the timeout? In some
> cases they may be interested in long-running signals. In other cases the
> whole second timeout may be much too long.

I'm not opposed to it.  How do you suggest we do that?

> > +	/*
> > +	 * In case we woke up for another reason than completion
> > +	 * make sure to disable the capture.
> > +	 */
> > +	regmap_write(pc->regmap, PWM_CPT_EDGE(channel), CPT_EDGE_DISABLED);
> 
> The comment here is slightly confusing because it implies that disabling
> the capture should be done conditionally, whereas it is always disabled.

Not really.  We do it unconditionally for reason explained.

It says:

  "disable the capture just in case X happens"

rather than

  "disable the capture if X happens".

Perhaps the language is too subtle.  I can reword for clarity.

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

Re: [RESEND 09/11] pwm: sti: Add PWM Capture call-back

[Thierry Reding]

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

On Wed, Apr 13, 2016 at 11:25:54AM +0100, Lee Jones wrote:
> On Tue, 12 Apr 2016, Thierry Reding wrote:
> 
> > On Wed, Mar 02, 2016 at 03:32:07PM +0000, Lee Jones wrote:
> > > Once a PWM Capture has been initiated, the capture call
> > > enables a rising edge detection IRQ, then waits.  Once each
> > > of the 3 phase changes have been recorded the thread then
> > > wakes.  The remaining part of the call carries out the
> > > relevant calculations and passes back a formatted string to
> > > the caller.
> > > 
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/pwm/pwm-sti.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 72 insertions(+)
> > > 
> > > diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> > > index 82a69e4..8de9b4a 100644
> > > --- a/drivers/pwm/pwm-sti.c
> > > +++ b/drivers/pwm/pwm-sti.c
> > > @@ -309,7 +309,79 @@ static void sti_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > >  	clear_bit(pwm->hwpwm, &pc->configured);
> > >  }
> > >  
> > > +static int sti_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +			   int channel, char *buf)
> > > +{
> > > +	struct sti_pwm_chip *pc = to_sti_pwmchip(chip);
> > > +	struct sti_pwm_compat_data *cdata = pc->cdata;
> > > +	struct sti_cpt_data *d = pc->cpt_data[channel];
> > > +	struct device *dev = pc->dev;
> > > +	unsigned int f, dc;
> > > +	unsigned int high, low;
> > > +	bool level;
> > > +	int ret;
> > > +
> > > +	if (channel > cdata->cpt_num_chan - 1) {
> > > +		dev_err(dev, "Channel %d is not valid\n", channel);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	mutex_lock(&d->lock);
> > 
> > Should this perhaps reuse the struct pwm_device's ->lock?

That was actually a stupid suggestion by me, because that lock is being
removed in an unrelated patch series.

> > > +	/* Prepare capture measurement */
> > > +	d->index = 0;
> > > +	regmap_write(pc->regmap, PWM_CPT_EDGE(channel), CPT_EDGE_RISING);
> > > +	regmap_field_write(pc->pwm_cpt_int_en, BIT(channel));
> > > +	ret = wait_event_interruptible_timeout(d->wait, d->index > 1, HZ);
> > 
> > The timeout here should make sure callers don't hang forever. But maybe
> > you can still make sure that when the PWM gets disabled the wait queue
> > is woken and perhaps return an appropriate error code to let users know
> > that the operation was interrupted.
> 
> Sure.  I'll look into that.
> 
> > Also, how about letting callers choose the value of the timeout? In some
> > cases they may be interested in long-running signals. In other cases the
> > whole second timeout may be much too long.
> 
> I'm not opposed to it.  How do you suggest we do that?

The easiest would probably be to add an unsigned long timeout parameter
to the pwm_capture() function and ->capture() callbacks.

But thinking about this further I'm wondering if it might not be easier
and more flexible to move the timeout completely outside of this code
and into callers. I suspect that the most simple way to do that would be
to add a completion to struct pwm_capture that callers can use to wait
for completion of a capture. This would make the whole process
asynchronous and allow interesting things like making the sysfs capture
file pollable, for example.

> > > +	/*
> > > +	 * In case we woke up for another reason than completion
> > > +	 * make sure to disable the capture.
> > > +	 */
> > > +	regmap_write(pc->regmap, PWM_CPT_EDGE(channel), CPT_EDGE_DISABLED);
> > 
> > The comment here is slightly confusing because it implies that disabling
> > the capture should be done conditionally, whereas it is always disabled.
> 
> Not really.  We do it unconditionally for reason explained.
> 
> It says:
> 
>   "disable the capture just in case X happens"
> 
> rather than
> 
>   "disable the capture if X happens".
> 
> Perhaps the language is too subtle.  I can reword for clarity.

I'd be okay with just dropping the comment altogether, it seems rather
obvious to me. But clarifying is okay with me, too.

Thierry

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

Re: [RESEND 09/11] pwm: sti: Add PWM Capture call-back

[Lee Jones]

On Wed, 13 Apr 2016, Thierry Reding wrote:

> On Wed, Apr 13, 2016 at 11:25:54AM +0100, Lee Jones wrote:
> > On Tue, 12 Apr 2016, Thierry Reding wrote:
> > 
> > > On Wed, Mar 02, 2016 at 03:32:07PM +0000, Lee Jones wrote:
> > > > Once a PWM Capture has been initiated, the capture call
> > > > enables a rising edge detection IRQ, then waits.  Once each
> > > > of the 3 phase changes have been recorded the thread then
> > > > wakes.  The remaining part of the call carries out the
> > > > relevant calculations and passes back a formatted string to
> > > > the caller.
> > > > 
> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > ---
> > > >  drivers/pwm/pwm-sti.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 72 insertions(+)
> > > > 
> > > > diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> > > > index 82a69e4..8de9b4a 100644
> > > > --- a/drivers/pwm/pwm-sti.c
> > > > +++ b/drivers/pwm/pwm-sti.c

[...]

> > > > +	/* Prepare capture measurement */
> > > > +	d->index = 0;
> > > > +	regmap_write(pc->regmap, PWM_CPT_EDGE(channel), CPT_EDGE_RISING);
> > > > +	regmap_field_write(pc->pwm_cpt_int_en, BIT(channel));
> > > > +	ret = wait_event_interruptible_timeout(d->wait, d->index > 1, HZ);
> > > 
> > > The timeout here should make sure callers don't hang forever. But maybe
> > > you can still make sure that when the PWM gets disabled the wait queue
> > > is woken and perhaps return an appropriate error code to let users know
> > > that the operation was interrupted.
> > 
> > Sure.  I'll look into that.
> > 
> > > Also, how about letting callers choose the value of the timeout? In some
> > > cases they may be interested in long-running signals. In other cases the
> > > whole second timeout may be much too long.
> > 
> > I'm not opposed to it.  How do you suggest we do that?
> 
> The easiest would probably be to add an unsigned long timeout parameter
> to the pwm_capture() function and ->capture() callbacks.
> 
> But thinking about this further I'm wondering if it might not be easier
> and more flexible to move the timeout completely outside of this code
> and into callers. I suspect that the most simple way to do that would be
> to add a completion to struct pwm_capture that callers can use to wait
> for completion of a capture. This would make the whole process
> asynchronous and allow interesting things like making the sysfs capture
> file pollable, for example.

Okay, so how do you propose we handle this with sysfs?  Perhaps
another RW file to set it?

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

Re: [RESEND 09/11] pwm: sti: Add PWM Capture call-back

[Thierry Reding]

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

On Fri, Apr 15, 2016 at 09:29:00AM +0100, Lee Jones wrote:
> On Wed, 13 Apr 2016, Thierry Reding wrote:
> 
> > On Wed, Apr 13, 2016 at 11:25:54AM +0100, Lee Jones wrote:
> > > On Tue, 12 Apr 2016, Thierry Reding wrote:
> > > 
> > > > On Wed, Mar 02, 2016 at 03:32:07PM +0000, Lee Jones wrote:
> > > > > Once a PWM Capture has been initiated, the capture call
> > > > > enables a rising edge detection IRQ, then waits.  Once each
> > > > > of the 3 phase changes have been recorded the thread then
> > > > > wakes.  The remaining part of the call carries out the
> > > > > relevant calculations and passes back a formatted string to
> > > > > the caller.
> > > > > 
> > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > ---
> > > > >  drivers/pwm/pwm-sti.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 72 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> > > > > index 82a69e4..8de9b4a 100644
> > > > > --- a/drivers/pwm/pwm-sti.c
> > > > > +++ b/drivers/pwm/pwm-sti.c
> 
> [...]
> 
> > > > > +	/* Prepare capture measurement */
> > > > > +	d->index = 0;
> > > > > +	regmap_write(pc->regmap, PWM_CPT_EDGE(channel), CPT_EDGE_RISING);
> > > > > +	regmap_field_write(pc->pwm_cpt_int_en, BIT(channel));
> > > > > +	ret = wait_event_interruptible_timeout(d->wait, d->index > 1, HZ);
> > > > 
> > > > The timeout here should make sure callers don't hang forever. But maybe
> > > > you can still make sure that when the PWM gets disabled the wait queue
> > > > is woken and perhaps return an appropriate error code to let users know
> > > > that the operation was interrupted.
> > > 
> > > Sure.  I'll look into that.
> > > 
> > > > Also, how about letting callers choose the value of the timeout? In some
> > > > cases they may be interested in long-running signals. In other cases the
> > > > whole second timeout may be much too long.
> > > 
> > > I'm not opposed to it.  How do you suggest we do that?
> > 
> > The easiest would probably be to add an unsigned long timeout parameter
> > to the pwm_capture() function and ->capture() callbacks.
> > 
> > But thinking about this further I'm wondering if it might not be easier
> > and more flexible to move the timeout completely outside of this code
> > and into callers. I suspect that the most simple way to do that would be
> > to add a completion to struct pwm_capture that callers can use to wait
> > for completion of a capture. This would make the whole process
> > asynchronous and allow interesting things like making the sysfs capture
> > file pollable, for example.
> 
> Okay, so how do you propose we handle this with sysfs?  Perhaps
> another RW file to set it?

I'm unfamiliar with how this is done in other drivers, so I'd have to
look at them first. I suspect that it would be fine for now to simply
redesign the PWM API parts and keep some default timeout in sysfs. It
could be extended with some mechanism to override the default timeout
in the future.

Thierry

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

[RESEND 10/11] pwm: sti: Enable PWM Capture

[Lee Jones]

Once all functionality is in place, we provide the means to
enable PWM Capture.  Here we are simply obtaining the
associated regmap and twiddling the relevant enable register
bits.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/pwm-sti.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 8de9b4a..93cf20e 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -279,6 +279,13 @@ static int sti_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 				pwm->hwpwm);
 			goto out;
 		}
+
+		ret = regmap_field_write(pc->pwm_cpt_en, 1);
+		if (ret) {
+			dev_err(dev, "failed to enable PWM capture:%d\n",
+				pwm->hwpwm);
+			goto out;
+		}
 	}
 	pc->en_count++;
 out:
@@ -296,6 +303,7 @@ static void sti_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 		return;
 	}
 	regmap_field_write(pc->pwm_out_en, 0);
+	regmap_field_write(pc->pwm_cpt_en, 0);
 
 	clk_disable(pc->pwm_clk);
 	clk_disable(pc->cpt_clk);
@@ -492,6 +500,11 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
 	if (IS_ERR(pc->pwm_out_en))
 		return PTR_ERR(pc->pwm_out_en);
 
+	pc->pwm_cpt_en = devm_regmap_field_alloc(dev, pc->regmap,
+						 reg_fields[PWM_CPT_EN]);
+	if (IS_ERR(pc->pwm_cpt_en))
+		return PTR_ERR(pc->pwm_cpt_en);
+
 	pc->pwm_cpt_int_en = devm_regmap_field_alloc(dev, pc->regmap,
 						 reg_fields[PWM_CPT_INT_EN]);
 	if (IS_ERR(pc->pwm_cpt_int_en))
-- 
1.9.1

Re: [RESEND 10/11] pwm: sti: Enable PWM Capture

[Thierry Reding]

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

On Wed, Mar 02, 2016 at 03:32:08PM +0000, Lee Jones wrote:
> Once all functionality is in place, we provide the means to
> enable PWM Capture.  Here we are simply obtaining the
> associated regmap and twiddling the relevant enable register
> bits.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/pwm/pwm-sti.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> index 8de9b4a..93cf20e 100644
> --- a/drivers/pwm/pwm-sti.c
> +++ b/drivers/pwm/pwm-sti.c
> @@ -279,6 +279,13 @@ static int sti_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  				pwm->hwpwm);
>  			goto out;
>  		}
> +
> +		ret = regmap_field_write(pc->pwm_cpt_en, 1);
> +		if (ret) {
> +			dev_err(dev, "failed to enable PWM capture:%d\n",
> +				pwm->hwpwm);
> +			goto out;
> +		}

Should this perhaps be part of the driver's ->capture() implementation?
It seems redundant to have this logic enabled if we may never use it.

Thierry

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

[RESEND 11/11] pwm: sti: Take the opportunity to conduct a little house keeping

[Lee Jones]

This includes fixing some Coding Style issues and re-ordering/
simplifying a little code.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/pwm-sti.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 93cf20e..93e3abf 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -144,13 +144,13 @@ static int sti_pwm_get_prescale(struct sti_pwm_chip *pc, unsigned long period,
 	val = NSEC_PER_SEC / clk_rate;
 	val *= cdata->max_pwm_cnt + 1;
 
-	if (period % val) {
+	if (period % val)
 		return -EINVAL;
-	} else {
-		ps  = period / val - 1;
-		if (ps > cdata->max_prescale)
-			return -EINVAL;
-	}
+
+	ps  = period / val - 1;
+	if (ps > cdata->max_prescale)
+		return -EINVAL;
+
 	*prescale = ps;
 
 	return 0;
@@ -166,7 +166,7 @@ static int sti_pwm_get_prescale(struct sti_pwm_chip *pc, unsigned long period,
  * 256 values.
  */
 static int sti_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			 int duty_ns, int period_ns)
+			int duty_ns, int period_ns)
 {
 	struct sti_pwm_chip *pc = to_sti_pwmchip(chip);
 	struct sti_pwm_compat_data *cdata = pc->cdata;
@@ -212,7 +212,7 @@ static int sti_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 			ret =
 			regmap_field_write(pc->prescale_low,
-					   prescale & PWM_PRESCALE_LOW_MASK);
+				prescale & PWM_PRESCALE_LOW_MASK);
 			if (ret)
 				goto clk_dis;
 
@@ -506,7 +506,7 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
 		return PTR_ERR(pc->pwm_cpt_en);
 
 	pc->pwm_cpt_int_en = devm_regmap_field_alloc(dev, pc->regmap,
-						 reg_fields[PWM_CPT_INT_EN]);
+						reg_fields[PWM_CPT_INT_EN]);
 	if (IS_ERR(pc->pwm_cpt_int_en))
 		return PTR_ERR(pc->pwm_cpt_int_en);
 
@@ -570,10 +570,11 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	 * Setup PWM data with default values: some values could be replaced
 	 * with specific ones provided from Device Tree.
 	 */
-	cdata->reg_fields   = &sti_pwm_regfields[0];
-	cdata->max_prescale = 0xff;
-	cdata->max_pwm_cnt  = 255;
-	cdata->pwm_num_chan     = 1;
+
+	cdata->reg_fields	= &sti_pwm_regfields[0];
+	cdata->max_prescale	= 0xff;
+	cdata->max_pwm_cnt	= 255;
+	cdata->pwm_num_chan	= 1;
 	cdata->cpt_num_chan	= 0;
 
 	pc->cdata = cdata;
-- 
1.9.1

Re: [RESEND 00/11] pwm: Add support for PWM Capture

[Lee Jones]

On Wed, 02 Mar 2016, Lee Jones wrote:

> The first part of this set extends the current PWM API to allow external
> code to request a PWM Capture.  Subsequent patches then make use of the
> new API by providing a userspace offering via /sysfs.  The final part of
> the set supplies PWM Capture functionality into the already existing STi
> PWM driver.
>  
> This patch-set has been tested end to end via /sysfs.

Hopefully you still have this set in your inbox.

Please let me know if you wish me to resend it.

> Lee Jones (11):
>   pwm: Add PWM Capture support
>   pwm: sysfs: Add PWM Capture support
>   pwm: sti: Reorganise register names in preparation for new
>     functionality
>   pwm: sti: Only request clock rate when you need to
>   pwm: sti: Supply PWM Capture register addresses and bit locations
>   pwm: sti: Supply PWM Capture clock handling
>   pwm: sti: Initialise PWM Capture channel data
>   pwm: sti: Add support for PWM Capture IRQs
>   pwm: sti: Add PWM Capture call-back
>   pwm: sti: Enable PWM Capture
>   pwm: sti: Take the opportunity to conduct a little house keeping
> 
>  drivers/pwm/core.c    |  26 ++++
>  drivers/pwm/pwm-sti.c | 384 ++++++++++++++++++++++++++++++++++++++++++--------
>  drivers/pwm/sysfs.c   |  28 ++++
>  include/linux/pwm.h   |  13 ++
>  4 files changed, 392 insertions(+), 59 deletions(-)
> 

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