Hi Andy, On Wed, Apr 11, 2018 at 12:54 AM Yeh, Andy <andy.yeh@xxxxxxxxx> wrote: > Hi Jacopo, > Excuse for late reply, we were busy in past weeks for major milestone. Please kindly check the revised V7 which has been uploaded. > https://patchwork.linuxtv.org/patch/48589/ > Responded to your comments as below. > Cc in Tomasz for unintentionally missed. > Regards, Andy [snip] > > +static int dw9807_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh > > +*fh) { > > + int rval; > > + > > + rval = pm_runtime_get_sync(sd->dev); > > + if (rval < 0) { > > + pm_runtime_put_noidle(sd->dev); > > If you fail to get pm context, no need to put it back (I presume) > According to Sakari Ailus's comment on LinuxTV. > (pm_runtime_get() must be followed by pm_runtime_put() whether the former > succeeds or not.) > So it is no need to modify. Andy is right. pm_runtime_get() always acquires a PM runtime count, even in case of error. [snip] > > +static const struct of_device_id dw9807_of_table[] = { > > + { .compatible = "dongwoon,dw9807" }, > > + { { 0 } } > > { } is enough. > According to Sakari Ailus's comment on LinuxTV. > { } is GCC specific while { { 0 } } isn't. > And if I remove it, compile error will occur. Hmm, we're in the heavy nitpicking territory here, but { }, is the typical pattern used throughout the kernel. I personally actually put a comment inside: { /* sentinel */ }, Just my opinion. I'm fine with keeping it either way, if no need to re-spin for other changes. Best regards, Tomasz
