On Tue, 8 Jan 2013, Laurent Pinchart wrote: > Hi Guennadi, > > 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. > > > > > > +void v4l2_async_subdev_unbind(struct v4l2_async_subdev_list *asdl); > > > > > > +#endif Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- 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
