Hi Guennadi, On Tuesday 15 October 2013 10:05:45 Guennadi Liakhovetski wrote: > On Thu, 10 Oct 2013, Laurent Pinchart wrote: > > On Tuesday 08 October 2013 23:57:55 Guennadi Liakhovetski wrote: > > > Hi Mauro, > > > > > > Thanks for your long detailed mail. For the sake of brevity however I'll > > > drop most of it in this my reply, everybody interested should be able to > > > read the original. > > > > > > On Wed, 9 Oct 2013, Mauro Carvalho Chehab wrote: > > > > > > [snip] > > > > > > > In other words, what you're actually proposing is to change the > > > > default used by most drivers since 1997 from a POWER ON/CLOCK ON > > > > default, into a POWER OFF/ CLOCK OFF default. > > > > > > To remind, we are now trying to fix a problem, present in the current > > > kernel. In one specific driver. And the proposed fix only affects one > > > specific (family of) driver(s) - the em28xx USB driver. The two patches > > > are quite simple: > > > > > > (1) the first patch adds a clock to the em28xx driver, which only > > > affects ov2640, because only it uses that clock > > > > > > (2) the second patch adds a call to subdev's .s_power(1) method. And I > > > cannot see how this change can be a problem either. Firstly I haven't > > > found many subdevices, used by em28xx, that implement .s_power(). > > > Secondly, I don't think any of them does any kind of depth-counting in > > > that method, apart from the one, that we're trying to fix - ov2640. > > > > > > > Well, for me, it sounds that someone will need to re-test all > > > > supported devices, to be sure that such change won't cause > > > > regressions. > > > > > > > > If you are willing to do such tests (and to get all those hardware to > > > > be sure that nothing will break) or to find someone to do it for you, > > > > I'm ok with such change. > > > > > > I'm willing to try to identify all subdevices, used by em28xx, look at > > > their .s_power() methods and report my analysis, whether calling > > > .s_power(1) for those respective drivers could cause problems. Would > > > this suffice? > > > > From a high level point of view, I believe that's the way to go. V4L2 > > clock enable/disable calls must be balanced, as we will later switch to > > the non-V4L2 clock API that requires calls to be balanced. > > > > This pushes the problem back to the .s_power() implementation that call > > the clock enable/disable functions. As a temporary measure, we could add a > > use count to the .s_power() handlers of drivers used by both power- > > unbalanced and power-balanced bridges that call the clock API or the > > regulator API in their .s_power() implementation (that's just ov2640 if > > I'm not mistaken). This would ensure that clock calls are always balanced, > > even if the .s_power() calls are not. > > > > Now I'd like to avoid that as possible: In the long term I believe we > > should switch all .s_power() calls to balanced mode, a detailed analysis > > of the subdevices used by em28xx would thus have my preference. However, > > if it helps solving the issue right now, buying us time to fix the > > problem correctly, I could live with it. > > Please, correct me if I'm wrong, but I seem to remember, that we wanted to > eliminate .s_power() methods completely eventually. We could try to find > that old discussion, but it would need some searching. In short - only > subdevice drivers know, when their devices need power or clock. Higher > layers just request specific functions - setting parameters, starting or > stopping streaming etc., and subdev drivers decide when they have to > access (I2C) registers, which regulators they have to turn on for that, > when they have to power on the sensor array and activate the data > interface... IIRC we were thinking about some exceptions like SoC internal > subdevices, which are initialised by the main SoC camera interface driver. > It was then suggested, that that central camera interface driver also > knows when those internal subdevices should be turned up and down. > Although I'm not sure even that would be needed. > > Shall we not maybe move in that direction? I believe you remember correctly, and that's indeed a good idea. However, now might not be the best time to do so, we need to fix the em28xx problem. What's your preferred solution there ? -- Regards, Laurent Pinchart -- 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
