[PATCH v3 0/6] pwm: ensure pwm_apply_state() doesn't modify the state argument

[PATCH v3 0/6] pwm: ensure pwm_apply_state() doesn't modify the state argument

[Uwe Kleine-König]

Hello,

this series eventually changes the prototype of pwm_apply_state to take
a const struct pwm_state *, see the last patch for a rationale.

Changes since v2 apart from rebasing and so covering a few more drivers
is mainly addressing the concern that after state was rounded and
applied at least pwm_get_state should return the rounded values. See
patch 2.

Uwe Kleine-König (6):
  pwm: introduce local struct pwm_chip in pwm_apply_state()
  pwm: let pwm_get_state() return the last implemented state
  pwm: rockchip: Don't update the state for the caller of
    pwm_apply_state()
  pwm: sun4i: Don't update the state for the caller of pwm_apply_state()
  pwm: fsl-ftm: Don't update the state for the caller of
    pwm_apply_state()
  pwm: ensure pwm_apply_state() doesn't modify the state argument

 drivers/pwm/core.c            | 39 +++++++++++++++++++++--------------
 drivers/pwm/pwm-atmel-hlcdc.c |  2 +-
 drivers/pwm/pwm-atmel.c       |  2 +-
 drivers/pwm/pwm-bcm-iproc.c   |  2 +-
 drivers/pwm/pwm-cros-ec.c     |  2 +-
 drivers/pwm/pwm-fsl-ftm.c     |  8 ++-----
 drivers/pwm/pwm-hibvt.c       |  2 +-
 drivers/pwm/pwm-imx-tpm.c     |  4 ++--
 drivers/pwm/pwm-imx27.c       |  2 +-
 drivers/pwm/pwm-jz4740.c      |  2 +-
 drivers/pwm/pwm-lpss.c        |  2 +-
 drivers/pwm/pwm-meson.c       |  4 ++--
 drivers/pwm/pwm-rcar.c        |  2 +-
 drivers/pwm/pwm-rockchip.c    | 10 ++-------
 drivers/pwm/pwm-sifive.c      |  2 +-
 drivers/pwm/pwm-stm32-lp.c    |  2 +-
 drivers/pwm/pwm-stm32.c       |  4 ++--
 drivers/pwm/pwm-sun4i.c       | 10 ++-------
 drivers/pwm/pwm-zx.c          |  2 +-
 include/linux/pwm.h           |  4 ++--
 20 files changed, 49 insertions(+), 58 deletions(-)

-- 
2.20.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v3 1/6] pwm: introduce local struct pwm_chip in pwm_apply_state()

[Uwe Kleine-König]

pwm->chip is dereferenced several times in the pwm_apply_state()
function. Introducing a local variable for it helps keeping some lines a
bit shorter.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/pwm/core.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 8edfac17364e..72347ca4a647 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -455,6 +455,7 @@ EXPORT_SYMBOL_GPL(pwm_free);
 int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
 {
 	int err;
+	struct pwm_chip *chip;
 
 	if (!pwm || !state || !state->period ||
 	    state->duty_cycle > state->period)
@@ -466,8 +467,9 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
 	    state->enabled == pwm->state.enabled)
 		return 0;
 
-	if (pwm->chip->ops->apply) {
-		err = pwm->chip->ops->apply(pwm->chip, pwm, state);
+	chip = pwm->chip;
+	if (chip->ops->apply) {
+		err = chip->ops->apply(chip, pwm, state);
 		if (err)
 			return err;
 
@@ -477,7 +479,7 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
 		 * FIXME: restore the initial state in case of error.
 		 */
 		if (state->polarity != pwm->state.polarity) {
-			if (!pwm->chip->ops->set_polarity)
+			if (!chip->ops->set_polarity)
 				return -ENOTSUPP;
 
 			/*
@@ -486,12 +488,12 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
 			 * ->apply().
 			 */
 			if (pwm->state.enabled) {
-				pwm->chip->ops->disable(pwm->chip, pwm);
+				chip->ops->disable(chip, pwm);
 				pwm->state.enabled = false;
 			}
 
-			err = pwm->chip->ops->set_polarity(pwm->chip, pwm,
-							   state->polarity);
+			err = chip->ops->set_polarity(chip, pwm,
+						      state->polarity);
 			if (err)
 				return err;
 
@@ -500,9 +502,9 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
 
 		if (state->period != pwm->state.period ||
 		    state->duty_cycle != pwm->state.duty_cycle) {
-			err = pwm->chip->ops->config(pwm->chip, pwm,
-						     state->duty_cycle,
-						     state->period);
+			err = chip->ops->config(pwm->chip, pwm,
+						state->duty_cycle,
+						state->period);
 			if (err)
 				return err;
 
@@ -512,11 +514,11 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
 
 		if (state->enabled != pwm->state.enabled) {
 			if (state->enabled) {
-				err = pwm->chip->ops->enable(pwm->chip, pwm);
+				err = chip->ops->enable(chip, pwm);
 				if (err)
 					return err;
 			} else {
-				pwm->chip->ops->disable(pwm->chip, pwm);
+				chip->ops->disable(chip, pwm);
 			}
 
 			pwm->state.enabled = state->enabled;
-- 
2.20.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v3 2/6] pwm: let pwm_get_state() return the last implemented state

[Uwe Kleine-König]

When pwm_apply_state() is called the lowlevel driver usually has to
apply some rounding because the hardware doesn't support nanosecond
resolution. So let pwm_get_state() return the actually implemented state
instead of the last applied one if possible.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/pwm/core.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 72347ca4a647..92333b89bf02 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -473,7 +473,14 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
 		if (err)
 			return err;
 
-		pwm->state = *state;
+		/*
+		 * .apply might have to round some values in *state, if possible
+		 * read the actually implemented value back.
+		 */
+		if (chip->ops->get_state)
+			chip->ops->get_state(chip, pwm, &pwm->state);
+		else
+			pwm->state = *state;
 	} else {
 		/*
 		 * FIXME: restore the initial state in case of error.
-- 
2.20.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v3 3/6] pwm: rockchip: Don't update the state for the caller of pwm_apply_state()

[Uwe Kleine-König]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

The pwm-rockchip driver is one of only three PWM drivers which updates
the state for the caller of pwm_apply_state(). This might have
surprising results if the caller reuses the values expecting them to
still represent the same state.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-rockchip.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 51b96cb7dd25..cc502c8d7e9c 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -212,12 +212,6 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			goto out;
 	}
 
-	/*
-	 * Update the state with the real hardware, which can differ a bit
-	 * because of period/duty_cycle approximation.
-	 */
-	rockchip_pwm_get_state(chip, pwm, state);
-
 out:
 	clk_disable(pc->pclk);
 
-- 
2.20.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v3 4/6] pwm: sun4i: Don't update the state for the caller of pwm_apply_state()

[Uwe Kleine-König]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

The pwm-sun4i driver is one of only three PWM drivers which updates the
state for the caller of pwm_apply_state(). This might have surprising
results if the caller reuses the values expecting them to still
represent the same state.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-sun4i.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index de78c824bbfd..39007a7c0d83 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -192,12 +192,6 @@ static int sun4i_pwm_calculate(struct sun4i_pwm_chip *sun4i_pwm,
 	*dty = div;
 	*prsclr = prescaler;
 
-	div = (u64)pval * NSEC_PER_SEC * *prd;
-	state->period = DIV_ROUND_CLOSEST_ULL(div, clk_rate);
-
-	div = (u64)pval * NSEC_PER_SEC * *dty;
-	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(div, clk_rate);
-
 	return 0;
 }
 
-- 
2.20.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v3 5/6] pwm: fsl-ftm: Don't update the state for the caller of pwm_apply_state()

[Uwe Kleine-König]

The pwm-fsl-ftm driver is one of only three PWM drivers which updates
the state for the caller of pwm_apply_state(). This might have
surprising results if the caller reuses the values expecting them to
still represent the same state.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/pwm/pwm-fsl-ftm.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
index 9d31a217111d..3c9738617ceb 100644
--- a/drivers/pwm/pwm-fsl-ftm.c
+++ b/drivers/pwm/pwm-fsl-ftm.c
@@ -292,10 +292,6 @@ static int fsl_pwm_apply_config(struct fsl_pwm_chip *fpc,
 
 	regmap_update_bits(fpc->regmap, FTM_POL, BIT(pwm->hwpwm), reg_polarity);
 
-	newstate->period = fsl_pwm_ticks_to_ns(fpc,
-					       fpc->period.mod_period + 1);
-	newstate->duty_cycle = fsl_pwm_ticks_to_ns(fpc, duty);
-
 	ftm_set_write_protection(fpc);
 
 	return 0;
-- 
2.20.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v3 6/6] pwm: ensure pwm_apply_state() doesn't modify the state argument

[Uwe Kleine-König]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

It is surprising for a PWM consumer when the variable holding the
requested state is modified by pwm_apply_state(). Consider for example a
driver doing:

        #define PERIOD 5000000
        #define DUTY_LITTLE 10
        ...
        struct pwm_state state = {
                .period = PERIOD,
                .duty_cycle = DUTY_LITTLE,
                .polarity = PWM_POLARITY_NORMAL,
                .enabled = true,
        };

        pwm_apply_state(mypwm, &state);
        ...
        state.duty_cycle = PERIOD / 2;
        pwm_apply_state(mypwm, &state);

For sure the second call to pwm_apply_state() should still have
state.period = PERIOD and not something the hardware driver chose for a
reason that doesn't necessarily apply to the second call.

So declare the state argument as a pointer to a const type and adapt all
drivers' .apply callbacks.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/core.c            | 6 ++----
 drivers/pwm/pwm-atmel-hlcdc.c | 2 +-
 drivers/pwm/pwm-atmel.c       | 2 +-
 drivers/pwm/pwm-bcm-iproc.c   | 2 +-
 drivers/pwm/pwm-cros-ec.c     | 2 +-
 drivers/pwm/pwm-fsl-ftm.c     | 4 ++--
 drivers/pwm/pwm-hibvt.c       | 2 +-
 drivers/pwm/pwm-imx-tpm.c     | 4 ++--
 drivers/pwm/pwm-imx27.c       | 2 +-
 drivers/pwm/pwm-jz4740.c      | 2 +-
 drivers/pwm/pwm-lpss.c        | 2 +-
 drivers/pwm/pwm-meson.c       | 4 ++--
 drivers/pwm/pwm-rcar.c        | 2 +-
 drivers/pwm/pwm-rockchip.c    | 4 ++--
 drivers/pwm/pwm-sifive.c      | 2 +-
 drivers/pwm/pwm-stm32-lp.c    | 2 +-
 drivers/pwm/pwm-stm32.c       | 4 ++--
 drivers/pwm/pwm-sun4i.c       | 4 ++--
 drivers/pwm/pwm-zx.c          | 2 +-
 include/linux/pwm.h           | 4 ++--
 20 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 92333b89bf02..95f0d0054910 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -448,11 +448,9 @@ EXPORT_SYMBOL_GPL(pwm_free);
 /**
  * pwm_apply_state() - atomically apply a new state to a PWM device
  * @pwm: PWM device
- * @state: new state to apply. This can be adjusted by the PWM driver
- *	   if the requested config is not achievable, for example,
- *	   ->duty_cycle and ->period might be approximated.
+ * @state: new state to apply.
  */
-int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
+int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
 {
 	int err;
 	struct pwm_chip *chip;
diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
index d13a83f430ac..dcbc0489dfd4 100644
--- a/drivers/pwm/pwm-atmel-hlcdc.c
+++ b/drivers/pwm/pwm-atmel-hlcdc.c
@@ -39,7 +39,7 @@ static inline struct atmel_hlcdc_pwm *to_atmel_hlcdc_pwm(struct pwm_chip *chip)
 }
 
 static int atmel_hlcdc_pwm_apply(struct pwm_chip *c, struct pwm_device *pwm,
-				 struct pwm_state *state)
+				 const struct pwm_state *state)
 {
 	struct atmel_hlcdc_pwm *chip = to_atmel_hlcdc_pwm(c);
 	struct atmel_hlcdc *hlcdc = chip->hlcdc;
diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index e5e1eaf372fa..53bc7b9b3581 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -209,7 +209,7 @@ static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm,
 }
 
 static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			   struct pwm_state *state)
+			   const struct pwm_state *state)
 {
 	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
 	struct pwm_state cstate;
diff --git a/drivers/pwm/pwm-bcm-iproc.c b/drivers/pwm/pwm-bcm-iproc.c
index d961a8207b1c..56c38cfae92c 100644
--- a/drivers/pwm/pwm-bcm-iproc.c
+++ b/drivers/pwm/pwm-bcm-iproc.c
@@ -115,7 +115,7 @@ static void iproc_pwmc_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 }
 
 static int iproc_pwmc_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			    struct pwm_state *state)
+			    const struct pwm_state *state)
 {
 	unsigned long prescale = IPROC_PWM_PRESCALE_MIN;
 	struct iproc_pwmc *ip = to_iproc_pwmc(chip);
diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
index 98f6ac6cf6ab..db5faa79c33f 100644
--- a/drivers/pwm/pwm-cros-ec.c
+++ b/drivers/pwm/pwm-cros-ec.c
@@ -93,7 +93,7 @@ static int cros_ec_pwm_get_duty(struct cros_ec_device *ec, u8 index)
 }
 
 static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			     struct pwm_state *state)
+			     const struct pwm_state *state)
 {
 	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
 	int duty_cycle;
diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
index 3c9738617ceb..59272a920479 100644
--- a/drivers/pwm/pwm-fsl-ftm.c
+++ b/drivers/pwm/pwm-fsl-ftm.c
@@ -227,7 +227,7 @@ static bool fsl_pwm_is_other_pwm_enabled(struct fsl_pwm_chip *fpc,
 
 static int fsl_pwm_apply_config(struct fsl_pwm_chip *fpc,
 				struct pwm_device *pwm,
-				struct pwm_state *newstate)
+				const struct pwm_state *newstate)
 {
 	unsigned int duty;
 	u32 reg_polarity;
@@ -298,7 +298,7 @@ static int fsl_pwm_apply_config(struct fsl_pwm_chip *fpc,
 }
 
 static int fsl_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			 struct pwm_state *newstate)
+			 const struct pwm_state *newstate)
 {
 	struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
 	struct pwm_state *oldstate = &pwm->state;
diff --git a/drivers/pwm/pwm-hibvt.c b/drivers/pwm/pwm-hibvt.c
index 753bd58111e4..ad205fdad372 100644
--- a/drivers/pwm/pwm-hibvt.c
+++ b/drivers/pwm/pwm-hibvt.c
@@ -149,7 +149,7 @@ static void hibvt_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 }
 
 static int hibvt_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-				struct pwm_state *state)
+			   const struct pwm_state *state)
 {
 	struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
 
diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
index e8385c1cf342..9145f6160649 100644
--- a/drivers/pwm/pwm-imx-tpm.c
+++ b/drivers/pwm/pwm-imx-tpm.c
@@ -89,7 +89,7 @@ to_imx_tpm_pwm_chip(struct pwm_chip *chip)
 static int pwm_imx_tpm_round_state(struct pwm_chip *chip,
 				   struct imx_tpm_pwm_param *p,
 				   struct pwm_state *real_state,
-				   struct pwm_state *state)
+				   const struct pwm_state *state)
 {
 	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
 	u32 rate, prescale, period_count, clock_unit;
@@ -289,7 +289,7 @@ static int pwm_imx_tpm_apply_hw(struct pwm_chip *chip,
 
 static int pwm_imx_tpm_apply(struct pwm_chip *chip,
 			     struct pwm_device *pwm,
-			     struct pwm_state *state)
+			     const struct pwm_state *state)
 {
 	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
 	struct imx_tpm_pwm_param param;
diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 434a351fb626..aa456de2f98d 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -205,7 +205,7 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
 }
 
 static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			   struct pwm_state *state)
+			   const struct pwm_state *state)
 {
 	unsigned long period_cycles, duty_cycles, prescale;
 	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index f901e8a0d33d..f57e60da5c61 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -83,7 +83,7 @@ static void jz4740_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 }
 
 static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			    struct pwm_state *state)
+			    const struct pwm_state *state)
 {
 	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
 	unsigned long long tmp;
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 4098a4601691..75bbfe5f3bc2 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -122,7 +122,7 @@ static inline void pwm_lpss_cond_enable(struct pwm_device *pwm, bool cond)
 }
 
 static int pwm_lpss_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			  struct pwm_state *state)
+			  const struct pwm_state *state)
 {
 	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
 	int ret;
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 3cbff5cbb789..6245bbdb6e6c 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -159,7 +159,7 @@ static void meson_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 }
 
 static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
-			  struct pwm_state *state)
+			  const struct pwm_state *state)
 {
 	struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
 	unsigned int duty, period, pre_div, cnt, duty_cnt;
@@ -265,7 +265,7 @@ static void meson_pwm_disable(struct meson_pwm *meson, struct pwm_device *pwm)
 }
 
 static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			   struct pwm_state *state)
+			   const struct pwm_state *state)
 {
 	struct meson_pwm_channel *channel = pwm_get_chip_data(pwm);
 	struct meson_pwm *meson = to_meson_pwm(chip);
diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
index 5b2b8ecc354c..a77fbf29114e 100644
--- a/drivers/pwm/pwm-rcar.c
+++ b/drivers/pwm/pwm-rcar.c
@@ -158,7 +158,7 @@ static void rcar_pwm_disable(struct rcar_pwm_chip *rp)
 }
 
 static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			  struct pwm_state *state)
+			  const struct pwm_state *state)
 {
 	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
 	struct pwm_state cur_state;
diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index cc502c8d7e9c..45330d7b0f33 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -99,7 +99,7 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip,
 }
 
 static void rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			       struct pwm_state *state)
+			       const struct pwm_state *state)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
 	unsigned long period, duty;
@@ -183,7 +183,7 @@ static int rockchip_pwm_enable(struct pwm_chip *chip,
 }
 
 static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			      struct pwm_state *state)
+			      const struct pwm_state *state)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
 	struct pwm_state curstate;
diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
index a7c107f19e66..6ae580ef5c6e 100644
--- a/drivers/pwm/pwm-sifive.c
+++ b/drivers/pwm/pwm-sifive.c
@@ -147,7 +147,7 @@ static int pwm_sifive_enable(struct pwm_chip *chip, bool enable)
 }
 
 static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			    struct pwm_state *state)
+			    const struct pwm_state *state)
 {
 	struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
 	struct pwm_state cur_state;
diff --git a/drivers/pwm/pwm-stm32-lp.c b/drivers/pwm/pwm-stm32-lp.c
index 2211a642066d..21cb260dc2c0 100644
--- a/drivers/pwm/pwm-stm32-lp.c
+++ b/drivers/pwm/pwm-stm32-lp.c
@@ -32,7 +32,7 @@ static inline struct stm32_pwm_lp *to_stm32_pwm_lp(struct pwm_chip *chip)
 #define STM32_LPTIM_MAX_PRESCALER	128
 
 static int stm32_pwm_lp_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			      struct pwm_state *state)
+			      const struct pwm_state *state)
 {
 	struct stm32_pwm_lp *priv = to_stm32_pwm_lp(chip);
 	unsigned long long prd, div, dty;
diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 740e2dec8313..359b08596d9e 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -440,7 +440,7 @@ static void stm32_pwm_disable(struct stm32_pwm *priv, int ch)
 }
 
 static int stm32_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			   struct pwm_state *state)
+			   const struct pwm_state *state)
 {
 	bool enabled;
 	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
@@ -468,7 +468,7 @@ static int stm32_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 }
 
 static int stm32_pwm_apply_locked(struct pwm_chip *chip, struct pwm_device *pwm,
-				  struct pwm_state *state)
+				  const struct pwm_state *state)
 {
 	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
 	int ret;
diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 39007a7c0d83..6f5840a1a82d 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -145,7 +145,7 @@ static void sun4i_pwm_get_state(struct pwm_chip *chip,
 }
 
 static int sun4i_pwm_calculate(struct sun4i_pwm_chip *sun4i_pwm,
-			       struct pwm_state *state,
+			       const struct pwm_state *state,
 			       u32 *dty, u32 *prd, unsigned int *prsclr)
 {
 	u64 clk_rate, div = 0;
@@ -196,7 +196,7 @@ static int sun4i_pwm_calculate(struct sun4i_pwm_chip *sun4i_pwm,
 }
 
 static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			   struct pwm_state *state)
+			   const struct pwm_state *state)
 {
 	struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
 	struct pwm_state cstate;
diff --git a/drivers/pwm/pwm-zx.c b/drivers/pwm/pwm-zx.c
index e24f4be35316..e2c21cc34a96 100644
--- a/drivers/pwm/pwm-zx.c
+++ b/drivers/pwm/pwm-zx.c
@@ -148,7 +148,7 @@ static int zx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 }
 
 static int zx_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			struct pwm_state *state)
+			const struct pwm_state *state)
 {
 	struct zx_pwm_chip *zpc = to_zx_pwm_chip(chip);
 	struct pwm_state cstate;
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 24632a7a7d11..b2c9c460947d 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -262,7 +262,7 @@ struct pwm_ops {
 	int (*capture)(struct pwm_chip *chip, struct pwm_device *pwm,
 		       struct pwm_capture *result, unsigned long timeout);
 	int (*apply)(struct pwm_chip *chip, struct pwm_device *pwm,
-		     struct pwm_state *state);
+		     const struct pwm_state *state);
 	void (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,
 			  struct pwm_state *state);
 	struct module *owner;
@@ -316,7 +316,7 @@ struct pwm_capture {
 /* PWM user APIs */
 struct pwm_device *pwm_request(int pwm_id, const char *label);
 void pwm_free(struct pwm_device *pwm);
-int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state);
+int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);
 int pwm_adjust_config(struct pwm_device *pwm);
 
 /**
-- 
2.20.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v3 5/6] pwm: fsl-ftm: Don't update the state for the caller of pwm_apply_state()

[Doug Anderson]

Hi,

On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>
> The pwm-fsl-ftm driver is one of only three PWM drivers which updates
> the state for the caller of pwm_apply_state(). This might have
> surprising results if the caller reuses the values expecting them to
> still represent the same state.
>
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
>  drivers/pwm/pwm-fsl-ftm.c | 4 ----
>  1 file changed, 4 deletions(-)

Presumably this patch could break something since the pwm-fsl-ftm
driver doesn't appear to implement the get_state() function.  ...or
did I miss it?

-Doug

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v3 2/6] pwm: let pwm_get_state() return the last implemented state

[Heiko Stuebner]

Am Samstag, 24. August 2019, 17:37:03 CEST schrieb Uwe Kleine-König:
> When pwm_apply_state() is called the lowlevel driver usually has to
> apply some rounding because the hardware doesn't support nanosecond
> resolution. So let pwm_get_state() return the actually implemented state
> instead of the last applied one if possible.
> 
> Signed-off-by: Uwe Kleine-König <uwe-rXY34ruvC2xidJT2blvkqNi2O/JbrIOy@public.gmane.org>

With this applied, the display brightness on my rk3399-gru-scarlet gets
inverted. Now it's very bright on level 1 and very dim on the max level.

According to the debugfs, the inverted state changes:

OLD STATE:
----------
root@localhost:~# cat /debug/pwm
platform/ff420030.pwm, 1 PWM device
 pwm-0   (ppvar-bigcpu-pwm    ): requested enabled period: 3334 ns duty: 331 ns polarity: normal

platform/ff420020.pwm, 1 PWM device
 pwm-0   (ppvar-litcpu-pwm    ): requested enabled period: 3334 ns duty: 414 ns polarity: normal

platform/ff420010.pwm, 1 PWM device
 pwm-0   (backlight           ): requested enabled period: 999996 ns duty: 941148 ns polarity: normal

platform/ff420000.pwm, 1 PWM device
 pwm-0   (ppvar-gpu-pwm       ): requested enabled period: 3334 ns duty: 3334 ns polarity: normal

NEW STATE:
----------
root@localhost:~# cat /debug/pwm 
platform/ff420030.pwm, 1 PWM device
 pwm-0   (ppvar-bigcpu-pwm    ): requested enabled period: 3334 ns duty: 331 ns polarity: normal

platform/ff420020.pwm, 1 PWM device
 pwm-0   (ppvar-litcpu-pwm    ): requested enabled period: 3334 ns duty: 414 ns polarity: normal

platform/ff420010.pwm, 1 PWM device
 pwm-0   (backlight           ): requested enabled period: 999996 ns duty: 941148 ns polarity: inverse

platform/ff420000.pwm, 1 PWM device
 pwm-0   (ppvar-gpu-pwm       ): requested enabled period: 3334 ns duty: 3334 ns polarity: normal


And the reason is below.

> ---
>  drivers/pwm/core.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 72347ca4a647..92333b89bf02 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -473,7 +473,14 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
>  		if (err)
>  			return err;
>  
> -		pwm->state = *state;
> +		/*
> +		 * .apply might have to round some values in *state, if possible
> +		 * read the actually implemented value back.
> +		 */
> +		if (chip->ops->get_state)
> +			chip->ops->get_state(chip, pwm, &pwm->state);
> +		else
> +			pwm->state = *state;

This should probably become
>-		pwm->state = *state;
> +
> +		/*
> +		 * .apply might have to round some values in *state, if possible
> +		 * read the actually implemented value back.
> +		 */
> +		if (chip->ops->get_state)
> +			chip->ops->get_state(chip, pwm, &pwm->state);

so always initialize the state to the provided one and then let the driver
override values?

The inversion case stems from the Rockchip pwm driver (wrongly?) only
setting the polarity field when actually inverted, so here the polarity field
probably never actually got set at all.

But while we should probably fix the rockchip driver to set polarity all the
time, this is still being true for possible future state-fields, which also
wouldn't get initialzed from all drivers, which might need an adaption
first?


Heiko


>  	} else {
>  		/*
>  		 * FIXME: restore the initial state in case of error.
> 

Re: [PATCH v3 5/6] pwm: fsl-ftm: Don't update the state for the caller of pwm_apply_state()

[Uwe Kleine-König]

On Fri, Aug 30, 2019 at 10:39:16AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe-rXY34ruvC2xidJT2blvkqNi2O/JbrIOy@public.gmane.org> wrote:
> >
> > The pwm-fsl-ftm driver is one of only three PWM drivers which updates
> > the state for the caller of pwm_apply_state(). This might have
> > surprising results if the caller reuses the values expecting them to
> > still represent the same state.
> >
> > Signed-off-by: Uwe Kleine-König <uwe-rXY34ruvC2xidJT2blvkqNi2O/JbrIOy@public.gmane.org>
> > ---
> >  drivers/pwm/pwm-fsl-ftm.c | 4 ----
> >  1 file changed, 4 deletions(-)
> 
> Presumably this patch could break something since the pwm-fsl-ftm
> driver doesn't appear to implement the get_state() function.  ...or
> did I miss it?

I don't expect breakage. We have more than 50 pwm drivers and only three
of them made use of adapting the passed state. So unless you do
something special with the PWM (i.e. more than backlight, LED or fan
control) I don't think a consumer might care. But it might well be that
I miss something so feel free to prove me wrong.

Best regards
Uwe

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

Re: [PATCH v3 2/6] pwm: let pwm_get_state() return the last implemented state

[Uwe Kleine-König]

On Fri, Aug 30, 2019 at 07:48:35PM +0200, Heiko Stuebner wrote:
> Am Samstag, 24. August 2019, 17:37:03 CEST schrieb Uwe Kleine-König:
> > When pwm_apply_state() is called the lowlevel driver usually has to
> > apply some rounding because the hardware doesn't support nanosecond
> > resolution. So let pwm_get_state() return the actually implemented state
> > instead of the last applied one if possible.
> > 
> > Signed-off-by: Uwe Kleine-König <uwe-rXY34ruvC2xidJT2blvkqNi2O/JbrIOy@public.gmane.org>
> 
> With this applied, the display brightness on my rk3399-gru-scarlet gets
> inverted. Now it's very bright on level 1 and very dim on the max level.
> 
> According to the debugfs, the inverted state changes:
> 
> OLD STATE:
> ----------
> root@localhost:~# cat /debug/pwm
> platform/ff420030.pwm, 1 PWM device
>  pwm-0   (ppvar-bigcpu-pwm    ): requested enabled period: 3334 ns duty: 331 ns polarity: normal
> 
> platform/ff420020.pwm, 1 PWM device
>  pwm-0   (ppvar-litcpu-pwm    ): requested enabled period: 3334 ns duty: 414 ns polarity: normal
> 
> platform/ff420010.pwm, 1 PWM device
>  pwm-0   (backlight           ): requested enabled period: 999996 ns duty: 941148 ns polarity: normal
> 
> platform/ff420000.pwm, 1 PWM device
>  pwm-0   (ppvar-gpu-pwm       ): requested enabled period: 3334 ns duty: 3334 ns polarity: normal
> 
> NEW STATE:
> ----------
> root@localhost:~# cat /debug/pwm 
> platform/ff420030.pwm, 1 PWM device
>  pwm-0   (ppvar-bigcpu-pwm    ): requested enabled period: 3334 ns duty: 331 ns polarity: normal
> 
> platform/ff420020.pwm, 1 PWM device
>  pwm-0   (ppvar-litcpu-pwm    ): requested enabled period: 3334 ns duty: 414 ns polarity: normal
> 
> platform/ff420010.pwm, 1 PWM device
>  pwm-0   (backlight           ): requested enabled period: 999996 ns duty: 941148 ns polarity: inverse
> 
> platform/ff420000.pwm, 1 PWM device
>  pwm-0   (ppvar-gpu-pwm       ): requested enabled period: 3334 ns duty: 3334 ns polarity: normal
> 
> 
> And the reason is below.
> 
> > ---
> >  drivers/pwm/core.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 72347ca4a647..92333b89bf02 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -473,7 +473,14 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
> >  		if (err)
> >  			return err;
> >  
> > -		pwm->state = *state;
> > +		/*
> > +		 * .apply might have to round some values in *state, if possible
> > +		 * read the actually implemented value back.
> > +		 */
> > +		if (chip->ops->get_state)
> > +			chip->ops->get_state(chip, pwm, &pwm->state);
> > +		else
> > +			pwm->state = *state;
> 
> This should probably become
> >-		pwm->state = *state;
> > +
> > +		/*
> > +		 * .apply might have to round some values in *state, if possible
> > +		 * read the actually implemented value back.
> > +		 */
> > +		if (chip->ops->get_state)
> > +			chip->ops->get_state(chip, pwm, &pwm->state);
> 
> so always initialize the state to the provided one and then let the driver
> override values?

This feels wrong. There is another thread that adds a developer knob
that emits some warnings. Catching this kind of error with it would be a
good idea IMHO.

> The inversion case stems from the Rockchip pwm driver (wrongly?) only
> setting the polarity field when actually inverted, so here the polarity field
> probably never actually got set at all.
> 
> But while we should probably fix the rockchip driver to set polarity all the
> time, this is still being true for possible future state-fields, which also
> wouldn't get initialzed from all drivers, which might need an adaption
> first?

Actually I would prefer that all drivers do it right and rely on this.

Thanks for testing
Uwe

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

Re: [PATCH v3 0/6] pwm: ensure pwm_apply_state() doesn't modify the state argument

[Uwe Kleine-König]

On Sat, Aug 24, 2019 at 05:37:01PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> this series eventually changes the prototype of pwm_apply_state to take
> a const struct pwm_state *, see the last patch for a rationale.
> 
> Changes since v2 apart from rebasing and so covering a few more drivers
> is mainly addressing the concern that after state was rounded and
> applied at least pwm_get_state should return the rounded values. See
> patch 2.

I thought a bit on this series and there is a question about it.
Depending on what is considered the right answer for it, this series
might have to change.

The key question is:

  - Is pwm_get_state() supposed to return
    a) the last requested configuration; or
    b) the last actually implemented configuration?

(If the difference is unclear: consider a PWM that can only implement
the following periods:

	1 ms, 2 ms, 3 ms, 4 ms, 5 ms, 6 ms, ...

and a consumer requested 4.2 ms. Should pwm_get_state() return .period = 4
ms (assuming this was picked) or 4.2 ms?)

As of v5.3-rc5 it depends on the backend driver. For most of them 4.2 ms
is returned. My series tries to push it to return 4 ms instead. (But
this only works for backends that implement the .get_state() callback.
And it gets more complicated as the drivers are not free of surprises.)

Maybe we need functions for both answers?

Best regards
Uwe

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

Re: [PATCH v3 2/6] pwm: let pwm_get_state() return the last implemented state

[Uwe Kleine-König]

On Mon, Sep 02, 2019 at 04:32:31PM +0200, Uwe Kleine-König wrote:
> On Fri, Aug 30, 2019 at 07:48:35PM +0200, Heiko Stuebner wrote:
> > Am Samstag, 24. August 2019, 17:37:03 CEST schrieb Uwe Kleine-König:
> > > ---
> > >  drivers/pwm/core.c | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > index 72347ca4a647..92333b89bf02 100644
> > > --- a/drivers/pwm/core.c
> > > +++ b/drivers/pwm/core.c
> > > @@ -473,7 +473,14 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
> > >  		if (err)
> > >  			return err;
> > >  
> > > -		pwm->state = *state;
> > > +		/*
> > > +		 * .apply might have to round some values in *state, if possible
> > > +		 * read the actually implemented value back.
> > > +		 */
> > > +		if (chip->ops->get_state)
> > > +			chip->ops->get_state(chip, pwm, &pwm->state);
> > > +		else
> > > +			pwm->state = *state;
> > 
> > This should probably become
> > >-		pwm->state = *state;
> > > +
> > > +		/*
> > > +		 * .apply might have to round some values in *state, if possible
> > > +		 * read the actually implemented value back.
> > > +		 */
> > > +		if (chip->ops->get_state)
> > > +			chip->ops->get_state(chip, pwm, &pwm->state);
> > 
> > so always initialize the state to the provided one and then let the driver
> > override values?
> 
> This feels wrong. There is another thread that adds a developer knob
> that emits some warnings. Catching this kind of error with it would be a
> good idea IMHO.
> 
> > The inversion case stems from the Rockchip pwm driver (wrongly?) only
> > setting the polarity field when actually inverted, so here the polarity field
> > probably never actually got set at all.
> > 
> > But while we should probably fix the rockchip driver to set polarity all the
> > time, this is still being true for possible future state-fields, which also
> > wouldn't get initialzed from all drivers, which might need an adaption
> > first?
> 
> Actually I would prefer that all drivers do it right and rely on this.

FTR: I sent a patch for the rockchip driver to do the right thing here.

Best regards
Uwe

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

Re: [PATCH v3 5/6] pwm: fsl-ftm: Don't update the state for the caller of pwm_apply_state()

[Doug Anderson]

Hi,

On Mon, Sep 2, 2019 at 7:27 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Fri, Aug 30, 2019 at 10:39:16AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > >
> > > The pwm-fsl-ftm driver is one of only three PWM drivers which updates
> > > the state for the caller of pwm_apply_state(). This might have
> > > surprising results if the caller reuses the values expecting them to
> > > still represent the same state.
> > >
> > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > > ---
> > >  drivers/pwm/pwm-fsl-ftm.c | 4 ----
> > >  1 file changed, 4 deletions(-)
> >
> > Presumably this patch could break something since the pwm-fsl-ftm
> > driver doesn't appear to implement the get_state() function.  ...or
> > did I miss it?
>
> I don't expect breakage. We have more than 50 pwm drivers and only three
> of them made use of adapting the passed state. So unless you do
> something special with the PWM (i.e. more than backlight, LED or fan
> control) I don't think a consumer might care. But it might well be that
> I miss something so feel free to prove me wrong.

I don't have this hardware so I can't prove you wrong.  ...but
presumably someone added the code to return the state on purpose?

Maybe you could implement get_state() for this driver in your series?

-Doug

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v3 5/6] pwm: fsl-ftm: Don't update the state for the caller of pwm_apply_state()

[Uwe Kleine-König]

Hello,

On Tue, Sep 03, 2019 at 09:54:37AM -0700, Doug Anderson wrote:
> On Mon, Sep 2, 2019 at 7:27 AM Uwe Kleine-König
> <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > On Fri, Aug 30, 2019 at 10:39:16AM -0700, Doug Anderson wrote:
> > > On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > > >
> > > > The pwm-fsl-ftm driver is one of only three PWM drivers which updates
> > > > the state for the caller of pwm_apply_state(). This might have
> > > > surprising results if the caller reuses the values expecting them to
> > > > still represent the same state.
> > > >
> > > > Signed-off-by: Uwe Kleine-König <uwe-rXY34ruvC2xidJT2blvkqNi2O/JbrIOy@public.gmane.org>
> > > > ---
> > > >  drivers/pwm/pwm-fsl-ftm.c | 4 ----
> > > >  1 file changed, 4 deletions(-)
> > >
> > > Presumably this patch could break something since the pwm-fsl-ftm
> > > driver doesn't appear to implement the get_state() function.  ...or
> > > did I miss it?
> >
> > I don't expect breakage. We have more than 50 pwm drivers and only three
> > of them made use of adapting the passed state. So unless you do
> > something special with the PWM (i.e. more than backlight, LED or fan
> > control) I don't think a consumer might care. But it might well be that
> > I miss something so feel free to prove me wrong.
> 
> I don't have this hardware so I can't prove you wrong.  ...but
> presumably someone added the code to return the state on purpose?
> 
> Maybe you could implement get_state() for this driver in your series?

Sure, I could. But I don't have hardware either and so I'm not in a
better position than anybody else on this list.

I suggest to apply as is during the merge window, and let affected
user report problems (or patches) if there really is an issue.
Guessing what people might suffer from and trying to cure this with
untested patches won't help I think.

Best regards
Uwe

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

Re: [PATCH v3 5/6] pwm: fsl-ftm: Don't update the state for the caller of pwm_apply_state()

[Doug Anderson]

Hi,

On Tue, Sep 3, 2019 at 11:48 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Tue, Sep 03, 2019 at 09:54:37AM -0700, Doug Anderson wrote:
> > On Mon, Sep 2, 2019 at 7:27 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Fri, Aug 30, 2019 at 10:39:16AM -0700, Doug Anderson wrote:
> > > > On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > > > >
> > > > > The pwm-fsl-ftm driver is one of only three PWM drivers which updates
> > > > > the state for the caller of pwm_apply_state(). This might have
> > > > > surprising results if the caller reuses the values expecting them to
> > > > > still represent the same state.
> > > > >
> > > > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > > > > ---
> > > > >  drivers/pwm/pwm-fsl-ftm.c | 4 ----
> > > > >  1 file changed, 4 deletions(-)
> > > >
> > > > Presumably this patch could break something since the pwm-fsl-ftm
> > > > driver doesn't appear to implement the get_state() function.  ...or
> > > > did I miss it?
> > >
> > > I don't expect breakage. We have more than 50 pwm drivers and only three
> > > of them made use of adapting the passed state. So unless you do
> > > something special with the PWM (i.e. more than backlight, LED or fan
> > > control) I don't think a consumer might care. But it might well be that
> > > I miss something so feel free to prove me wrong.
> >
> > I don't have this hardware so I can't prove you wrong.  ...but
> > presumably someone added the code to return the state on purpose?
> >
> > Maybe you could implement get_state() for this driver in your series?
>
> Sure, I could. But I don't have hardware either and so I'm not in a
> better position than anybody else on this list.
>
> I suggest to apply as is during the merge window, and let affected
> user report problems (or patches) if there really is an issue.
> Guessing what people might suffer from and trying to cure this with
> untested patches won't help I think.

I suppose it's not up to me, but I would rather have a patch that
attempts to keep things working like they did before rather than one
that is known to change behavior.  Even worse is that your patch
description doesn't mention this functionality change at all.

I will also note that not everyone does a deep test of all
functionality during every kernel merge window.  ...so your change in
functionality certain has a pretty high chance of remaining broken for
a while.  In addition if a PWM is used for something like a PWM
regulator then subtle changes can cause totally non-obvious breakages,
maybe adjusting regulators by a very small percentage.  If you
implement the getter it seems (to me) more likely you'll either get it
right or it will totally break things.  That's actually a much better
failure mode...


-Doug

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v3 5/6] pwm: fsl-ftm: Don't update the state for the caller of pwm_apply_state()

[Uwe Kleine-König]

On Tue, Sep 03, 2019 at 12:35:25PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, Sep 3, 2019 at 11:48 AM Uwe Kleine-König
> <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> >
> > Hello,
> >
> > On Tue, Sep 03, 2019 at 09:54:37AM -0700, Doug Anderson wrote:
> > > On Mon, Sep 2, 2019 at 7:27 AM Uwe Kleine-König
> > > <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > > On Fri, Aug 30, 2019 at 10:39:16AM -0700, Doug Anderson wrote:
> > > > > On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > > > > >
> > > > > > The pwm-fsl-ftm driver is one of only three PWM drivers which updates
> > > > > > the state for the caller of pwm_apply_state(). This might have
> > > > > > surprising results if the caller reuses the values expecting them to
> > > > > > still represent the same state.
> > > > > >
> > > > > > Signed-off-by: Uwe Kleine-König <uwe-rXY34ruvC2xidJT2blvkqNi2O/JbrIOy@public.gmane.org>
> > > > > > ---
> > > > > >  drivers/pwm/pwm-fsl-ftm.c | 4 ----
> > > > > >  1 file changed, 4 deletions(-)
> > > > >
> > > > > Presumably this patch could break something since the pwm-fsl-ftm
> > > > > driver doesn't appear to implement the get_state() function.  ...or
> > > > > did I miss it?
> > > >
> > > > I don't expect breakage. We have more than 50 pwm drivers and only three
> > > > of them made use of adapting the passed state. So unless you do
> > > > something special with the PWM (i.e. more than backlight, LED or fan
> > > > control) I don't think a consumer might care. But it might well be that
> > > > I miss something so feel free to prove me wrong.
> > >
> > > I don't have this hardware so I can't prove you wrong.  ...but
> > > presumably someone added the code to return the state on purpose?
> > >
> > > Maybe you could implement get_state() for this driver in your series?
> >
> > Sure, I could. But I don't have hardware either and so I'm not in a
> > better position than anybody else on this list.
> >
> > I suggest to apply as is during the merge window, and let affected
> > user report problems (or patches) if there really is an issue.
> > Guessing what people might suffer from and trying to cure this with
> > untested patches won't help I think.
> 
> I suppose it's not up to me, but I would rather have a patch that
> attempts to keep things working like they did before rather than one
> that is known to change behavior.  Even worse is that your patch
> description doesn't mention this functionality change at all.

I suggest to add

	As the driver doesn't provide a .get_state() callback it is
	expected that this changes behaviour slightly as pwm_get_state()
	will yield the last set instead of the last implemented setting.

to the commit log to fix this.

> I will also note that not everyone does a deep test of all
> functionality during every kernel merge window.  ...so your change in
> functionality certain has a pretty high chance of remaining broken for
> a while.

I don't expect any real breakage. The changed behaviour only affects
users of pwm_get_state() that is called after pwm_apply_state(). 

> In addition if a PWM is used for something like a PWM
> regulator then subtle changes can cause totally non-obvious breakages,
> maybe adjusting regulators by a very small percentage.

So for drivers/regulator/pwm-regulator.c this affects the .get_voltage()
call only. Note that .set_voltage() does call pwm_get_state() but
doesn't use the result. I don't see how my change would affect the
configuration written to the PWM registers when the PWM regulator driver
is its user. So if you want to convince me that the PWM regulator is one
of the potentially affected consumers, you have to work a bit harder.
:-)

> If you implement the getter it seems (to me) more likely you'll either
> get it right or it will totally break things.  That's actually a much
> better failure mode...

I don't see how it would totally break even if the untested .get_state()
returns bogus values.

Best regards
Uwe

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

Re: [PATCH v3 5/6] pwm: fsl-ftm: Don't update the state for the caller of pwm_apply_state()

[Doug Anderson]

Hi,

On Tue, Sep 3, 2019 at 1:15 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Tue, Sep 03, 2019 at 12:35:25PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Sep 3, 2019 at 11:48 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > Hello,
> > >
> > > On Tue, Sep 03, 2019 at 09:54:37AM -0700, Doug Anderson wrote:
> > > > On Mon, Sep 2, 2019 at 7:27 AM Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > On Fri, Aug 30, 2019 at 10:39:16AM -0700, Doug Anderson wrote:
> > > > > > On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > > > > > >
> > > > > > > The pwm-fsl-ftm driver is one of only three PWM drivers which updates
> > > > > > > the state for the caller of pwm_apply_state(). This might have
> > > > > > > surprising results if the caller reuses the values expecting them to
> > > > > > > still represent the same state.
> > > > > > >
> > > > > > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > > > > > > ---
> > > > > > >  drivers/pwm/pwm-fsl-ftm.c | 4 ----
> > > > > > >  1 file changed, 4 deletions(-)
> > > > > >
> > > > > > Presumably this patch could break something since the pwm-fsl-ftm
> > > > > > driver doesn't appear to implement the get_state() function.  ...or
> > > > > > did I miss it?
> > > > >
> > > > > I don't expect breakage. We have more than 50 pwm drivers and only three
> > > > > of them made use of adapting the passed state. So unless you do
> > > > > something special with the PWM (i.e. more than backlight, LED or fan
> > > > > control) I don't think a consumer might care. But it might well be that
> > > > > I miss something so feel free to prove me wrong.
> > > >
> > > > I don't have this hardware so I can't prove you wrong.  ...but
> > > > presumably someone added the code to return the state on purpose?
> > > >
> > > > Maybe you could implement get_state() for this driver in your series?
> > >
> > > Sure, I could. But I don't have hardware either and so I'm not in a
> > > better position than anybody else on this list.
> > >
> > > I suggest to apply as is during the merge window, and let affected
> > > user report problems (or patches) if there really is an issue.
> > > Guessing what people might suffer from and trying to cure this with
> > > untested patches won't help I think.
> >
> > I suppose it's not up to me, but I would rather have a patch that
> > attempts to keep things working like they did before rather than one
> > that is known to change behavior.  Even worse is that your patch
> > description doesn't mention this functionality change at all.
>
> I suggest to add
>
>         As the driver doesn't provide a .get_state() callback it is
>         expected that this changes behaviour slightly as pwm_get_state()
>         will yield the last set instead of the last implemented setting.
>
> to the commit log to fix this.
>
> > I will also note that not everyone does a deep test of all
> > functionality during every kernel merge window.  ...so your change in
> > functionality certain has a pretty high chance of remaining broken for
> > a while.
>
> I don't expect any real breakage. The changed behaviour only affects
> users of pwm_get_state() that is called after pwm_apply_state().
>
> > In addition if a PWM is used for something like a PWM
> > regulator then subtle changes can cause totally non-obvious breakages,
> > maybe adjusting regulators by a very small percentage.
>
> So for drivers/regulator/pwm-regulator.c this affects the .get_voltage()
> call only. Note that .set_voltage() does call pwm_get_state() but
> doesn't use the result. I don't see how my change would affect the
> configuration written to the PWM registers when the PWM regulator driver
> is its user. So if you want to convince me that the PWM regulator is one
> of the potentially affected consumers, you have to work a bit harder.
> :-)

Prior to your patch, pwm_apply_state() would call the ->apply()
function, right?  That would modify the state.  Then pwm_apply_state()
would store the state (after it had been modified) into pwm->state.
All future calls to pwm_get_state() would return the modified state.

...this means that the call to pwm_get_state() in
pwm_regulator_get_voltage() would return the actual hardware state.

After your patch series pwm_get_state() will not return the actual
hardware state for "pwm-fsl-ftm.c", it will return the state that was
programmed.

While pwm_set_voltage() will not necessarily be affected, future calls
to pwm_regulator_get_voltage() could be affected.  Unless you are
asserting that 100% of the calls to pwm_get_voltage() cosmetic.


Please correct anything I got wrong there.

-Doug

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v3 5/6] pwm: fsl-ftm: Don't update the state for the caller of pwm_apply_state()

[Uwe Kleine-König]

On Tue, Sep 03, 2019 at 01:50:27PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, Sep 3, 2019 at 1:15 PM Uwe Kleine-König
> <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> >
> > On Tue, Sep 03, 2019 at 12:35:25PM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Tue, Sep 3, 2019 at 11:48 AM Uwe Kleine-König
> > > <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > >
> > > > Hello,
> > > >
> > > > On Tue, Sep 03, 2019 at 09:54:37AM -0700, Doug Anderson wrote:
> > > > > On Mon, Sep 2, 2019 at 7:27 AM Uwe Kleine-König
> > > > > <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > > > > On Fri, Aug 30, 2019 at 10:39:16AM -0700, Doug Anderson wrote:
> > > > > > > On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > > > > > > >
> > > > > > > > The pwm-fsl-ftm driver is one of only three PWM drivers which updates
> > > > > > > > the state for the caller of pwm_apply_state(). This might have
> > > > > > > > surprising results if the caller reuses the values expecting them to
> > > > > > > > still represent the same state.
> > > > > > > >
> > > > > > > > Signed-off-by: Uwe Kleine-König <uwe-rXY34ruvC2xidJT2blvkqNi2O/JbrIOy@public.gmane.org>
> > > > > > > > ---
> > > > > > > >  drivers/pwm/pwm-fsl-ftm.c | 4 ----
> > > > > > > >  1 file changed, 4 deletions(-)
> > > > > > >
> > > > > > > Presumably this patch could break something since the pwm-fsl-ftm
> > > > > > > driver doesn't appear to implement the get_state() function.  ...or
> > > > > > > did I miss it?
> > > > > >
> > > > > > I don't expect breakage. We have more than 50 pwm drivers and only three
> > > > > > of them made use of adapting the passed state. So unless you do
> > > > > > something special with the PWM (i.e. more than backlight, LED or fan
> > > > > > control) I don't think a consumer might care. But it might well be that
> > > > > > I miss something so feel free to prove me wrong.
> > > > >
> > > > > I don't have this hardware so I can't prove you wrong.  ...but
> > > > > presumably someone added the code to return the state on purpose?
> > > > >
> > > > > Maybe you could implement get_state() for this driver in your series?
> > > >
> > > > Sure, I could. But I don't have hardware either and so I'm not in a
> > > > better position than anybody else on this list.
> > > >
> > > > I suggest to apply as is during the merge window, and let affected
> > > > user report problems (or patches) if there really is an issue.
> > > > Guessing what people might suffer from and trying to cure this with
> > > > untested patches won't help I think.
> > >
> > > I suppose it's not up to me, but I would rather have a patch that
> > > attempts to keep things working like they did before rather than one
> > > that is known to change behavior.  Even worse is that your patch
> > > description doesn't mention this functionality change at all.
> >
> > I suggest to add
> >
> >         As the driver doesn't provide a .get_state() callback it is
> >         expected that this changes behaviour slightly as pwm_get_state()
> >         will yield the last set instead of the last implemented setting.
> >
> > to the commit log to fix this.
> >
> > > I will also note that not everyone does a deep test of all
> > > functionality during every kernel merge window.  ...so your change in
> > > functionality certain has a pretty high chance of remaining broken for
> > > a while.
> >
> > I don't expect any real breakage. The changed behaviour only affects
> > users of pwm_get_state() that is called after pwm_apply_state().
> >
> > > In addition if a PWM is used for something like a PWM
> > > regulator then subtle changes can cause totally non-obvious breakages,
> > > maybe adjusting regulators by a very small percentage.
> >
> > So for drivers/regulator/pwm-regulator.c this affects the .get_voltage()
> > call only. Note that .set_voltage() does call pwm_get_state() but
> > doesn't use the result. I don't see how my change would affect the
> > configuration written to the PWM registers when the PWM regulator driver
> > is its user. So if you want to convince me that the PWM regulator is one
> > of the potentially affected consumers, you have to work a bit harder.
> > :-)
> 
> Prior to your patch, pwm_apply_state() would call the ->apply()
> function, right?  That would modify the state.  Then pwm_apply_state()
> would store the state (after it had been modified) into pwm->state.
> All future calls to pwm_get_state() would return the modified state.
> 
> ...this means that the call to pwm_get_state() in
> pwm_regulator_get_voltage() would return the actual hardware state.
> 
> After your patch series pwm_get_state() will not return the actual
> hardware state for "pwm-fsl-ftm.c", it will return the state that was
> programmed.
> 
> While pwm_set_voltage() will not necessarily be affected, future calls
> to pwm_regulator_get_voltage() could be affected.  Unless you are
> asserting that 100% of the calls to pwm_get_voltage() cosmetic.
>
> 
> Please correct anything I got wrong there.

I think this is all true. The key question here is then: Who calls the
.get_voltage() callback and cares about the result? Yes, it changes a
few files in sysfs but apart from that?

Best regards
Uwe

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

Re: [PATCH v3 5/6] pwm: fsl-ftm: Don't update the state for the caller of pwm_apply_state()

[Doug Anderson]

Hi,

On Tue, Sep 3, 2019 at 2:07 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Tue, Sep 03, 2019 at 01:50:27PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Sep 3, 2019 at 1:15 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > On Tue, Sep 03, 2019 at 12:35:25PM -0700, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Tue, Sep 3, 2019 at 11:48 AM Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > >
> > > > > Hello,
> > > > >
> > > > > On Tue, Sep 03, 2019 at 09:54:37AM -0700, Doug Anderson wrote:
> > > > > > On Mon, Sep 2, 2019 at 7:27 AM Uwe Kleine-König
> > > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > > On Fri, Aug 30, 2019 at 10:39:16AM -0700, Doug Anderson wrote:
> > > > > > > > On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > > > > > > > >
> > > > > > > > > The pwm-fsl-ftm driver is one of only three PWM drivers which updates
> > > > > > > > > the state for the caller of pwm_apply_state(). This might have
> > > > > > > > > surprising results if the caller reuses the values expecting them to
> > > > > > > > > still represent the same state.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > > > > > > > > ---
> > > > > > > > >  drivers/pwm/pwm-fsl-ftm.c | 4 ----
> > > > > > > > >  1 file changed, 4 deletions(-)
> > > > > > > >
> > > > > > > > Presumably this patch could break something since the pwm-fsl-ftm
> > > > > > > > driver doesn't appear to implement the get_state() function.  ...or
> > > > > > > > did I miss it?
> > > > > > >
> > > > > > > I don't expect breakage. We have more than 50 pwm drivers and only three
> > > > > > > of them made use of adapting the passed state. So unless you do
> > > > > > > something special with the PWM (i.e. more than backlight, LED or fan
> > > > > > > control) I don't think a consumer might care. But it might well be that
> > > > > > > I miss something so feel free to prove me wrong.
> > > > > >
> > > > > > I don't have this hardware so I can't prove you wrong.  ...but
> > > > > > presumably someone added the code to return the state on purpose?
> > > > > >
> > > > > > Maybe you could implement get_state() for this driver in your series?
> > > > >
> > > > > Sure, I could. But I don't have hardware either and so I'm not in a
> > > > > better position than anybody else on this list.
> > > > >
> > > > > I suggest to apply as is during the merge window, and let affected
> > > > > user report problems (or patches) if there really is an issue.
> > > > > Guessing what people might suffer from and trying to cure this with
> > > > > untested patches won't help I think.
> > > >
> > > > I suppose it's not up to me, but I would rather have a patch that
> > > > attempts to keep things working like they did before rather than one
> > > > that is known to change behavior.  Even worse is that your patch
> > > > description doesn't mention this functionality change at all.
> > >
> > > I suggest to add
> > >
> > >         As the driver doesn't provide a .get_state() callback it is
> > >         expected that this changes behaviour slightly as pwm_get_state()
> > >         will yield the last set instead of the last implemented setting.
> > >
> > > to the commit log to fix this.
> > >
> > > > I will also note that not everyone does a deep test of all
> > > > functionality during every kernel merge window.  ...so your change in
> > > > functionality certain has a pretty high chance of remaining broken for
> > > > a while.
> > >
> > > I don't expect any real breakage. The changed behaviour only affects
> > > users of pwm_get_state() that is called after pwm_apply_state().
> > >
> > > > In addition if a PWM is used for something like a PWM
> > > > regulator then subtle changes can cause totally non-obvious breakages,
> > > > maybe adjusting regulators by a very small percentage.
> > >
> > > So for drivers/regulator/pwm-regulator.c this affects the .get_voltage()
> > > call only. Note that .set_voltage() does call pwm_get_state() but
> > > doesn't use the result. I don't see how my change would affect the
> > > configuration written to the PWM registers when the PWM regulator driver
> > > is its user. So if you want to convince me that the PWM regulator is one
> > > of the potentially affected consumers, you have to work a bit harder.
> > > :-)
> >
> > Prior to your patch, pwm_apply_state() would call the ->apply()
> > function, right?  That would modify the state.  Then pwm_apply_state()
> > would store the state (after it had been modified) into pwm->state.
> > All future calls to pwm_get_state() would return the modified state.
> >
> > ...this means that the call to pwm_get_state() in
> > pwm_regulator_get_voltage() would return the actual hardware state.
> >
> > After your patch series pwm_get_state() will not return the actual
> > hardware state for "pwm-fsl-ftm.c", it will return the state that was
> > programmed.
> >
> > While pwm_set_voltage() will not necessarily be affected, future calls
> > to pwm_regulator_get_voltage() could be affected.  Unless you are
> > asserting that 100% of the calls to pwm_get_voltage() cosmetic.
> >
> >
> > Please correct anything I got wrong there.
>
> I think this is all true. The key question here is then: Who calls the
> .get_voltage() callback and cares about the result? Yes, it changes a
> few files in sysfs but apart from that?

There are lots of drivers that call get_voltage() for things other
than sysfs, but without auditing each one I can't say if any of them
would change behavior in a way that would matter.

-Doug

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v3 5/6] pwm: fsl-ftm: Don't update the state for the caller of pwm_apply_state()

[Uwe Kleine-König]

On Tue, Sep 03, 2019 at 02:48:54PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, Sep 3, 2019 at 2:07 PM Uwe Kleine-König
> <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> >
> > On Tue, Sep 03, 2019 at 01:50:27PM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Tue, Sep 3, 2019 at 1:15 PM Uwe Kleine-König
> > > <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > >
> > > > On Tue, Sep 03, 2019 at 12:35:25PM -0700, Doug Anderson wrote:
> > > > > Hi,
> > > > >
> > > > > On Tue, Sep 3, 2019 at 11:48 AM Uwe Kleine-König
> > > > > <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > > > >
> > > > > > Hello,
> > > > > >
> > > > > > On Tue, Sep 03, 2019 at 09:54:37AM -0700, Doug Anderson wrote:
> > > > > > > On Mon, Sep 2, 2019 at 7:27 AM Uwe Kleine-König
> > > > > > > <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > > > > > > On Fri, Aug 30, 2019 at 10:39:16AM -0700, Doug Anderson wrote:
> > > > > > > > > On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > > > > > > > > >
> > > > > > > > > > The pwm-fsl-ftm driver is one of only three PWM drivers which updates
> > > > > > > > > > the state for the caller of pwm_apply_state(). This might have
> > > > > > > > > > surprising results if the caller reuses the values expecting them to
> > > > > > > > > > still represent the same state.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Uwe Kleine-König <uwe-rXY34ruvC2xidJT2blvkqA@public.gmane.orgg>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/pwm/pwm-fsl-ftm.c | 4 ----
> > > > > > > > > >  1 file changed, 4 deletions(-)
> > > > > > > > >
> > > > > > > > > Presumably this patch could break something since the pwm-fsl-ftm
> > > > > > > > > driver doesn't appear to implement the get_state() function.  ...or
> > > > > > > > > did I miss it?
> > > > > > > >
> > > > > > > > I don't expect breakage. We have more than 50 pwm drivers and only three
> > > > > > > > of them made use of adapting the passed state. So unless you do
> > > > > > > > something special with the PWM (i.e. more than backlight, LED or fan
> > > > > > > > control) I don't think a consumer might care. But it might well be that
> > > > > > > > I miss something so feel free to prove me wrong.
> > > > > > >
> > > > > > > I don't have this hardware so I can't prove you wrong.  ...but
> > > > > > > presumably someone added the code to return the state on purpose?
> > > > > > >
> > > > > > > Maybe you could implement get_state() for this driver in your series?
> > > > > >
> > > > > > Sure, I could. But I don't have hardware either and so I'm not in a
> > > > > > better position than anybody else on this list.
> > > > > >
> > > > > > I suggest to apply as is during the merge window, and let affected
> > > > > > user report problems (or patches) if there really is an issue.
> > > > > > Guessing what people might suffer from and trying to cure this with
> > > > > > untested patches won't help I think.
> > > > >
> > > > > I suppose it's not up to me, but I would rather have a patch that
> > > > > attempts to keep things working like they did before rather than one
> > > > > that is known to change behavior.  Even worse is that your patch
> > > > > description doesn't mention this functionality change at all.
> > > >
> > > > I suggest to add
> > > >
> > > >         As the driver doesn't provide a .get_state() callback it is
> > > >         expected that this changes behaviour slightly as pwm_get_state()
> > > >         will yield the last set instead of the last implemented setting.
> > > >
> > > > to the commit log to fix this.
> > > >
> > > > > I will also note that not everyone does a deep test of all
> > > > > functionality during every kernel merge window.  ...so your change in
> > > > > functionality certain has a pretty high chance of remaining broken for
> > > > > a while.
> > > >
> > > > I don't expect any real breakage. The changed behaviour only affects
> > > > users of pwm_get_state() that is called after pwm_apply_state().
> > > >
> > > > > In addition if a PWM is used for something like a PWM
> > > > > regulator then subtle changes can cause totally non-obvious breakages,
> > > > > maybe adjusting regulators by a very small percentage.
> > > >
> > > > So for drivers/regulator/pwm-regulator.c this affects the .get_voltage()
> > > > call only. Note that .set_voltage() does call pwm_get_state() but
> > > > doesn't use the result. I don't see how my change would affect the
> > > > configuration written to the PWM registers when the PWM regulator driver
> > > > is its user. So if you want to convince me that the PWM regulator is one
> > > > of the potentially affected consumers, you have to work a bit harder.
> > > > :-)
> > >
> > > Prior to your patch, pwm_apply_state() would call the ->apply()
> > > function, right?  That would modify the state.  Then pwm_apply_state()
> > > would store the state (after it had been modified) into pwm->state.
> > > All future calls to pwm_get_state() would return the modified state.
> > >
> > > ...this means that the call to pwm_get_state() in
> > > pwm_regulator_get_voltage() would return the actual hardware state.
> > >
> > > After your patch series pwm_get_state() will not return the actual
> > > hardware state for "pwm-fsl-ftm.c", it will return the state that was
> > > programmed.
> > >
> > > While pwm_set_voltage() will not necessarily be affected, future calls
> > > to pwm_regulator_get_voltage() could be affected.  Unless you are
> > > asserting that 100% of the calls to pwm_get_voltage() cosmetic.
> > >
> > >
> > > Please correct anything I got wrong there.
> >
> > I think this is all true. The key question here is then: Who calls the
> > .get_voltage() callback and cares about the result? Yes, it changes a
> > few files in sysfs but apart from that?
> 
> There are lots of drivers that call get_voltage() for things other
> than sysfs, but without auditing each one I can't say if any of them
> would change behavior in a way that would matter.

In my book it is ok to do such a change. The driver continues to compile
just fine, it isn't knowingly broken. And if someone finds a regression
we can fix it then and have someone who cares for testing.

And even if a regulator changes its behaviour slightly and breaks in a
hardly detectable way, I would bet that in 95% of the cases it only
worked by chance before. And I hope for the remaining 5% who seem to
care about correctness, that the reverify on a kernel upgrade.

Yes, there might be some cases falling through the cracks, but if we
start to demand this kind of care from people who work on generic code
(here: PWM core) we will be stuck for a long time and scare people with
motivation away. So the lesser evil in my eyes is to accept that not all
corner cases might be handled because they are unknown.

Best regards
Uwe

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