Message ID | 4E5A2657.7030605@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi Sylwester, On Sunday 28 August 2011 13:28:23 Sylwester Nawrocki wrote: > On 08/08/2011 05:50 PM, Laurent Pinchart wrote: > > Subdevs hierachy, Linux device model > > ------------------------------------ > > > > Preliminary conclusions: > > > > - With the move to device tree on ARM (and other platforms), I2C, SPI > > and > > > > platform subdevs should be created from board code, not from > > bridge/host drivers. > > > > - Bus notifiers should be used by bridge/host drivers to wait for all > > > > required subdevs. V4L2 core should provide helper functions. > > > > - struct clk should be used to handle clocks provided by hosts to > > subdevs. > > I have been investigating recently possible ways to correct the external > clock handling in Samsung FIMC driver and this led me up to the device > tree stuff. I.e. in order to be able to register any I2C client device > there is a need to enable its master clock at the v4l2 host/bridge driver. To be completely generic, the subdev master clock can come from anywhere, not only from the V4L2 host/bridge (although that's the usual case). > There is an issue that the v4l2_device (host)/v4l2_subdev hierarchy is not > reflected by the linux device tree model, e.g. the host might be a platform > device while the client an I2C client device. Thus a proper device/driver > registration order is not assured by the device driver core from v4l2 POV. > > I thought about embedding some API in a struct v4l2_device for the subdevs > to be able to get their master clock(s) as they need it. But this would > work only when a v4l2_device and v4l2_subdev are matched (registered) > before I2C client's probe(), or alternatively > subdev_internal_ops::registered() callback, is called. > > Currently such requirement is satisfied when the I2C client/v4l2 subdev > devices are registered from within a v4l2 bridge/host driver initialization > routine. But we may need to stop doing this to adhere to the DT rules. Right, that's my understanding as well. > I guess above you didn't really mean to create subdevs from board code? > The I2C client registration is now done at the I2C bus drivers, using the > OF helpers to retrieve the child devices list from fdt. I meant registering the I2C board information from board code (for non-DT platforms) or from the device tree (for DT platforms) instead of V4L2 host/bridge drivers. > I guess we could try to create some sort of replacement for > v4l2_i2c_new_subdev_board() function in linux/drivers/of/* (of_i2c.c ?), > similar to of_i2c_register_devices(). > > But first we would have somehow to make sure the host drivers are registered > and initialized first. I'm not sure how to do it. > Plus such a new subdev registration method would have to obtain a relevant > struct v4l2_device object reference during the process; which is getting > a bit cumbersome.. > > Also, if we used a 'struct clk' to handle clocks provided by hosts to > subdevs, could we use any subdev operation callback to pass a reference to > such object from host to subdev? I doubt since the clock may be needed in > the subdev before it is allocated and fully initialized, (i.e. available > in the host). > > If we have embedded a 'struct clk' pointer into struct v4l2_device, it > would have probably to be an array of clocks and the subdev would have to > be able to find out which clock applies to it. > > So I thought about doing something like: > > diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h > index d61febf..9888f7d 100644 > --- a/include/media/v4l2-device.h > +++ b/include/media/v4l2-device.h > @@ -54,6 +54,7 @@ struct v4l2_device { > /* notify callback called by some sub-devices. */ > void (*notify)(struct v4l2_subdev *sd, > unsigned int notification, void *arg); > + const struct clk * (*clock_get)(struct v4l2_subdev *sd); > /* The control handler. May be NULL. */ > struct v4l2_ctrl_handler *ctrl_handler; > /* Device's priority state */ > > This would allow the host to return proper clock for a subdev. > But it won't work unless the initialization order is assured.. My idea was to let the kernel register all devices based on the DT or board code. When the V4L2 host/bridge driver gets registered, it will then call a V4L2 core function with a list of subdevs it needs. The V4L2 core would store that information and react to bus notifier events to notify the V4L2 host/bridge driver when all subdevs are present. At that point the host/bridge driver will get hold of all the subdevs and call (probably through the V4L2 core) their .registered operation. That's where the subdevs will get access to their clock using clk_get(). This is really a rough idea, we will probably run into unexpected issues. I'm not even sure if this can work out in the end, but I don't really see another clean solution for now.
On Mon, 29 Aug 2011, Laurent Pinchart wrote: [snip] > My idea was to let the kernel register all devices based on the DT or board > code. When the V4L2 host/bridge driver gets registered, it will then call a > V4L2 core function with a list of subdevs it needs. The V4L2 core would store > that information and react to bus notifier events to notify the V4L2 > host/bridge driver when all subdevs are present. At that point the host/bridge > driver will get hold of all the subdevs and call (probably through the V4L2 > core) their .registered operation. That's where the subdevs will get access to > their clock using clk_get(). Correct me, if I'm wrong, but this seems to be the case of sensor (and other i2c-client) drivers having to succeed their probe() methods without being able to actually access the hardware? 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi, On Tuesday 30 August 2011 00:20:09 Guennadi Liakhovetski wrote: > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > [snip] > > > My idea was to let the kernel register all devices based on the DT or > > board code. When the V4L2 host/bridge driver gets registered, it will > > then call a V4L2 core function with a list of subdevs it needs. The V4L2 > > core would store that information and react to bus notifier events to > > notify the V4L2 host/bridge driver when all subdevs are present. At that > > point the host/bridge driver will get hold of all the subdevs and call > > (probably through the V4L2 core) their .registered operation. That's > > where the subdevs will get access to their clock using clk_get(). > > Correct me, if I'm wrong, but this seems to be the case of sensor (and > other i2c-client) drivers having to succeed their probe() methods without > being able to actually access the hardware? That's right. I'd love to find a better way :-) Note that this is already the case for many subdev drivers that probe the hardware in the .registered() operation instead of the probe() method.
On Tue, 30 Aug 2011, Laurent Pinchart wrote: > Hi Guennadi, > > On Tuesday 30 August 2011 00:20:09 Guennadi Liakhovetski wrote: > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > > > [snip] > > > > > My idea was to let the kernel register all devices based on the DT or > > > board code. When the V4L2 host/bridge driver gets registered, it will > > > then call a V4L2 core function with a list of subdevs it needs. The V4L2 > > > core would store that information and react to bus notifier events to > > > notify the V4L2 host/bridge driver when all subdevs are present. At that > > > point the host/bridge driver will get hold of all the subdevs and call > > > (probably through the V4L2 core) their .registered operation. That's > > > where the subdevs will get access to their clock using clk_get(). > > > > Correct me, if I'm wrong, but this seems to be the case of sensor (and > > other i2c-client) drivers having to succeed their probe() methods without > > being able to actually access the hardware? > > That's right. I'd love to find a better way :-) Note that this is already the > case for many subdev drivers that probe the hardware in the .registered() > operation instead of the probe() method. Then why do you think it is better, than adding devices from bridge drivers? Think about hotpluggable devices - drivers create devices all the time - USB etc. Why cannot we do the same? As a historic reference: soc-camera used to do this too before - probe without hardware access and "really-probe" after the host turns on the clock. Then we switched to registering devices later. I like the present approach better. 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 30, 2011 at 12:20:09AM +0200, Guennadi Liakhovetski wrote: > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > My idea was to let the kernel register all devices based on the DT or board > > code. When the V4L2 host/bridge driver gets registered, it will then call a > > V4L2 core function with a list of subdevs it needs. The V4L2 core would store > > that information and react to bus notifier events to notify the V4L2 > > host/bridge driver when all subdevs are present. At that point the host/bridge > > driver will get hold of all the subdevs and call (probably through the V4L2 > > core) their .registered operation. That's where the subdevs will get access to > > their clock using clk_get(). > Correct me, if I'm wrong, but this seems to be the case of sensor (and > other i2c-client) drivers having to succeed their probe() methods without > being able to actually access the hardware? The events should only be generated after the probe() has succeeded so if the driver talks to the hardware then it can fail probe() if need be. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 30, 2011 at 02:41:48PM +0100, Mark Brown wrote: > On Tue, Aug 30, 2011 at 12:20:09AM +0200, Guennadi Liakhovetski wrote: > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > > > My idea was to let the kernel register all devices based on the DT or board > > > code. When the V4L2 host/bridge driver gets registered, it will then call a > > > V4L2 core function with a list of subdevs it needs. The V4L2 core would store > > > that information and react to bus notifier events to notify the V4L2 > > > host/bridge driver when all subdevs are present. At that point the host/bridge Sounds a lot like what ASoC is currently doing. > > > driver will get hold of all the subdevs and call (probably through the V4L2 > > > core) their .registered operation. That's where the subdevs will get access to > > > their clock using clk_get(). > > > Correct me, if I'm wrong, but this seems to be the case of sensor (and > > other i2c-client) drivers having to succeed their probe() methods without > > being able to actually access the hardware? It indeed sounds like that, which also concerns me. ASoC and other subsystems have exactly the same problem where the 'device' is actually an aggregate of multiple devices attached to different busses. My personal opinion is that the best way to handle this is to support deferred probing so that a driver can fail with -EAGAIN if all the resources that it requires are not available immediately, and have the driver core retry the probe after other devices have successfully probed. I've got prototype code for this, but it needs some more work before being mainlined. > The events should only be generated after the probe() has succeeded so > if the driver talks to the hardware then it can fail probe() if need be. I'm a bit confused here. Which events are you referring to, and which .probe call? (the i2c/spi/whatever probe, or the aggregate v4l2 probe?) -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 30, 2011 at 07:56:09AM -0600, Grant Likely wrote: > On Tue, Aug 30, 2011 at 02:41:48PM +0100, Mark Brown wrote: > > The events should only be generated after the probe() has succeeded so > > if the driver talks to the hardware then it can fail probe() if need be. > I'm a bit confused here. Which events are you referring to, and which > .probe call? (the i2c/spi/whatever probe, or the aggregate v4l2 probe?) There's some driver model core level notifiers that are generated when things manage to bind (postdating all the ASoC stuff for this IIRC, and not covering the suspend/resume ordering issues). Actually, thinking about it they may be per bus. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Grant On Tue, 30 Aug 2011, Grant Likely wrote: > On Tue, Aug 30, 2011 at 02:41:48PM +0100, Mark Brown wrote: > > On Tue, Aug 30, 2011 at 12:20:09AM +0200, Guennadi Liakhovetski wrote: > > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > > > > > > My idea was to let the kernel register all devices based on the DT or board > > > > code. When the V4L2 host/bridge driver gets registered, it will then call a > > > > V4L2 core function with a list of subdevs it needs. The V4L2 core would store > > > > that information and react to bus notifier events to notify the V4L2 > > > > host/bridge driver when all subdevs are present. At that point the host/bridge > > Sounds a lot like what ASoC is currently doing. > > > > > driver will get hold of all the subdevs and call (probably through the V4L2 > > > > core) their .registered operation. That's where the subdevs will get access to > > > > their clock using clk_get(). > > > > > Correct me, if I'm wrong, but this seems to be the case of sensor (and > > > other i2c-client) drivers having to succeed their probe() methods without > > > being able to actually access the hardware? > > It indeed sounds like that, which also concerns me. ASoC and other > subsystems have exactly the same problem where the 'device' is > actually an aggregate of multiple devices attached to different > busses. My personal opinion is that the best way to handle this is to > support deferred probing Yes, that's also what I think should be done. But I was thinking about a slightly different approach - a dependency-based probing. I.e., you should be able to register a device, depending on another one (parent?), and only after the latter one has successfully probed, the driver core should be allowed to probe the child. Of course, devices can depend on multiple other devices, so, a single parent might not be enough. Thanks Guennadi > so that a driver can fail with -EAGAIN if all > the resources that it requires are not available immediately, and have > the driver core retry the probe after other devices have successfully > probed. > > I've got prototype code for this, but it needs some more work before > being mainlined. > > > The events should only be generated after the probe() has succeeded so > > if the driver talks to the hardware then it can fail probe() if need be. > > I'm a bit confused here. Which events are you referring to, and which > .probe call? (the i2c/spi/whatever probe, or the aggregate v4l2 probe?) > --- 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 30, 2011 at 8:03 AM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > Hi Grant > > On Tue, 30 Aug 2011, Grant Likely wrote: > >> On Tue, Aug 30, 2011 at 02:41:48PM +0100, Mark Brown wrote: >> > On Tue, Aug 30, 2011 at 12:20:09AM +0200, Guennadi Liakhovetski wrote: >> > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: >> > >> > > > My idea was to let the kernel register all devices based on the DT or board >> > > > code. When the V4L2 host/bridge driver gets registered, it will then call a >> > > > V4L2 core function with a list of subdevs it needs. The V4L2 core would store >> > > > that information and react to bus notifier events to notify the V4L2 >> > > > host/bridge driver when all subdevs are present. At that point the host/bridge >> >> Sounds a lot like what ASoC is currently doing. >> >> > > > driver will get hold of all the subdevs and call (probably through the V4L2 >> > > > core) their .registered operation. That's where the subdevs will get access to >> > > > their clock using clk_get(). >> > >> > > Correct me, if I'm wrong, but this seems to be the case of sensor (and >> > > other i2c-client) drivers having to succeed their probe() methods without >> > > being able to actually access the hardware? >> >> It indeed sounds like that, which also concerns me. ASoC and other >> subsystems have exactly the same problem where the 'device' is >> actually an aggregate of multiple devices attached to different >> busses. My personal opinion is that the best way to handle this is to >> support deferred probing > > Yes, that's also what I think should be done. But I was thinking about a > slightly different approach - a dependency-based probing. I.e., you should > be able to register a device, depending on another one (parent?), and only > after the latter one has successfully probed, the driver core should be > allowed to probe the child. Of course, devices can depend on multiple > other devices, so, a single parent might not be enough. Yes, a dependency system would be lovely... but it gets really complex in a hurry, especially when faced with heterogeneous device registrations. A deferral system ends up being really simple to implement and probably work just as well. g. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 30 August 2011 17:18:31 Grant Likely wrote: > On Tue, Aug 30, 2011 at 8:03 AM, Guennadi Liakhovetski > > <g.liakhovetski@gmx.de> wrote: > > Hi Grant > > > > On Tue, 30 Aug 2011, Grant Likely wrote: > >> On Tue, Aug 30, 2011 at 02:41:48PM +0100, Mark Brown wrote: > >> > On Tue, Aug 30, 2011 at 12:20:09AM +0200, Guennadi Liakhovetski wrote: > >> > > On Mon, 29 Aug 2011, Laurent Pinchart wrote: > >> > > > My idea was to let the kernel register all devices based on the DT > >> > > > or board code. When the V4L2 host/bridge driver gets registered, > >> > > > it will then call a V4L2 core function with a list of subdevs it > >> > > > needs. The V4L2 core would store that information and react to > >> > > > bus notifier events to notify the V4L2 host/bridge driver when > >> > > > all subdevs are present. At that point the host/bridge > >> > >> Sounds a lot like what ASoC is currently doing. > >> > >> > > > driver will get hold of all the subdevs and call (probably through > >> > > > the V4L2 core) their .registered operation. That's where the > >> > > > subdevs will get access to their clock using clk_get(). > >> > > > >> > > Correct me, if I'm wrong, but this seems to be the case of sensor > >> > > (and other i2c-client) drivers having to succeed their probe() > >> > > methods without being able to actually access the hardware? > >> > >> It indeed sounds like that, which also concerns me. ASoC and other > >> subsystems have exactly the same problem where the 'device' is > >> actually an aggregate of multiple devices attached to different > >> busses. My personal opinion is that the best way to handle this is to > >> support deferred probing > > > > Yes, that's also what I think should be done. But I was thinking about a > > slightly different approach - a dependency-based probing. I.e., you > > should be able to register a device, depending on another one (parent?), > > and only after the latter one has successfully probed, the driver core > > should be allowed to probe the child. Of course, devices can depend on > > multiple other devices, so, a single parent might not be enough. > > Yes, a dependency system would be lovely... but it gets really complex > in a hurry, especially when faced with heterogeneous device > registrations. A deferral system ends up being really simple to > implement and probably work just as well. The core issue is that physical device trees, clock trees, power trees and logical device tress are not always aligned. Instanciating devices based on the parent-child device relationships will always lead to situations where a device probe() method will be called with clocks or power sources not available yet. A dependency system is tempting but will be very complex to implement properly, especially when faced with cyclic dependencies. For instance the OMAP3 ISP driver requires the camera sensor device to be present to proceed, and the camera sensor requires a clock provided by the OMAP3 ISP. To solve this we need to probe the OMAP3 ISP first, have it register its clock devices, and then wait until all sensors become available. A probe deferral system is probably simpler, but it will have its share of problems as well. In the above example, if the sensor is probed first, the driver can return -EAGAIN in the probe() method as the clock isn't available yet (I'm not sure how to differentiate between "not available yet" and "not present in the system" though). However, if the OMAP3 ISP is probed first, returning -EAGAIN in its probe() method won't really help, as we need to register the clock before waiting for the sensor.
On Tue, Aug 30, 2011 at 05:42:55PM +0200, Laurent Pinchart wrote: > A dependency system is tempting but will be very complex to implement > properly, especially when faced with cyclic dependencies. For instance the > OMAP3 ISP driver requires the camera sensor device to be present to proceed, > and the camera sensor requires a clock provided by the OMAP3 ISP. To solve > this we need to probe the OMAP3 ISP first, have it register its clock devices, > and then wait until all sensors become available. With composite devices like that where the borad has sufficient interesting stuff on it representing the board itself as a device (this is what ASoC does). > A probe deferral system is probably simpler, but it will have its share of > problems as well. In the above example, if the sensor is probed first, the > driver can return -EAGAIN in the probe() method as the clock isn't available > yet (I'm not sure how to differentiate between "not available yet" and "not > present in the system" though). However, if the OMAP3 ISP is probed first, > returning -EAGAIN in its probe() method won't really help, as we need to > register the clock before waiting for the sensor. Having a device for the camera subsystem as a whole breaks this loop as the probe of that device triggers the overall system probe. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 30 Aug 2011, Laurent Pinchart wrote: > A dependency system is tempting but will be very complex to implement > properly, especially when faced with cyclic dependencies. For instance the > OMAP3 ISP driver requires the camera sensor device to be present to proceed, Switching to a notifier instead of waiting in probe() might be a good idea (TM). > and the camera sensor requires a clock provided by the OMAP3 ISP. To solve > this we need to probe the OMAP3 ISP first, have it register its clock devices, > and then wait until all sensors become available. 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, On Tuesday 30 August 2011 17:46:42 Mark Brown wrote: > On Tue, Aug 30, 2011 at 05:42:55PM +0200, Laurent Pinchart wrote: > > A dependency system is tempting but will be very complex to implement > > properly, especially when faced with cyclic dependencies. For instance > > the OMAP3 ISP driver requires the camera sensor device to be present to > > proceed, and the camera sensor requires a clock provided by the OMAP3 > > ISP. To solve this we need to probe the OMAP3 ISP first, have it > > register its clock devices, and then wait until all sensors become > > available. > > With composite devices like that where the borad has sufficient > interesting stuff on it representing the board itself as a device (this > is what ASoC does). > > > A probe deferral system is probably simpler, but it will have its share > > of problems as well. In the above example, if the sensor is probed > > first, the driver can return -EAGAIN in the probe() method as the clock > > isn't available yet (I'm not sure how to differentiate between "not > > available yet" and "not present in the system" though). However, if the > > OMAP3 ISP is probed first, returning -EAGAIN in its probe() method won't > > really help, as we need to register the clock before waiting for the > > sensor. > > Having a device for the camera subsystem as a whole breaks this loop as > the probe of that device triggers the overall system probe. The exact same idea crossed my mind after hitting the "Send" button :-) Would such a device be included in the DT ? My understanding is that the DT should only describe the hardware.
On Tue, Aug 30, 2011 at 10:12:30PM +0200, Laurent Pinchart wrote: > Would such a device be included in the DT ? My understanding is that the DT > should only describe the hardware. For ASoC they will be, the view is that the schematic for the board is sufficiently interesting to count as hardware. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 30 August 2011 22:35:59 Grant Likely wrote: > On Aug 30, 2011 2:19 PM, "Mark Brown" wrote: > > On Tue, Aug 30, 2011 at 10:12:30PM +0200, Laurent Pinchart wrote: > > > Would such a device be included in the DT ? My understanding is that > > > the DT should only describe the hardware. > > > > For ASoC they will be, the view is that the schematic for the board is > > sufficiently interesting to count as hardware. > > Yes. Aggregate devices are sufficiently complex that there is a strong > argument for using a node to describe one. Is there any document or sample implementation ?
On Tue, Aug 30, 2011 at 10:37:21PM +0200, Laurent Pinchart wrote: > On Tuesday 30 August 2011 22:35:59 Grant Likely wrote: > > Yes. Aggregate devices are sufficiently complex that there is a strong > > argument for using a node to describe one. > Is there any document or sample implementation ? ASoC. Unfortunately there are several rather annoying problems in the Linux device model which make this substantially less straightforward than it might otherwise be, the main ones being waiting for everything to probe and making sure everything suspends and resumes in the correct order. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h index d61febf..9888f7d 100644 --- a/include/media/v4l2-device.h +++ b/include/media/v4l2-device.h @@ -54,6 +54,7 @@ struct v4l2_device { /* notify callback called by some sub-devices. */ void (*notify)(struct v4l2_subdev *sd, unsigned int notification, void *arg); + const struct clk * (*clock_get)(struct v4l2_subdev *sd); /* The control handler. May be NULL. */ struct v4l2_ctrl_handler *ctrl_handler; /* Device's priority state */