[PATCH v3 00/20] pwm: Add support for PWM Capture

[PATCH v3 00/20] 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.

v2 => v3:
- Supply DT documentation changes
- Submit DTS additions as part of this set
- Default Capture and PWM-out number of channels to 0
  - Do additional checking, to ensure at least one channel is requested
- Use global locking, instead of lock in device data

v1 => v2:
- API change
- Use a struct to carry the result back to the caller
- Use 'struct pwm' to store device specific data
- Make timeout configurable
- Don't use clear_bit(), instead use raw bit logic
- Propagate return value of platform_get_irq()
- Don't cast to (void *)
- Move to subsystem terminology (channels => devices)
- Remove channel select feature
- Enable Capture IP during capture

Lee Jones (20):
  ARM: dts: STi: Rename properites in line with PWM naming conventions
  ARM: dts: STiH407: Supply PWM Capture IRQ
  ARM: dts: STiH407: Declare PWM Capture data lines via Pinctrl
  ARM: dts: STiH416: Supply PWM Capture IRQs
  ARM: dts: STiH416: Declare PWM Capture data lines via Pinctrl
  ARM: dts: STiH416: Define PWM Capture clock
  ARM: dts: STiH416: Define the number of PWM Capture channels
  pwm: Add PWM Capture support
  pwm: sti: Rename channel => device
  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 device data
  pwm: sti: Add support for PWM Capture IRQs
  pwm: sti: Add PWM Capture call-back
  pwm: sti: It's now valid for number of PWM channels to be zero
  pwm: sti: Take the opportunity to conduct a little house keeping
  dt-bindings: pwm: sti: Update DT bindings with recent changes

 Documentation/devicetree/bindings/pwm/pwm-st.txt |  10 +-
 arch/arm/boot/dts/stih407-family.dtsi            |   5 +-
 arch/arm/boot/dts/stih407-pinctrl.dtsi           |   3 +
 arch/arm/boot/dts/stih416-pinctrl.dtsi           |   5 +
 arch/arm/boot/dts/stih416.dtsi                   |  12 +-
 drivers/pwm/core.c                               |  27 ++
 drivers/pwm/pwm-sti.c                            | 424 +++++++++++++++++++----
 drivers/pwm/sysfs.c                              |  17 +
 include/linux/pwm.h                              |  25 ++
 9 files changed, 447 insertions(+), 81 deletions(-)

-- 
2.8.3

[PATCH v3 01/20] ARM: dts: STi: Rename properites in line with PWM naming conventions

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih407-family.dtsi | 4 ++--
 arch/arm/boot/dts/stih416.dtsi        | 5 +++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index 5947878..0723900 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -668,7 +668,7 @@
 			pinctrl-0	= <&pinctrl_pwm0_chan0_default>;
 			clock-names	= "pwm";
 			clocks		= <&clk_sysin>;
-			st,pwm-num-chan = <1>;
+			st,pwm-num-devs	= <1>;
 
 			status		= "okay";
 		};
@@ -685,7 +685,7 @@
 					&pinctrl_pwm1_chan3_default>;
 			clock-names	= "pwm";
 			clocks		= <&clk_sysin>;
-			st,pwm-num-chan = <4>;
+			st,pwm-num-devs	= <4>;
 
 			status		= "disabled";
 		};
diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
index 0afb4f7..82091072 100644
--- a/arch/arm/boot/dts/stih416.dtsi
+++ b/arch/arm/boot/dts/stih416.dtsi
@@ -484,7 +484,8 @@
 
 			clock-names	= "pwm";
 			clocks		= <&clk_sysin>;
-			st,pwm-num-chan = <4>;
+
+			st,pwm-num-devs = <4>;
 		};
 
 		/* SBC PWM Module */
@@ -508,7 +509,7 @@
 
 			clock-names	= "pwm";
 			clocks		= <&clk_sysin>;
-			st,pwm-num-chan = <3>;
+			st,pwm-num-devs = <3>;
 		};
 	};
 };
-- 
2.8.3

[PATCH v3 02/20] ARM: dts: STiH407: Supply PWM Capture IRQ

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih407-family.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index 0723900..37a3bcc 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -664,6 +664,7 @@
 			compatible	= "st,sti-pwm";
 			#pwm-cells	= <2>;
 			reg		= <0x9810000 0x68>;
+			interrupts      = <GIC_SPI 128 IRQ_TYPE_NONE>;
 			pinctrl-names	= "default";
 			pinctrl-0	= <&pinctrl_pwm0_chan0_default>;
 			clock-names	= "pwm";
-- 
2.8.3

[PATCH v3 03/20] ARM: dts: STiH407: Declare PWM Capture data lines via Pinctrl

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih407-pinctrl.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/boot/dts/stih407-pinctrl.dtsi b/arch/arm/boot/dts/stih407-pinctrl.dtsi
index a538ae5..bc22122 100644
--- a/arch/arm/boot/dts/stih407-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stih407-pinctrl.dtsi
@@ -289,10 +289,12 @@
 				pinctrl_pwm1_chan0_default: pwm1-0-default {
 					st,pins {
 						pwm-out = <&pio3 0 ALT1 OUT>;
+						pwm-capturein = <&pio3 2 ALT1 IN>;
 					};
 				};
 				pinctrl_pwm1_chan1_default: pwm1-1-default {
 					st,pins {
+						pwm-capturein = <&pio4 3 ALT1 IN>;
 						pwm-out = <&pio4 4 ALT1 OUT>;
 					};
 				};
@@ -1030,6 +1032,7 @@
 			pwm0 {
 				pinctrl_pwm0_chan0_default: pwm0-0-default {
 					st,pins {
+						pwm-capturein = <&pio31 0 ALT1 IN>;
 						pwm-out = <&pio31 1 ALT1 OUT>;
 					};
 				};
-- 
2.8.3

[PATCH v3 04/20] ARM: dts: STiH416: Supply PWM Capture IRQs

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih416.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
index 82091072..28d84c71 100644
--- a/arch/arm/boot/dts/stih416.dtsi
+++ b/arch/arm/boot/dts/stih416.dtsi
@@ -475,6 +475,7 @@
 			status		= "okay";
 			#pwm-cells	= <2>;
 			reg		= <0xfed10000 0x68>;
+			interrupts      = <GIC_SPI 200 IRQ_TYPE_NONE>;
 
 			pinctrl-names	= "default";
 			pinctrl-0 = 	<&pinctrl_pwm0_chan0_default
@@ -494,6 +495,7 @@
 			status		= "disabled";
 			#pwm-cells	= <2>;
 			reg		= <0xfe510000 0x68>;
+			interrupts      = <GIC_SPI 202 IRQ_TYPE_NONE>;
 
 			pinctrl-names	= "default";
 			pinctrl-0	= <&pinctrl_pwm1_chan0_default
-- 
2.8.3

[PATCH v3 05/20] ARM: dts: STiH416: Declare PWM Capture data lines via Pinctrl

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih416-pinctrl.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/stih416-pinctrl.dtsi b/arch/arm/boot/dts/stih416-pinctrl.dtsi
index 051fc16..252b677 100644
--- a/arch/arm/boot/dts/stih416-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stih416-pinctrl.dtsi
@@ -221,11 +221,14 @@
 				pinctrl_pwm1_chan0_default: pwm1-0-default {
 					st,pins {
 						pwm-out    = <&pio3 0 ALT1 OUT>;
+						pwm-capturein = <&pio3 2 ALT1 IN>;
+
 					};
 				};
 				pinctrl_pwm1_chan1_default: pwm1-1-default {
 					st,pins {
 						pwm-out    = <&pio4 4 ALT1 OUT>;
+						pwm-capturein = <&pio4 3 ALT1 IN>;
 					};
 				};
 				pinctrl_pwm1_chan2_default: pwm1-2-default {
@@ -337,6 +340,7 @@
 				pinctrl_pwm0_chan0_default: pwm0-0-default {
 					st,pins {
 						pwm-out    = <&pio9 7 ALT2 OUT>;
+						pwm-capturein = <&pio9 6 ALT2 IN>;
 					};
 				};
 			};
@@ -576,6 +580,7 @@
 				pinctrl_pwm0_chan1_default: pwm0-1-default {
 					st,pins {
 						pwm-out    = <&pio13 2 ALT2 OUT>;
+						pwm-capturein = <&pio13 1 ALT2 IN>;
 					};
 				};
 				pinctrl_pwm0_chan2_default: pwm0-2-default {
-- 
2.8.3

[PATCH v3 06/20] ARM: dts: STiH416: Define PWM Capture clock

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih416.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
index 28d84c71..c93e78d2 100644
--- a/arch/arm/boot/dts/stih416.dtsi
+++ b/arch/arm/boot/dts/stih416.dtsi
@@ -483,8 +483,8 @@
 					&pinctrl_pwm0_chan2_default
 					&pinctrl_pwm0_chan3_default>;
 
-			clock-names	= "pwm";
-			clocks		= <&clk_sysin>;
+			clock-names	= "pwm", "capture";
+			clocks		= <&clk_sysin>, <&clk_s_a0_ls CLK_ICN_REG>;
 
 			st,pwm-num-devs = <4>;
 		};
-- 
2.8.3

[PATCH v3 07/20] ARM: dts: STiH416: Define the number of PWM Capture channels

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih416.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
index c93e78d2..7621fcd 100644
--- a/arch/arm/boot/dts/stih416.dtsi
+++ b/arch/arm/boot/dts/stih416.dtsi
@@ -487,6 +487,7 @@
 			clocks		= <&clk_sysin>, <&clk_s_a0_ls CLK_ICN_REG>;
 
 			st,pwm-num-devs = <4>;
+			st,capture-num-devs = <2>;
 		};
 
 		/* SBC PWM Module */
-- 
2.8.3

[PATCH v3 08/20] 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  | 27 +++++++++++++++++++++++++++
 include/linux/pwm.h | 25 +++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index dba3843..4678de6 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -525,6 +525,33 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
 EXPORT_SYMBOL_GPL(pwm_apply_state);
 
 /**
+ * pwm_capture() - capture and report a PWM signal
+ * @pwm: PWM device
+ * @result: struct to fill with capture result
+ * @timeout_ms: time to wait, in milliseconds, before giving up on capture
+ *
+ * Returns: 0 on success or a negative error code on failure.
+ */
+int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
+		unsigned int timeout_ms)
+{
+	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, result, timeout_ms);
+	mutex_unlock(&pwm_lock);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(pwm_capture);
+
+/**
  * pwm_adjust_config() - adjust the current PWM config to the PWM arguments
  * @pwm: PWM device
  *
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 17018f3..13cac27 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -5,7 +5,9 @@
 #include <linux/mutex.h>
 #include <linux/of.h>
 
+struct pwm_capture;
 struct seq_file;
+
 struct pwm_chip;
 
 /**
@@ -153,6 +155,7 @@ static inline void pwm_get_args(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
  * @apply: atomically apply a new PWM config. The state argument
@@ -172,6 +175,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,
+		       struct pwm_capture *result, unsigned int timeout_ms);
 	int (*enable)(struct pwm_chip *chip, struct pwm_device *pwm);
 	void (*disable)(struct pwm_chip *chip, struct pwm_device *pwm);
 	int (*apply)(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -212,6 +217,16 @@ struct pwm_chip {
 	bool can_sleep;
 };
 
+/**
+ * struct pwm_capture - PWM capture data
+ * @period: period of the PWM signal (in nanoseconds)
+ * @duty_cycle: duty cycle of the PWM signal (in nanoseconds)
+ */
+struct pwm_capture {
+	unsigned long long period;
+	unsigned long long duty_cycle;
+};
+
 #if IS_ENABLED(CONFIG_PWM)
 /* PWM user APIs */
 struct pwm_device *pwm_request(int pwm_id, const char *label);
@@ -322,6 +337,9 @@ static inline void pwm_disable(struct pwm_device *pwm)
 
 
 /* PWM provider APIs */
+int pwm_capture(struct pwm_device *pwm,
+		struct pwm_capture *result,
+		unsigned int timeout_ms);
 int pwm_set_chip_data(struct pwm_device *pwm, void *data);
 void *pwm_get_chip_data(struct pwm_device *pwm);
 
@@ -373,6 +391,13 @@ static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
 	return -EINVAL;
 }
 
+static inline int pwm_capture(struct pwm_device *pwm,
+			      struct pwm_capture *result,
+			      unsigned int timeout_ms)
+{
+	return -EINVAL;
+}
+
 static inline int pwm_set_polarity(struct pwm_device *pwm,
 				   enum pwm_polarity polarity)
 {
-- 
2.8.3

Re: [PATCH v3 08/20] pwm: Add PWM Capture support

[Thierry Reding]

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

On Wed, Jun 08, 2016 at 10:21:23AM +0100, 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  | 27 +++++++++++++++++++++++++++
>  include/linux/pwm.h | 25 +++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index dba3843..4678de6 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -525,6 +525,33 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
>  EXPORT_SYMBOL_GPL(pwm_apply_state);
>  
>  /**
> + * pwm_capture() - capture and report a PWM signal
> + * @pwm: PWM device
> + * @result: struct to fill with capture result
> + * @timeout_ms: time to wait, in milliseconds, before giving up on capture
> + *
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
> +		unsigned int timeout_ms)
> +{
> +	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, result, timeout_ms);
> +	mutex_unlock(&pwm_lock);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(pwm_capture);
> +
> +/**
>   * pwm_adjust_config() - adjust the current PWM config to the PWM arguments
>   * @pwm: PWM device
>   *
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 17018f3..13cac27 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -5,7 +5,9 @@
>  #include <linux/mutex.h>
>  #include <linux/of.h>
>  
> +struct pwm_capture;
>  struct seq_file;
> +
>  struct pwm_chip;
>  
>  /**
> @@ -153,6 +155,7 @@ static inline void pwm_get_args(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
>   * @apply: atomically apply a new PWM config. The state argument
> @@ -172,6 +175,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,
> +		       struct pwm_capture *result, unsigned int timeout_ms);

Can we please drop the _ms suffix. It's already documented to be in
milliseconds. Also maybe make that unsigned long for consistency with
the type of the timeout parameter elsewhere in the kernel.

>  	int (*enable)(struct pwm_chip *chip, struct pwm_device *pwm);
>  	void (*disable)(struct pwm_chip *chip, struct pwm_device *pwm);
>  	int (*apply)(struct pwm_chip *chip, struct pwm_device *pwm,
> @@ -212,6 +217,16 @@ struct pwm_chip {
>  	bool can_sleep;
>  };
>  
> +/**
> + * struct pwm_capture - PWM capture data
> + * @period: period of the PWM signal (in nanoseconds)
> + * @duty_cycle: duty cycle of the PWM signal (in nanoseconds)
> + */
> +struct pwm_capture {
> +	unsigned long long period;
> +	unsigned long long duty_cycle;
> +};

I'd prefer these to be unsigned int, for symmetry with the PWM output
part of the framework. With 32 bits you get about 4.2 seconds of period
and duty cycle, and I doubt that any reasonable signal would extend
beyond that.

> @@ -322,6 +337,9 @@ static inline void pwm_disable(struct pwm_device *pwm)
>  
>  
>  /* PWM provider APIs */
> +int pwm_capture(struct pwm_device *pwm,
> +		struct pwm_capture *result,
> +		unsigned int timeout_ms);

This fits into 2 lines. And same comments on the timeout parameter.

>  int pwm_set_chip_data(struct pwm_device *pwm, void *data);
>  void *pwm_get_chip_data(struct pwm_device *pwm);
>  
> @@ -373,6 +391,13 @@ static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
>  	return -EINVAL;
>  }
>  
> +static inline int pwm_capture(struct pwm_device *pwm,
> +			      struct pwm_capture *result,
> +			      unsigned int timeout_ms)

Same here.

Otherwise this looks really nice to me from an API point of view.

Thierry

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

Re: [PATCH v3 08/20] pwm: Add PWM Capture support

[Lee Jones]

On Fri, 10 Jun 2016, Thierry Reding wrote:

> On Wed, Jun 08, 2016 at 10:21:23AM +0100, 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  | 27 +++++++++++++++++++++++++++
> >  include/linux/pwm.h | 25 +++++++++++++++++++++++++
> >  2 files changed, 52 insertions(+)

So do you want me to re-spin?

Before you said you'd make adjustments on patches 8 through 10, so I'm
a little confused.

> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index dba3843..4678de6 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -525,6 +525,33 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
> >  EXPORT_SYMBOL_GPL(pwm_apply_state);
> >  
> >  /**
> > + * pwm_capture() - capture and report a PWM signal
> > + * @pwm: PWM device
> > + * @result: struct to fill with capture result
> > + * @timeout_ms: time to wait, in milliseconds, before giving up on capture
> > + *
> > + * Returns: 0 on success or a negative error code on failure.
> > + */
> > +int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
> > +		unsigned int timeout_ms)
> > +{
> > +	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, result, timeout_ms);
> > +	mutex_unlock(&pwm_lock);
> > +
> > +	return err;
> > +}
> > +EXPORT_SYMBOL_GPL(pwm_capture);
> > +
> > +/**
> >   * pwm_adjust_config() - adjust the current PWM config to the PWM arguments
> >   * @pwm: PWM device
> >   *
> > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > index 17018f3..13cac27 100644
> > --- a/include/linux/pwm.h
> > +++ b/include/linux/pwm.h
> > @@ -5,7 +5,9 @@
> >  #include <linux/mutex.h>
> >  #include <linux/of.h>
> >  
> > +struct pwm_capture;
> >  struct seq_file;
> > +
> >  struct pwm_chip;
> >  
> >  /**
> > @@ -153,6 +155,7 @@ static inline void pwm_get_args(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
> >   * @apply: atomically apply a new PWM config. The state argument
> > @@ -172,6 +175,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,
> > +		       struct pwm_capture *result, unsigned int timeout_ms);
> 
> Can we please drop the _ms suffix. It's already documented to be in
> milliseconds. Also maybe make that unsigned long for consistency with
> the type of the timeout parameter elsewhere in the kernel.
> 
> >  	int (*enable)(struct pwm_chip *chip, struct pwm_device *pwm);
> >  	void (*disable)(struct pwm_chip *chip, struct pwm_device *pwm);
> >  	int (*apply)(struct pwm_chip *chip, struct pwm_device *pwm,
> > @@ -212,6 +217,16 @@ struct pwm_chip {
> >  	bool can_sleep;
> >  };
> >  
> > +/**
> > + * struct pwm_capture - PWM capture data
> > + * @period: period of the PWM signal (in nanoseconds)
> > + * @duty_cycle: duty cycle of the PWM signal (in nanoseconds)
> > + */
> > +struct pwm_capture {
> > +	unsigned long long period;
> > +	unsigned long long duty_cycle;
> > +};
> 
> I'd prefer these to be unsigned int, for symmetry with the PWM output
> part of the framework. With 32 bits you get about 4.2 seconds of period
> and duty cycle, and I doubt that any reasonable signal would extend
> beyond that.
> 
> > @@ -322,6 +337,9 @@ static inline void pwm_disable(struct pwm_device *pwm)
> >  
> >  
> >  /* PWM provider APIs */
> > +int pwm_capture(struct pwm_device *pwm,
> > +		struct pwm_capture *result,
> > +		unsigned int timeout_ms);
> 
> This fits into 2 lines. And same comments on the timeout parameter.
> 
> >  int pwm_set_chip_data(struct pwm_device *pwm, void *data);
> >  void *pwm_get_chip_data(struct pwm_device *pwm);
> >  
> > @@ -373,6 +391,13 @@ static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
> >  	return -EINVAL;
> >  }
> >  
> > +static inline int pwm_capture(struct pwm_device *pwm,
> > +			      struct pwm_capture *result,
> > +			      unsigned int timeout_ms)
> 
> Same here.
> 
> Otherwise this looks really nice to me from an API point of view.
> 
> Thierry



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

Re: [PATCH v3 08/20] pwm: Add PWM Capture support

[Thierry Reding]

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

On Fri, Jun 10, 2016 at 03:38:54PM +0100, Lee Jones wrote:
> On Fri, 10 Jun 2016, Thierry Reding wrote:
> 
> > On Wed, Jun 08, 2016 at 10:21:23AM +0100, 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  | 27 +++++++++++++++++++++++++++
> > >  include/linux/pwm.h | 25 +++++++++++++++++++++++++
> > >  2 files changed, 52 insertions(+)
> 
> So do you want me to re-spin?
> 
> Before you said you'd make adjustments on patches 8 through 10, so I'm
> a little confused.

Yeah, I only realized that it might be quicker to make these adjustments
myself when I had finished looking at patch 10. I've pushed the adjusted
patches to a for-4.8/pwm-capture branch, so if you're going to re-spin
the remainder of the patches, perhaps that could be your base?

	https://git.kernel.org/cgit/linux/kernel/git/thierry.reding/linux-pwm.git/log/?h=for-4.8/pwm-capture

If you don't have any objections I can put those into linux-next early
next week.

Thierry

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

Re: [PATCH v3 08/20] pwm: Add PWM Capture support

[Lee Jones]

On Fri, 10 Jun 2016, Thierry Reding wrote:

> On Fri, Jun 10, 2016 at 03:38:54PM +0100, Lee Jones wrote:
> > On Fri, 10 Jun 2016, Thierry Reding wrote:
> > 
> > > On Wed, Jun 08, 2016 at 10:21:23AM +0100, 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  | 27 +++++++++++++++++++++++++++
> > > >  include/linux/pwm.h | 25 +++++++++++++++++++++++++
> > > >  2 files changed, 52 insertions(+)
> > 
> > So do you want me to re-spin?
> > 
> > Before you said you'd make adjustments on patches 8 through 10, so I'm
> > a little confused.
> 
> Yeah, I only realized that it might be quicker to make these adjustments
> myself when I had finished looking at patch 10. I've pushed the adjusted
> patches to a for-4.8/pwm-capture branch, so if you're going to re-spin
> the remainder of the patches, perhaps that could be your base?
> 
> 	https://git.kernel.org/cgit/linux/kernel/git/thierry.reding/linux-pwm.git/log/?h=for-4.8/pwm-capture
> 
> If you don't have any objections I can put those into linux-next early
> next week.

That's fine by me.  I will rebase.

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

[PATCH v3 09/20] pwm: sti: Rename channel => device

[Lee Jones]

This is to bring the terminology used in the STi PWM driver more
into line with the PWM subsystem.

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

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 92abbd5..3dae127 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -21,7 +21,7 @@
 #include <linux/slab.h>
 #include <linux/time.h>
 
-#define STI_DS_REG(ch)	(4 * (ch))	/* Channel's Duty Cycle register */
+#define STI_DS_REG(ch)	(4 * (ch))	/* Device's Duty Cycle register */
 #define STI_PWMCR	0x50		/* Control/Config register */
 #define STI_INTEN	0x54		/* Interrupt Enable/Disable register */
 #define PWM_PRESCALE_LOW_MASK		0x0f
@@ -40,7 +40,7 @@ enum {
 
 struct sti_pwm_compat_data {
 	const struct reg_field *reg_fields;
-	unsigned int num_chan;
+	unsigned int num_devs;
 	unsigned int max_pwm_cnt;
 	unsigned int max_prescale;
 };
@@ -130,13 +130,13 @@ static int sti_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	/* Allow configuration changes if one of the
 	 * following conditions satisfy.
-	 * 1. No channels have been configured.
-	 * 2. Only one channel has been configured and the new request
-	 *    is for the same channel.
-	 * 3. Only one channel has been configured and the new request is
-	 *    for a new channel and period of the new channel is same as
+	 * 1. No devices have been configured.
+	 * 2. Only one device has been configured and the new request
+	 *    is for the same device.
+	 * 3. Only one device has been configured and the new request is
+	 *    for a new device and period of the new device is same as
 	 *    the current configured period.
-	 * 4. More than one channels are configured and period of the new
+	 * 4. More than one devices are configured and period of the new
 	 *    requestis the same as the current period.
 	 */
 	if (!ncfg ||
@@ -201,7 +201,7 @@ static int sti_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	int ret = 0;
 
 	/*
-	 * Since we have a common enable for all PWM channels,
+	 * Since we have a common enable for all PWM devices,
 	 * do not enable if already enabled.
 	 */
 	mutex_lock(&pc->sti_pwm_lock);
@@ -259,11 +259,11 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
 	const struct reg_field *reg_fields;
 	struct device_node *np = dev->of_node;
 	struct sti_pwm_compat_data *cdata = pc->cdata;
-	u32 num_chan;
+	u32 num_devs;
 
-	of_property_read_u32(np, "st,pwm-num-chan", &num_chan);
-	if (num_chan)
-		cdata->num_chan = num_chan;
+	of_property_read_u32(np, "st,pwm-num-devs", &num_devs);
+	if (num_devs)
+		cdata->num_devs = num_devs;
 
 	reg_fields = cdata->reg_fields;
 
@@ -330,7 +330,7 @@ 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->num_devs     = 1;
 
 	pc->cdata = cdata;
 	pc->dev = dev;
@@ -362,7 +362,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->num_devs;
 	pc->chip.can_sleep = true;
 
 	ret = pwmchip_add(&pc->chip);
@@ -381,7 +381,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->num_devs; i++)
 		pwm_disable(&pc->chip.pwms[i]);
 
 	clk_unprepare(pc->clk);
-- 
2.8.3

[PATCH v3 10/20] pwm: sysfs: Add PWM Capture support

[Lee Jones]

Allow a user to read PWM Capture results from /sysfs.

To start a capture and read the result, simply read the file:

  $ cat $PWMCHIP/capture

The output format is "<period>:<duty_cycle>".

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

diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index d985992..901e647 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -208,16 +208,33 @@ 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);
+	struct pwm_capture result;
+	int ret;
+
+	ret = pwm_capture(pwm, &result, jiffies_to_msecs(HZ));
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "%llu %llu\n", result.period, result.duty_cycle);
+}
+
 static DEVICE_ATTR_RW(period);
 static DEVICE_ATTR_RW(duty_cycle);
 static DEVICE_ATTR_RW(enable);
 static DEVICE_ATTR_RW(polarity);
+static DEVICE_ATTR_RO(capture);
 
 static 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);
-- 
2.8.3

Re: [PATCH v3 10/20] pwm: sysfs: Add PWM Capture support

[Thierry Reding]

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

On Wed, Jun 08, 2016 at 10:21:25AM +0100, Lee Jones wrote:
> Allow a user to read PWM Capture results from /sysfs.
> 
> To start a capture and read the result, simply read the file:
> 
>   $ cat $PWMCHIP/capture
> 
> The output format is "<period>:<duty_cycle>".

The format below is "<period> <duty cycle>".

No need to rev the series for this, I can take patches 8 and 10 as-is
and make those modifications myself.

Thierry

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

Re: [PATCH v3 10/20] pwm: sysfs: Add PWM Capture support

[Lee Jones]

On Fri, 10 Jun 2016, Thierry Reding wrote:

> On Wed, Jun 08, 2016 at 10:21:25AM +0100, Lee Jones wrote:
> > Allow a user to read PWM Capture results from /sysfs.
> > 
> > To start a capture and read the result, simply read the file:
> > 
> >   $ cat $PWMCHIP/capture
> > 
> > The output format is "<period>:<duty_cycle>".
> 
> The format below is "<period> <duty cycle>".
> 
> No need to rev the series for this, I can take patches 8 and 10 as-is
> and make those modifications myself.

Ideal, thanks.

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

[PATCH v3 11/20] 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 3dae127..5fbee61 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))	/* Device'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))) /* Device'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_devs; i++)
 		pwm_disable(&pc->chip.pwms[i]);
 
-	clk_unprepare(pc->clk);
+	clk_unprepare(pc->pwm_clk);
 
 	return pwmchip_remove(&pc->chip);
 }
-- 
2.8.3

[PATCH v3 12/20] 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 5fbee61..c4a34af 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");
-- 
2.8.3

[PATCH v3 13/20] 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 c4a34af..2f61e1e 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))) /* Device'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_DEVS		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_devs;
@@ -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)
-- 
2.8.3

[PATCH v3 14/20] 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 2f61e1e..78979d0 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);
 }
-- 
2.8.3

[PATCH v3 15/20] pwm: sti: Initialise PWM Capture device data

[Lee Jones]

Each PWM Capture device is allocated a structure to hold its own
state.  During a capture the device 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 device 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 devices.

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

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 78979d0..9d597bb 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -11,6 +11,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/interrupt.h>
 #include <linux/math64.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
@@ -18,8 +19,10 @@
 #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))) /* Device's Duty Cycle register */
 #define PWM_CPT_VAL(x)	(0x10 + (4 * (x))) /* Capture value */
@@ -64,9 +67,17 @@ enum sti_cpt_edge {
 	CPT_EDGE_BOTH,
 };
 
+struct sti_cpt_ddata {
+	u32 snapshot[3];
+	int index;
+	struct mutex lock;
+	wait_queue_head_t wait;
+};
+
 struct sti_pwm_compat_data {
 	const struct reg_field *reg_fields;
-	unsigned int num_devs;
+	unsigned int pwm_num_devs;
+	unsigned int cpt_num_devs;
 	unsigned int max_pwm_cnt;
 	unsigned int max_prescale;
 };
@@ -307,10 +318,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_devs;
+	int ret;
 
-	of_property_read_u32(np, "st,pwm-num-devs", &num_devs);
-	if (num_devs)
-		cdata->num_devs = num_devs;
+	ret = of_property_read_u32(np, "st,pwm-num-devs", &num_devs);
+	if (!ret)
+		cdata->pwm_num_devs = num_devs;
+
+	ret = of_property_read_u32(np, "st,capture-num-devs", &num_devs);
+	if (!ret)
+		cdata->cpt_num_devs = num_devs;
 
 	reg_fields = cdata->reg_fields;
 
@@ -350,6 +366,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	struct sti_pwm_compat_data *cdata;
 	struct sti_pwm_chip *pc;
 	struct resource *res;
+	unsigned int devnum;
 	int ret;
 
 	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
@@ -378,7 +395,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_devs     = 1;
+	cdata->pwm_num_devs = 1;
+	cdata->cpt_num_devs = 0;
 
 	pc->cdata = cdata;
 	pc->dev = dev;
@@ -416,7 +434,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_devs;
+	pc->chip.npwm = pc->cdata->pwm_num_devs;
 	pc->chip.can_sleep = true;
 
 	ret = pwmchip_add(&pc->chip);
@@ -426,6 +444,19 @@ static int sti_pwm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	for (devnum = 0; devnum < cdata->cpt_num_devs; devnum++) {
+		struct sti_cpt_ddata *ddata;
+
+		ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
+		if (!ddata)
+			return -ENOMEM;
+
+		init_waitqueue_head(&ddata->wait);
+		mutex_init(&ddata->lock);
+
+		pwm_set_chip_data(&pc->chip.pwms[devnum], ddata);
+	}
+
 	platform_set_drvdata(pdev, pc);
 
 	return 0;
@@ -436,7 +467,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_devs; i++)
+	for (i = 0; i < pc->cdata->pwm_num_devs; i++)
 		pwm_disable(&pc->chip.pwms[i]);
 
 	clk_unprepare(pc->pwm_clk);
-- 
2.8.3

[PATCH v3 16/20] 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 | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 91 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 9d597bb..2230afb 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -91,7 +91,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;
@@ -311,6 +313,76 @@ 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_ddata *ddata;
+	int devicenum;
+	unsigned int cpt_int_stat;
+	unsigned 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) {
+		devicenum = ffs(cpt_int_stat) - 1;
+
+		ddata = pwm_get_chip_data(&pc->chip.pwms[devicenum]);
+
+		/*
+		 * 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(devicenum),
+			    &ddata->snapshot[ddata->index]);
+
+		switch (ddata->index) {
+		case 0:
+		case 1:
+			regmap_read(pc->regmap, PWM_CPT_EDGE(devicenum), &reg);
+			reg ^= PWM_CPT_EDGE_MASK;
+			regmap_write(pc->regmap, PWM_CPT_EDGE(devicenum), reg);
+
+			ddata->index++;
+			break;
+		case 2:
+			regmap_write(pc->regmap,
+				     PWM_CPT_EDGE(devicenum),
+				     CPT_EDGE_DISABLED);
+			wake_up(&ddata->wait);
+			break;
+		default:
+			dev_err(dev, "Internal error\n");
+		}
+
+		cpt_int_stat &= ~BIT_MASK(devicenum);
+
+		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;
@@ -351,6 +423,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 (PTR_ERR_OR_ZERO(pc->pwm_cpt_int_stat))
+		return PTR_ERR(pc->pwm_cpt_int_stat);
+
 	return 0;
 }
 
@@ -367,7 +444,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	struct sti_pwm_chip *pc;
 	struct resource *res;
 	unsigned int devnum;
-	int ret;
+	int irq, ret;
 
 	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
 	if (!pc)
@@ -388,6 +465,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 irq;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq, sti_pwm_interrupt,
+			       0, pdev->name, 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.
-- 
2.8.3

[PATCH v3 17/20] 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 | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 2230afb..cecb6d4 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -24,6 +24,8 @@
 #include <linux/time.h>
 #include <linux/wait.h>
 
+#define SECS_TO_NANOSECS(x) ((x) * 1000 * 1000 * 1000)
+
 #define PWM_OUT_VAL(x)	(0x00 + (4 * (x))) /* Device'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 */
@@ -305,7 +307,88 @@ 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,
+			   struct pwm_capture *result, unsigned int timeout_ms)
+{
+	struct sti_pwm_chip *pc = to_sti_pwmchip(chip);
+	struct sti_pwm_compat_data *cdata = pc->cdata;
+	struct sti_cpt_ddata *ddata = pwm_get_chip_data(pwm);
+	struct device *dev = pc->dev;
+	unsigned int effective_ticks;
+	unsigned long long high, low;
+	int ret;
+
+	if (pwm->hwpwm > cdata->cpt_num_devs - 1) {
+		dev_err(dev, "Device %d is not valid\n", pwm->hwpwm);
+		return -EINVAL;
+	}
+
+	mutex_lock(&ddata->lock);
+
+	/* Prepare capture measurement */
+	ddata->index = 0;
+	regmap_write(pc->regmap, PWM_CPT_EDGE(pwm->hwpwm), CPT_EDGE_RISING);
+	regmap_field_write(pc->pwm_cpt_int_en, BIT(pwm->hwpwm));
+
+	/* Enable capture */
+	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;
+	}
+
+	ret = wait_event_interruptible_timeout(ddata->wait,
+					       ddata->index > 1,
+					       msecs_to_jiffies(timeout_ms));
+
+	regmap_write(pc->regmap, PWM_CPT_EDGE(pwm->hwpwm), CPT_EDGE_DISABLED);
+
+	if (ret == -ERESTARTSYS)
+		goto out;
+
+	switch (ddata->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
+		 */
+
+		result->period = 0;
+		result->duty_cycle = 0;
+
+		break;
+	case 2:
+		/* We have everying we need */
+		high = ddata->snapshot[1] - ddata->snapshot[0];
+		low  = ddata->snapshot[2] - ddata->snapshot[1];
+
+		effective_ticks = clk_get_rate(pc->cpt_clk);
+
+		result->period = SECS_TO_NANOSECS(high + low);
+		do_div(result->period, effective_ticks);
+
+		result->duty_cycle = SECS_TO_NANOSECS(high);
+		do_div(result->duty_cycle, effective_ticks);
+
+		break;
+	default:
+		dev_err(dev, "Internal error\n");
+	}
+
+out:
+	/* Disable capture */
+	regmap_field_write(pc->pwm_cpt_en, 0);
+
+	mutex_unlock(&ddata->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,
@@ -418,6 +501,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))
-- 
2.8.3

[PATCH v3 18/20] pwm: sti: It's now valid for number of PWM channels to be zero

[Lee Jones]

Setting up the STI PWM IP as capture only, with zero PWM-out devices
is a perfectly valued configuration.  It is no longer okay to assume
that there must be at least 1 PWM-out devices.  In this patch we make
the default number of PWM-out devices zero and only configure channels
explicitly requested.

Reported-by: Peter Griffin <peter.griffin@linaro.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/pwm-sti.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index cecb6d4..64db5a5 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -483,6 +483,11 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
 	if (!ret)
 		cdata->cpt_num_devs = num_devs;
 
+	if (cdata->pwm_num_devs && !cdata->cpt_num_devs) {
+		dev_err(dev, "No channels configured\n");
+		return -EINVAL;
+	}
+
 	reg_fields = cdata->reg_fields;
 
 	pc->prescale_low = devm_regmap_field_alloc(dev, pc->regmap,
@@ -573,7 +578,7 @@ 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->pwm_num_devs = 1;
+	cdata->pwm_num_devs = 0;
 	cdata->cpt_num_devs = 0;
 
 	pc->cdata = cdata;
@@ -585,6 +590,9 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	if (!cdata->pwm_num_devs)
+		goto skip_pwm;
+
 	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");
@@ -597,6 +605,10 @@ static int sti_pwm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+skip_pwm:
+	if (!cdata->cpt_num_devs)
+		goto skip_cpt;
+
 	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");
@@ -609,6 +621,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+skip_cpt:
 	pc->chip.dev = dev;
 	pc->chip.ops = &sti_pwm_ops;
 	pc->chip.base = -1;
-- 
2.8.3

[PATCH v3 19/20] 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 | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 64db5a5..4987a19 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -1,8 +1,10 @@
 /*
- * PWM device driver for ST SoCs.
- * Author: Ajit Pal Singh <ajitpal.singh@st.com>
+ * PWM device driver for ST SoCs
+ *
+ * Copyright (C) 2013-2016 STMicroelectronics (R&D) Limited
  *
- * Copyright (C) 2013-2014 STMicroelectronics (R&D) Limited
+ * Author: Ajit Pal Singh <ajitpal.singh@st.com>
+ *         Lee Jones <lee.jones@linaro.org>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -142,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;
@@ -164,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;
@@ -210,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;
 
@@ -273,7 +275,7 @@ static int sti_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 
 		ret = regmap_field_write(pc->pwm_out_en, 1);
 		if (ret) {
-			dev_err(dev, "failed to enable PWM device:%d\n",
+			dev_err(dev, "failed to enable PWM device %d\n",
 				pwm->hwpwm);
 			goto out;
 		}
@@ -293,10 +295,12 @@ 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_out_en, 0);
 
 	clk_disable(pc->pwm_clk);
 	clk_disable(pc->cpt_clk);
+
 	mutex_unlock(&pc->sti_pwm_lock);
 }
 
@@ -512,7 +516,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);
 
@@ -575,11 +579,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_devs = 0;
-	cdata->cpt_num_devs = 0;
+	cdata->reg_fields	= &sti_pwm_regfields[0];
+	cdata->max_prescale	= 0xff;
+	cdata->max_pwm_cnt	= 255;
+	cdata->pwm_num_devs	= 0;
+	cdata->cpt_num_devs	= 0;
 
 	pc->cdata = cdata;
 	pc->dev = dev;
-- 
2.8.3

[PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with recent changes

[Lee Jones]

We're renaming the 'st,pwm-num-chan' binding to 'st,pwm-num-devs' to
be more inline with the naming conventions of the subsystem.  Where
we used to treat each line as a channel, the PWM convention is to
describe them as devices.

The second documentation adaption entails adding support for PWM
capture devices.  A new clock is required as well as an IRQ line.
We're also adding a new property similar to the one described
above, but for capture channels.  Typically, there will be less
capture channels than PWM-out, since all channels have the latter
capability, but only some have capture support.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 Documentation/devicetree/bindings/pwm/pwm-st.txt | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-st.txt b/Documentation/devicetree/bindings/pwm/pwm-st.txt
index 84d2fb8..b09f3a4 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-st.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-st.txt
@@ -13,13 +13,14 @@ Required parameters:
 - pinctrl-0: 		List of phandles pointing to pin configuration nodes
 			for PWM module.
 			For Pinctrl properties, please refer to [1].
-- clock-names: 		Set to "pwm".
+- clock-names: 		Valid entries are "pwm" and/or "capture".
 - clocks: 		phandle of the clock used by the PWM module.
 			For Clk properties, please refer to [2].
+- interrupts:		IRQ for the Capture device
 
 Optional properties:
-- st,pwm-num-chan:	Number of available channels. If not passed, the driver
-			will consider single channel by default.
+- st,pwm-num-devs:	Number of available PWM channels.  Default is 0.
+- st,capture-num-devs:	Number of available Capture channels.  Default is 0.
 
 [1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
 [2] Documentation/devicetree/bindings/clock/clock-bindings.txt
@@ -37,5 +38,6 @@ pwm1: pwm@fe510000 {
 		     &pinctrl_pwm1_chan3_default>;
 	clocks = <&clk_sysin>;
 	clock-names = "pwm";
-	st,pwm-num-chan = <4>;
+	st,pwm-num-devs = <4>;
+	st,capture-num-devs = <2>;
 };
-- 
2.8.3

Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with recent changes

[Rob Herring]

On Wed, Jun 08, 2016 at 10:21:35AM +0100, Lee Jones wrote:
> We're renaming the 'st,pwm-num-chan' binding to 'st,pwm-num-devs' to
> be more inline with the naming conventions of the subsystem.  Where
> we used to treat each line as a channel, the PWM convention is to
> describe them as devices.

That's all linux implementation details and you are breaking 
compatibility.

> The second documentation adaption entails adding support for PWM
> capture devices.  A new clock is required as well as an IRQ line.
> We're also adding a new property similar to the one described
> above, but for capture channels.  Typically, there will be less
> capture channels than PWM-out, since all channels have the latter
> capability, but only some have capture support.

Humm, sounds like all of this should be implied from compatible strings.

> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-st.txt | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-st.txt b/Documentation/devicetree/bindings/pwm/pwm-st.txt
> index 84d2fb8..b09f3a4 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm-st.txt
> +++ b/Documentation/devicetree/bindings/pwm/pwm-st.txt
> @@ -13,13 +13,14 @@ Required parameters:
>  - pinctrl-0: 		List of phandles pointing to pin configuration nodes
>  			for PWM module.
>  			For Pinctrl properties, please refer to [1].
> -- clock-names: 		Set to "pwm".
> +- clock-names: 		Valid entries are "pwm" and/or "capture".
>  - clocks: 		phandle of the clock used by the PWM module.
>  			For Clk properties, please refer to [2].
> +- interrupts:		IRQ for the Capture device
>  
>  Optional properties:
> -- st,pwm-num-chan:	Number of available channels. If not passed, the driver
> -			will consider single channel by default.
> +- st,pwm-num-devs:	Number of available PWM channels.  Default is 0.
> +- st,capture-num-devs:	Number of available Capture channels.  Default is 0.
>  
>  [1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>  [2] Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -37,5 +38,6 @@ pwm1: pwm@fe510000 {
>  		     &pinctrl_pwm1_chan3_default>;
>  	clocks = <&clk_sysin>;
>  	clock-names = "pwm";
> -	st,pwm-num-chan = <4>;
> +	st,pwm-num-devs = <4>;
> +	st,capture-num-devs = <2>;
>  };
> -- 
> 2.8.3
> 

Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with recent changes

[Lee Jones]

On Wed, 08 Jun 2016, Rob Herring wrote:

> On Wed, Jun 08, 2016 at 10:21:35AM +0100, Lee Jones wrote:
> > We're renaming the 'st,pwm-num-chan' binding to 'st,pwm-num-devs' to
> > be more inline with the naming conventions of the subsystem.  Where
> > we used to treat each line as a channel, the PWM convention is to
> > describe them as devices.
> 
> That's all linux implementation details and you are breaking 
> compatibility.

Normally I'd agree with you, but I happen to know that a) this IP is
currently unused and b) up until this point (and probably beyond), ST
always ship the DTB with the kernel, so there will be no breakage.

> > The second documentation adaption entails adding support for PWM
> > capture devices.  A new clock is required as well as an IRQ line.
> > We're also adding a new property similar to the one described
> > above, but for capture channels.  Typically, there will be less
> > capture channels than PWM-out, since all channels have the latter
> > capability, but only some have capture support.
> 
> Humm, sounds like all of this should be implied from compatible strings.

You mean have a bunch of of_machine_is_compatibles() scattered around?

> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/pwm/pwm-st.txt | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-st.txt b/Documentation/devicetree/bindings/pwm/pwm-st.txt
> > index 84d2fb8..b09f3a4 100644
> > --- a/Documentation/devicetree/bindings/pwm/pwm-st.txt
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-st.txt
> > @@ -13,13 +13,14 @@ Required parameters:
> >  - pinctrl-0: 		List of phandles pointing to pin configuration nodes
> >  			for PWM module.
> >  			For Pinctrl properties, please refer to [1].
> > -- clock-names: 		Set to "pwm".
> > +- clock-names: 		Valid entries are "pwm" and/or "capture".
> >  - clocks: 		phandle of the clock used by the PWM module.
> >  			For Clk properties, please refer to [2].
> > +- interrupts:		IRQ for the Capture device
> >  
> >  Optional properties:
> > -- st,pwm-num-chan:	Number of available channels. If not passed, the driver
> > -			will consider single channel by default.
> > +- st,pwm-num-devs:	Number of available PWM channels.  Default is 0.
> > +- st,capture-num-devs:	Number of available Capture channels.  Default is 0.
> >  
> >  [1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> >  [2] Documentation/devicetree/bindings/clock/clock-bindings.txt
> > @@ -37,5 +38,6 @@ pwm1: pwm@fe510000 {
> >  		     &pinctrl_pwm1_chan3_default>;
> >  	clocks = <&clk_sysin>;
> >  	clock-names = "pwm";
> > -	st,pwm-num-chan = <4>;
> > +	st,pwm-num-devs = <4>;
> > +	st,capture-num-devs = <2>;
> >  };

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

Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with recent changes

[Thierry Reding]

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

On Thu, Jun 09, 2016 at 12:41:07PM +0100, Lee Jones wrote:
> On Wed, 08 Jun 2016, Rob Herring wrote:
> 
> > On Wed, Jun 08, 2016 at 10:21:35AM +0100, Lee Jones wrote:
> > > We're renaming the 'st,pwm-num-chan' binding to 'st,pwm-num-devs' to
> > > be more inline with the naming conventions of the subsystem.  Where
> > > we used to treat each line as a channel, the PWM convention is to
> > > describe them as devices.
> > 
> > That's all linux implementation details and you are breaking 
> > compatibility.
> 
> Normally I'd agree with you, but I happen to know that a) this IP is
> currently unused and b) up until this point (and probably beyond), ST
> always ship the DTB with the kernel, so there will be no breakage.

Heh... I've long given up on trying to make that argument go anywhere.
The general rule is that once we support a binding in a kernel release
we have to support it indefinitely. If you really want to go ahead with
this change (I don't think you should), you'd at least have to document
both properties and support st,pwm-num-chan in the driver for backwards
compatibility.

> > > The second documentation adaption entails adding support for PWM
> > > capture devices.  A new clock is required as well as an IRQ line.
> > > We're also adding a new property similar to the one described
> > > above, but for capture channels.  Typically, there will be less
> > > capture channels than PWM-out, since all channels have the latter
> > > capability, but only some have capture support.
> > 
> > Humm, sounds like all of this should be implied from compatible strings.
> 
> You mean have a bunch of of_machine_is_compatibles() scattered around?

I don't understand why you need this at all. Quite frankly I don't even
know why st,pwm-num-devs exists. I probably missed it back at the time.
Usually, like Rob suggests, this should be inferred from the compatible
string. One commonly used way to avoid scattering explicit checks for
the compatible string is to add this information to the of_device_id
table. See a bunch of existing drivers for reference.

Also, why make a separation of output vs. capture channels at this
point? Could you not simply obtain the total number of PWM channels,
preferably from SoC data associated with the compatible string, and
check at ->capture() time whether or not the particular PWM supports
this?

As-is, you imply that you have n (output) + m (capture) channels, and
that 0..n-1 are output and n..n+m-1 are capture channels. What if that
no longer holds true, but 0 and 2 are the only ones that support
capture?

If you check for this at runtime you can avoid complicated DT parsing
code, but still get the safety check which should be enough to encourage
people to use the right channels in DT.

Thierry

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

Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with recent changes

[Lee Jones]

On Fri, 10 Jun 2016, Thierry Reding wrote:

> On Thu, Jun 09, 2016 at 12:41:07PM +0100, Lee Jones wrote:
> > On Wed, 08 Jun 2016, Rob Herring wrote:
> > 
> > > On Wed, Jun 08, 2016 at 10:21:35AM +0100, Lee Jones wrote:
> > > > We're renaming the 'st,pwm-num-chan' binding to 'st,pwm-num-devs' to
> > > > be more inline with the naming conventions of the subsystem.  Where
> > > > we used to treat each line as a channel, the PWM convention is to
> > > > describe them as devices.
> > > 
> > > That's all linux implementation details and you are breaking 
> > > compatibility.
> > 
> > Normally I'd agree with you, but I happen to know that a) this IP is
> > currently unused and b) up until this point (and probably beyond), ST
> > always ship the DTB with the kernel, so there will be no breakage.
> 
> Heh... I've long given up on trying to make that argument go anywhere.
> The general rule is that once we support a binding in a kernel release
> we have to support it indefinitely. If you really want to go ahead with
> this change (I don't think you should), you'd at least have to document
> both properties and support st,pwm-num-chan in the driver for backwards
> compatibility.

I understand what the *general* rule is, but we have to remember why
this rule was put into place and apply some common sense.  In some
Enterprise user-cases where DTBs are written into ROM or where they
are difficult /impossible to update, I can completely understand the
requirement to support previous incarnations.  However in this, the
real world, DTBs are shipped with their corresponding kernels.  We
would lack a great deal of functionality if they weren't.  It is
therefor, foolhardy and inappropriate to stick to this rule just
'cos.

> > > > The second documentation adaption entails adding support for PWM
> > > > capture devices.  A new clock is required as well as an IRQ line.
> > > > We're also adding a new property similar to the one described
> > > > above, but for capture channels.  Typically, there will be less
> > > > capture channels than PWM-out, since all channels have the latter
> > > > capability, but only some have capture support.
> > > 
> > > Humm, sounds like all of this should be implied from compatible strings.
> > 
> > You mean have a bunch of of_machine_is_compatibles() scattered around?
> 
> I don't understand why you need this at all. Quite frankly I don't even
> know why st,pwm-num-devs exists. I probably missed it back at the time.
> Usually, like Rob suggests, this should be inferred from the compatible
> string. One commonly used way to avoid scattering explicit checks for
> the compatible string is to add this information to the of_device_id
> table. See a bunch of existing drivers for reference.

Yes, I am aware of the strategy, and happy to oblige if this is your
suggestion.  I'll move all platform data into the driver and eradicate
the DT properties.

> Also, why make a separation of output vs. capture channels at this
> point? Could you not simply obtain the total number of PWM channels,
> preferably from SoC data associated with the compatible string, and
> check at ->capture() time whether or not the particular PWM supports
> this?
> 
> As-is, you imply that you have n (output) + m (capture) channels, and
> that 0..n-1 are output and n..n+m-1 are capture channels. What if that
> no longer holds true, but 0 and 2 are the only ones that support
> capture?

We do?  What makes you think that?

> If you check for this at runtime you can avoid complicated DT parsing
> code, but still get the safety check which should be enough to encourage
> people to use the right channels in DT.

I'm pretty sure I can move all the data into the driver.  I did want
to avoid having lots of different compatible strings, but if that's
what you're suggesting, I can introduce one per supported platform.

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

Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with recent changes

[Thierry Reding]

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

On Fri, Jun 10, 2016 at 03:06:35PM +0100, Lee Jones wrote:
> On Fri, 10 Jun 2016, Thierry Reding wrote:
> 
> > On Thu, Jun 09, 2016 at 12:41:07PM +0100, Lee Jones wrote:
> > > On Wed, 08 Jun 2016, Rob Herring wrote:
> > > 
> > > > On Wed, Jun 08, 2016 at 10:21:35AM +0100, Lee Jones wrote:
> > > > > We're renaming the 'st,pwm-num-chan' binding to 'st,pwm-num-devs' to
> > > > > be more inline with the naming conventions of the subsystem.  Where
> > > > > we used to treat each line as a channel, the PWM convention is to
> > > > > describe them as devices.
> > > > 
> > > > That's all linux implementation details and you are breaking 
> > > > compatibility.
> > > 
> > > Normally I'd agree with you, but I happen to know that a) this IP is
> > > currently unused and b) up until this point (and probably beyond), ST
> > > always ship the DTB with the kernel, so there will be no breakage.
> > 
> > Heh... I've long given up on trying to make that argument go anywhere.
> > The general rule is that once we support a binding in a kernel release
> > we have to support it indefinitely. If you really want to go ahead with
> > this change (I don't think you should), you'd at least have to document
> > both properties and support st,pwm-num-chan in the driver for backwards
> > compatibility.
> 
> I understand what the *general* rule is, but we have to remember why
> this rule was put into place and apply some common sense.  In some
> Enterprise user-cases where DTBs are written into ROM or where they
> are difficult /impossible to update, I can completely understand the
> requirement to support previous incarnations.  However in this, the
> real world, DTBs are shipped with their corresponding kernels.  We
> would lack a great deal of functionality if they weren't.  It is
> therefor, foolhardy and inappropriate to stick to this rule just
> 'cos.

I used to advocate the very same point of view a couple of years ago but
was repeatedly told that it's irrelevant, and everybody was to be held
to the same standards, irrespective of how easy it is to update the DTB
in lockstep with the kernel.

Part of me still wishes that the rules were a little less strict, but a
decision was made on this years ago, so I'm just repeating this here in
an attempt to save you from wasting your time arguing.

Then again, there have been occasions where decisions were undone, so
you might have better luck nowadays if you're willing to take this to
the DT bindings and ARM-SoC maintainers.

Bottom line: NAK on the backwards-incompatible DT binding change from me
based on earlier decisions made on this topic. But I may be swayed if
everyone else thinks ABI stability is no longer something we consider
important for DT.

> > > > > The second documentation adaption entails adding support for PWM
> > > > > capture devices.  A new clock is required as well as an IRQ line.
> > > > > We're also adding a new property similar to the one described
> > > > > above, but for capture channels.  Typically, there will be less
> > > > > capture channels than PWM-out, since all channels have the latter
> > > > > capability, but only some have capture support.
> > > > 
> > > > Humm, sounds like all of this should be implied from compatible strings.
> > > 
> > > You mean have a bunch of of_machine_is_compatibles() scattered around?
> > 
> > I don't understand why you need this at all. Quite frankly I don't even
> > know why st,pwm-num-devs exists. I probably missed it back at the time.
> > Usually, like Rob suggests, this should be inferred from the compatible
> > string. One commonly used way to avoid scattering explicit checks for
> > the compatible string is to add this information to the of_device_id
> > table. See a bunch of existing drivers for reference.
> 
> Yes, I am aware of the strategy, and happy to oblige if this is your
> suggestion.  I'll move all platform data into the driver and eradicate
> the DT properties.

Great!

> > Also, why make a separation of output vs. capture channels at this
> > point? Could you not simply obtain the total number of PWM channels,
> > preferably from SoC data associated with the compatible string, and
> > check at ->capture() time whether or not the particular PWM supports
> > this?
> > 
> > As-is, you imply that you have n (output) + m (capture) channels, and
> > that 0..n-1 are output and n..n+m-1 are capture channels. What if that
> > no longer holds true, but 0 and 2 are the only ones that support
> > capture?
> 
> We do?  What makes you think that?

Because there's nothing saying otherwise? The DT binding is completely
silent on the matter, and the above is the most logical interpretation
that I came up with.

Looking at the driver it seems like it's actually the other way around
and you have the first m channels that support both output and capture
functionality. But the issue remains that this isn't documented in the
DT binding documentation. And the current properties are also not very
flexible because they allow for only a single scheme of assigning the
capability.

If you move all of that knowledge into the driver, things become a lot
easier, in my opinion.

> > If you check for this at runtime you can avoid complicated DT parsing
> > code, but still get the safety check which should be enough to encourage
> > people to use the right channels in DT.
> 
> I'm pretty sure I can move all the data into the driver.  I did want
> to avoid having lots of different compatible strings, but if that's
> what you're suggesting, I can introduce one per supported platform.

That sounds like the simplest solution to me.

Thierry

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

Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with recent changes

[Lee Jones]

On Fri, 10 Jun 2016, Thierry Reding wrote:
> On Fri, Jun 10, 2016 at 03:06:35PM +0100, Lee Jones wrote:
> > On Fri, 10 Jun 2016, Thierry Reding wrote:
> > 
> > > On Thu, Jun 09, 2016 at 12:41:07PM +0100, Lee Jones wrote:
> > > > On Wed, 08 Jun 2016, Rob Herring wrote:
> > > > 
> > > > > On Wed, Jun 08, 2016 at 10:21:35AM +0100, Lee Jones wrote:
> > > > > > We're renaming the 'st,pwm-num-chan' binding to 'st,pwm-num-devs' to
> > > > > > be more inline with the naming conventions of the subsystem.  Where
> > > > > > we used to treat each line as a channel, the PWM convention is to
> > > > > > describe them as devices.
> > > > > 
> > > > > That's all linux implementation details and you are breaking 
> > > > > compatibility.
> > > > 
> > > > Normally I'd agree with you, but I happen to know that a) this IP is
> > > > currently unused and b) up until this point (and probably beyond), ST
> > > > always ship the DTB with the kernel, so there will be no breakage.
> > > 
> > > Heh... I've long given up on trying to make that argument go anywhere.
> > > The general rule is that once we support a binding in a kernel release
> > > we have to support it indefinitely. If you really want to go ahead with
> > > this change (I don't think you should), you'd at least have to document
> > > both properties and support st,pwm-num-chan in the driver for backwards
> > > compatibility.
> > 
> > I understand what the *general* rule is, but we have to remember why
> > this rule was put into place and apply some common sense.  In some
> > Enterprise user-cases where DTBs are written into ROM or where they
> > are difficult /impossible to update, I can completely understand the
> > requirement to support previous incarnations.  However in this, the
> > real world, DTBs are shipped with their corresponding kernels.  We
> > would lack a great deal of functionality if they weren't.  It is
> > therefor, foolhardy and inappropriate to stick to this rule just
> > 'cos.
> 
> I used to advocate the very same point of view a couple of years ago but
> was repeatedly told that it's irrelevant, and everybody was to be held
> to the same standards, irrespective of how easy it is to update the DTB
> in lockstep with the kernel.
> 
> Part of me still wishes that the rules were a little less strict, but a
> decision was made on this years ago, so I'm just repeating this here in
> an attempt to save you from wasting your time arguing.
> 
> Then again, there have been occasions where decisions were undone, so
> you might have better luck nowadays if you're willing to take this to
> the DT bindings and ARM-SoC maintainers.
> 
> Bottom line: NAK on the backwards-incompatible DT binding change from me
> based on earlier decisions made on this topic. But I may be swayed if
> everyone else thinks ABI stability is no longer something we consider
> important for DT.

I've seen properties come and go.  Both using the deprecation process
and otherwise.  By NAKing these types of changes, you're just
exacerbating the situation.  If you know it's the right
(non-foolhardy) thing to do, just do it.

Although, I feel the point is moot for this particular driver now,
what with all of the pdata being moved into the driver.

> > > > > > The second documentation adaption entails adding support for PWM
> > > > > > capture devices.  A new clock is required as well as an IRQ line.
> > > > > > We're also adding a new property similar to the one described
> > > > > > above, but for capture channels.  Typically, there will be less
> > > > > > capture channels than PWM-out, since all channels have the latter
> > > > > > capability, but only some have capture support.
> > > > > 
> > > > > Humm, sounds like all of this should be implied from compatible strings.
> > > > 
> > > > You mean have a bunch of of_machine_is_compatibles() scattered around?
> > > 
> > > I don't understand why you need this at all. Quite frankly I don't even
> > > know why st,pwm-num-devs exists. I probably missed it back at the time.
> > > Usually, like Rob suggests, this should be inferred from the compatible
> > > string. One commonly used way to avoid scattering explicit checks for
> > > the compatible string is to add this information to the of_device_id
> > > table. See a bunch of existing drivers for reference.
> > 
> > Yes, I am aware of the strategy, and happy to oblige if this is your
> > suggestion.  I'll move all platform data into the driver and eradicate
> > the DT properties.
> 
> Great!
> 
> > > Also, why make a separation of output vs. capture channels at this
> > > point? Could you not simply obtain the total number of PWM channels,
> > > preferably from SoC data associated with the compatible string, and
> > > check at ->capture() time whether or not the particular PWM supports
> > > this?
> > > 
> > > As-is, you imply that you have n (output) + m (capture) channels, and
> > > that 0..n-1 are output and n..n+m-1 are capture channels. What if that
> > > no longer holds true, but 0 and 2 are the only ones that support
> > > capture?
> > 
> > We do?  What makes you think that?
> 
> Because there's nothing saying otherwise? The DT binding is completely
> silent on the matter, and the above is the most logical interpretation
> that I came up with.

But you made it up. :)

It's not documented because it's irrelevant how channels are numbered.

Actually, looking at the docs, the IP doesn't change from one channel
to another, so in theory they all support capture.  The only issue is
whether they are plumbed in or not.  I'll take a  closer look at all
the platforms we support when I move the pdata.

> Looking at the driver it seems like it's actually the other way around
> and you have the first m channels that support both output and capture
> functionality. But the issue remains that this isn't documented in the
> DT binding documentation. And the current properties are also not very
> flexible because they allow for only a single scheme of assigning the
> capability.
> 
> If you move all of that knowledge into the driver, things become a lot
> easier, in my opinion.

Sure.

> > > If you check for this at runtime you can avoid complicated DT parsing
> > > code, but still get the safety check which should be enough to encourage
> > > people to use the right channels in DT.
> > 
> > I'm pretty sure I can move all the data into the driver.  I did want
> > to avoid having lots of different compatible strings, but if that's
> > what you're suggesting, I can introduce one per supported platform.
> 
> That sounds like the simplest solution to me.

Okay.

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

Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with recent changes

[Lee Jones]

On Fri, 10 Jun 2016, Thierry Reding wrote:
> On Fri, Jun 10, 2016 at 03:06:35PM +0100, Lee Jones wrote:
> > On Fri, 10 Jun 2016, Thierry Reding wrote:
> > 
> > > > > > The second documentation adaption entails adding support for PWM
> > > > > > capture devices.  A new clock is required as well as an IRQ line.
> > > > > > We're also adding a new property similar to the one described
> > > > > > above, but for capture channels.  Typically, there will be less
> > > > > > capture channels than PWM-out, since all channels have the latter
> > > > > > capability, but only some have capture support.
> > > > > 
> > > > > Humm, sounds like all of this should be implied from compatible strings.
> > > > 
> > > > You mean have a bunch of of_machine_is_compatibles() scattered around?
> > > 
> > > I don't understand why you need this at all. Quite frankly I don't even
> > > know why st,pwm-num-devs exists. I probably missed it back at the time.
> > > Usually, like Rob suggests, this should be inferred from the compatible
> > > string. One commonly used way to avoid scattering explicit checks for
> > > the compatible string is to add this information to the of_device_id
> > > table. See a bunch of existing drivers for reference.
> > 
> > Yes, I am aware of the strategy, and happy to oblige if this is your
> > suggestion.  I'll move all platform data into the driver and eradicate
> > the DT properties.
> 
> Great!

Sorry it's taken so long to get back around to this.

So I just had a crack at implementing this solution and ran into
issues.  Unfortunately, the configuration isn't easy to describe using
compatible strings.

Unless we are prepared to have strings like:

  st,sti-pwm-stih407-pwm0
  st,sti-pwm-stih407-pwm1
  st,sti-pwm-stih416-pwm0
  st,sti-pwm-stih416-pwm1

Or perhaps:

  st,sti-pwm-4out-2cpt
  st,sti-pwm-1out-1cpt

... since some of the PWMs have an odd number of OUT and CPT
channels/devices.  In my opinion however, this is ugly.  I don't think
we do this for any other type of channel/device/port etc, do we?

Personally I think there isn't anything wrong with having DT
properties describing how many channels/devices there are, but I'm
okay with whatever you both decide.  No skin of my nose really. :)

What say you?

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

Re: [PATCH v3 20/20] dt-bindings: pwm: sti: Update DT bindings with recent changes

[Thierry Reding]

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

On Wed, Jun 08, 2016 at 03:51:52PM -0500, Rob Herring wrote:
> On Wed, Jun 08, 2016 at 10:21:35AM +0100, Lee Jones wrote:
> > We're renaming the 'st,pwm-num-chan' binding to 'st,pwm-num-devs' to
> > be more inline with the naming conventions of the subsystem.  Where
> > we used to treat each line as a channel, the PWM convention is to
> > describe them as devices.
> 
> That's all linux implementation details and you are breaking 
> compatibility.

While it's true that the API calls channels struct pwm_device, that's
primarily for historical reasons. It's completely fine, and in fact most
drivers already do exactly that, to internally refer to channels as
channels.

Thierry

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