On Mon October 8 2012 17:15:53 Guennadi Liakhovetski wrote: > On Mon, 8 Oct 2012, Hans Verkuil wrote: > > > On Mon October 8 2012 16:30:53 Guennadi Liakhovetski wrote: > > > On Mon, 8 Oct 2012, Hans Verkuil wrote: > > > > > > > On Mon October 8 2012 14:23:25 Guennadi Liakhovetski wrote: > > > > > Hi Hans > > > > > > > > > > On Fri, 5 Oct 2012, Hans Verkuil wrote: > > > > > > > > > > [snip] > > > > > > > > > > > I think the soc_camera patches should be left out for now. I suspect that > > > > > > by adding core support for async i2c handling first, > > > > > > > > > > Ok, let's think, what this meacs - async I2C in media / V4L2 core. > > > > > > > > > > The main reason for our probing order problem is the master clock, > > > > > typically supplied from the camera bridge to I2C subdevices, which we only > > > > > want to start when necessary, i.e. when accessing the subdevice. And the > > > > > subdevice driver needs that clock running during its .probe() to be able > > > > > to access and verify or configure the hardware. Our current solution is to > > > > > not register I2C subdevices from the platform data, as is usual for all > > > > > I2C devices, but from the bridge driver and only after it has switched on > > > > > the master clock. After the subdevice driver has completed its probing we > > > > > switch the clock back off until the subdevice has to be activated, e.g. > > > > > for video capture. > > > > > > > > > > Also important - when we want to unregister the bridge driver we just also > > > > > unregister the I2C device. > > > > > > > > > > Now, to reverse the whole thing and to allow I2C devices be registered as > > > > > usual - via platform data or OF, first of all we have to teach I2C > > > > > subdevice drivers to recognise the "too early" situation and request > > > > > deferred probing in such a case. Then it will be reprobed after each new > > > > > successful probe or unregister on the system. After the bridge driver has > > > > > successfully probed the subdevice driver will be re-probed and at that > > > > > time it should succeed. Now, there is a problem here too: who should > > > > > switch on and off the master clock? > > > > > > > > > > If we do it from the bridge driver, we could install an I2C bus-notifier, > > > > > _before_ the subdevice driver is probed, i.e. upon the > > > > > BUS_NOTIFY_BIND_DRIVER event we could turn on the clock. If subdevice > > > > > probing was successful, we can then wait for the BUS_NOTIFY_BOUND_DRIVER > > > > > event to switch the clock back off. BUT - if the subdevice fails probing? > > > > > How do we find out about that and turn the clock back off? There is no > > > > > notification event for that... Possible solutions: > > > > > > > > > > 1. timer - ugly and unreliable. > > > > > 2. add a "probing failed" notifier event to the device core - would this > > > > > be accepted? > > > > > 3. let the subdevice turn the master clock on and off for the duration of > > > > > probing. > > > > > > > > > > My vote goes for (3). Ideally this should be done using the generic clock > > > > > framework. But can we really expect all drivers and platforms to switch to > > > > > it quickly enough? If not, we need a V4L2-specific callback from subdevice > > > > > drivers to bridge drivers to turn the clock on and off. That's what I've > > > > > done "temporarily" in this patch series for soc-camera. > > > > > > > > > > Suppose we decide to do the same for V4L2 centrally - add call-backs. Then > > > > > we can think what else we need to add to V4L2 to support asynchronous > > > > > subdevice driver probing. > > > > > > > > I wonder, don't we have the necessary code already? V4L2 subdev drivers can > > > > have internal_ops with register/unregister ops. These are called by > > > > v4l2_device_register_subdev. This happens during the bridge driver's probe. > > > > > > > > Suppose the subdev's probe does not actually access the i2c device, but > > > > instead defers that to the register callback? The bridge driver will turn on > > > > the clock before calling v4l2_device_register_subdev to ensure that the > > > > register callback can access the i2c registers. The register callback will > > > > do any initialization and can return an error. In case of an error the i2c > > > > client is automatically unregistered as well. > > > > > > Yes, if v4l2_i2c_new_subdev_board() is used. This has been discussed > > > several times before and always what I didn't like in this is, that I2C > > > device probe() in this case succeeds without even trying to access the > > > hardware. And think about DT. In this case we don't instantiate the I2C > > > device, OF code does it for us. What do you do then? If you let probe() > > > succeed, then you will have to somehow remember the subdevice to later > > > match it against bridges... > > > > Yes, but you need that information anyway. The bridge still needs to call > > v4l2_device_register_subdev so it needs to know which subdevs are loaded. > > But how do you get the subdev pointer? With the notifier I get it from > i2c_get_clientdata(client) and what do you do without it? How do you get > to the client? > > > And can't it get that from DT as well? > > No, I don't think there is a way to get a device pointer from a DT node. Not a device pointer, but the i2c bus and i2c address. With that information you can get the i2c_client, and with that you can get the subdev pointer. If there is no way to get that information from the proposed V4L2 DT, then it needs to be modified since a bridge driver really needs to know which subdevs it has to register with the v4l2_device struct. That information is also board specific so it should be part of the DT. > > > In my view you cannot do a proper initialization unless you have both the > > bridge driver and all subdev drivers loaded and instantiated. They need one > > another. So I am perfectly fine with letting the probe function do next to > > nothing and postponing that until register() is called. I2C and actual probing > > to check if it's the right device is a bad idea in general since you have no > > idea what a hardware access to an unknown i2c device will do. There are still > > some corner cases where that is needed, but I do not think that that is an > > issue here. > > > > It would simplify things a lot IMHO. Also note that the register() op will > > work with any device, not just i2c. That may be a useful property as well. > > And what if the subdevice device is not yet instantiated by OF by the time > your bridge driver probes? That is where you still need to have a bus notifier mechanism. You have to be able to wait until all dependent drivers are loaded/instantiated, or alternatively you have to be able to load them explicitly. But this should be relatively easy to implement in a generic manner. I still think this sucks (excuse my language), but I see no way around it as long as there is no explicit probe order one can rely on. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
