Re: RESEND[PATCH v6 2/2] media: dw9807: Add dw9807 vcm driver

[Tomasz Figa <tfiga@chromium.org>]

Hi Andy,

On Wed, Apr 11, 2018 at 12:54 AM Yeh, Andy <andy.yeh@intel.com> 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