Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1376721 > unrolled thread

Re: [PATCH v5 05/46] pwm: introduce the pwm_args concept

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 05/46] pwm: introduce the pwm_args concept Thierry Reding <thierry.reding@gmail.com> - 2016-04-12 13:40 +0200
    Re: [PATCH v5 05/46] pwm: introduce the pwm_args concept Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-04-12 14:10 +0200
      Re: [PATCH v5 05/46] pwm: introduce the pwm_args concept Thierry Reding <thierry.reding@gmail.com> - 2016-04-12 14:30 +0200
        Re: [PATCH v5 05/46] pwm: introduce the pwm_args concept Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-04-12 15:00 +0200
    Re: [PATCH v5 05/46] pwm: introduce the pwm_args concept Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-04-12 15:10 +0200
      Re: [PATCH v5 05/46] pwm: introduce the pwm_args concept Thierry Reding <thierry.reding@gmail.com> - 2016-04-12 15:20 +0200

#1376721 — Re: [PATCH v5 05/46] pwm: introduce the pwm_args concept

[Multipart message — attachments visible in raw view] — view raw

On Wed, Mar 30, 2016 at 10:03:28PM +0200, Boris Brezillon wrote:
> Currently the PWM core mixes the current PWM state with the per-platform
> reference config (specified through the PWM lookup table, DT definition or
> directly hardcoded in PWM drivers).
> 
> Create a pwm_args struct to store this reference config, so that PWM users
> can differentiate the current config from the reference one.
> 
> Patch all places where pwm->args should be initialized. We keep the
> pwm_set_polarity/period() calls until all PWM users are patched to
> use pwm_args instead of pwm_get_period/polarity().

Perhaps a helper would be useful? Something like:

	static inline void
	pwm_apply_args(struct pwm_device *pwm, const struct pwm_args *args)
	{
		pwm_set_duty_cycle(pwm, args->duty_cycle);
		pwm_set_period(pwm, args->period);
	}

? That would make it slightly easier to get rid of it again after all
clients have been converted.

With the exception of pwm-clps711x all of these args are set at of_xlate
time (for DT) or from the lookup table in pwm_get() (for non-DT), so it
might even be possible to move this call to the core, so that removal of
it will be a one-liner.

Thierry

[toc] | [next] | [standalone]

#1376734

On Tue, 12 Apr 2016 13:39:12 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Wed, Mar 30, 2016 at 10:03:28PM +0200, Boris Brezillon wrote:
> > Currently the PWM core mixes the current PWM state with the per-platform
> > reference config (specified through the PWM lookup table, DT definition or
> > directly hardcoded in PWM drivers).
> > 
> > Create a pwm_args struct to store this reference config, so that PWM users
> > can differentiate the current config from the reference one.
> > 
> > Patch all places where pwm->args should be initialized. We keep the
> > pwm_set_polarity/period() calls until all PWM users are patched to
> > use pwm_args instead of pwm_get_period/polarity().
> 
> Perhaps a helper would be useful? Something like:
> 
> 	static inline void
> 	pwm_apply_args(struct pwm_device *pwm, const struct pwm_args *args)
> 	{
> 		pwm_set_duty_cycle(pwm, args->duty_cycle);
> 		pwm_set_period(pwm, args->period);
> 	}
> 
> ? That would make it slightly easier to get rid of it again after all
> clients have been converted.

Sure. I'll add this helper.

> 
> With the exception of pwm-clps711x all of these args are set at of_xlate
> time (for DT) or from the lookup table in pwm_get() (for non-DT), so it
> might even be possible to move this call to the core, so that removal of
> it will be a one-liner.

Not sure I get that one. Some drivers are implementing their own
->of_xlate() method, how would you get rid of this pwm_apply_args() in
those custom implementations?

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1376748

[Multipart message — attachments visible in raw view] — view raw

On Tue, Apr 12, 2016 at 02:04:12PM +0200, Boris Brezillon wrote:
> On Tue, 12 Apr 2016 13:39:12 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Wed, Mar 30, 2016 at 10:03:28PM +0200, Boris Brezillon wrote:
> > > Currently the PWM core mixes the current PWM state with the per-platform
> > > reference config (specified through the PWM lookup table, DT definition or
> > > directly hardcoded in PWM drivers).
> > > 
> > > Create a pwm_args struct to store this reference config, so that PWM users
> > > can differentiate the current config from the reference one.
> > > 
> > > Patch all places where pwm->args should be initialized. We keep the
> > > pwm_set_polarity/period() calls until all PWM users are patched to
> > > use pwm_args instead of pwm_get_period/polarity().
> > 
> > Perhaps a helper would be useful? Something like:
> > 
> > 	static inline void
> > 	pwm_apply_args(struct pwm_device *pwm, const struct pwm_args *args)
> > 	{
> > 		pwm_set_duty_cycle(pwm, args->duty_cycle);
> > 		pwm_set_period(pwm, args->period);
> > 	}
> > 
> > ? That would make it slightly easier to get rid of it again after all
> > clients have been converted.
> 
> Sure. I'll add this helper.
> 
> > 
> > With the exception of pwm-clps711x all of these args are set at of_xlate
> > time (for DT) or from the lookup table in pwm_get() (for non-DT), so it
> > might even be possible to move this call to the core, so that removal of
> > it will be a one-liner.
> 
> Not sure I get that one. Some drivers are implementing their own
> ->of_xlate() method, how would you get rid of this pwm_apply_args() in
> those custom implementations?

I was proposing to have pwm_apply_args() called from the core.
of_pwm_get() is where ->of_xlate() is called from, and the lookup table
arguments would be applied in pwm_get(). Taking into account clps711x,
which sets the arguments in ->request() it might be possible to simply
call pwm_apply_args() from pwm_device_request(), since that's also
called by all other request functions, even the legacy ones.

That said, the amount of code to modify isn't that large, so I'm fine if
you want to keep sprinkling the calls across multiple files, especially
since it's temporary.

Thierry

[toc] | [prev] | [next] | [standalone]

#1376781

On Tue, 12 Apr 2016 14:20:29 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Tue, Apr 12, 2016 at 02:04:12PM +0200, Boris Brezillon wrote:
> > On Tue, 12 Apr 2016 13:39:12 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> > 
> > > On Wed, Mar 30, 2016 at 10:03:28PM +0200, Boris Brezillon wrote:
> > > > Currently the PWM core mixes the current PWM state with the per-platform
> > > > reference config (specified through the PWM lookup table, DT definition or
> > > > directly hardcoded in PWM drivers).
> > > > 
> > > > Create a pwm_args struct to store this reference config, so that PWM users
> > > > can differentiate the current config from the reference one.
> > > > 
> > > > Patch all places where pwm->args should be initialized. We keep the
> > > > pwm_set_polarity/period() calls until all PWM users are patched to
> > > > use pwm_args instead of pwm_get_period/polarity().
> > > 
> > > Perhaps a helper would be useful? Something like:
> > > 
> > > 	static inline void
> > > 	pwm_apply_args(struct pwm_device *pwm, const struct pwm_args *args)
> > > 	{
> > > 		pwm_set_duty_cycle(pwm, args->duty_cycle);
> > > 		pwm_set_period(pwm, args->period);
> > > 	}
> > > 
> > > ? That would make it slightly easier to get rid of it again after all
> > > clients have been converted.
> > 
> > Sure. I'll add this helper.
> > 
> > > 
> > > With the exception of pwm-clps711x all of these args are set at of_xlate
> > > time (for DT) or from the lookup table in pwm_get() (for non-DT), so it
> > > might even be possible to move this call to the core, so that removal of
> > > it will be a one-liner.
> > 
> > Not sure I get that one. Some drivers are implementing their own
> > ->of_xlate() method, how would you get rid of this pwm_apply_args() in
> > those custom implementations?
> 
> I was proposing to have pwm_apply_args() called from the core.
> of_pwm_get() is where ->of_xlate() is called from, and the lookup table
> arguments would be applied in pwm_get(). Taking into account clps711x,
> which sets the arguments in ->request() it might be possible to simply
> call pwm_apply_args() from pwm_device_request(), since that's also
> called by all other request functions, even the legacy ones.
> 
> That said, the amount of code to modify isn't that large, so I'm fine if
> you want to keep sprinkling the calls across multiple files, especially
> since it's temporary.

No, I'm fine addressing that, but I just don't get where you'd get the
pwm_args to apply. Do you suggest to pass 'struct pwm_args *' to the
->of_xlate() and ->request() methods?

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1376785

On Tue, 12 Apr 2016 13:39:12 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Wed, Mar 30, 2016 at 10:03:28PM +0200, Boris Brezillon wrote:
> > Currently the PWM core mixes the current PWM state with the per-platform
> > reference config (specified through the PWM lookup table, DT definition or
> > directly hardcoded in PWM drivers).
> > 
> > Create a pwm_args struct to store this reference config, so that PWM users
> > can differentiate the current config from the reference one.
> > 
> > Patch all places where pwm->args should be initialized. We keep the
> > pwm_set_polarity/period() calls until all PWM users are patched to
> > use pwm_args instead of pwm_get_period/polarity().
> 
> Perhaps a helper would be useful? Something like:
> 
> 	static inline void
> 	pwm_apply_args(struct pwm_device *pwm, const struct pwm_args *args)
> 	{
> 		pwm_set_duty_cycle(pwm, args->duty_cycle);
> 		pwm_set_period(pwm, args->period);
> 	}
> 
> ? That would make it slightly easier to get rid of it again after all
> clients have been converted.
> 
> With the exception of pwm-clps711x all of these args are set at of_xlate
> time (for DT) or from the lookup table in pwm_get() (for non-DT), so it
> might even be possible to move this call to the core, so that removal of
> it will be a one-liner.

Okay, I think I misunderstood your suggestion. I thought you wanted
this helper to set the reference config, but you actually want to apply
a new state based on the PWM reference values.

Except that pwm_args does not contain all the required information to
apply a full config (args->duty_cycle and args->enable do not exist).

This being said, in my v6 I moved the content of
pwm_regulator_adjust_pwm_config() (patch 27) into a generic helper
(pwm_adjust_config()). This helper is doing pretty much what you're
suggesting here (but again, I'm not sure I correctly understood your
suggestion :-/).

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1376790

[Multipart message — attachments visible in raw view] — view raw

On Tue, Apr 12, 2016 at 03:06:27PM +0200, Boris Brezillon wrote:
> On Tue, 12 Apr 2016 13:39:12 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Wed, Mar 30, 2016 at 10:03:28PM +0200, Boris Brezillon wrote:
> > > Currently the PWM core mixes the current PWM state with the per-platform
> > > reference config (specified through the PWM lookup table, DT definition or
> > > directly hardcoded in PWM drivers).
> > > 
> > > Create a pwm_args struct to store this reference config, so that PWM users
> > > can differentiate the current config from the reference one.
> > > 
> > > Patch all places where pwm->args should be initialized. We keep the
> > > pwm_set_polarity/period() calls until all PWM users are patched to
> > > use pwm_args instead of pwm_get_period/polarity().
> > 
> > Perhaps a helper would be useful? Something like:
> > 
> > 	static inline void
> > 	pwm_apply_args(struct pwm_device *pwm, const struct pwm_args *args)
> > 	{
> > 		pwm_set_duty_cycle(pwm, args->duty_cycle);
> > 		pwm_set_period(pwm, args->period);
> > 	}
> > 
> > ? That would make it slightly easier to get rid of it again after all
> > clients have been converted.
> > 
> > With the exception of pwm-clps711x all of these args are set at of_xlate
> > time (for DT) or from the lookup table in pwm_get() (for non-DT), so it
> > might even be possible to move this call to the core, so that removal of
> > it will be a one-liner.
> 
> Okay, I think I misunderstood your suggestion. I thought you wanted
> this helper to set the reference config, but you actually want to apply
> a new state based on the PWM reference values.
> 
> Except that pwm_args does not contain all the required information to
> apply a full config (args->duty_cycle and args->enable do not exist).
> 
> This being said, in my v6 I moved the content of
> pwm_regulator_adjust_pwm_config() (patch 27) into a generic helper
> (pwm_adjust_config()). This helper is doing pretty much what you're
> suggesting here (but again, I'm not sure I correctly understood your
> suggestion :-/).

I'm not suggesting that pwm_apply_args() apply any state. I think we
both agreed earlier that the initial state (represented by pwm_args) was
never to be automatically applied.

What I was suggesting is that we move all the calls to pwm_set_period()
and pwm_set_duty_cycle() into a central location to make it easier to
remove them later in the series. This is really only temporary, so I
don't mind if we leave the calls sprinkled all over the place. At least
that way I hope we'll avoid confusion about what we're talking about =)

Thierry

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web