[1/4] pwm: Read initial hardware state at request time
diff mbox series

Message ID 20191021105739.1357629-1-thierry.reding@gmail.com
State New
Headers show
Series
  • [1/4] pwm: Read initial hardware state at request time
Related show

Commit Message

Thierry Reding Oct. 21, 2019, 10:57 a.m. UTC
The PWM core doesn't need to know about the hardware state of a PWM
unless there is a user for it. Defer initial hardware readout until
a PWM is requested.

As a side-effect, this allows the ->get_state() callback to rely on
per-PWM data.

Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
---
 drivers/pwm/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Uwe Kleine-König Oct. 21, 2019, 11:11 a.m. UTC | #1
Hello Thierry,

On Mon, Oct 21, 2019 at 12:57:36PM +0200, Thierry Reding wrote:
> The PWM core doesn't need to know about the hardware state of a PWM
> unless there is a user for it. Defer initial hardware readout until
> a PWM is requested.

A side effect is that for an unused PWM the get_state callback is never
called (which is good), in return it is called more than once if the PWM
is requested more often (which is bearable).

> As a side-effect, this allows the ->get_state() callback to rely on
> per-PWM data.

Given that this is the motivation for your change I'd give more stress
to this part of the commit log. Also I think this could be more
understandable if you point out that the effect is that .get_state is
only called after .request was called successfully which gives the low
level driver more freedom by (for example) relying on memory allocated
there.

I assume you target the next merge window for this change?

Best regards
Uwe
Michal Vokáč Oct. 21, 2019, 1:34 p.m. UTC | #2
On 21. 10. 19 12:57, Thierry Reding wrote:
> The PWM core doesn't need to know about the hardware state of a PWM
> unless there is a user for it. Defer initial hardware readout until
> a PWM is requested.
> 
> As a side-effect, this allows the ->get_state() callback to rely on
> per-PWM data.

I tried this on top of v5.4-rc3 on imx6dl-yapp4-draco with pwm-backlight
consumer and on imx6dl-yapp4-hydra with PWM from sysfs and I do not see
any obvious problems.

Tested-by: Michal Vokáč <michal.vokac@ysoft.com>

> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> ---
>   drivers/pwm/core.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index f877e77d9184..e067873c6cc5 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -114,6 +114,9 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
>   		}
>   	}
>   
> +	if (pwm->chip->ops->get_state)
> +		pwm->chip->ops->get_state(pwm->chip, pwm, &pwm->state);
> +
>   	set_bit(PWMF_REQUESTED, &pwm->flags);
>   	pwm->label = label;
>   
> @@ -283,9 +286,6 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
>   		pwm->hwpwm = i;
>   		pwm->state.polarity = polarity;
>   
> -		if (chip->ops->get_state)
> -			chip->ops->get_state(chip, pwm, &pwm->state);
> -
>   		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
>   	}
>   
>
Enric Balletbo i Serra Oct. 21, 2019, 1:46 p.m. UTC | #3
Hi,

On 21/10/19 12:57, Thierry Reding wrote:
> The PWM core doesn't need to know about the hardware state of a PWM
> unless there is a user for it. Defer initial hardware readout until
> a PWM is requested.
> 
> As a side-effect, this allows the ->get_state() callback to rely on
> per-PWM data.
> 
> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>

Tested on top of 5.4.0-rc4 with 2/4 applied this patch fixes the NULL pointer
dereference as expected. So,

Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Thanks,
 Enric

> ---
>  drivers/pwm/core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index f877e77d9184..e067873c6cc5 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -114,6 +114,9 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
>  		}
>  	}
>  
> +	if (pwm->chip->ops->get_state)
> +		pwm->chip->ops->get_state(pwm->chip, pwm, &pwm->state);
> +
>  	set_bit(PWMF_REQUESTED, &pwm->flags);
>  	pwm->label = label;
>  
> @@ -283,9 +286,6 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
>  		pwm->hwpwm = i;
>  		pwm->state.polarity = polarity;
>  
> -		if (chip->ops->get_state)
> -			chip->ops->get_state(chip, pwm, &pwm->state);
> -
>  		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
>  	}
>  
>
Thierry Reding Oct. 21, 2019, 2:27 p.m. UTC | #4
On Mon, Oct 21, 2019 at 01:11:12PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Mon, Oct 21, 2019 at 12:57:36PM +0200, Thierry Reding wrote:
> > The PWM core doesn't need to know about the hardware state of a PWM
> > unless there is a user for it. Defer initial hardware readout until
> > a PWM is requested.
> 
> A side effect is that for an unused PWM the get_state callback is never
> called (which is good), in return it is called more than once if the PWM
> is requested more often (which is bearable).

You can't request a PWM more than once. PWMs are always exclusive to a
single driver. Now I suppose you could have a single driver request it
multiple times (that driver would then also have to release it before
requesting it again), but I think it's reasonable for the subsystem to
query the hardware state everytime before a PWM is handed to a consumer.
The hardware state could have changed between the time where a consumer
releases the PWM and another requests it.

> 
> > As a side-effect, this allows the ->get_state() callback to rely on
> > per-PWM data.
> 
> Given that this is the motivation for your change I'd give more stress
> to this part of the commit log. Also I think this could be more
> understandable if you point out that the effect is that .get_state is
> only called after .request was called successfully which gives the low
> level driver more freedom by (for example) relying on memory allocated
> there.

Isn't that pretty much already in the above commit message just with
different words? I can try to reword this in a different way if that
makes you happier.

> I assume you target the next merge window for this change?

Yes. I'm not sure yet about the remainder of the series. Depending on
what we decide to do about drivers that can't (or don't want to) write
all state through to the hardware, patches 2-4 may become moot.

Thierry

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

Patch
diff mbox series

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index f877e77d9184..e067873c6cc5 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -114,6 +114,9 @@  static int pwm_device_request(struct pwm_device *pwm, const char *label)
 		}
 	}
 
+	if (pwm->chip->ops->get_state)
+		pwm->chip->ops->get_state(pwm->chip, pwm, &pwm->state);
+
 	set_bit(PWMF_REQUESTED, &pwm->flags);
 	pwm->label = label;
 
@@ -283,9 +286,6 @@  int pwmchip_add_with_polarity(struct pwm_chip *chip,
 		pwm->hwpwm = i;
 		pwm->state.polarity = polarity;
 
-		if (chip->ops->get_state)
-			chip->ops->get_state(chip, pwm, &pwm->state);
-
 		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
 	}