[PATCH v14 2/2] media: i2c: Add OV02A10 image sensor driver

[Tomasz Figa]

On Fri, Sep 4, 2020 at 4:06 PM Andy Shevchenko
<andy.shevchenko at gmail.com> wrote:
>
> On Fri, Sep 4, 2020 at 4:48 PM Dongchun Zhu <dongchun.zhu at mediatek.com> wrote:
> > On Wed, 2020-09-02 at 16:44 +0300, Andy Shevchenko wrote:
> > > On Wed, Sep 02, 2020 at 08:01:22PM +0800, Dongchun Zhu wrote:
>
> ...
>
> > > > +   struct i2c_client *client = to_i2c_client(dev);
> > > > +   struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > >
> > >       struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > >
> > > Same for the rest similar cases.
> >
> > We've discussed the issue in DW9768 V2.
> >
> > For V4L2 sub-device drivers, dev_get_drvdata() shouldn't be used
> > directly.
> >
> > More details please check the Google Issue:
> > https://partnerissuetracker.corp.google.com/issues/147957975
>
> This is not a public link. Can you remind me what was the issue?
>

v4l2-subdev framework uses dev drvdata for its own purposes. However,
that problem was about the driver setting its own drvdata and having
it overridden by the framework. There is nothing wrong in using
dev_get_drvdata(), assuming the correct type is known and here it's
guaranteed to be v4l2_subdev for the v4l2-subdev framework.

In fact i2c_get_clientdata() [1] is just a wrapper that calls
dev_get_drvdata(&client->dev).

[1] https://elixir.bootlin.com/linux/v5.9-rc3/source/include/linux/i2c.h#L351

> ...
>
> > > > +   if (!bus_cfg.nr_of_link_frequencies) {
> > > > +           dev_err(dev, "no link frequencies defined\n");
> > > > +           ret = -EINVAL;
> > > > +           goto check_hwcfg_error;
> > > > +   }
> > >
> > > If it's 0, the below will break on 'if (j == 0)' with slightly different but
> > > informative enough message. What do you keep above check for?
> >
> > I still prefer to the original version.
> > If 'bus_cfg.nr_of_link_frequencies' is 0, shouldn't we directly return
> > error?
>
> But that will happen anyway. I will leave this to Sakari and
> maintainers to decide.
>

I agree with Andy on this. The check is redundant. In fact, the later
error message is more meaningful, because it at least suggests a
frequency that must be supported, while the earlier one only states
the fact.

Best regards,
Tomasz