[PATCH 1/3] pwm: imx: Sort include files

[PATCH 1/3] pwm: imx: Sort include files

[Michal Vokáč]

Sort included header files alphabetically.

Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
 drivers/pwm/pwm-imx.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 1d5242c..bcbcac4 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -5,17 +5,17 @@
  * Derived from pxa PWM driver by eric miao <eric.miao@marvell.com>
  */
 
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/platform_device.h>
-#include <linux/slab.h>
-#include <linux/err.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/err.h>
 #include <linux/io.h>
-#include <linux/pwm.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
 
 /* i.MX1 and i.MX21 share the same PWM function block: */
 
-- 
2.1.4

[PATCH 2/3] pwm: imx: Use bitops and bitfield macros to define register values

[Michal Vokáč]

Use existing macros to define register fields instead of manually shifting
the bit masks. Also define some more register bits.

Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
 drivers/pwm/pwm-imx.c | 78 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 58 insertions(+), 20 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index bcbcac4..7a4907b 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -5,6 +5,8 @@
  * Derived from pxa PWM driver by eric miao <eric.miao@marvell.com>
  */
 
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/err.h>
@@ -23,7 +25,7 @@
 #define MX1_PWMS			0x04   /* PWM Sample Register */
 #define MX1_PWMP			0x08   /* PWM Period Register */
 
-#define MX1_PWMC_EN			(1 << 4)
+#define MX1_PWMC_EN			BIT(4)
 
 /* i.MX27, i.MX31, i.MX35 share the same PWM function block: */
 
@@ -31,18 +33,53 @@
 #define MX3_PWMSR			0x04    /* PWM Status Register */
 #define MX3_PWMSAR			0x0C    /* PWM Sample Register */
 #define MX3_PWMPR			0x10    /* PWM Period Register */
-#define MX3_PWMCR_PRESCALER(x)		((((x) - 1) & 0xFFF) << 4)
-#define MX3_PWMCR_STOPEN		(1 << 25)
-#define MX3_PWMCR_DOZEEN		(1 << 24)
-#define MX3_PWMCR_WAITEN		(1 << 23)
-#define MX3_PWMCR_DBGEN			(1 << 22)
-#define MX3_PWMCR_POUTC			(1 << 18)
-#define MX3_PWMCR_CLKSRC_IPG_HIGH	(2 << 16)
-#define MX3_PWMCR_CLKSRC_IPG		(1 << 16)
-#define MX3_PWMCR_SWR			(1 << 3)
-#define MX3_PWMCR_EN			(1 << 0)
-#define MX3_PWMSR_FIFOAV_4WORDS		0x4
-#define MX3_PWMSR_FIFOAV_MASK		0x7
+
+#define MX3_PWMCR_FWM			GENMASK(27, 26)
+#define MX3_PWMCR_STOPEN		BIT(25)
+#define MX3_PWMCR_DOZEN			BIT(24)
+#define MX3_PWMCR_WAITEN		BIT(23)
+#define MX3_PWMCR_DBGEN			BIT(22)
+#define MX3_PWMCR_BCTR			BIT(21)
+#define MX3_PWMCR_HCTR			BIT(20)
+
+#define MX3_PWMCR_POUTC			GENMASK(19, 18)
+#define MX3_PWMCR_POUTC_NORMAL		0
+#define MX3_PWMCR_POUTC_INVERTED	1
+#define MX3_PWMCR_POUTC_OFF		2
+
+#define MX3_PWMCR_CLKSRC		GENMASK(17, 16)
+#define MX3_PWMCR_CLKSRC_OFF		0
+#define MX3_PWMCR_CLKSRC_IPG		1
+#define MX3_PWMCR_CLKSRC_IPG_HIGH	2
+#define MX3_PWMCR_CLKSRC_IPG_32K	3
+
+#define MX3_PWMCR_PRESCALER		GENMASK(15, 4)
+
+#define MX3_PWMCR_SWR			BIT(3)
+
+#define MX3_PWMCR_REPEAT		GENMASK(2, 1)
+#define MX3_PWMCR_REPEAT_1X		0
+#define MX3_PWMCR_REPEAT_2X		1
+#define MX3_PWMCR_REPEAT_4X		2
+#define MX3_PWMCR_REPEAT_8X		3
+
+#define MX3_PWMCR_EN			BIT(0)
+
+#define MX3_PWMSR_FWE			BIT(6)
+#define MX3_PWMSR_CMP			BIT(5)
+#define MX3_PWMSR_ROV			BIT(4)
+#define MX3_PWMSR_FE			BIT(3)
+
+#define MX3_PWMSR_FIFOAV		GENMASK(2, 0)
+#define MX3_PWMSR_FIFOAV_EMPTY		0
+#define MX3_PWMSR_FIFOAV_1WORD		1
+#define MX3_PWMSR_FIFOAV_2WORDS		2
+#define MX3_PWMSR_FIFOAV_3WORDS		3
+#define MX3_PWMSR_FIFOAV_4WORDS		4
+
+#define MX3_PWMCR_PRESCALER_SET(x)	FIELD_PREP(MX3_PWMCR_PRESCALER, (x) - 1)
+#define MX3_PWMCR_PRESCALER_GET(x)	(FIELD_GET(MX3_PWMCR_PRESCALER, \
+						   (x)) + 1)
 
 #define MX3_PWM_SWR_LOOP		5
 
@@ -142,14 +179,14 @@ static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip,
 	u32 sr;
 
 	sr = readl(imx->mmio_base + MX3_PWMSR);
-	fifoav = sr & MX3_PWMSR_FIFOAV_MASK;
+	fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
 	if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
 		period_ms = DIV_ROUND_UP(pwm_get_period(pwm),
 					 NSEC_PER_MSEC);
 		msleep(period_ms);
 
 		sr = readl(imx->mmio_base + MX3_PWMSR);
-		if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
+		if (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr))
 			dev_warn(dev, "there is no free FIFO slot\n");
 	}
 }
@@ -207,13 +244,14 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
 		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
 		writel(period_cycles, imx->mmio_base + MX3_PWMPR);
 
-		cr = MX3_PWMCR_PRESCALER(prescale) |
-		     MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
-		     MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH |
-		     MX3_PWMCR_EN;
+		cr = MX3_PWMCR_PRESCALER_SET(prescale) |
+		     MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
+		     FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) |
+		     MX3_PWMCR_DBGEN | MX3_PWMCR_EN;
 
 		if (state->polarity == PWM_POLARITY_INVERSED)
-			cr |= MX3_PWMCR_POUTC;
+			cr |= FIELD_PREP(MX3_PWMCR_POUTC,
+					MX3_PWMCR_POUTC_INVERTED);
 
 		writel(cr, imx->mmio_base + MX3_PWMCR);
 	} else if (cstate.enabled) {
-- 
2.1.4

[PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout

[Michal Vokáč]

Implement the get_state() function and set the initial state to reflect
real state of the hardware. This allows to keep the PWM running if it was
enabled in bootloader. It is very similar to the GPIO behavior. GPIO pin
set as output in bootloader keep the same setting in Linux unless it is
reconfigured.

If we find the PWM block enabled we need to prepare and enable its source
clock otherwise the clock will be disabled late in the boot as unused.
That will leave the PWM in enabled state but with disabled clock. That has
a side effect that the PWM output is left at its current level at which
the clock was disabled. It is totally non-deterministic and it may be LOW
or HIGH.

Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
 drivers/pwm/pwm-imx.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 7a4907b..6cd3b72 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -83,6 +83,9 @@
 
 #define MX3_PWM_SWR_LOOP		5
 
+/* PWMPR register value of 0xffff has the same effect as 0xfffe */
+#define MX3_PWMPR_MAX			0xfffe
+
 struct imx_chip {
 	struct clk	*clk_per;
 
@@ -93,6 +96,55 @@ struct imx_chip {
 
 #define to_imx_chip(chip)	container_of(chip, struct imx_chip, chip)
 
+static void imx_pwm_get_state(struct pwm_chip *chip,
+		struct pwm_device *pwm, struct pwm_state *state)
+{
+	struct imx_chip *imx = to_imx_chip(chip);
+	u32 period, prescaler, pwm_clk, ret, val;
+	u64 tmp;
+
+	val = readl(imx->mmio_base + MX3_PWMCR);
+
+	if (val & MX3_PWMCR_EN) {
+		state->enabled = true;
+		ret = clk_prepare_enable(imx->clk_per);
+		if (ret)
+			return;
+	} else {
+		state->enabled = false;
+	}
+
+	switch (FIELD_GET(MX3_PWMCR_POUTC, val)) {
+	case MX3_PWMCR_POUTC_NORMAL:
+		state->polarity = PWM_POLARITY_NORMAL;
+		break;
+	case MX3_PWMCR_POUTC_INVERTED:
+		state->polarity = PWM_POLARITY_INVERSED;
+		break;
+	default:
+		dev_warn(chip->dev, "can't set polarity, output disconnected");
+	}
+
+	prescaler = MX3_PWMCR_PRESCALER_GET(val);
+	pwm_clk = clk_get_rate(imx->clk_per);
+	pwm_clk = DIV_ROUND_CLOSEST_ULL(pwm_clk, prescaler);
+	val = readl(imx->mmio_base + MX3_PWMPR);
+	period = val >= MX3_PWMPR_MAX ? MX3_PWMPR_MAX : val;
+
+	/* PWMOUT (Hz) = PWMCLK / (PWMPR + 2) */
+	tmp = NSEC_PER_SEC * (u64)(period + 2);
+	state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
+
+	/* PWMSAR can be read only if PWM is enabled */
+	if (state->enabled) {
+		val = readl(imx->mmio_base + MX3_PWMSAR);
+		tmp = NSEC_PER_SEC * (u64)(val);
+		state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
+	} else {
+		state->duty_cycle = 0;
+	}
+}
+
 static int imx_pwm_config_v1(struct pwm_chip *chip,
 		struct pwm_device *pwm, int duty_ns, int period_ns)
 {
@@ -272,6 +324,7 @@ static const struct pwm_ops imx_pwm_ops_v1 = {
 
 static const struct pwm_ops imx_pwm_ops_v2 = {
 	.apply = imx_pwm_apply_v2,
+	.get_state = imx_pwm_get_state,
 	.owner = THIS_MODULE,
 };
 
-- 
2.1.4

Re: [3/3] pwm: imx: Implement get_state() function for hardware readout

[Uwe Kleine-König]

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

Hello,

On Mon, Oct 01, 2018 at 04:19:48PM +0200, Michal Vokáč wrote:
> Implement the get_state() function and set the initial state to reflect
> real state of the hardware. This allows to keep the PWM running if it was
> enabled in bootloader. It is very similar to the GPIO behavior. GPIO pin
> set as output in bootloader keep the same setting in Linux unless it is
> reconfigured.
> 
> If we find the PWM block enabled we need to prepare and enable its source
> clock otherwise the clock will be disabled late in the boot as unused.
> That will leave the PWM in enabled state but with disabled clock. That has
> a side effect that the PWM output is left at its current level at which
> the clock was disabled. It is totally non-deterministic and it may be LOW
> or HIGH.

Does this problem still exist if the pwm-imx driver is a module?

> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> ---
>  drivers/pwm/pwm-imx.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index 7a4907b..6cd3b72 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -83,6 +83,9 @@
>  
>  #define MX3_PWM_SWR_LOOP		5
>  
> +/* PWMPR register value of 0xffff has the same effect as 0xfffe */
> +#define MX3_PWMPR_MAX			0xfffe
> +
>  struct imx_chip {
>  	struct clk	*clk_per;
>  
> @@ -93,6 +96,55 @@ struct imx_chip {
>  
>  #define to_imx_chip(chip)	container_of(chip, struct imx_chip, chip)
>  
> +static void imx_pwm_get_state(struct pwm_chip *chip,
> +		struct pwm_device *pwm, struct pwm_state *state)

broken alignment.

> +{
> +	struct imx_chip *imx = to_imx_chip(chip);
> +	u32 period, prescaler, pwm_clk, ret, val;
> +	u64 tmp;
> +
> +	val = readl(imx->mmio_base + MX3_PWMCR);
> +
> +	if (val & MX3_PWMCR_EN) {
> +		state->enabled = true;
> +		ret = clk_prepare_enable(imx->clk_per);
> +		if (ret)
> +			return;
> +	} else {
> +		state->enabled = false;
> +	}
> +
> +	switch (FIELD_GET(MX3_PWMCR_POUTC, val)) {
> +	case MX3_PWMCR_POUTC_NORMAL:
> +		state->polarity = PWM_POLARITY_NORMAL;
> +		break;
> +	case MX3_PWMCR_POUTC_INVERTED:
> +		state->polarity = PWM_POLARITY_INVERSED;
> +		break;
> +	default:
> +		dev_warn(chip->dev, "can't set polarity, output disconnected");

Should we return an error here?

> +	}
> +
> +	prescaler = MX3_PWMCR_PRESCALER_GET(val);
> +	pwm_clk = clk_get_rate(imx->clk_per);
> +	pwm_clk = DIV_ROUND_CLOSEST_ULL(pwm_clk, prescaler);
> +	val = readl(imx->mmio_base + MX3_PWMPR);

It would be more cautionous to not rely on the reserved bits to read as
zero. So I suggest to mask the value with 0xffff.

> +	period = val >= MX3_PWMPR_MAX ? MX3_PWMPR_MAX : val;
> +
> +	/* PWMOUT (Hz) = PWMCLK / (PWMPR + 2) */
> +	tmp = NSEC_PER_SEC * (u64)(period + 2);
> +	state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);

Would it make sense to introduce a policy about how to round in this
case? (Similarily for .apply?) This is of course out of scope for this
patch.

> +
> +	/* PWMSAR can be read only if PWM is enabled */
> +	if (state->enabled) {
> +		val = readl(imx->mmio_base + MX3_PWMSAR);
> +		tmp = NSEC_PER_SEC * (u64)(val);
> +		state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
> +	} else {
> +		state->duty_cycle = 0;
> +	}
> +}
> +
>  static int imx_pwm_config_v1(struct pwm_chip *chip,
>  		struct pwm_device *pwm, int duty_ns, int period_ns)
>  {
> @@ -272,6 +324,7 @@ static const struct pwm_ops imx_pwm_ops_v1 = {
>  
>  static const struct pwm_ops imx_pwm_ops_v2 = {
>  	.apply = imx_pwm_apply_v2,
> +	.get_state = imx_pwm_get_state,
>  	.owner = THIS_MODULE,
>  };
>  

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

Re: [PATCH 1/3] pwm: imx: Sort include files

[Thierry Reding]

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

On Mon, Oct 01, 2018 at 04:19:46PM +0200, Michal Vokáč wrote:
> Sort included header files alphabetically.
> 
> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> ---
>  drivers/pwm/pwm-imx.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

Applied, thanks.

Thierry

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

Re: [PATCH 2/3] pwm: imx: Use bitops and bitfield macros to define register values

[Thierry Reding]

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

On Mon, Oct 01, 2018 at 04:19:47PM +0200, Michal Vokáč wrote:
> Use existing macros to define register fields instead of manually shifting
> the bit masks. Also define some more register bits.
> 
> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> ---
>  drivers/pwm/pwm-imx.c | 78 ++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 58 insertions(+), 20 deletions(-)

Applied, thanks.

Thierry

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

Re: [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout

[Thierry Reding]

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

On Mon, Oct 01, 2018 at 04:19:48PM +0200, Michal Vokáč wrote:
> Implement the get_state() function and set the initial state to reflect
> real state of the hardware. This allows to keep the PWM running if it was
> enabled in bootloader. It is very similar to the GPIO behavior. GPIO pin
> set as output in bootloader keep the same setting in Linux unless it is
> reconfigured.
> 
> If we find the PWM block enabled we need to prepare and enable its source
> clock otherwise the clock will be disabled late in the boot as unused.
> That will leave the PWM in enabled state but with disabled clock. That has
> a side effect that the PWM output is left at its current level at which
> the clock was disabled. It is totally non-deterministic and it may be LOW
> or HIGH.
> 
> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> ---
>  drivers/pwm/pwm-imx.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)

Applied, thanks.

Thierry

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

Re: [2/3] pwm: imx: Use bitops and bitfield macros to define register values

[Uwe Kleine-König]

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

On Mon, Oct 01, 2018 at 04:19:47PM +0200, Michal Vokáč wrote:
> Use existing macros to define register fields instead of manually shifting
> the bit masks. Also define some more register bits.

I didn't check, but wonder if these additional register bits are then
used in the next patch. Maybe I'd change this patch to not introduce
something new, only let it modify the already existing stuff and then
introduce the new bits in the patch that makes use of them.

> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> ---
>  drivers/pwm/pwm-imx.c | 78 ++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 58 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index bcbcac4..7a4907b 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -5,6 +5,8 @@
>   * Derived from pxa PWM driver by eric miao <eric.miao@marvell.com>
>   */
>  
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
> @@ -23,7 +25,7 @@
>  #define MX1_PWMS			0x04   /* PWM Sample Register */
>  #define MX1_PWMP			0x08   /* PWM Period Register */
>  
> -#define MX1_PWMC_EN			(1 << 4)
> +#define MX1_PWMC_EN			BIT(4)
>  
>  /* i.MX27, i.MX31, i.MX35 share the same PWM function block: */
>  
> @@ -31,18 +33,53 @@
>  #define MX3_PWMSR			0x04    /* PWM Status Register */
>  #define MX3_PWMSAR			0x0C    /* PWM Sample Register */
>  #define MX3_PWMPR			0x10    /* PWM Period Register */
> -#define MX3_PWMCR_PRESCALER(x)		((((x) - 1) & 0xFFF) << 4)
> -#define MX3_PWMCR_STOPEN		(1 << 25)
> -#define MX3_PWMCR_DOZEEN		(1 << 24)
> -#define MX3_PWMCR_WAITEN		(1 << 23)
> -#define MX3_PWMCR_DBGEN			(1 << 22)
> -#define MX3_PWMCR_POUTC			(1 << 18)
> -#define MX3_PWMCR_CLKSRC_IPG_HIGH	(2 << 16)
> -#define MX3_PWMCR_CLKSRC_IPG		(1 << 16)
> -#define MX3_PWMCR_SWR			(1 << 3)
> -#define MX3_PWMCR_EN			(1 << 0)
> -#define MX3_PWMSR_FIFOAV_4WORDS		0x4
> -#define MX3_PWMSR_FIFOAV_MASK		0x7
> +
> +#define MX3_PWMCR_FWM			GENMASK(27, 26)
> +#define MX3_PWMCR_STOPEN		BIT(25)
> +#define MX3_PWMCR_DOZEN			BIT(24)
> +#define MX3_PWMCR_WAITEN		BIT(23)
> +#define MX3_PWMCR_DBGEN			BIT(22)
> +#define MX3_PWMCR_BCTR			BIT(21)
> +#define MX3_PWMCR_HCTR			BIT(20)
> +
> +#define MX3_PWMCR_POUTC			GENMASK(19, 18)
> +#define MX3_PWMCR_POUTC_NORMAL		0
> +#define MX3_PWMCR_POUTC_INVERTED	1
> +#define MX3_PWMCR_POUTC_OFF		2
> +
> +#define MX3_PWMCR_CLKSRC		GENMASK(17, 16)
> +#define MX3_PWMCR_CLKSRC_OFF		0
> +#define MX3_PWMCR_CLKSRC_IPG		1
> +#define MX3_PWMCR_CLKSRC_IPG_HIGH	2
> +#define MX3_PWMCR_CLKSRC_IPG_32K	3
> +
> +#define MX3_PWMCR_PRESCALER		GENMASK(15, 4)
> +
> +#define MX3_PWMCR_SWR			BIT(3)
> +
> +#define MX3_PWMCR_REPEAT		GENMASK(2, 1)
> +#define MX3_PWMCR_REPEAT_1X		0
> +#define MX3_PWMCR_REPEAT_2X		1
> +#define MX3_PWMCR_REPEAT_4X		2
> +#define MX3_PWMCR_REPEAT_8X		3
> +
> +#define MX3_PWMCR_EN			BIT(0)
> +
> +#define MX3_PWMSR_FWE			BIT(6)
> +#define MX3_PWMSR_CMP			BIT(5)
> +#define MX3_PWMSR_ROV			BIT(4)
> +#define MX3_PWMSR_FE			BIT(3)
> +
> +#define MX3_PWMSR_FIFOAV		GENMASK(2, 0)
> +#define MX3_PWMSR_FIFOAV_EMPTY		0
> +#define MX3_PWMSR_FIFOAV_1WORD		1
> +#define MX3_PWMSR_FIFOAV_2WORDS		2
> +#define MX3_PWMSR_FIFOAV_3WORDS		3
> +#define MX3_PWMSR_FIFOAV_4WORDS		4
> +
> +#define MX3_PWMCR_PRESCALER_SET(x)	FIELD_PREP(MX3_PWMCR_PRESCALER, (x) - 1)
> +#define MX3_PWMCR_PRESCALER_GET(x)	(FIELD_GET(MX3_PWMCR_PRESCALER, \
> +						   (x)) + 1)

I wouldn't hide the +1 and -1 in a macro but as this was already the
case before your patch, that's ok.

>  #define MX3_PWM_SWR_LOOP		5
>  
> @@ -142,14 +179,14 @@ static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip,
>  	u32 sr;
>  
>  	sr = readl(imx->mmio_base + MX3_PWMSR);
> -	fifoav = sr & MX3_PWMSR_FIFOAV_MASK;
> +	fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
>  	if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
>  		period_ms = DIV_ROUND_UP(pwm_get_period(pwm),
>  					 NSEC_PER_MSEC);
>  		msleep(period_ms);
>  
>  		sr = readl(imx->mmio_base + MX3_PWMSR);
> -		if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
> +		if (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr))
>  			dev_warn(dev, "there is no free FIFO slot\n");
>  	}
>  }
> @@ -207,13 +244,14 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
>  		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>  		writel(period_cycles, imx->mmio_base + MX3_PWMPR);
>  
> -		cr = MX3_PWMCR_PRESCALER(prescale) |
> -		     MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> -		     MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH |
> -		     MX3_PWMCR_EN;
> +		cr = MX3_PWMCR_PRESCALER_SET(prescale) |
> +		     MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> +		     FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) |
> +		     MX3_PWMCR_DBGEN | MX3_PWMCR_EN;
>  
>  		if (state->polarity == PWM_POLARITY_INVERSED)
> -			cr |= MX3_PWMCR_POUTC;
> +			cr |= FIELD_PREP(MX3_PWMCR_POUTC,
> +					MX3_PWMCR_POUTC_INVERTED);
>  
>  		writel(cr, imx->mmio_base + MX3_PWMCR);
>  	} else if (cstate.enabled) {

Best regards
Uwe

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

Re: [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout

[Uwe Kleine-König]

On Wed, Dec 12, 2018 at 11:54:32AM +0100, Thierry Reding wrote:
> On Mon, Oct 01, 2018 at 04:19:48PM +0200, Michal Vokáč wrote:
> > Implement the get_state() function and set the initial state to reflect
> > real state of the hardware. This allows to keep the PWM running if it was
> > enabled in bootloader. It is very similar to the GPIO behavior. GPIO pin
> > set as output in bootloader keep the same setting in Linux unless it is
> > reconfigured.
> > 
> > If we find the PWM block enabled we need to prepare and enable its source
> > clock otherwise the clock will be disabled late in the boot as unused.
> > That will leave the PWM in enabled state but with disabled clock. That has
> > a side effect that the PWM output is left at its current level at which
> > the clock was disabled. It is totally non-deterministic and it may be LOW
> > or HIGH.
> > 
> > Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> > ---
> >  drivers/pwm/pwm-imx.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 53 insertions(+)
> 
> Applied, thanks.

Did you miss my feedback for this patch or did you choose to ignore it?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout

[Thierry Reding]

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

On Thu, Dec 13, 2018 at 09:52:13AM +0100, Uwe Kleine-König wrote:
> On Wed, Dec 12, 2018 at 11:54:32AM +0100, Thierry Reding wrote:
> > On Mon, Oct 01, 2018 at 04:19:48PM +0200, Michal Vokáč wrote:
> > > Implement the get_state() function and set the initial state to reflect
> > > real state of the hardware. This allows to keep the PWM running if it was
> > > enabled in bootloader. It is very similar to the GPIO behavior. GPIO pin
> > > set as output in bootloader keep the same setting in Linux unless it is
> > > reconfigured.
> > > 
> > > If we find the PWM block enabled we need to prepare and enable its source
> > > clock otherwise the clock will be disabled late in the boot as unused.
> > > That will leave the PWM in enabled state but with disabled clock. That has
> > > a side effect that the PWM output is left at its current level at which
> > > the clock was disabled. It is totally non-deterministic and it may be LOW
> > > or HIGH.
> > > 
> > > Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> > > ---
> > >  drivers/pwm/pwm-imx.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 53 insertions(+)
> > 
> > Applied, thanks.
> 
> Did you miss my feedback for this patch or did you choose to ignore it?

Don't have anything in my inbox, but I see that it's there on patchwork,
so I suspect it was eaten by the spam filter. Let me copy-paste here and
go through it.

> > @@ -93,6 +96,55 @@ struct imx_chip {
> >
> >  #define to_imx_chip(chip) container_of(chip, struct imx_chip, chip)
> >
> > +static void imx_pwm_get_state(struct pwm_chip *chip,
> > +  struct pwm_device *pwm, struct pwm_state *state)
> 
> broken alignment.

I can fix that up, no need to resend for that.

> > +{
> > + struct imx_chip *imx = to_imx_chip(chip);
> > + u32 period, prescaler, pwm_clk, ret, val;
> > + u64 tmp;
> > +
> > + val = readl(imx->mmio_base + MX3_PWMCR);
> > +
> > + if (val & MX3_PWMCR_EN) {
> > +  state->enabled = true;
> > +  ret = clk_prepare_enable(imx->clk_per);
> > +  if (ret)
> > +   return;
> > + } else {
> > +  state->enabled = false;
> > + }
> > +
> > + switch (FIELD_GET(MX3_PWMCR_POUTC, val)) {
> > + case MX3_PWMCR_POUTC_NORMAL:
> > +  state->polarity = PWM_POLARITY_NORMAL;
> > +  break;
> > + case MX3_PWMCR_POUTC_INVERTED:
> > +  state->polarity = PWM_POLARITY_INVERSED;
> > +  break;
> > + default:
> > +  dev_warn(chip->dev, "can't set polarity, output disconnected");
> 
> Should we return an error here?

We can't return an error here because the function has a void return
type. I'm not sure what it means if the POUTC is neither "normal" nor
"inverted", but perhaps a good idea would be to default to either of
those two in that case, or perhaps forcibly disable the PWM so that
we get known-good values in the registers?

I'm tempted not to treat this as a blocker because it's not actually
a bug or anything. Prior to this patch we also ignore whatever this
field was set to.

> > + }
> > +
> > + prescaler = MX3_PWMCR_PRESCALER_GET(val);
> > + pwm_clk = clk_get_rate(imx->clk_per);
> > + pwm_clk = DIV_ROUND_CLOSEST_ULL(pwm_clk, prescaler);
> > + val = readl(imx->mmio_base + MX3_PWMPR);
> 
> It would be more cautionous to not rely on the reserved bits to read as
> zero. So I suggest to mask the value with 0xffff.

If that's what the documentation says I think it's okay to rely on it.
But I can add the mask if we want to play it extra safe.

> > + period = val >= MX3_PWMPR_MAX ? MX3_PWMPR_MAX : val;
> > +
> > + /* PWMOUT (Hz) = PWMCLK / (PWMPR + 2) */
> > + tmp = NSEC_PER_SEC * (u64)(period + 2);
> > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
> 
> Would it make sense to introduce a policy about how to round in this
> case? (Similarily for .apply?) This is of course out of scope for this
> patch.

I'm not sure what you means by policy. The above already does round to
closest. Is that not an appropriate policy?

Thierry

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

Re: [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout

[Uwe Kleine-König]

Hello Thierry,

On Thu, Dec 13, 2018 at 06:00:00PM +0100, Thierry Reding wrote:
> On Thu, Dec 13, 2018 at 09:52:13AM +0100, Uwe Kleine-König wrote:
> > On Wed, Dec 12, 2018 at 11:54:32AM +0100, Thierry Reding wrote:
> > > On Mon, Oct 01, 2018 at 04:19:48PM +0200, Michal Vokáč wrote:
> > > > Implement the get_state() function and set the initial state to reflect
> > > > real state of the hardware. This allows to keep the PWM running if it was
> > > > enabled in bootloader. It is very similar to the GPIO behavior. GPIO pin
> > > > set as output in bootloader keep the same setting in Linux unless it is
> > > > reconfigured.
> > > > 
> > > > If we find the PWM block enabled we need to prepare and enable its source
> > > > clock otherwise the clock will be disabled late in the boot as unused.
> > > > That will leave the PWM in enabled state but with disabled clock. That has
> > > > a side effect that the PWM output is left at its current level at which
> > > > the clock was disabled. It is totally non-deterministic and it may be LOW
> > > > or HIGH.
> > > > 
> > > > Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> > > > ---
> > > >  drivers/pwm/pwm-imx.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 53 insertions(+)
> > > 
> > > Applied, thanks.
> > 
> > Did you miss my feedback for this patch or did you choose to ignore it?
> 
> Don't have anything in my inbox, but I see that it's there on patchwork,
> so I suspect it was eaten by the spam filter.

:-|

> Let me copy-paste here and go through it.
> 
> > > @@ -93,6 +96,55 @@ struct imx_chip {
> > >
> > >  #define to_imx_chip(chip) container_of(chip, struct imx_chip, chip)
> > >
> > > +static void imx_pwm_get_state(struct pwm_chip *chip,
> > > +  struct pwm_device *pwm, struct pwm_state *state)
> > 
> > broken alignment.
> 
> I can fix that up, no need to resend for that.

fine.

> > > +{
> > > + struct imx_chip *imx = to_imx_chip(chip);
> > > + u32 period, prescaler, pwm_clk, ret, val;
> > > + u64 tmp;
> > > +
> > > + val = readl(imx->mmio_base + MX3_PWMCR);
> > > +
> > > + if (val & MX3_PWMCR_EN) {
> > > +  state->enabled = true;
> > > +  ret = clk_prepare_enable(imx->clk_per);
> > > +  if (ret)
> > > +   return;
> > > + } else {
> > > +  state->enabled = false;
> > > + }
> > > +
> > > + switch (FIELD_GET(MX3_PWMCR_POUTC, val)) {
> > > + case MX3_PWMCR_POUTC_NORMAL:
> > > +  state->polarity = PWM_POLARITY_NORMAL;
> > > +  break;
> > > + case MX3_PWMCR_POUTC_INVERTED:
> > > +  state->polarity = PWM_POLARITY_INVERSED;
> > > +  break;
> > > + default:
> > > +  dev_warn(chip->dev, "can't set polarity, output disconnected");
> > 
> > Should we return an error here?
> 
> We can't return an error here because the function has a void return
> type. I'm not sure what it means if the POUTC is neither "normal" nor
> "inverted", but perhaps a good idea would be to default to either of
> those two in that case, or perhaps forcibly disable the PWM so that
> we get known-good values in the registers?

The manual says "PWM output is disconnected". So I think the right thing
to do is to consider the pwm as disabled. (But checking how the output
actually behaves would be great.) (Maybe this setting is even the
solution to our "Make the output high-Z on disable" discussion?)

> I'm tempted not to treat this as a blocker because it's not actually
> a bug or anything. Prior to this patch we also ignore whatever this
> field was set to.

Prior to this patch this field was never read as this is only needed in
.get_state. And in .apply the value is correctly written.

> > > + }
> > > +
> > > + prescaler = MX3_PWMCR_PRESCALER_GET(val);
> > > + pwm_clk = clk_get_rate(imx->clk_per);
> > > + pwm_clk = DIV_ROUND_CLOSEST_ULL(pwm_clk, prescaler);
> > > + val = readl(imx->mmio_base + MX3_PWMPR);
> > 
> > It would be more cautionous to not rely on the reserved bits to read as
> > zero. So I suggest to mask the value with 0xffff.
> 
> If that's what the documentation says I think it's okay to rely on it.
> But I can add the mask if we want to play it extra safe.

The PERIOD field only covers bits 0 to 15 and the upper 16 bits of this
register are reserved. The registers are documented to read as 0, still
I'd prefer to mask them out for maximal correctness.

> > > + period = val >= MX3_PWMPR_MAX ? MX3_PWMPR_MAX : val;
> > > +
> > > + /* PWMOUT (Hz) = PWMCLK / (PWMPR + 2) */
> > > + tmp = NSEC_PER_SEC * (u64)(period + 2);
> > > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
> > 
> > Would it make sense to introduce a policy about how to round in this
> > case? (Similarily for .apply?) This is of course out of scope for this
> > patch.
> 
> I'm not sure what you means by policy. The above already does round to
> closest. Is that not an appropriate policy?

I mean to document the expectation of the pwm framework how the driver
is supposed to configure the hardware when it should program the PWM
with say duty_cycle=100ns/period=300ns and but the hardware can only
implement both duty_cycle and period in steps of 3ns. So the (IMHO) good
candidates are:

	duty_cycle = 99ns / period = 297ns
	duty_cycle = 99ns / period = 300ns
	duty_cycle = 102ns / period = 300ns
	duty_cycle = 102ns / period = 306ns

As it's not obvious which is the best here (or is it?) and taking into
account that the PWM user might care, it would be right to specify
something like:

  The driver is supposed to configure the biggest duty_cycle that
  is not greater than the requested duty_cycle. Similarily it should
  pick the biggest possible period that is not greater than the
  requested period.

To actually allow the user to find the setting that he or she prefers
something like the clk framework provides with clk_round_rate() would be
needed of course.

And for .get_state I'd specify something similar that ensures that in
the sequence

	myops->get_state(chip, pwm, state);
	myops->apply(chip, pwm, state);

the latter is a noop.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [3/3] pwm: imx: Implement get_state() function for hardware readout

[Vokáč Michal]

On 12.12.2018 11:51, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Oct 01, 2018 at 04:19:48PM +0200, Michal Vokáč wrote:
>> Implement the get_state() function and set the initial state to reflect
>> real state of the hardware. This allows to keep the PWM running if it was
>> enabled in bootloader. It is very similar to the GPIO behavior. GPIO pin
>> set as output in bootloader keep the same setting in Linux unless it is
>> reconfigured.
>>
>> If we find the PWM block enabled we need to prepare and enable its source
>> clock otherwise the clock will be disabled late in the boot as unused.
>> That will leave the PWM in enabled state but with disabled clock. That has
>> a side effect that the PWM output is left at its current level at which
>> the clock was disabled. It is totally non-deterministic and it may be LOW
>> or HIGH.
> 
> Does this problem still exist if the pwm-imx driver is a module?

Yes. The source clock for PWM is stopped by the clock core shortly
before init process is started and it does not matter if the pwm-imx
driver is built as a module or built into the kernel.

When the module is loaded, the .get_state function is executed and
reads the PWM registers. If the EN bit is set the clock is started
and PWM continues from where it was stopped.

Michal

Re: [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout

[Vokáč Michal]

On 13.12.2018 21:14, Uwe Kleine-König wrote:> On Thu, Dec 13, 2018 at 06:00:00PM +0100, Thierry Reding wrote:
>> On Thu, Dec 13, 2018 at 09:52:13AM +0100, Uwe Kleine-König wrote:
>>> On Wed, Dec 12, 2018 at 11:54:32AM +0100, Thierry Reding wrote:
>>>> On Mon, Oct 01, 2018 at 04:19:48PM +0200, Michal Vokáč wrote:
>>>>> Implement the get_state() function and set the initial state to reflect
>>>>> real state of the hardware. This allows to keep the PWM running if it was
>>>>> enabled in bootloader. It is very similar to the GPIO behavior. GPIO pin
>>>>> set as output in bootloader keep the same setting in Linux unless it is
>>>>> reconfigured.
>>>>>
>>>>> If we find the PWM block enabled we need to prepare and enable its source
>>>>> clock otherwise the clock will be disabled late in the boot as unused.
>>>>> That will leave the PWM in enabled state but with disabled clock. That has
>>>>> a side effect that the PWM output is left at its current level at which
>>>>> the clock was disabled. It is totally non-deterministic and it may be LOW
>>>>> or HIGH.
>>>>>
>>>>> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
>>>>> ---
>>>>>   drivers/pwm/pwm-imx.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>   1 file changed, 53 insertions(+)
>>>>
>>>> Applied, thanks.
>>>
>>> Did you miss my feedback for this patch or did you choose to ignore it?
>>
>> Don't have anything in my inbox, but I see that it's there on patchwork,
>> so I suspect it was eaten by the spam filter.
> 
> :-|
> 
>> Let me copy-paste here and go through it.
>>
>>>> @@ -93,6 +96,55 @@ struct imx_chip {
>>>>
>>>>   #define to_imx_chip(chip) container_of(chip, struct imx_chip, chip)
>>>>
>>>> +static void imx_pwm_get_state(struct pwm_chip *chip,
>>>> +  struct pwm_device *pwm, struct pwm_state *state)
>>>
>>> broken alignment.
>>
>> I can fix that up, no need to resend for that.
> 
> fine.
> 
>>>> +{
>>>> + struct imx_chip *imx = to_imx_chip(chip);
>>>> + u32 period, prescaler, pwm_clk, ret, val;
>>>> + u64 tmp;
>>>> +
>>>> + val = readl(imx->mmio_base + MX3_PWMCR);
>>>> +
>>>> + if (val & MX3_PWMCR_EN) {
>>>> +  state->enabled = true;
>>>> +  ret = clk_prepare_enable(imx->clk_per);
>>>> +  if (ret)
>>>> +   return;
>>>> + } else {
>>>> +  state->enabled = false;
>>>> + }
>>>> +
>>>> + switch (FIELD_GET(MX3_PWMCR_POUTC, val)) {
>>>> + case MX3_PWMCR_POUTC_NORMAL:
>>>> +  state->polarity = PWM_POLARITY_NORMAL;
>>>> +  break;
>>>> + case MX3_PWMCR_POUTC_INVERTED:
>>>> +  state->polarity = PWM_POLARITY_INVERSED;
>>>> +  break;
>>>> + default:
>>>> +  dev_warn(chip->dev, "can't set polarity, output disconnected");
>>>
>>> Should we return an error here?
>>
>> We can't return an error here because the function has a void return
>> type. I'm not sure what it means if the POUTC is neither "normal" nor
>> "inverted", but perhaps a good idea would be to default to either of
>> those two in that case, or perhaps forcibly disable the PWM so that
>> we get known-good values in the registers?
> 
> The manual says "PWM output is disconnected". So I think the right thing
> to do is to consider the pwm as disabled. (But checking how the output
> actually behaves would be great.) (Maybe this setting is even the
> solution to our "Make the output high-Z on disable" discussion?)

I checked the behavior. It can be used to disconnect the output even
if the PWM is enabled. It does not solve the problem with the LOW
output level in disabled state. This gave me the same results.
When PWM is enabled and the output is disconnected with POUTC = 2,
the output level is always LOW and the DSE, ODE, and PUS pad control
settings have no effect. I suppose the main purpose of this is to
allow to quickly enable/disable the output without the need to enable/
disable the whole PWM block. So we can not really say the PWM is
enabled/disabled here as there is another (EN) bit in the PWMCR
register that controls that.

>> I'm tempted not to treat this as a blocker because it's not actually
>> a bug or anything. Prior to this patch we also ignore whatever this
>> field was set to.
> 
> Prior to this patch this field was never read as this is only needed in
> .get_state. And in .apply the value is correctly written.
> 
>>>> + }
>>>> +
>>>> + prescaler = MX3_PWMCR_PRESCALER_GET(val);
>>>> + pwm_clk = clk_get_rate(imx->clk_per);
>>>> + pwm_clk = DIV_ROUND_CLOSEST_ULL(pwm_clk, prescaler);
>>>> + val = readl(imx->mmio_base + MX3_PWMPR);
>>>
>>> It would be more cautionous to not rely on the reserved bits to read as
>>> zero. So I suggest to mask the value with 0xffff.
>>
>> If that's what the documentation says I think it's okay to rely on it.
>> But I can add the mask if we want to play it extra safe.
> 
> The PERIOD field only covers bits 0 to 15 and the upper 16 bits of this
> register are reserved. The registers are documented to read as 0, still
> I'd prefer to mask them out for maximal correctness.

OK.

>>>> + period = val >= MX3_PWMPR_MAX ? MX3_PWMPR_MAX : val;
>>>> +
>>>> + /* PWMOUT (Hz) = PWMCLK / (PWMPR + 2) */
>>>> + tmp = NSEC_PER_SEC * (u64)(period + 2);
>>>> + state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
>>>
>>> Would it make sense to introduce a policy about how to round in this
>>> case? (Similarily for .apply?) This is of course out of scope for this
>>> patch.
>>
>> I'm not sure what you means by policy. The above already does round to
>> closest. Is that not an appropriate policy?
> 
> I mean to document the expectation of the pwm framework how the driver
> is supposed to configure the hardware when it should program the PWM
> with say duty_cycle=100ns/period=300ns and but the hardware can only
> implement both duty_cycle and period in steps of 3ns. So the (IMHO) good
> candidates are:
> 
> 	duty_cycle = 99ns / period = 297ns
> 	duty_cycle = 99ns / period = 300ns
> 	duty_cycle = 102ns / period = 300ns
> 	duty_cycle = 102ns / period = 306ns
> 
> As it's not obvious which is the best here (or is it?) and taking into
> account that the PWM user might care, it would be right to specify
> something like:
> 
>    The driver is supposed to configure the biggest duty_cycle that
>    is not greater than the requested duty_cycle. Similarily it should
>    pick the biggest possible period that is not greater than the
>    requested period.
> 
> To actually allow the user to find the setting that he or she prefers
> something like the clk framework provides with clk_round_rate() would be
> needed of course.
> 
> And for .get_state I'd specify something similar that ensures that in
> the sequence
> 
> 	myops->get_state(chip, pwm, state);
> 	myops->apply(chip, pwm, state);
> 
> the latter is a noop.

Sounds good. Would be great to find something like this in the docs
while I was implementing the .get_state.

If this is out of scope for this patch then there are only two minor
issues to fix - the incorrect indentation and the mask for the period
register, right?

So given the fact that Thierry already applied the series I can send
a separate patch with these fixes, OK? Or you will fix it up all
yourself, Thierry?

Best regards,
Michal