On Tue, 8 Jan 2013, Laurent Pinchart wrote: > Hi Guennadi, > > 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? Thanks Guennadi > > > > +void v4l2_async_subdev_unbind(struct v4l2_async_subdev_list *asdl); > > > > +#endif > > -- > Regards, > > Laurent Pinchart > --- 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
