Re: [PATCH v4 6/9] media: i2c: ov5645: Use runtime PM

[Lad, Prabhakar]

Hi Hans,

On Mon, Nov 28, 2022 at 1:49 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On 11/1/22 00:21, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
<snip>
> > -static void ov5645_set_power_off(struct ov5645 *ov5645)
> > -{
> > -     gpiod_set_value_cansleep(ov5645->rst_gpio, 1);
> > -     gpiod_set_value_cansleep(ov5645->enable_gpio, 0);
> > -     clk_disable_unprepare(ov5645->xclk);
> > -     regulator_bulk_disable(OV5645_NUM_SUPPLIES, ov5645->supplies);
> > -}
> > -
> > -static int ov5645_s_power(struct v4l2_subdev *sd, int on)
> > -{
> > -     struct ov5645 *ov5645 = to_ov5645(sd);
> > -     int ret = 0;
> > -
> > -     mutex_lock(&ov5645->power_lock);
> > -
> > -     /* If the power count is modified from 0 to != 0 or from != 0 to 0,
> > -      * update the power state.
> > -      */
> > -     if (ov5645->power_count == !on) {
> > -             if (on) {
> > -                     ret = ov5645_set_power_on(ov5645);
> > -                     if (ret < 0)
> > -                             goto exit;
> > -
> > -                     ret = ov5645_set_register_array(ov5645,
> > -                                     ov5645_global_init_setting,
> > +     ret = ov5645_set_register_array(ov5645, ov5645_global_init_setting,
> >                                       ARRAY_SIZE(ov5645_global_init_setting));
> > -                     if (ret < 0) {
> > -                             dev_err(ov5645->dev,
> > -                                     "could not set init registers\n");
> > -                             ov5645_set_power_off(ov5645);
> > -                             goto exit;
> > -                     }
> > -
> > -                     usleep_range(500, 1000);
> > -             } else {
> > -                     ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x58);
> > -                     ov5645_set_power_off(ov5645);
> > -             }
> > +     if (ret < 0) {
> > +             dev_err(ov5645->dev, "could not set init registers\n");
> > +             goto exit;
> >       }
> >
> > -     /* Update the power count. */
> > -     ov5645->power_count += on ? 1 : -1;
> > -     WARN_ON(ov5645->power_count < 0);
> > +     usleep_range(500, 1000);
> >
> > -exit:
> > -     mutex_unlock(&ov5645->power_lock);
> > +     return 0;
> >
> > +exit:
> > +     ov5645_set_power_off(dev);
> >       return ret;
>
> smatch gives this warning:
>
> drivers/media/i2c/ov5645.c:687 ov5645_set_power_on() warn: 'ov5645->xclk' from clk_prepare_enable() not released on lines: 687.
>
it's a false positive, as in case of error we jump to the exit label
which calls ov5645_set_power_off() which internally calls
clk_disable_unprepare() for xclk and on success we need to have xclk
ON during s_stream(ON) and eventually the
xclk will be unprepared in s_stream(OFF).

Cheers,
Prabhakar