Sorry for the delayed reply. A few remarks: Am 27.08.2013 14:52, schrieb Laurent Pinchart: > ... > > Even if the bridge doesn't control the clock, it provides a clock to the > sensor. As such, it's the responsibility of the bridge driver to provide the > clock to the sensor driver. The sensor driver knows that the sensor needs a > clock, and must thus get a clock object from somewhere. As Mauro already noticed: the clock may be provided by a simple crystal. Who is supposed to provide the clock object in this case ? Making a clock object mandatory isn't reasonable. And the second argument is, that even if the clock is provided/controlled by another chip, it often makes no sense to let the sensor control them. At least in em28xx USB devices, it's the _bridge_ that controls and configures the sensor, not the other way around. The bridge knows better than the sensor if it wants to drive the sensor with 6, 12 or 24 MHz, when power should be switched on or off etc. >From an em28xx bridges point of view, the whole soc_camera module isn't needed and the sensor driver design appears to be "upside-down". The problem with the sensor drivers in soc_camera seems to be, that the assumptions and driver design decisions seem to be based on typical embedded hardware only and don't match the requirements of others. Changing that will be a not-so-trivial long-term task... :/ >>> * possible fixes: several fixes have been proposed, e.g. >>> (a) implement a V4L2 clock in em28xx. >>> >>> Pro: logically correct - a clock is indeed present, local - no core >>> changes are needed >>> Contra: presumably relatively many devices will have such static >>> >>> always-on clocks. Implementing them in each of those drivers will >>> add copied code. Besides creating a clock name from I2C bus and >>> device numbers is ugly (a helper is needed). >>> >>> (b) make clocks optional in all subdevice drivers >>> >>> Pro: host / bridge drivers or core don't have to be modified >>> Contra: wrong in principle - those clocks are indeed compulsory >> I don't think that (b) is wrong: it is not a matter or clocks being >> compulsory or not. It is a matter of being able to be controlled or not. > No, it's a matter of providing a clock to a chip that needs one. If the chip > needs a clock, it must get one. Whether the clock can be controlled or not is > not relevant. IMHO it is relevant. And (as mentioned above) if the sensor _should_be_allowed_ to control the clock is also relevant. > Otherwise all clock users would need to implement several code > paths depending on whether the clock is controllable or not. That's indeed what the drivers should do. What's so unusual about it ? soc_camera already does it. ;) As you can see from the code (and also my RFC patch), these "code paths" are basically pretty simple. Of course, the async device registration mechanism needs to be fixed/improved. > That's something we wanted to avoid, as it would result in code bloat. Forcing drivers to implement pseudo/fake clocks is code bloat. Am 27.08.2013 18:00, schrieb Mauro Carvalho Chehab: > Em Tue, 27 Aug 2013 17:27:52 +0200 > Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> escreveu: > > ... >> The point is that the client driver knows that it needs a clock, and knows how >> to use it (for instance it knows that it should turn the clock on at least >> 100ms before sending the first I2C command). However, the client should not >> know how the clock is provided. That's the clock API abstraction layer. The >> client will request the clock and turn it on/off when it needs to, and if the >> clock source is a crystal it will always be on. On platforms where the clock >> can be controlled we will thus save power by disabling the clock when it's not >> used, and on other platforms the clock will just always be on, without any >> need to code this explictly in all client drivers. > On em28xx devices, power saving is done by enabling reset pin. On several > hardware, doing that internally disables the clock line. I'm not sure if > ov2640 supports this mode (Frank may know better how power saving is done > with those cameras). Other devices have an special pin for power off or > power saving. The EM25xx describes a standard mapping of GPIO pins to several functionalities (LEDs, buttons, sensor power on/off, ...). But hardware manufacturers can of course build circuits differently. The VAD Laplace webcam for example doesn't use the dedicated GPIO-pin for sensor power on/off. It's sensor seems to be powered all the time. > Anyway, that rises an interesting question: on devices with wired clocks, > the power saving mode should not be provided via clock API abstraction > layer, but via a callback to the bridge (as the bridge knows the GPIO > register/bit that corresponds to device reset and/or power off pin). Well, you can add power on/off callbacks to struct soc_camera_link for this. But - same question as above: who controls who ? ;) IMHO, it's the em28xx bridge that controls the sensor and decides when the sensor power needs to be switched on/off. Regards, Frank -- 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
