| Message ID | YXqv339PJTHcGxJg@fedora (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ir-rx51: Switch to atomic PWM API | expand |
Hello Maíra, On Thu, Oct 28, 2021 at 11:12:47AM -0300, Maíra Canal wrote: > Remove legacy PWM interface (pwm_config, pwm_enable, pwm_disable) and > replace it for the atomic PWM API. > > Signed-off-by: Maíra Canal <maira.canal@usp.br> > --- > drivers/media/rc/ir-rx51.c | 27 +++++++++++++-------------- > 1 file changed, 13 insertions(+), 14 deletions(-) > > diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c > index a0d9c02a7588..7a643a94e181 100644 > --- a/drivers/media/rc/ir-rx51.c > +++ b/drivers/media/rc/ir-rx51.c > @@ -19,6 +19,7 @@ > struct ir_rx51 { > struct rc_dev *rcdev; > struct pwm_device *pwm; > + struct pwm_state *state; > struct hrtimer timer; > struct device *dev; > wait_queue_head_t wqueue; > @@ -32,22 +33,22 @@ struct ir_rx51 { > > static inline void ir_rx51_on(struct ir_rx51 *ir_rx51) > { > - pwm_enable(ir_rx51->pwm); > + ir_rx51->state->enabled = true; > + pwm_apply_state(ir_rx51->pwm, ir_rx51->state); > } > > static inline void ir_rx51_off(struct ir_rx51 *ir_rx51) > { > - pwm_disable(ir_rx51->pwm); > + ir_rx51->state->enabled = false; > + pwm_apply_state(ir_rx51->pwm, ir_rx51->state); > } > > static int init_timing_params(struct ir_rx51 *ir_rx51) > { > - struct pwm_device *pwm = ir_rx51->pwm; > - int duty, period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, ir_rx51->freq); > + struct pwm_state *state = ir_rx51->state; > > - duty = DIV_ROUND_CLOSEST(ir_rx51->duty_cycle * period, 100); > - > - pwm_config(pwm, duty, period); > + state->period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, ir_rx51->freq); > + pwm_set_relative_duty_cycle(state, ir_rx51->duty_cycle, 100); > > return 0; > } > @@ -154,6 +155,8 @@ static int ir_rx51_open(struct rc_dev *dev) > return res; > } > > + pwm_init_state(ir_rx51->pwm, ir_rx51->state); > + Doing this here introduces a change in behaviour. Better do this after pwm_get(). > return 0; > } > > @@ -232,13 +235,9 @@ static int ir_rx51_probe(struct platform_device *dev) > struct rc_dev *rcdev; > > pwm = pwm_get(&dev->dev, NULL); > - if (IS_ERR(pwm)) { > - int err = PTR_ERR(pwm); > - > - if (err != -EPROBE_DEFER) > - dev_err(&dev->dev, "pwm_get failed: %d\n", err); > - return err; > - } > + if (IS_ERR(pwm)) > + return dev_err_probe(&dev->dev, PTR_ERR(pwm), "pwm_get failed: %ld\n", > + PTR_ERR(pwm)); > > /* Use default, in case userspace does not set the carrier */ > ir_rx51.freq = DIV_ROUND_CLOSEST_ULL(pwm_get_period(pwm), NSEC_PER_SEC); Conceptually this last hunk belongs in a separate patch. And you don't need to repeat PTR_ERR(pwm), as dev_err_probe already emits this information. So return dev_err_probe(&dev->dev, PTR_ERR(pwm), "pwm_get failed\n"); should be fine. Best regards Uwe
Hello Uwe, > Doing this here introduces a change in behaviour. Better do this after > pwm_get(). I didn't really get this feedback. Isn't pwm_init_state after pwm_get? Or should it be before the error treatment of pwm_get? > Conceptually this last hunk belongs in a separate patch. And you don't > need to repeat PTR_ERR(pwm), as dev_err_probe already emits this > information. So > > return dev_err_probe(&dev->dev, PTR_ERR(pwm), "pwm_get failed\n"); > > should be fine. Thank you for the suggestion! I will fix it on v2. Maíra
On Sat, Oct 30, 2021 at 08:20:50AM -0300, Maíra Canal wrote: > Hello Uwe, > > > Doing this here introduces a change in behaviour. Better do this after > > pwm_get(). > > I didn't really get this feedback. Isn't pwm_init_state after pwm_get? > Or should it be before the error treatment of pwm_get? It is, but it might be repeated resetting the pwm settings when the device is reopened. Best regards Uwe
diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c index a0d9c02a7588..7a643a94e181 100644 --- a/drivers/media/rc/ir-rx51.c +++ b/drivers/media/rc/ir-rx51.c @@ -19,6 +19,7 @@ struct ir_rx51 { struct rc_dev *rcdev; struct pwm_device *pwm; + struct pwm_state *state; struct hrtimer timer; struct device *dev; wait_queue_head_t wqueue; @@ -32,22 +33,22 @@ struct ir_rx51 { static inline void ir_rx51_on(struct ir_rx51 *ir_rx51) { - pwm_enable(ir_rx51->pwm); + ir_rx51->state->enabled = true; + pwm_apply_state(ir_rx51->pwm, ir_rx51->state); } static inline void ir_rx51_off(struct ir_rx51 *ir_rx51) { - pwm_disable(ir_rx51->pwm); + ir_rx51->state->enabled = false; + pwm_apply_state(ir_rx51->pwm, ir_rx51->state); } static int init_timing_params(struct ir_rx51 *ir_rx51) { - struct pwm_device *pwm = ir_rx51->pwm; - int duty, period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, ir_rx51->freq); + struct pwm_state *state = ir_rx51->state; - duty = DIV_ROUND_CLOSEST(ir_rx51->duty_cycle * period, 100); - - pwm_config(pwm, duty, period); + state->period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, ir_rx51->freq); + pwm_set_relative_duty_cycle(state, ir_rx51->duty_cycle, 100); return 0; } @@ -154,6 +155,8 @@ static int ir_rx51_open(struct rc_dev *dev) return res; } + pwm_init_state(ir_rx51->pwm, ir_rx51->state); + return 0; } @@ -232,13 +235,9 @@ static int ir_rx51_probe(struct platform_device *dev) struct rc_dev *rcdev; pwm = pwm_get(&dev->dev, NULL); - if (IS_ERR(pwm)) { - int err = PTR_ERR(pwm); - - if (err != -EPROBE_DEFER) - dev_err(&dev->dev, "pwm_get failed: %d\n", err); - return err; - } + if (IS_ERR(pwm)) + return dev_err_probe(&dev->dev, PTR_ERR(pwm), "pwm_get failed: %ld\n", + PTR_ERR(pwm)); /* Use default, in case userspace does not set the carrier */ ir_rx51.freq = DIV_ROUND_CLOSEST_ULL(pwm_get_period(pwm), NSEC_PER_SEC);
Remove legacy PWM interface (pwm_config, pwm_enable, pwm_disable) and replace it for the atomic PWM API. Signed-off-by: Maíra Canal <maira.canal@usp.br> --- drivers/media/rc/ir-rx51.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-)