Re: [PATCH v2] media: imx208: Add imx208 camera sensor driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jul 30, 2018 at 8:39 PM Sakari Ailus
<sakari.ailus@xxxxxxxxxxxxxxx> wrote:
>
> Hi Tomasz,
>
> On Mon, Jul 30, 2018 at 07:19:56PM +0900, Tomasz Figa wrote:
> ...
> > > +static int imx208_set_ctrl(struct v4l2_ctrl *ctrl)
> > > +{
> > > +       struct imx208 *imx208 =
> > > +               container_of(ctrl->handler, struct imx208, ctrl_handler);
> > > +       struct i2c_client *client = v4l2_get_subdevdata(&imx208->sd);
> > > +       int ret;
> > > +
> > > +       /*
> > > +        * Applying V4L2 control value only happens
> > > +        * when power is up for streaming
> > > +        */
> > > +       if (pm_runtime_get_if_in_use(&client->dev) <= 0)
> >
> > This is buggy, because it won't handle the case of runtime PM disabled
> > in kernel config. The check should be
> > (!pm_runtime_get_if_in_use(&client->dev)).
>
> Good find. This seems to be the case for most other sensor drivers that
> make use of the function. I can submit a fix for those as well.
>
> I suppose most people use these with runtime PM enabled as this hasn't been
> spotted previously.

Yeah, I spotted it first with imx258 and it took us few emails to get
to the right code. :)

These drivers probably don't have too many users yet in general, so I
guess this didn't manage to cause any problems yet, but it became a
good example of bug propagation via copy/paste. ;)

Fixing would be appreciated indeed!

Best regards,
Tomasz



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux