Re: [PATCH 1/6 v4] media: V4L2: support asynchronous subdevice registration

Re: [PATCH 1/6 v4] media: V4L2: support asynchronous subdevice registration

[Sylwester Nawrocki]

Hi Guennadi,

Cc: LKML

On 01/08/2013 11:26 AM, Guennadi Liakhovetski wrote:
> On Tue, 8 Jan 2013, Laurent Pinchart wrote:
>> On Tuesday 08 January 2013 10:56:43 Guennadi Liakhovetski wrote:
>>> On Tue, 8 Jan 2013, Laurent Pinchart wrote:
>>>> On Tuesday 08 January 2013 10:25:15 Guennadi Liakhovetski wrote:
>>>>> On Tue, 8 Jan 2013, Laurent Pinchart wrote:
>>>>>> On Monday 07 January 2013 11:23:55 Guennadi Liakhovetski wrote:
>>>>>>> > From 0e1eae338ba898dc25ec60e3dba99e5581edc199 Mon Sep 17 00:00:00
>>>>>>>> 2001
>>>
>>> [snip]
>>>
>>>>>>> +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);
>>>>>>> +/*
>>>>>>> + * If subdevice probing fails any time after
>>>>>>> v4l2_async_subdev_bind(),
>>>>>>> no
>>>>>>> + * clean up must be called. This function is only a message of
>>>>>>> intention.
>>>>>>> + */
>>>>>>> +int v4l2_async_subdev_bind(struct v4l2_async_subdev_list *asdl);
>>>>>>> +int v4l2_async_subdev_bound(struct v4l2_async_subdev_list *asdl);
>>>>>>
>>>>>> Could you please explain why you need both a bind notifier and a bound
>>>>>> notifier ? I was expecting a single v4l2_async_subdev_register() call
>>>>>> in subdev drivers (and, thinking about it, I would probably name it
>>>>>> v4l2_subdev_register()).
>>>>>
>>>>> I think I can, yes. Because between .bind() and .bound() the subdevice
>>>>> driver does the actual hardware probing. So, .bind() is used to make
>>>>> sure the hardware can be accessed, most importantly to provide a clock
>>>>> to the subdevice. You can look at soc_camera_async_bind(). There I'm
>>>>> registering the clock for the subdevice, about to bind. Why I cannot do
>>>>> it before, is because I need subdevice name for clock matching. With I2C
>>>>> subdevices the subdevice name contains the name of the driver, adapter
>>>>> number and i2c address. The latter 2 I've got from host subdevice list.
>>>>> But not the driver name. I thought about also passing the driver name
>>>>> there, but that seemed too limiting to me. I also request regulators
>>>>> there, because before ->bound() the sensor driver, but that could be
>>>>> done on the first call to soc_camera_power_on(), although doing this
>>>>> "first call" thingie is kind of hackish too. I could add one more soc-
>>>>> camera-power helper like soc_camera_prepare() or similar too.
>>>>
>>>> I think a soc_camera_power_init() function (or similar) would be a good
>>>> idea, yes.
>>>>
>>>>> So, the main problem is the clock
>>>>>
>>>>> subdevice name. Also see the comment in soc_camera.c:
>>>>> 	/*
>>>>> 	 * It is ok to keep the clock for the whole soc_camera_device
>>>>> 	 life-time,
>>>>> 	 * in principle it would be more logical to register the clock on icd
>>>>> 	 * creation, the only problem is, that at that time we don't know the
>>>>> 	 * driver name yet.
>>>>> 	 */
>>>>
>>>> I think we should fix that problem instead of shaping the async API around
>>>> a workaround :-)
>>>>
>>>>  From the subdevice point of view, the probe function should request
>>>> resources, perform whatever initialization is needed (including verifying
>>>> that the hardware is functional when possible), and the register the
>>>> subdev with the code if everything succeeded. Splitting registration into
>>>> bind() and bound() appears a bit as a workaround to me.
>>>>
>>>> If we need a workaround, I'd rather pass the device name in addition to
>>>> the I2C adapter number and address, instead of embedding the workaround in
>>>> this new API.
>>>
>>> ...or we can change the I2C subdevice name format. The actual need to do
>>>
>>> 	snprintf(clk_name, sizeof(clk_name), "%s %d-%04x",
>>> 		 asdl->dev->driver->name,
>>> 		 i2c_adapter_id(client->adapter), client->addr);
>>>
>>> in soc-camera now to exactly match the subdevice name, as created by
>>> v4l2_i2c_subdev_init(), doesn't make me specifically happy either. What if
>>> the latter changes at some point? Or what if one driver wishes to create
>>> several subdevices for one I2C device?
>>
>> The common clock framework uses %d-%04x, maybe we could use that as well for
>> clock names ?
>
> And preserve the subdevice names? Then matching would be more difficult
> and less precise. Or change subdevice names too? I think, we can do the
> latter, since anyway at any time only one driver can be attached to an I2C
> device.

I'm just wondering why we can't associate the clock with relevant device,
rather than its driver ? This could eliminate the problem of unknown
sub-device name at the host driver, before sub-device driver is actually
probed, couldn't it ?

--

Thanks,
Sylwester