Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1367564 > unrolled thread
| Started by | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| First post | 2016-03-30 22:20 +0200 |
| Last post | 2016-04-01 10:40 +0200 |
| Articles | 4 — 3 participants |
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.
[PATCH v5 08/46] hwmon: pwm-fan: use pwm_get_args() where appropriate Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-03-30 22:20 +0200
Re: [PATCH v5 08/46] hwmon: pwm-fan: use pwm_get_args() where appropriate Guenter Roeck <linux@roeck-us.net> - 2016-03-31 01:00 +0200
Re: [PATCH v5 08/46] hwmon: pwm-fan: use pwm_get_args() where appropriate Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-03-31 09:10 +0200
RE: [PATCH v5 08/46] hwmon: pwm-fan: use pwm_get_args() where appropriate Kamil Debski <k.debski@samsung.com> - 2016-04-01 10:40 +0200
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2016-03-30 22:20 +0200 |
| Subject | [PATCH v5 08/46] hwmon: pwm-fan: use pwm_get_args() where appropriate |
| Message-ID | <riuKS-2cZ-3@gated-at.bofh.it> |
The PWM framework has clarified the concept of reference PWM config
(the platform dependent config retrieved from the DT or the PWM
lookup table) and real PWM state.
Use pwm_get_args() when the PWM user wants to retrieve this reference
config and not the current state.
This is part of the rework allowing the PWM framework to support
hardware readout and expose real PWM state even when the PWM has
just been requested (before the user calls pwm_config/enable/disable()).
Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
drivers/hwmon/pwm-fan.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 3e23003..82c5656 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -40,15 +40,18 @@ struct pwm_fan_ctx {
static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
{
+ struct pwm_args pargs = { };
unsigned long duty;
int ret = 0;
+ pwm_get_args(ctx->pwm, &pargs);
+
mutex_lock(&ctx->lock);
if (ctx->pwm_value == pwm)
goto exit_set_pwm_err;
- duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM);
- ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
+ duty = DIV_ROUND_UP(pwm * (pargs.period - 1), MAX_PWM);
+ ret = pwm_config(ctx->pwm, duty, pargs.period);
if (ret)
goto exit_set_pwm_err;
@@ -214,6 +217,7 @@ static int pwm_fan_of_get_cooling_data(struct device *dev,
static int pwm_fan_probe(struct platform_device *pdev)
{
struct thermal_cooling_device *cdev;
+ struct pwm_args pargs = { };
struct pwm_fan_ctx *ctx;
struct device *hwmon;
int duty_cycle;
@@ -234,10 +238,11 @@ static int pwm_fan_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, ctx);
/* Set duty cycle to maximum allowed */
- duty_cycle = ctx->pwm->period - 1;
+ pwm_get_args(ctx->pwm, &pargs);
+ duty_cycle = pargs.period - 1;
ctx->pwm_value = MAX_PWM;
- ret = pwm_config(ctx->pwm, duty_cycle, ctx->pwm->period);
+ ret = pwm_config(ctx->pwm, duty_cycle, pargs.period);
if (ret) {
dev_err(&pdev->dev, "Failed to configure PWM\n");
return ret;
@@ -303,14 +308,16 @@ static int pwm_fan_suspend(struct device *dev)
static int pwm_fan_resume(struct device *dev)
{
struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
+ struct pwm_args pargs = { };
unsigned long duty;
int ret;
if (ctx->pwm_value == 0)
return 0;
- duty = DIV_ROUND_UP(ctx->pwm_value * (ctx->pwm->period - 1), MAX_PWM);
- ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
+ pwm_get_args(ctx->pwm, &pargs);
+ duty = DIV_ROUND_UP(ctx->pwm_value * (pargs.period - 1), MAX_PWM);
+ ret = pwm_config(ctx->pwm, duty, pargs.period);
if (ret)
return ret;
return pwm_enable(ctx->pwm);
--
2.5.0
[toc] | [next] | [standalone]
| From | Guenter Roeck <linux@roeck-us.net> |
|---|---|
| Date | 2016-03-31 01:00 +0200 |
| Subject | Re: [PATCH v5 08/46] hwmon: pwm-fan: use pwm_get_args() where appropriate |
| Message-ID | <rixfI-3IS-1@gated-at.bofh.it> |
| In reply to | #1367564 |
On Wed, Mar 30, 2016 at 10:03:31PM +0200, Boris Brezillon wrote:
> The PWM framework has clarified the concept of reference PWM config
> (the platform dependent config retrieved from the DT or the PWM
> lookup table) and real PWM state.
>
> Use pwm_get_args() when the PWM user wants to retrieve this reference
> config and not the current state.
>
> This is part of the rework allowing the PWM framework to support
> hardware readout and expose real PWM state even when the PWM has
> just been requested (before the user calls pwm_config/enable/disable()).
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> drivers/hwmon/pwm-fan.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index 3e23003..82c5656 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -40,15 +40,18 @@ struct pwm_fan_ctx {
>
> static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
> {
> + struct pwm_args pargs = { };
Hi Boris,
I guess I am missing some context; sorry for that. Unfortunately,
I did not easily find an explanation, so please bear with me.
Two questions: Why do we need a local copy of struct pwm_args instead
of a pointer to it ? If it can change while being used, isn't it
inconsistent anyway ?
Also, assuming the local copy is necessary, why initialize pargs ?
After all, pwm_get_args() just overwrites it.
Thanks,
Guenter
> unsigned long duty;
> int ret = 0;
>
> + pwm_get_args(ctx->pwm, &pargs);
> +
> mutex_lock(&ctx->lock);
> if (ctx->pwm_value == pwm)
> goto exit_set_pwm_err;
>
> - duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM);
> - ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
> + duty = DIV_ROUND_UP(pwm * (pargs.period - 1), MAX_PWM);
> + ret = pwm_config(ctx->pwm, duty, pargs.period);
> if (ret)
> goto exit_set_pwm_err;
>
> @@ -214,6 +217,7 @@ static int pwm_fan_of_get_cooling_data(struct device *dev,
> static int pwm_fan_probe(struct platform_device *pdev)
> {
> struct thermal_cooling_device *cdev;
> + struct pwm_args pargs = { };
> struct pwm_fan_ctx *ctx;
> struct device *hwmon;
> int duty_cycle;
> @@ -234,10 +238,11 @@ static int pwm_fan_probe(struct platform_device *pdev)
> platform_set_drvdata(pdev, ctx);
>
> /* Set duty cycle to maximum allowed */
> - duty_cycle = ctx->pwm->period - 1;
> + pwm_get_args(ctx->pwm, &pargs);
> + duty_cycle = pargs.period - 1;
> ctx->pwm_value = MAX_PWM;
>
> - ret = pwm_config(ctx->pwm, duty_cycle, ctx->pwm->period);
> + ret = pwm_config(ctx->pwm, duty_cycle, pargs.period);
> if (ret) {
> dev_err(&pdev->dev, "Failed to configure PWM\n");
> return ret;
> @@ -303,14 +308,16 @@ static int pwm_fan_suspend(struct device *dev)
> static int pwm_fan_resume(struct device *dev)
> {
> struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> + struct pwm_args pargs = { };
> unsigned long duty;
> int ret;
>
> if (ctx->pwm_value == 0)
> return 0;
>
> - duty = DIV_ROUND_UP(ctx->pwm_value * (ctx->pwm->period - 1), MAX_PWM);
> - ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
> + pwm_get_args(ctx->pwm, &pargs);
> + duty = DIV_ROUND_UP(ctx->pwm_value * (pargs.period - 1), MAX_PWM);
> + ret = pwm_config(ctx->pwm, duty, pargs.period);
> if (ret)
> return ret;
> return pwm_enable(ctx->pwm);
> --
> 2.5.0
>
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2016-03-31 09:10 +0200 |
| Subject | Re: [PATCH v5 08/46] hwmon: pwm-fan: use pwm_get_args() where appropriate |
| Message-ID | <riETU-1kS-3@gated-at.bofh.it> |
| In reply to | #1367657 |
Hi Guenter,
On Wed, 30 Mar 2016 15:52:44 -0700
Guenter Roeck <linux@roeck-us.net> wrote:
> On Wed, Mar 30, 2016 at 10:03:31PM +0200, Boris Brezillon wrote:
> > The PWM framework has clarified the concept of reference PWM config
> > (the platform dependent config retrieved from the DT or the PWM
> > lookup table) and real PWM state.
> >
> > Use pwm_get_args() when the PWM user wants to retrieve this reference
> > config and not the current state.
> >
> > This is part of the rework allowing the PWM framework to support
> > hardware readout and expose real PWM state even when the PWM has
> > just been requested (before the user calls pwm_config/enable/disable()).
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> > drivers/hwmon/pwm-fan.c | 19 +++++++++++++------
> > 1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> > index 3e23003..82c5656 100644
> > --- a/drivers/hwmon/pwm-fan.c
> > +++ b/drivers/hwmon/pwm-fan.c
> > @@ -40,15 +40,18 @@ struct pwm_fan_ctx {
> >
> > static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
> > {
> > + struct pwm_args pargs = { };
>
> Hi Boris,
>
> I guess I am missing some context; sorry for that. Unfortunately,
> I did not easily find an explanation, so please bear with me.
>
> Two questions: Why do we need a local copy of struct pwm_args instead
> of a pointer to it ? If it can change while being used, isn't it
> inconsistent anyway ?
It cannot change after pwm_get() is called. For the reason behind
prototype: I just followed the Thierry's proposal, but I'm perfectly
fine returning a const struct pwm_args pointer intead of passing
pwm_args as a parameter.
Thierry, what's your opinion?
>
> Also, assuming the local copy is necessary, why initialize pargs ?
> After all, pwm_get_args() just overwrites it.
It's a leftover from a previous version where pwm_get_args was
implemented this way:
static inline void pwm_get_args(pwm, args)
{
if (pwm)
*args = pwm->args
}
and this implementation was generating a lot of 'uninitialized
variable' warnings.
I just decided to drop the 'if (pwm)' test, because, IMO, this
should be checked way before calling pwm_get_args() is called.
Anyway, apparently I forgot to modify a few patches after this
modification.
Thanks for the review.
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[toc] | [prev] | [next] | [standalone]
| From | Kamil Debski <k.debski@samsung.com> |
|---|---|
| Date | 2016-04-01 10:40 +0200 |
| Subject | RE: [PATCH v5 08/46] hwmon: pwm-fan: use pwm_get_args() where appropriate |
| Message-ID | <rj2My-1GR-15@gated-at.bofh.it> |
| In reply to | #1367564 |
Hi Boris, From: Boris Brezillon [mailto:boris.brezillon@free-electrons.com] Sent: Wednesday, March 30, 2016 10:04 PM Subject: [PATCH v5 08/46] hwmon: pwm-fan: use pwm_get_args() where appropriate > > The PWM framework has clarified the concept of reference PWM config (the > platform dependent config retrieved from the DT or the PWM lookup table) > and real PWM state. > > Use pwm_get_args() when the PWM user wants to retrieve this reference > config and not the current state. > > This is part of the rework allowing the PWM framework to support hardware > readout and expose real PWM state even when the PWM has just been > requested (before the user calls pwm_config/enable/disable()). > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> Acked-by: Kamil Debski <k.debski@samsung.com> Best wishes, -- Kamil Debski Samsung R&D Institute Poland
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web