Re: [PATCH v10 16/21] V4L2: support asynchronous subdevice registration

Sylwester Nawrocki
2013-06-13T21:37:35Z

Hi Guennadi,

Overall it looks quite neat at this v10. :)

On 06/11/2013 10:23 AM, Guennadi Liakhovetski wrote:
> Currently bridge device drivers register devices for all subdevices
> synchronously, tupically, during their probing. E.g. if an I2C CMOS sensor

s/tupically/typically

> is attached to a video bridge device, the bridge driver will create an I2C

> +/**
> + * v4l2_async_subdev_list - provided by subdevices
> + * @list: links struct v4l2_async_subdev_list objects to a global list
> + * before probing, and onto notifier->done after probing
> + * @asd: pointer to respective struct v4l2_async_subdev
> + * @notifier: pointer to managing notifier
> + */
> +struct v4l2_async_subdev_list {
> + struct list_head list;
> + struct v4l2_async_subdev *asd;
> + struct v4l2_async_notifier *notifier;
> +};

I have a patch for this patch, which embeds members of this struct directly
into struct v4l2_subdev. My felling is that the code is simpler and easier
to follow this way, I might be missing some important details though.

> +/**
> + * v4l2_async_notifier - v4l2_device notifier data
> + * @subdev_num: number of subdevices
> + * @subdev: array of pointers to subdevices

How about changing this to:

@subdevs: array of pointers to the subdevice descriptors

I think it would be more immediately clear this is the actual subdevs array
pointer, and perhaps we could have subdev_num renamed to num_subdevs ?

> + * @v4l2_dev: pointer to struct v4l2_device
> + * @waiting: list of struct v4l2_async_subdev, waiting for their drivers
> + * @done: list of struct v4l2_async_subdev_list, already probed
> + * @list: member in a global list of notifiers
> + * @bound: a subdevice driver has successfully probed one of subdevices
> + * @complete: all subdevices have been probed successfully
> + * @unbind: a subdevice is leaving
> + */
> +struct v4l2_async_notifier {
> + unsigned int subdev_num;
> + struct v4l2_async_subdev **subdev;
> + struct v4l2_device *v4l2_dev;
> + struct list_head waiting;
> + struct list_head done;
> + struct list_head list;
> + int (*bound)(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_subdev *asd);
> + int (*complete)(struct v4l2_async_notifier *notifier);
> + void (*unbind)(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_subdev *asd);
> +};
> +
> +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> + struct v4l2_async_notifier *notifier);
> +void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier);
> +int v4l2_async_register_subdev(struct v4l2_subdev *sd);
> +void v4l2_async_unregister_subdev(struct v4l2_subdev *sd);

I still think "async_" in this public API is unnecessary, since we register/
unregister a subdev with the core and notifiers are intrinsically
asynchronous.
But your preference seems be otherwise, what could I do... :) At most it
just
means one less happy user of this interface.

So except this bikeshedding I don't really have other comments, I'm
going to
test this series with the s3c-camif/ov9650 drivers and will report back
soon.

It would have been a shame to not have this series in 3.11. I guess three
kernel cycles, since the initial implementation, time frame is sufficient
for having finally working camera devices on a device tree enabled system
in mainline.

Thanks for the idea and patience during reviews! :)

Regards,
Sylwester