[1/4] pwm: Add new helper to initialize a pwm_state variable with defaults

Message ID 20181026184157.16371-1-u.kleine-koenig@pengutronix.de
State New
Headers show
Series
  • [1/4] pwm: Add new helper to initialize a pwm_state variable with defaults
Related show

Commit Message

Uwe Kleine-König Oct. 26, 2018, 6:41 p.m.
This helps to convert drivers from the legacy API to pwm_apply_state without
having to make the aware of the configured polarity (and in some cases even
period).

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 include/linux/pwm.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Comments

Thierry Reding Oct. 29, 2018, 11:33 a.m. | #1
On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-König wrote:
> This helps to convert drivers from the legacy API to pwm_apply_state without
> having to make the aware of the configured polarity (and in some cases even
> period).
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  include/linux/pwm.h | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

You're going to have to at least Cc me on patches 2 & 3 so that I can
see how you intend this to be used.

Thierry
Uwe Kleine-König Nov. 3, 2018, 2:25 p.m. | #2
Hello Thierry,

On Mon, Oct 29, 2018 at 12:33:37PM +0100, Thierry Reding wrote:
> On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-König wrote:
> > This helps to convert drivers from the legacy API to pwm_apply_state without
> > having to make the aware of the configured polarity (and in some cases even
> > period).
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  include/linux/pwm.h | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> You're going to have to at least Cc me on patches 2 & 3 so that I can
> see how you intend this to be used.

you were in To: for all four patches:

  patch 1: https://www.spinics.net/lists/linux-pwm/msg08178.html
  patch 2: https://www.spinics.net/lists/linux-pwm/msg08179.html
  patch 3: https://www.spinics.net/lists/linux-pwm/msg08180.html
  patch 4: https://www.spinics.net/lists/linux-pwm/msg08181.html

Best regards
Uwe
Uwe Kleine-König Nov. 8, 2018, 3:13 p.m. | #3
Hello,

On Sat, Nov 03, 2018 at 03:25:45PM +0100, Uwe Kleine-König wrote:
> On Mon, Oct 29, 2018 at 12:33:37PM +0100, Thierry Reding wrote:
> > On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-König wrote:
> > > This helps to convert drivers from the legacy API to pwm_apply_state without
> > > having to make the aware of the configured polarity (and in some cases even
> > > period).
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  include/linux/pwm.h | 13 ++++++++++---
> > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > You're going to have to at least Cc me on patches 2 & 3 so that I can
> > see how you intend this to be used.
> 
> you were in To: for all four patches:
> 
>   patch 1: https://www.spinics.net/lists/linux-pwm/msg08178.html
>   patch 2: https://www.spinics.net/lists/linux-pwm/msg08179.html
>   patch 3: https://www.spinics.net/lists/linux-pwm/msg08180.html
>   patch 4: https://www.spinics.net/lists/linux-pwm/msg08181.html

Did you find those in your mailbox in the meantime? If google just
failed to put them in your inbox I can send you a bounce of 2 and 3 if
you want to. Just tell me (in irc if you want to).

Best regards
Uwe
Thierry Reding Nov. 14, 2018, 12:32 p.m. | #4
On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-König wrote:
> This helps to convert drivers from the legacy API to pwm_apply_state without
> having to make the aware of the configured polarity (and in some cases even
> period).
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  include/linux/pwm.h | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

So if I understand your intention correctly here you want consumers to
be able to get at the default state, which would be the state as it was
"configured" in DT or the PWM lookup table so that you can modify that
state (well, really just setting the duty-cycle) before you actually
send the state to the hardware.

So the difference to pwm_apply_args() is that you don't want the driver
to specifically program a duty cycle of 0 before setting the actual duty
cycle that you want to set.

Is that about right?

Isn't that exactly what pwm_init_state() already does? pwm-backlight
uses it for exactly the same purpose that you seem to be using it in
RX1950 backlight control.

Thierry
Uwe Kleine-König Nov. 15, 2018, 9:16 a.m. | #5
Hello Thierry,

On Wed, Nov 14, 2018 at 01:32:36PM +0100, Thierry Reding wrote:
> On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-König wrote:
> > This helps to convert drivers from the legacy API to pwm_apply_state without
> > having to make the aware of the configured polarity (and in some cases even
> > period).
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  include/linux/pwm.h | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> So if I understand your intention correctly here you want consumers to
> be able to get at the default state, which would be the state as it was
> "configured" in DT or the PWM lookup table so that you can modify that
> state (well, really just setting the duty-cycle) before you actually
> send the state to the hardware.
> 
> So the difference to pwm_apply_args() is that you don't want the driver
> to specifically program a duty cycle of 0 before setting the actual duty
> cycle that you want to set.
> 
> Is that about right?

Yes, and a resulting advantage is that the apply callback is only called
once.
 
> Isn't that exactly what pwm_init_state() already does? pwm-backlight
> uses it for exactly the same purpose that you seem to be using it in
> RX1950 backlight control.

I didn't use pwm_init_state because that doesn't give a helpful value
for state->enabled. (I think it is: if the driver provides a get_state
callback it is whatever that one set, otherwise it's off.)
Also the name pwm_init_state isn't that good IMHO. Just from the name
I'd expect it to be something like

	memset(state, 0, sizeof(&state));

instead of something more informed that depends on hardware state and/or
data provided by a device tree.

Best regards
Uwe
Thierry Reding Nov. 15, 2018, 4:21 p.m. | #6
On Thu, Nov 15, 2018 at 10:16:44AM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Wed, Nov 14, 2018 at 01:32:36PM +0100, Thierry Reding wrote:
> > On Fri, Oct 26, 2018 at 08:41:55PM +0200, Uwe Kleine-König wrote:
> > > This helps to convert drivers from the legacy API to pwm_apply_state without
> > > having to make the aware of the configured polarity (and in some cases even
> > > period).
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  include/linux/pwm.h | 13 ++++++++++---
> > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > So if I understand your intention correctly here you want consumers to
> > be able to get at the default state, which would be the state as it was
> > "configured" in DT or the PWM lookup table so that you can modify that
> > state (well, really just setting the duty-cycle) before you actually
> > send the state to the hardware.
> > 
> > So the difference to pwm_apply_args() is that you don't want the driver
> > to specifically program a duty cycle of 0 before setting the actual duty
> > cycle that you want to set.
> > 
> > Is that about right?
> 
> Yes, and a resulting advantage is that the apply callback is only called
> once.
>  
> > Isn't that exactly what pwm_init_state() already does? pwm-backlight
> > uses it for exactly the same purpose that you seem to be using it in
> > RX1950 backlight control.
> 
> I didn't use pwm_init_state because that doesn't give a helpful value
> for state->enabled. (I think it is: if the driver provides a get_state
> callback it is whatever that one set, otherwise it's off.)

That's correct. It's pretty much the same thing that you have, except
you explicitly set enable = false. But that's not really important
because consumers are supposed to explicitly override that anyway.

> Also the name pwm_init_state isn't that good IMHO. Just from the name
> I'd expect it to be something like
> 
> 	memset(state, 0, sizeof(&state));
> 
> instead of something more informed that depends on hardware state and/or
> data provided by a device tree.

Be that as it may, the kerneldoc is pretty explicit about what it does
and so is the code. Not liking the name shouldn't be an excuse for
duplicating functionality.

Thierry

Patch

diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index d5199b507d79..017d1e13e29d 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -522,6 +522,15 @@  static inline void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
 }
 #endif
 
+static inline void pwm_get_state_default(const struct pwm_device *pwm,
+					 struct pwm_state *state)
+{
+	state->period = pwm->args.period;
+	state->duty_cycle = 0;
+	state->polarity = pwm->args.polarity;
+	state->enabled = false;
+}
+
 static inline void pwm_apply_args(struct pwm_device *pwm)
 {
 	struct pwm_state state = { };
@@ -547,9 +556,7 @@  static inline void pwm_apply_args(struct pwm_device *pwm)
 	 * pwm_apply_args().
 	 */
 
-	state.enabled = false;
-	state.polarity = pwm->args.polarity;
-	state.period = pwm->args.period;
+	pwm_get_state_default(pwm, &state);
 
 	pwm_apply_state(pwm, &state);
 }