Re: [PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Tomasz Figa <tfiga@chromium.org>]

Hi Yong,

On Thu, Jul 13, 2017 at 8:20 AM, Zhi, Yong <yong.zhi@intel.com> wrote:
> Hi, Sakari,
>
> Thanks for the time spent on code review, acks to all the comments, except two places:
>
>> +/* .complete() is called after all subdevices have been located */
>> +static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
>> +{
>> +     struct cio2_device *cio2 = container_of(notifier, struct cio2_device,
>> +                                             notifier);
>> +     struct sensor_async_subdev *s_asd;
>> +     struct fwnode_handle *fwn_remote, *fwn_endpt, *fwn_remote_endpt;
>> +     struct cio2_queue *q;
>> +     struct fwnode_endpoint remote_endpt;
>> +     unsigned int i, pad;
>> +     int ret;
>> +
>> +     for (i = 0; i < notifier->num_subdevs; i++) {
>> +             s_asd = container_of(cio2->notifier.subdevs[i],
>> +                                     struct sensor_async_subdev,
>> +                                     asd);
>> +
>> +             fwn_remote = s_asd->asd.match.fwnode.fwnode;
>> +             fwn_endpt = (struct fwnode_handle *)
>> +                                     s_asd->vfwn_endpt.base.local_fwnode;
>
> Why do you need a cast?
>
> [YZ] With a cast results in compilation warning:

(I think you mean "without".)

>
> drivers/media/pci/ipu3/ipu3-cio2.c:1298:13: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
>    fwn_endpt = /*(struct fwnode_handle *)*/

This is a sign that the code is doing something wrong (in this case
probably trying to write to a const pointer), so casting just silences
the unfixed error.

>
>> +     ret = v4l2_async_notifier_register(&cio2->v4l2_dev, &cio2->notifier);
>> +     if (ret) {
>> +             cio2->notifier.num_subdevs = 0;
>
> No need to assign num_subdevs as 0.
>
> [YZ] _notifier_exit() will call _unregister() if this is not 0.

You shouldn't call _exit() if _init() failed. I noticed that many
error paths in your code does this. Please fix it.

Best regards,
Tomasz