Hi Yong,
On Thu, Jul 13, 2017 at 8:20 AM, Zhi, Yong <yong....@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
