| Message ID | Pine.LNX.4.64.0905151907460.4658@axis700.grange (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
On Friday 15 May 2009 19:20:18 Guennadi Liakhovetski wrote: > NOT FOR SUBMISSION. Probably, another solution has to be found. > soc-camera drivers need an .init() (marked as "don't use") and a .halt() > methods. > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > --- > > Hans, you moved s_standby to tuner_ops, and init is not recommended for > new drivers. Suggestions? Usual question: why do you need an init and halt? What do they do? One valid use case for init is to pass config data to the driver. I am considering to make it possible to setup the board_info data instead for an i2c subdev, so one can use the platform data to pass such info to the subdev driver. The disadvantage is that it cannot be used for pre-2.6.26 kernels. An alternative might be a s_config ops that serves a similar purpose. I want to leave s_standby in the tuner_ops: it's currently only used together with a tuner. It's also poorly designed. A new halt or standby core ops should be better designed and it should be clear what the relationship is to the suspend and resume i2c driver ops (see e.g. msp3400-driver.c). Regards, Hans > include/media/v4l2-subdev.h | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 1785608..ba907be 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -97,6 +97,7 @@ struct v4l2_subdev_core_ops { > int (*g_chip_ident)(struct v4l2_subdev *sd, struct v4l2_dbg_chip_ident > *chip); int (*log_status)(struct v4l2_subdev *sd); > int (*init)(struct v4l2_subdev *sd, u32 val); > + int (*s_standby)(struct v4l2_subdev *sd, u32 standby); > int (*load_fw)(struct v4l2_subdev *sd); > int (*reset)(struct v4l2_subdev *sd, u32 val); > int (*s_gpio)(struct v4l2_subdev *sd, u32 val);
On Thu, 21 May 2009, Hans Verkuil wrote: > On Friday 15 May 2009 19:20:18 Guennadi Liakhovetski wrote: > > NOT FOR SUBMISSION. Probably, another solution has to be found. > > soc-camera drivers need an .init() (marked as "don't use") and a .halt() > > methods. > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > --- > > > > Hans, you moved s_standby to tuner_ops, and init is not recommended for > > new drivers. Suggestions? > > Usual question: why do you need an init and halt? What do they do? Hm, maybe you're right, I don't need them. init() was used in soc_camera drivers on first open() to possibly reset the chip and put it in some reasonably pre-defined low-power state. But we can do this at the end of probe(), which even would be more correct, because even the first open should not change chip's configuration. And halt() (was called release() originally) is called on last close(). And it seems you shouldn't really do this at all - the chip should preserve its configuration between open/close cycles. Am I right? Does anyone among cc'ed authors have any objections against this change? The actual disable should indeed migrate to some PM functions, if implemented. 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 Friday 22 May 2009 16:23:36 Guennadi Liakhovetski wrote: > On Thu, 21 May 2009, Hans Verkuil wrote: > > On Friday 15 May 2009 19:20:18 Guennadi Liakhovetski wrote: > > > NOT FOR SUBMISSION. Probably, another solution has to be found. > > > soc-camera drivers need an .init() (marked as "don't use") and a > > > .halt() methods. > > > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > > --- > > > > > > Hans, you moved s_standby to tuner_ops, and init is not recommended > > > for new drivers. Suggestions? > > > > Usual question: why do you need an init and halt? What do they do? > > Hm, maybe you're right, I don't need them. init() was used in soc_camera > drivers on first open() to possibly reset the chip and put it in some > reasonably pre-defined low-power state. But we can do this at the end of > probe(), which even would be more correct, because even the first open > should not change chip's configuration. And halt() (was called release() > originally) is called on last close(). And it seems you shouldn't really > do this at all - the chip should preserve its configuration between > open/close cycles. Am I right? That's correct. It's interesting to see that init/halt/reset/powersaving type functions are usually not needed. I know that there are still a few i2c drivers implementing init and reset, and I also know that those can be removed since they are not needed at all. I just need to find some time to do the actual removal. So whenever I see these functions I always get suspicious :-) Regards, Hans > Does anyone among cc'ed authors have any > objections against this change? The actual disable should indeed migrate > to some PM functions, if implemented. > > Thanks > Guennadi > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > http://www.open-technology.de/
Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: >> Usual question: why do you need an init and halt? What do they do? > > Hm, maybe you're right, I don't need them. init() was used in soc_camera > drivers on first open() to possibly reset the chip and put it in some > reasonably pre-defined low-power state. But we can do this at the end of > probe(), which even would be more correct, because even the first open > should not change chip's configuration. And halt() (was called release() > originally) is called on last close(). And it seems you shouldn't really > do this at all - the chip should preserve its configuration between > open/close cycles. Am I right? > Does anyone among cc'ed authors have any objections against this change? The > actual disable should indeed migrate to some PM functions, if implemented. If I understand correctly, what was done before was that on last close, the sensor was disabled (through sensor->release() call). What will be done now is leave the sensor on. On an embedded system, the power eaten by an active sensor is usually too much compared to the other components. So, if there is a solution which enables, on last close, to power down the device (or put it in low power mode), in the new API, I'm OK, even if it's a new powersaving function. If there is no such function and there will be a gap (let's say kernel 2.6.31 to 2.6.35) where the sensor will be left activated all the time, then I'm against. Let me be even more precise about a usecase : - a user takes a picture with his smartphone - the same user then uses his phone to call his girlfriend - the girlfriend has a lot of things to say, it lasts for 1 hour In that case, the sensor _has_ to be switched off. Cheers. -- Robert -- 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 Fri, 22 May 2009, Robert Jarzmik wrote: > Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: > > >> Usual question: why do you need an init and halt? What do they do? > > > > Hm, maybe you're right, I don't need them. init() was used in soc_camera > > drivers on first open() to possibly reset the chip and put it in some > > reasonably pre-defined low-power state. But we can do this at the end of > > probe(), which even would be more correct, because even the first open > > should not change chip's configuration. And halt() (was called release() > > originally) is called on last close(). And it seems you shouldn't really > > do this at all - the chip should preserve its configuration between > > open/close cycles. Am I right? > > > > Does anyone among cc'ed authors have any objections against this change? The > > actual disable should indeed migrate to some PM functions, if implemented. > If I understand correctly, what was done before was that on last close, the > sensor was disabled (through sensor->release() call). What will be done now is > leave the sensor on. > > On an embedded system, the power eaten by an active sensor is usually too much > compared to the other components. > > So, if there is a solution which enables, on last close, to power down the > device (or put it in low power mode), in the new API, I'm OK, even if it's a new > powersaving function. If there is no such function and there will be a gap > (let's say kernel 2.6.31 to 2.6.35) where the sensor will be left activated all > the time, then I'm against. > > Let me be even more precise about a usecase : > - a user takes a picture with his smartphone > - the same user then uses his phone to call his girlfriend > - the girlfriend has a lot of things to say, it lasts for 1 hour > In that case, the sensor _has_ to be switched off. Nice example, thanks! Ok, of course, we must not leave the poor girl with her boyfriend's flat battery:-) I think we can put the camera to a low-power state in streamoff. But - not power it off! This has to be done from system's PM functions. What was there on linux-pm about managing power of single devices?... 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
Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: >> Let me be even more precise about a usecase : >> - a user takes a picture with his smartphone >> - the same user then uses his phone to call his girlfriend >> - the girlfriend has a lot of things to say, it lasts for 1 hour >> In that case, the sensor _has_ to be switched off. > > Nice example, thanks! Ok, of course, we must not leave the poor girl with > her boyfriend's flat battery:-) Dear, of course not, imagine what she would do to him ! :) > I think we can put the camera to a low-power state in streamoff. But - not > power it off! This has to be done from system's PM functions. That means, from a sensor POV, through icd->stop_capture(). For my single mt9m111, it's fine by me, as the mt9m111 does have a powersave mode. Yet I'm wondering if there are sensors without that capability, with only one control (ie. one GPIO line) to switch them on and off ... > What was there on linux-pm about managing power of single devices?... Reference ? Cheers. -- Robert -- 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 Sat, 23 May 2009, Robert Jarzmik wrote: > Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: > > > I think we can put the camera to a low-power state in streamoff. But - not > > power it off! This has to be done from system's PM functions. > That means, from a sensor POV, through icd->stop_capture(). > For my single mt9m111, it's fine by me, as the mt9m111 does have a powersave > mode. Yet I'm wondering if there are sensors without that capability, with only > one control (ie. one GPIO line) to switch them on and off ... Doesn't this actually belong to PM? I begin to think that our whole ->power() handling might be a bit suboptimally placed and should migrate to pm, one might give it some thinking... > > What was there on linux-pm about managing power of single devices?... > Reference ? I meant this thread http://thread.gmane.org/gmane.linux.power-management.general/4655/focus=4670 as you see, it is not very fresh, and I have no idea what came out of it, have a look if you're interested. 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
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index 1785608..ba907be 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -97,6 +97,7 @@ struct v4l2_subdev_core_ops { int (*g_chip_ident)(struct v4l2_subdev *sd, struct v4l2_dbg_chip_ident *chip); int (*log_status)(struct v4l2_subdev *sd); int (*init)(struct v4l2_subdev *sd, u32 val); + int (*s_standby)(struct v4l2_subdev *sd, u32 standby); int (*load_fw)(struct v4l2_subdev *sd); int (*reset)(struct v4l2_subdev *sd, u32 val); int (*s_gpio)(struct v4l2_subdev *sd, u32 val);
NOT FOR SUBMISSION. Probably, another solution has to be found. soc-camera drivers need an .init() (marked as "don't use") and a .halt() methods. Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> --- Hans, you moved s_standby to tuner_ops, and init is not recommended for new drivers. Suggestions? include/media/v4l2-subdev.h | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)