| Message ID | 1459368249-13241-6-git-send-email-boris.brezillon@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/30, Boris Brezillon wrote: > @@ -74,6 +74,23 @@ enum pwm_polarity { > PWM_POLARITY_INVERSED, > }; > > +/** > + * struct pwm_args - PWM arguments > + * @period: reference period > + * @polarity: reference polarity > + * > + * This structure describe board-dependent arguments attached to a PWM s/describe/describes/ > + * device. Those arguments are usually retrieved from the PWM lookup table or > + * DT definition. > + * This should not be confused with the PWM state: PWM args not representing s/not representing/don't represent/ ? > + * the current PWM state, but the configuration the PWM user plan to use s/plan/plans/ > + * on this PWM device. > + */ > +struct pwm_args { > + unsigned int period; > + enum pwm_polarity polarity; > +}; > +
On Wed, 30 Mar 2016 14:55:10 -0700 Stephen Boyd <sboyd@codeaurora.org> wrote: > On 03/30, Boris Brezillon wrote: > > @@ -74,6 +74,23 @@ enum pwm_polarity { > > PWM_POLARITY_INVERSED, > > }; > > > > +/** > > + * struct pwm_args - PWM arguments > > + * @period: reference period > > + * @polarity: reference polarity > > + * > > + * This structure describe board-dependent arguments attached to a PWM > > s/describe/describes/ > > > + * device. Those arguments are usually retrieved from the PWM lookup table or > > + * DT definition. > > + * This should not be confused with the PWM state: PWM args not representing > > s/not representing/don't represent/ ? > > > + * the current PWM state, but the configuration the PWM user plan to use > > s/plan/plans/ > > > + * on this PWM device. > > + */ > > +struct pwm_args { > > + unsigned int period; > > + enum pwm_polarity polarity; > > +}; > > + > Will fix these errors. Thanks, Boris
On Wed, 30 Mar 2016 14:55:10 -0700 Stephen Boyd <sboyd@codeaurora.org> wrote: > On 03/30, Boris Brezillon wrote: > > @@ -74,6 +74,23 @@ enum pwm_polarity { > > PWM_POLARITY_INVERSED, > > }; > > > > +/** > > + * struct pwm_args - PWM arguments > > + * @period: reference period > > + * @polarity: reference polarity > > + * > > + * This structure describe board-dependent arguments attached to a PWM > > s/describe/describes/ > > > + * device. Those arguments are usually retrieved from the PWM lookup table or > > + * DT definition. > > + * This should not be confused with the PWM state: PWM args not representing > > s/not representing/don't represent/ ? Yes, I meant "are not representing", but "don't represent" is fine. > > > + * the current PWM state, but the configuration the PWM user plan to use > > s/plan/plans/ > > > + * on this PWM device. > > + */ > > +struct pwm_args { > > + unsigned int period; > > + enum pwm_polarity polarity; > > +}; > > + >
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
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?
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
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?
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 :-/).
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
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 7c330ff..cd55d61 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -146,12 +146,14 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args) if (IS_ERR(pwm)) return pwm; - pwm_set_period(pwm, args->args[1]); + pwm->args.period = args->args[1]; + pwm_set_period(pwm, pwm->args.period); if (args->args[2] & PWM_POLARITY_INVERTED) - pwm_set_polarity(pwm, PWM_POLARITY_INVERSED); + pwm->args.polarity = PWM_POLARITY_INVERSED; else - pwm_set_polarity(pwm, PWM_POLARITY_NORMAL); + pwm->args.polarity = PWM_POLARITY_NORMAL; + pwm_set_polarity(pwm, pwm->args.polarity); return pwm; } @@ -172,7 +174,8 @@ of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) if (IS_ERR(pwm)) return pwm; - pwm_set_period(pwm, args->args[1]); + pwm->args.period = args->args[1]; + pwm_set_period(pwm, pwm->args.period); return pwm; } @@ -740,6 +743,8 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id) if (IS_ERR(pwm)) goto out; + pwm->args.period = chosen->period; + pwm->args.polarity = chosen->polarity; pwm_set_period(pwm, chosen->period); pwm_set_polarity(pwm, chosen->polarity); diff --git a/drivers/pwm/pwm-clps711x.c b/drivers/pwm/pwm-clps711x.c index a80c108..807a48d 100644 --- a/drivers/pwm/pwm-clps711x.c +++ b/drivers/pwm/pwm-clps711x.c @@ -60,7 +60,8 @@ static int clps711x_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) return -EINVAL; /* Store constant period value */ - pwm_set_period(pwm, DIV_ROUND_CLOSEST(NSEC_PER_SEC, freq)); + pwm->args.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, freq); + pwm_set_period(pwm, pwm->args.period); return 0; } diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c index cb2f702..3fcc886 100644 --- a/drivers/pwm/pwm-pxa.c +++ b/drivers/pwm/pwm-pxa.c @@ -160,6 +160,7 @@ pxa_pwm_of_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) if (IS_ERR(pwm)) return pwm; + pwm->args.period = args->args[0]; pwm_set_period(pwm, args->args[0]); return pwm; diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 6555f01..ed65354 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -74,6 +74,23 @@ enum pwm_polarity { PWM_POLARITY_INVERSED, }; +/** + * struct pwm_args - PWM arguments + * @period: reference period + * @polarity: reference polarity + * + * This structure describe board-dependent arguments attached to a PWM + * device. Those arguments are usually retrieved from the PWM lookup table or + * DT definition. + * This should not be confused with the PWM state: PWM args not representing + * the current PWM state, but the configuration the PWM user plan to use + * on this PWM device. + */ +struct pwm_args { + unsigned int period; + enum pwm_polarity polarity; +}; + enum { PWMF_REQUESTED = 1 << 0, PWMF_ENABLED = 1 << 1, @@ -91,6 +108,7 @@ enum { * @period: period of the PWM signal (in nanoseconds) * @duty_cycle: duty cycle of the PWM signal (in nanoseconds) * @polarity: polarity of the PWM signal + * @args: PWM arguments */ struct pwm_device { const char *label; @@ -103,6 +121,8 @@ struct pwm_device { unsigned int period; unsigned int duty_cycle; enum pwm_polarity polarity; + + struct pwm_args args; }; static inline bool pwm_is_enabled(const struct pwm_device *pwm) @@ -142,6 +162,12 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm) return pwm ? pwm->polarity : PWM_POLARITY_NORMAL; } +static inline void pwm_get_args(const struct pwm_device *pwm, + struct pwm_args *args) +{ + *args = pwm->args; +} + /** * struct pwm_ops - PWM controller operations * @request: optional hook for requesting a PWM
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(). Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> --- drivers/pwm/core.c | 13 +++++++++---- drivers/pwm/pwm-clps711x.c | 3 ++- drivers/pwm/pwm-pxa.c | 1 + include/linux/pwm.h | 26 ++++++++++++++++++++++++++ 4 files changed, 38 insertions(+), 5 deletions(-)