| Message ID | 20191021105739.1357629-1-thierry.reding@gmail.com |
|---|---|
| State | Accepted |
| Commit | cfc4c189bc70b1acc17e6f1abf1dc1c0ae890bd8 |
| Headers | show |
| Series | [1/4] pwm: Read initial hardware state at request time | expand |
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
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); > } > >
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); > } > >
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/ |
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); }
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(-)