[PATCH 1/4] pwm: Read initial hardware state at request time

[PATCH 1/4] pwm: Read initial hardware state at request time

[Thierry Reding]

The PWM core doesn't need to know about the hardware state of a PWM
unless there is a user for it. Defer initial hardware readout until
a PWM is requested.

As a side-effect, this allows the ->get_state() callback to rely on
per-PWM data.

Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
---
 drivers/pwm/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index f877e77d9184..e067873c6cc5 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -114,6 +114,9 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
 		}
 	}
 
+	if (pwm->chip->ops->get_state)
+		pwm->chip->ops->get_state(pwm->chip, pwm, &pwm->state);
+
 	set_bit(PWMF_REQUESTED, &pwm->flags);
 	pwm->label = label;
 
@@ -283,9 +286,6 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
 		pwm->hwpwm = i;
 		pwm->state.polarity = polarity;
 
-		if (chip->ops->get_state)
-			chip->ops->get_state(chip, pwm, &pwm->state);
-
 		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
 	}
 
-- 
2.23.0

[PATCH 2/4] pwm: cros-ec: Cache duty cycle value

[Thierry Reding]

The ChromeOS embedded controller doesn't differentiate between disabled
and duty cycle being 0. In order not to potentially confuse consumers,
cache the duty cycle and return the cached value instead of the real
value when the PWM is disabled.

Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
---
 drivers/pwm/pwm-cros-ec.c | 58 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 54 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
index 89497448d217..09c08dee099e 100644
--- a/drivers/pwm/pwm-cros-ec.c
+++ b/drivers/pwm/pwm-cros-ec.c
@@ -25,11 +25,39 @@ struct cros_ec_pwm_device {
 	struct pwm_chip chip;
 };
 
+/**
+ * struct cros_ec_pwm - per-PWM driver data
+ * @duty_cycle: cached duty cycle
+ */
+struct cros_ec_pwm {
+	u16 duty_cycle;
+};
+
 static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c)
 {
 	return container_of(c, struct cros_ec_pwm_device, chip);
 }
 
+static int cros_ec_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct cros_ec_pwm *channel;
+
+	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
+	if (!channel)
+		return -ENOMEM;
+
+	pwm_set_chip_data(pwm, channel);
+
+	return 0;
+}
+
+static void cros_ec_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
+
+	kfree(channel);
+}
+
 static int cros_ec_pwm_set_duty(struct cros_ec_device *ec, u8 index, u16 duty)
 {
 	struct {
@@ -96,7 +124,9 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			     const struct pwm_state *state)
 {
 	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
-	int duty_cycle;
+	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
+	u16 duty_cycle;
+	int ret;
 
 	/* The EC won't let us change the period */
 	if (state->period != EC_PWM_MAX_DUTY)
@@ -108,13 +138,20 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	 */
 	duty_cycle = state->enabled ? state->duty_cycle : 0;
 
-	return cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle);
+	ret = cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle);
+	if (ret < 0)
+		return ret;
+
+	channel->duty_cycle = state->duty_cycle;
+
+	return 0;
 }
 
 static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 				  struct pwm_state *state)
 {
 	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
+	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
 	int ret;
 
 	ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
@@ -126,8 +163,19 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 	state->enabled = (ret > 0);
 	state->period = EC_PWM_MAX_DUTY;
 
-	/* Note that "disabled" and "duty cycle == 0" are treated the same */
-	state->duty_cycle = ret;
+	/*
+	 * Note that "disabled" and "duty cycle == 0" are treated the same. If
+	 * the cached duty cycle is not zero, used the cached duty cycle. This
+	 * ensures that the configured duty cycle is kept across a disable and
+	 * enable operation and avoids potentially confusing consumers.
+	 *
+	 * For the case of the initial hardware readout, channel->duty_cycle
+	 * will be 0 and the actual duty cycle read from the EC is used.
+	 */
+	if (ret == 0 && channel->duty_cycle > 0)
+		state->duty_cycle = channel->duty_cycle;
+	else
+		state->duty_cycle = ret;
 }
 
 static struct pwm_device *
@@ -149,6 +197,8 @@ cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
 }
 
 static const struct pwm_ops cros_ec_pwm_ops = {
+	.request = cros_ec_pwm_request,
+	.free = cros_ec_pwm_free,
 	.get_state	= cros_ec_pwm_get_state,
 	.apply		= cros_ec_pwm_apply,
 	.owner		= THIS_MODULE,
-- 
2.23.0

[PATCH 3/4] pwm: imx27: Cache duty cycle register value

[Thierry Reding]

The hardware register containing the duty cycle value cannot be accessed
when the PWM is disabled. This causes the ->get_state() callback to read
back a duty cycle value of 0, which can confuse consumer drivers.

Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
---
 drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index ae11d8577f18..4113d5cd4c62 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -85,6 +85,13 @@ struct pwm_imx27_chip {
 	struct clk	*clk_per;
 	void __iomem	*mmio_base;
 	struct pwm_chip	chip;
+
+	/*
+	 * The driver cannot read the current duty cycle from the hardware if
+	 * the hardware is disabled. Cache the last programmed duty cycle
+	 * value to return in that case.
+	 */
+	unsigned int duty_cycle;
 };
 
 #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
@@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip,
 	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) {
+	/*
+	 * PWMSAR can be read only if PWM is enabled. If the PWM is disabled,
+	 * use the cached value.
+	 */
+	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;
-	}
+	else
+		val = imx->duty_cycle;
+
+	tmp = NSEC_PER_SEC * (u64)(val);
+	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
 
 	if (!state->enabled)
 		pwm_imx27_clk_disable_unprepare(chip);
@@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
 		writel(period_cycles, imx->mmio_base + MX3_PWMPR);
 
+		/*
+		 * Store the duty cycle for future reference in cases where
+		 * the MX3_PWMSAR register can't be read (i.e. when the PWM
+		 * is disabled).
+		 */
+		imx->duty_cycle = duty_cycles;
+
 		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) |
-- 
2.23.0

[PATCH 4/4] pwm: imx27: Unconditionally write state to hardware

[Thierry Reding]

The i.MX driver currently uses a shortcut and doesn't write all of the
state through to the hardware when the PWM is disabled. This causes an
inconsistent state to be read back by consumers with the result of them
malfunctioning.

Fix this by always writing the full state through to the hardware
registers so that the correct state can always be read back.

Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
---
 drivers/pwm/pwm-imx27.c | 120 ++++++++++++++++++++--------------------
 1 file changed, 59 insertions(+), 61 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 4113d5cd4c62..59d8b1289808 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -230,70 +230,68 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	pwm_get_state(pwm, &cstate);
 
-	if (state->enabled) {
-		c = clk_get_rate(imx->clk_per);
-		c *= state->period;
-
-		do_div(c, 1000000000);
-		period_cycles = c;
-
-		prescale = period_cycles / 0x10000 + 1;
-
-		period_cycles /= prescale;
-		c = (unsigned long long)period_cycles * state->duty_cycle;
-		do_div(c, state->period);
-		duty_cycles = c;
-
-		/*
-		 * according to imx pwm RM, the real period value should be
-		 * PERIOD value in PWMPR plus 2.
-		 */
-		if (period_cycles > 2)
-			period_cycles -= 2;
-		else
-			period_cycles = 0;
-
-		/*
-		 * Wait for a free FIFO slot if the PWM is already enabled, and
-		 * flush the FIFO if the PWM was disabled and is about to be
-		 * enabled.
-		 */
-		if (cstate.enabled) {
-			pwm_imx27_wait_fifo_slot(chip, pwm);
-		} else {
-			ret = pwm_imx27_clk_prepare_enable(chip);
-			if (ret)
-				return ret;
-
-			pwm_imx27_sw_reset(chip);
-		}
-
-		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
-		writel(period_cycles, imx->mmio_base + MX3_PWMPR);
-
-		/*
-		 * Store the duty cycle for future reference in cases where
-		 * the MX3_PWMSAR register can't be read (i.e. when the PWM
-		 * is disabled).
-		 */
-		imx->duty_cycle = duty_cycles;
-
-		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 |= FIELD_PREP(MX3_PWMCR_POUTC,
-					MX3_PWMCR_POUTC_INVERTED);
-
-		writel(cr, imx->mmio_base + MX3_PWMCR);
-	} else if (cstate.enabled) {
-		writel(0, imx->mmio_base + MX3_PWMCR);
+	c = clk_get_rate(imx->clk_per);
+	c *= state->period;
 
-		pwm_imx27_clk_disable_unprepare(chip);
+	do_div(c, 1000000000);
+	period_cycles = c;
+
+	prescale = period_cycles / 0x10000 + 1;
+
+	period_cycles /= prescale;
+	c = (unsigned long long)period_cycles * state->duty_cycle;
+	do_div(c, state->period);
+	duty_cycles = c;
+
+	/*
+	 * according to imx pwm RM, the real period value should be PERIOD
+	 * value in PWMPR plus 2.
+	 */
+	if (period_cycles > 2)
+		period_cycles -= 2;
+	else
+		period_cycles = 0;
+
+	/*
+	 * Wait for a free FIFO slot if the PWM is already enabled, and flush
+	 * the FIFO if the PWM was disabled and is about to be enabled.
+	 */
+	if (cstate.enabled) {
+		pwm_imx27_wait_fifo_slot(chip, pwm);
+	} else {
+		ret = pwm_imx27_clk_prepare_enable(chip);
+		if (ret)
+			return ret;
+
+		pwm_imx27_sw_reset(chip);
 	}
 
+	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
+	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
+
+	/*
+	 * Store the duty cycle for future reference in cases where the
+	 * MX3_PWMSAR register can't be read (i.e. when the PWM is disabled).
+	 */
+	imx->duty_cycle = duty_cycles;
+
+	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;
+
+	if (state->polarity == PWM_POLARITY_INVERSED)
+		cr |= FIELD_PREP(MX3_PWMCR_POUTC,
+				MX3_PWMCR_POUTC_INVERTED);
+
+	if (state->enabled)
+		cr |= MX3_PWMCR_EN;
+
+	writel(cr, imx->mmio_base + MX3_PWMCR);
+
+	if (!state->enabled && cstate.enabled)
+		pwm_imx27_clk_disable_unprepare(chip);
+
 	return 0;
 }
 
-- 
2.23.0

Re: [PATCH 1/4] pwm: Read initial hardware state at request time

[Uwe Kleine-König]

Hello Thierry,

On Mon, Oct 21, 2019 at 12:57:36PM +0200, Thierry Reding wrote:
> The PWM core doesn't need to know about the hardware state of a PWM
> unless there is a user for it. Defer initial hardware readout until
> a PWM is requested.

A side effect is that for an unused PWM the get_state callback is never
called (which is good), in return it is called more than once if the PWM
is requested more often (which is bearable).

> As a side-effect, this allows the ->get_state() callback to rely on
> per-PWM data.

Given that this is the motivation for your change I'd give more stress
to this part of the commit log. Also I think this could be more
understandable if you point out that the effect is that .get_state is
only called after .request was called successfully which gives the low
level driver more freedom by (for example) relying on memory allocated
there.

I assume you target the next merge window for this change?

Best regards
Uwe

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

Re: [PATCH 1/4] pwm: Read initial hardware state at request time

[Michal Vokáč]

On 21. 10. 19 12:57, Thierry Reding wrote:
> The PWM core doesn't need to know about the hardware state of a PWM
> unless there is a user for it. Defer initial hardware readout until
> a PWM is requested.
> 
> As a side-effect, this allows the ->get_state() callback to rely on
> per-PWM data.

I tried this on top of v5.4-rc3 on imx6dl-yapp4-draco with pwm-backlight
consumer and on imx6dl-yapp4-hydra with PWM from sysfs and I do not see
any obvious problems.

Tested-by: Michal Vokáč <michal.vokac@ysoft.com>

> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> ---
>   drivers/pwm/core.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index f877e77d9184..e067873c6cc5 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -114,6 +114,9 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
>   		}
>   	}
>   
> +	if (pwm->chip->ops->get_state)
> +		pwm->chip->ops->get_state(pwm->chip, pwm, &pwm->state);
> +
>   	set_bit(PWMF_REQUESTED, &pwm->flags);
>   	pwm->label = label;
>   
> @@ -283,9 +286,6 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
>   		pwm->hwpwm = i;
>   		pwm->state.polarity = polarity;
>   
> -		if (chip->ops->get_state)
> -			chip->ops->get_state(chip, pwm, &pwm->state);
> -
>   		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
>   	}
>   
> 

Re: [PATCH 1/4] pwm: Read initial hardware state at request time

[Enric Balletbo i Serra]

Hi,

On 21/10/19 12:57, Thierry Reding wrote:
> The PWM core doesn't need to know about the hardware state of a PWM
> unless there is a user for it. Defer initial hardware readout until
> a PWM is requested.
> 
> As a side-effect, this allows the ->get_state() callback to rely on
> per-PWM data.
> 
> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>

Tested on top of 5.4.0-rc4 with 2/4 applied this patch fixes the NULL pointer
dereference as expected. So,

Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Thanks,
 Enric

> ---
>  drivers/pwm/core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index f877e77d9184..e067873c6cc5 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -114,6 +114,9 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
>  		}
>  	}
>  
> +	if (pwm->chip->ops->get_state)
> +		pwm->chip->ops->get_state(pwm->chip, pwm, &pwm->state);
> +
>  	set_bit(PWMF_REQUESTED, &pwm->flags);
>  	pwm->label = label;
>  
> @@ -283,9 +286,6 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
>  		pwm->hwpwm = i;
>  		pwm->state.polarity = polarity;
>  
> -		if (chip->ops->get_state)
> -			chip->ops->get_state(chip, pwm, &pwm->state);
> -
>  		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
>  	}
>  
> 

Re: [PATCH 3/4] pwm: imx27: Cache duty cycle register value

[Michal Vokáč]

+Adam

On 21. 10. 19 12:57, Thierry Reding wrote:
> The hardware register containing the duty cycle value cannot be accessed
> when the PWM is disabled. This causes the ->get_state() callback to read
> back a duty cycle value of 0, which can confuse consumer drivers.

Me and Adam Ford already tested the patches [3/4] and [4/4] and gave ours
Tested-by tags in the previous thread but do not see those here.
I re-tested these again and have no issues.

Tested-by: Michal Vokáč <michal.vokac@ysoft.com>

Thank you Thierry,
Michal


> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> ---
>   drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++-------
>   1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index ae11d8577f18..4113d5cd4c62 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -85,6 +85,13 @@ struct pwm_imx27_chip {
>   	struct clk	*clk_per;
>   	void __iomem	*mmio_base;
>   	struct pwm_chip	chip;
> +
> +	/*
> +	 * The driver cannot read the current duty cycle from the hardware if
> +	 * the hardware is disabled. Cache the last programmed duty cycle
> +	 * value to return in that case.
> +	 */
> +	unsigned int duty_cycle;
>   };
>   
>   #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
> @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip,
>   	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) {
> +	/*
> +	 * PWMSAR can be read only if PWM is enabled. If the PWM is disabled,
> +	 * use the cached value.
> +	 */
> +	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;
> -	}
> +	else
> +		val = imx->duty_cycle;
> +
> +	tmp = NSEC_PER_SEC * (u64)(val);
> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
>   
>   	if (!state->enabled)
>   		pwm_imx27_clk_disable_unprepare(chip);
> @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>   		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>   		writel(period_cycles, imx->mmio_base + MX3_PWMPR);
>   
> +		/*
> +		 * Store the duty cycle for future reference in cases where
> +		 * the MX3_PWMSAR register can't be read (i.e. when the PWM
> +		 * is disabled).
> +		 */
> +		imx->duty_cycle = duty_cycles;
> +
>   		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) |
> 

Re: [PATCH 2/4] pwm: cros-ec: Cache duty cycle value

[Enric Balletbo i Serra]

Hi,

On 21/10/19 12:57, Thierry Reding wrote:
> The ChromeOS embedded controller doesn't differentiate between disabled
> and duty cycle being 0. In order not to potentially confuse consumers,
> cache the duty cycle and return the cached value instead of the real
> value when the PWM is disabled.
> 
> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>

One nit and tested on top of 5.4.0-rc4. The backlight is alive again. So,

Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Thanks,
 Enric

> ---
>  drivers/pwm/pwm-cros-ec.c | 58 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 54 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
> index 89497448d217..09c08dee099e 100644
> --- a/drivers/pwm/pwm-cros-ec.c
> +++ b/drivers/pwm/pwm-cros-ec.c
> @@ -25,11 +25,39 @@ struct cros_ec_pwm_device {
>  	struct pwm_chip chip;
>  };
>  
> +/**
> + * struct cros_ec_pwm - per-PWM driver data
> + * @duty_cycle: cached duty cycle
> + */
> +struct cros_ec_pwm {
> +	u16 duty_cycle;
> +};
> +
>  static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c)
>  {
>  	return container_of(c, struct cros_ec_pwm_device, chip);
>  }
>  
> +static int cros_ec_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct cros_ec_pwm *channel;
> +
> +	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> +	if (!channel)
> +		return -ENOMEM;
> +
> +	pwm_set_chip_data(pwm, channel);
> +
> +	return 0;
> +}
> +
> +static void cros_ec_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
> +
> +	kfree(channel);
> +}
> +
>  static int cros_ec_pwm_set_duty(struct cros_ec_device *ec, u8 index, u16 duty)
>  {
>  	struct {
> @@ -96,7 +124,9 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			     const struct pwm_state *state)
>  {
>  	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
> -	int duty_cycle;
> +	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
> +	u16 duty_cycle;
> +	int ret;
>  
>  	/* The EC won't let us change the period */
>  	if (state->period != EC_PWM_MAX_DUTY)
> @@ -108,13 +138,20 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 */
>  	duty_cycle = state->enabled ? state->duty_cycle : 0;
>  
> -	return cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle);
> +	ret = cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle);
> +	if (ret < 0)
> +		return ret;
> +
> +	channel->duty_cycle = state->duty_cycle;
> +
> +	return 0;
>  }
>  
>  static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  				  struct pwm_state *state)
>  {
>  	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
> +	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
>  	int ret;
>  
>  	ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
> @@ -126,8 +163,19 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  	state->enabled = (ret > 0);
>  	state->period = EC_PWM_MAX_DUTY;
>  
> -	/* Note that "disabled" and "duty cycle == 0" are treated the same */
> -	state->duty_cycle = ret;
> +	/*
> +	 * Note that "disabled" and "duty cycle == 0" are treated the same. If
> +	 * the cached duty cycle is not zero, used the cached duty cycle. This
> +	 * ensures that the configured duty cycle is kept across a disable and
> +	 * enable operation and avoids potentially confusing consumers.
> +	 *
> +	 * For the case of the initial hardware readout, channel->duty_cycle
> +	 * will be 0 and the actual duty cycle read from the EC is used.
> +	 */
> +	if (ret == 0 && channel->duty_cycle > 0)
> +		state->duty_cycle = channel->duty_cycle;
> +	else
> +		state->duty_cycle = ret;
>  }
>  
>  static struct pwm_device *
> @@ -149,6 +197,8 @@ cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
>  }
>  
>  static const struct pwm_ops cros_ec_pwm_ops = {
> +	.request = cros_ec_pwm_request,
> +	.free = cros_ec_pwm_free,

nit: Align using tabs for readability.

>  	.get_state	= cros_ec_pwm_get_state,
>  	.apply		= cros_ec_pwm_apply,
>  	.owner		= THIS_MODULE,
> 

Re: [PATCH 4/4] pwm: imx27: Unconditionally write state to hardware

[Michal Vokáč]

+Adam

On 21. 10. 19 12:57, Thierry Reding wrote:
> The i.MX driver currently uses a shortcut and doesn't write all of the
> state through to the hardware when the PWM is disabled. This causes an
> inconsistent state to be read back by consumers with the result of them
> malfunctioning.
> 
> Fix this by always writing the full state through to the hardware
> registers so that the correct state can always be read back.

Gave it another shot and got expected results.

Tested-by: Michal Vokáč <michal.vokac@ysoft.com>

> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> ---
>   drivers/pwm/pwm-imx27.c | 120 ++++++++++++++++++++--------------------
>   1 file changed, 59 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 4113d5cd4c62..59d8b1289808 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -230,70 +230,68 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>   
>   	pwm_get_state(pwm, &cstate);
>   
> -	if (state->enabled) {
> -		c = clk_get_rate(imx->clk_per);
> -		c *= state->period;
> -
> -		do_div(c, 1000000000);
> -		period_cycles = c;
> -
> -		prescale = period_cycles / 0x10000 + 1;
> -
> -		period_cycles /= prescale;
> -		c = (unsigned long long)period_cycles * state->duty_cycle;
> -		do_div(c, state->period);
> -		duty_cycles = c;
> -
> -		/*
> -		 * according to imx pwm RM, the real period value should be
> -		 * PERIOD value in PWMPR plus 2.
> -		 */
> -		if (period_cycles > 2)
> -			period_cycles -= 2;
> -		else
> -			period_cycles = 0;
> -
> -		/*
> -		 * Wait for a free FIFO slot if the PWM is already enabled, and
> -		 * flush the FIFO if the PWM was disabled and is about to be
> -		 * enabled.
> -		 */
> -		if (cstate.enabled) {
> -			pwm_imx27_wait_fifo_slot(chip, pwm);
> -		} else {
> -			ret = pwm_imx27_clk_prepare_enable(chip);
> -			if (ret)
> -				return ret;
> -
> -			pwm_imx27_sw_reset(chip);
> -		}
> -
> -		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> -		writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> -
> -		/*
> -		 * Store the duty cycle for future reference in cases where
> -		 * the MX3_PWMSAR register can't be read (i.e. when the PWM
> -		 * is disabled).
> -		 */
> -		imx->duty_cycle = duty_cycles;
> -
> -		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 |= FIELD_PREP(MX3_PWMCR_POUTC,
> -					MX3_PWMCR_POUTC_INVERTED);
> -
> -		writel(cr, imx->mmio_base + MX3_PWMCR);
> -	} else if (cstate.enabled) {
> -		writel(0, imx->mmio_base + MX3_PWMCR);
> +	c = clk_get_rate(imx->clk_per);
> +	c *= state->period;
>   
> -		pwm_imx27_clk_disable_unprepare(chip);
> +	do_div(c, 1000000000);
> +	period_cycles = c;
> +
> +	prescale = period_cycles / 0x10000 + 1;
> +
> +	period_cycles /= prescale;
> +	c = (unsigned long long)period_cycles * state->duty_cycle;
> +	do_div(c, state->period);
> +	duty_cycles = c;
> +
> +	/*
> +	 * according to imx pwm RM, the real period value should be PERIOD
> +	 * value in PWMPR plus 2.
> +	 */
> +	if (period_cycles > 2)
> +		period_cycles -= 2;
> +	else
> +		period_cycles = 0;
> +
> +	/*
> +	 * Wait for a free FIFO slot if the PWM is already enabled, and flush
> +	 * the FIFO if the PWM was disabled and is about to be enabled.
> +	 */
> +	if (cstate.enabled) {
> +		pwm_imx27_wait_fifo_slot(chip, pwm);
> +	} else {
> +		ret = pwm_imx27_clk_prepare_enable(chip);
> +		if (ret)
> +			return ret;
> +
> +		pwm_imx27_sw_reset(chip);
>   	}
>   
> +	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> +	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> +
> +	/*
> +	 * Store the duty cycle for future reference in cases where the
> +	 * MX3_PWMSAR register can't be read (i.e. when the PWM is disabled).
> +	 */
> +	imx->duty_cycle = duty_cycles;
> +
> +	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;
> +
> +	if (state->polarity == PWM_POLARITY_INVERSED)
> +		cr |= FIELD_PREP(MX3_PWMCR_POUTC,
> +				MX3_PWMCR_POUTC_INVERTED);
> +
> +	if (state->enabled)
> +		cr |= MX3_PWMCR_EN;
> +
> +	writel(cr, imx->mmio_base + MX3_PWMCR);
> +
> +	if (!state->enabled && cstate.enabled)
> +		pwm_imx27_clk_disable_unprepare(chip);
> +
>   	return 0;
>   }
>   
> 

Re: [PATCH 3/4] pwm: imx27: Cache duty cycle register value

[Adam Ford]

On Mon, Oct 21, 2019 at 8:46 AM Michal Vokáč <michal.vokac@ysoft.com> wrote:
>
> +Adam
>
> On 21. 10. 19 12:57, Thierry Reding wrote:
> > The hardware register containing the duty cycle value cannot be accessed
> > when the PWM is disabled. This causes the ->get_state() callback to read
> > back a duty cycle value of 0, which can confuse consumer drivers.
>
> Me and Adam Ford already tested the patches [3/4] and [4/4] and gave ours
> Tested-by tags in the previous thread but do not see those here.
> I re-tested these again and have no issues.
>
> Tested-by: Michal Vokáč <michal.vokac@ysoft.com>

Tested-by: Adam Ford <aford173@gmail.com>

>
> Thank you Thierry,
> Michal
>
>
> > Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> > ---
> >   drivers/pwm/pwm-imx27.c | 31 ++++++++++++++++++++++++-------
> >   1 file changed, 24 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> > index ae11d8577f18..4113d5cd4c62 100644
> > --- a/drivers/pwm/pwm-imx27.c
> > +++ b/drivers/pwm/pwm-imx27.c
> > @@ -85,6 +85,13 @@ struct pwm_imx27_chip {
> >       struct clk      *clk_per;
> >       void __iomem    *mmio_base;
> >       struct pwm_chip chip;
> > +
> > +     /*
> > +      * The driver cannot read the current duty cycle from the hardware if
> > +      * the hardware is disabled. Cache the last programmed duty cycle
> > +      * value to return in that case.
> > +      */
> > +     unsigned int duty_cycle;
> >   };
> >
> >   #define to_pwm_imx27_chip(chip)     container_of(chip, struct pwm_imx27_chip, chip)
> > @@ -155,14 +162,17 @@ static void pwm_imx27_get_state(struct pwm_chip *chip,
> >       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) {
> > +     /*
> > +      * PWMSAR can be read only if PWM is enabled. If the PWM is disabled,
> > +      * use the cached value.
> > +      */
> > +     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;
> > -     }
> > +     else
> > +             val = imx->duty_cycle;
> > +
> > +     tmp = NSEC_PER_SEC * (u64)(val);
> > +     state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
> >
> >       if (!state->enabled)
> >               pwm_imx27_clk_disable_unprepare(chip);
> > @@ -261,6 +271,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >               writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> >               writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> >
> > +             /*
> > +              * Store the duty cycle for future reference in cases where
> > +              * the MX3_PWMSAR register can't be read (i.e. when the PWM
> > +              * is disabled).
> > +              */
> > +             imx->duty_cycle = duty_cycles;
> > +
> >               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) |
> >
>

Re: [PATCH 4/4] pwm: imx27: Unconditionally write state to hardware

[Adam Ford]

On Mon, Oct 21, 2019 at 8:49 AM Michal Vokáč <michal.vokac@ysoft.com> wrote:
>
> +Adam
>
> On 21. 10. 19 12:57, Thierry Reding wrote:
> > The i.MX driver currently uses a shortcut and doesn't write all of the
> > state through to the hardware when the PWM is disabled. This causes an
> > inconsistent state to be read back by consumers with the result of them
> > malfunctioning.
> >
> > Fix this by always writing the full state through to the hardware
> > registers so that the correct state can always be read back.
>
> Gave it another shot and got expected results.
>
> Tested-by: Michal Vokáč <michal.vokac@ysoft.com>

Tested-by: Adam Ford <aford173@gmail.com>

>
> > Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> > ---
> >   drivers/pwm/pwm-imx27.c | 120 ++++++++++++++++++++--------------------
> >   1 file changed, 59 insertions(+), 61 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> > index 4113d5cd4c62..59d8b1289808 100644
> > --- a/drivers/pwm/pwm-imx27.c
> > +++ b/drivers/pwm/pwm-imx27.c
> > @@ -230,70 +230,68 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >
> >       pwm_get_state(pwm, &cstate);
> >
> > -     if (state->enabled) {
> > -             c = clk_get_rate(imx->clk_per);
> > -             c *= state->period;
> > -
> > -             do_div(c, 1000000000);
> > -             period_cycles = c;
> > -
> > -             prescale = period_cycles / 0x10000 + 1;
> > -
> > -             period_cycles /= prescale;
> > -             c = (unsigned long long)period_cycles * state->duty_cycle;
> > -             do_div(c, state->period);
> > -             duty_cycles = c;
> > -
> > -             /*
> > -              * according to imx pwm RM, the real period value should be
> > -              * PERIOD value in PWMPR plus 2.
> > -              */
> > -             if (period_cycles > 2)
> > -                     period_cycles -= 2;
> > -             else
> > -                     period_cycles = 0;
> > -
> > -             /*
> > -              * Wait for a free FIFO slot if the PWM is already enabled, and
> > -              * flush the FIFO if the PWM was disabled and is about to be
> > -              * enabled.
> > -              */
> > -             if (cstate.enabled) {
> > -                     pwm_imx27_wait_fifo_slot(chip, pwm);
> > -             } else {
> > -                     ret = pwm_imx27_clk_prepare_enable(chip);
> > -                     if (ret)
> > -                             return ret;
> > -
> > -                     pwm_imx27_sw_reset(chip);
> > -             }
> > -
> > -             writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > -             writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> > -
> > -             /*
> > -              * Store the duty cycle for future reference in cases where
> > -              * the MX3_PWMSAR register can't be read (i.e. when the PWM
> > -              * is disabled).
> > -              */
> > -             imx->duty_cycle = duty_cycles;
> > -
> > -             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 |= FIELD_PREP(MX3_PWMCR_POUTC,
> > -                                     MX3_PWMCR_POUTC_INVERTED);
> > -
> > -             writel(cr, imx->mmio_base + MX3_PWMCR);
> > -     } else if (cstate.enabled) {
> > -             writel(0, imx->mmio_base + MX3_PWMCR);
> > +     c = clk_get_rate(imx->clk_per);
> > +     c *= state->period;
> >
> > -             pwm_imx27_clk_disable_unprepare(chip);
> > +     do_div(c, 1000000000);
> > +     period_cycles = c;
> > +
> > +     prescale = period_cycles / 0x10000 + 1;
> > +
> > +     period_cycles /= prescale;
> > +     c = (unsigned long long)period_cycles * state->duty_cycle;
> > +     do_div(c, state->period);
> > +     duty_cycles = c;
> > +
> > +     /*
> > +      * according to imx pwm RM, the real period value should be PERIOD
> > +      * value in PWMPR plus 2.
> > +      */
> > +     if (period_cycles > 2)
> > +             period_cycles -= 2;
> > +     else
> > +             period_cycles = 0;
> > +
> > +     /*
> > +      * Wait for a free FIFO slot if the PWM is already enabled, and flush
> > +      * the FIFO if the PWM was disabled and is about to be enabled.
> > +      */
> > +     if (cstate.enabled) {
> > +             pwm_imx27_wait_fifo_slot(chip, pwm);
> > +     } else {
> > +             ret = pwm_imx27_clk_prepare_enable(chip);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             pwm_imx27_sw_reset(chip);
> >       }
> >
> > +     writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > +     writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> > +
> > +     /*
> > +      * Store the duty cycle for future reference in cases where the
> > +      * MX3_PWMSAR register can't be read (i.e. when the PWM is disabled).
> > +      */
> > +     imx->duty_cycle = duty_cycles;
> > +
> > +     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;
> > +
> > +     if (state->polarity == PWM_POLARITY_INVERSED)
> > +             cr |= FIELD_PREP(MX3_PWMCR_POUTC,
> > +                             MX3_PWMCR_POUTC_INVERTED);
> > +
> > +     if (state->enabled)
> > +             cr |= MX3_PWMCR_EN;
> > +
> > +     writel(cr, imx->mmio_base + MX3_PWMCR);
> > +
> > +     if (!state->enabled && cstate.enabled)
> > +             pwm_imx27_clk_disable_unprepare(chip);
> > +
> >       return 0;
> >   }
> >
> >
>

Re: [PATCH 1/4] pwm: Read initial hardware state at request time

[Thierry Reding]

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

On Mon, Oct 21, 2019 at 01:11:12PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Mon, Oct 21, 2019 at 12:57:36PM +0200, Thierry Reding wrote:
> > The PWM core doesn't need to know about the hardware state of a PWM
> > unless there is a user for it. Defer initial hardware readout until
> > a PWM is requested.
> 
> A side effect is that for an unused PWM the get_state callback is never
> called (which is good), in return it is called more than once if the PWM
> is requested more often (which is bearable).

You can't request a PWM more than once. PWMs are always exclusive to a
single driver. Now I suppose you could have a single driver request it
multiple times (that driver would then also have to release it before
requesting it again), but I think it's reasonable for the subsystem to
query the hardware state everytime before a PWM is handed to a consumer.
The hardware state could have changed between the time where a consumer
releases the PWM and another requests it.

> 
> > As a side-effect, this allows the ->get_state() callback to rely on
> > per-PWM data.
> 
> Given that this is the motivation for your change I'd give more stress
> to this part of the commit log. Also I think this could be more
> understandable if you point out that the effect is that .get_state is
> only called after .request was called successfully which gives the low
> level driver more freedom by (for example) relying on memory allocated
> there.

Isn't that pretty much already in the above commit message just with
different words? I can try to reword this in a different way if that
makes you happier.

> I assume you target the next merge window for this change?

Yes. I'm not sure yet about the remainder of the series. Depending on
what we decide to do about drivers that can't (or don't want to) write
all state through to the hardware, patches 2-4 may become moot.

Thierry

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

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

Re: [PATCH 2/4] pwm: cros-ec: Cache duty cycle value

[Uwe Kleine-König]

Hello Enric,

On Mon, Oct 21, 2019 at 03:48:59PM +0200, Enric Balletbo i Serra wrote:
> On 21/10/19 12:57, Thierry Reding wrote:
> >  static const struct pwm_ops cros_ec_pwm_ops = {
> > +	.request = cros_ec_pwm_request,
> > +	.free = cros_ec_pwm_free,
> 
> nit: Align using tabs for readability.

My personal opinion here is that not aligning is saner in the long run.
For me at least it doesn't disturb readability, and once you have

	.request	= cros_ec_pwm_request,
	.free		= cros_ec_pwm_free,
	.get_state	= cros_ec_pwm_get_state,
	.apply		= cros_ec_pwm_apply,
	.owner		= THIS_MODULE,

and want to set a new member with a long name, fixing the unrelated
lines adds churn and not fixing them looks as ugly as it does with mixed
styling. So I prefer to go with "<space>=<space>" from the start.

> >  	.get_state	= cros_ec_pwm_get_state,
> >  	.apply		= cros_ec_pwm_apply,
> >  	.owner		= THIS_MODULE,

But given that the already existing members are already using some
indention following this style seems right.

@Thierry: You didn't pick up this in your pull request. Should it stay
as it is now with the mixed style to not add churn, or should we fix to
something uniform?

Best regards
Uwe

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