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

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

 



On Fri, Sep 4, 2020 at 4:06 PM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
> On Fri, Sep 4, 2020 at 4:48 PM Dongchun Zhu <dongchun.zhu@xxxxxxxxxxxx> 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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux