Re: [PATCH 3/6] pwm: renesas-tpu: Implement .apply() callback

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Simon Horman <horms+renesas@verge.net.au>,
	Magnus Damm <magnus.damm@gmail.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	Lee Jones <lee.jones@linaro.org>,
	Linux PWM List <linux-pwm@vger.kernel.org>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	Sascha Hauer <kernel@pengutronix.de>
Subject: Re: [PATCH 3/6] pwm: renesas-tpu: Implement .apply() callback
Date: Thu, 14 Apr 2022 14:16:01 +0200	[thread overview]
Message-ID: <CAMuHMdVzyZMdTciqscW8r8+9qpyFUJkKs10O8fsOa++tvc3LxA@mail.gmail.com> (raw)
In-Reply-To: <CAMuHMdUsmjjKWBWNWr9DCmbWM8CJzXxDbcpjzystYhT3tgFtjg@mail.gmail.com>

Hi Uwe,

On Thu, Apr 14, 2022 at 11:18 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Wed, Apr 13, 2022 at 10:51 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > To eventually get rid of all legacy drivers convert this driver to the
> > modern world implementing .apply().
> >
> > As pwm->state might not be updated in tpu_pwm_apply() before calling
> > tpu_pwm_config(), an additional parameter is needed for tpu_pwm_config()
> > to not change the implemented logic.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>
> LGTM, so
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> The display backlight still works fine on r8a7740/armadillo, so
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Oops, I spoke too soon...

> > @@ -366,13 +366,45 @@ static void tpu_pwm_disable(struct pwm_chip *chip, struct pwm_device *_pwm)
> >         tpu_pwm_timer_stop(pwm);
> >  }
> >
> > +static int tpu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                        const struct pwm_state *state)
> > +{
> > +       int err;
> > +       bool enabled = pwm->state.enabled;
> > +
> > +       if (state->polarity != pwm->state.polarity) {
> > +               if (enabled) {
> > +                       tpu_pwm_disable(chip, pwm);
> > +                       enabled = false;
> > +               }
> > +
> > +               err = tpu_pwm_set_polarity(chip, pwm, state->polarity);
> > +               if (err)
> > +                       return err;
> > +       }
> > +
> > +       if (!state->enabled) {
> > +               if (enabled)
> > +                       chip->ops->disable(chip, pwm);

tpu_pwm_disable

else it crashes with a NULL-pointer dereference (e.g. during system
shutdown).

> > +
> > +               return 0;
> > +       }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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