Hi Geert-san,
> From: Geert Uytterhoeven, Sent: Friday, May 31, 2019 4:34 PM
>
> Hi Shimoda-san,
>
> On Thu, May 30, 2019 at 12:21 PM Yoshihiro Shimoda
> <[email protected]> wrote:
> > According to the Documentation/pwm.txt, all PWM consumers should have
> > power management. Since this sysfs interface is one of consumers so that
> > this patch adds suspend/resume support.
> >
> > Signed-off-by: Yoshihiro Shimoda <[email protected]>
>
> Thanks for your patch!
>
> Reviewed-by: Geert Uytterhoeven <[email protected]>
Thank you for your review!
I'll add comments you described on v3 patch.
Best regards,
Yoshihiro Shimoda
> > --- a/drivers/pwm/sysfs.c
> > +++ b/drivers/pwm/sysfs.c
>
> > @@ -372,10 +373,109 @@ static struct attribute *pwm_chip_attrs[] = {
> > };
> > ATTRIBUTE_GROUPS(pwm_chip);
> >
>
> You may want to add a comment "takes export->lock on success" here...
>
> > +static struct pwm_export *pwm_class_get_state(struct device *parent,
> > + struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct device *child;
> > + struct pwm_export *export;
> > +
> > + if (!test_bit(PWMF_EXPORTED, &pwm->flags))
> > + return NULL;
> > +
> > + child = device_find_child(parent, pwm, pwm_unexport_match);
> > + if (!child)
> > + return NULL;
> > +
> > + export = child_to_pwm_export(child);
> > + put_device(child); /* for device_find_child() */
> > +
> > + mutex_lock(&export->lock);
> > + pwm_get_state(pwm, state);
> > +
> > + return export;
> > +}
> > +
> > +static int pwm_class_apply_state(struct pwm_export *export,
> > + struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + int ret = pwm_apply_state(pwm, state);
> > +
>
> ... and "release lock taken in pwm_class_get_state()" here.
>
> > + mutex_unlock(&export->lock);
> > +
> > + return ret;
> > +}
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
