| Message ID | 20220818144712.997477-4-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] media: mt9m111: add V4L2_CID_PIXEL_RATE support | expand |
Hi Marco On Thu, Aug 18, 2022 at 04:47:12PM +0200, Marco Felsch wrote: > This is in preparation of switching to the generic dev PM mechanism. > Since the .s_power callback will be removed in the near future move the > powering into the .s_stream callback. So this driver no longer depends > on the .s_power mechanism. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> If you want to move to runtime_pm, I would implement it first and have s_power call the _resume and _suspend routines, as some platform drivers still use s_power() and some of them might depend on it. It's a slippery slope.. I would love to get rid of s_power() but if any platform uses it with this sensor, it would stop working after this change. > --- > drivers/media/i2c/mt9m111.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c > index cd74c408e110..8e8ba5a8e6ea 100644 > --- a/drivers/media/i2c/mt9m111.c > +++ b/drivers/media/i2c/mt9m111.c > @@ -1065,7 +1065,6 @@ static const struct v4l2_ctrl_ops mt9m111_ctrl_ops = { > }; > > static const struct v4l2_subdev_core_ops mt9m111_subdev_core_ops = { > - .s_power = mt9m111_s_power, > .log_status = v4l2_ctrl_subdev_log_status, > .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > .unsubscribe_event = v4l2_event_subdev_unsubscribe, > @@ -1136,8 +1135,14 @@ static int mt9m111_enum_mbus_code(struct v4l2_subdev *sd, > static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable) > { > struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev); > + int ret; > > mt9m111->is_streaming = !!enable; > + > + ret = mt9m111_s_power(sd, enable); > + if (ret) > + return ret; > + > return 0; > } > > -- > 2.30.2 >
Hi Jacopo, thanks for your fast feedback :) On 22-08-18, Jacopo Mondi wrote: > Hi Marco > > On Thu, Aug 18, 2022 at 04:47:12PM +0200, Marco Felsch wrote: > > This is in preparation of switching to the generic dev PM mechanism. > > Since the .s_power callback will be removed in the near future move the > > powering into the .s_stream callback. So this driver no longer depends > > on the .s_power mechanism. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > If you want to move to runtime_pm, I would implement it first and have > s_power call the _resume and _suspend routines, as some platform > drivers still use s_power() and some of them might depend on it. Do we really have platforms which depend on this? IMHO if that is the case than we should fix those platfoms. Since new drivers shouldn't use this callback anymore. In my case, I worked on [1] and wondered why the sensor was enabled before I told him to do so. Since I didn't implement the s_power() callback, I had no chance to get enabled before. Can we please decide: - Do we wanna get rid of the s_power() callback? - If not, how do we handle those devices then with drivers not implementing this callback? [1] https://lore.kernel.org/all/20220818143307.967150-1-m.felsch@pengutronix.de/ > It's a slippery slope.. I would love to get rid of s_power() but if > any platform uses it with this sensor, it would stop working after > this change. > > > --- > > drivers/media/i2c/mt9m111.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c > > index cd74c408e110..8e8ba5a8e6ea 100644 > > --- a/drivers/media/i2c/mt9m111.c > > +++ b/drivers/media/i2c/mt9m111.c > > @@ -1065,7 +1065,6 @@ static const struct v4l2_ctrl_ops mt9m111_ctrl_ops = { > > }; > > > > static const struct v4l2_subdev_core_ops mt9m111_subdev_core_ops = { > > - .s_power = mt9m111_s_power, > > .log_status = v4l2_ctrl_subdev_log_status, > > .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > > .unsubscribe_event = v4l2_event_subdev_unsubscribe, > > @@ -1136,8 +1135,14 @@ static int mt9m111_enum_mbus_code(struct v4l2_subdev *sd, > > static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable) > > { > > struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev); > > + int ret; > > > > mt9m111->is_streaming = !!enable; > > + > > + ret = mt9m111_s_power(sd, enable); > > + if (ret) > > + return ret; > > + > > return 0; > > } > > > > -- > > 2.30.2 > > >
Hi Marco On Fri, Aug 19, 2022 at 09:18:32AM +0200, Marco Felsch wrote: > Hi Jacopo, > > thanks for your fast feedback :) > > On 22-08-18, Jacopo Mondi wrote: > > Hi Marco > > > > On Thu, Aug 18, 2022 at 04:47:12PM +0200, Marco Felsch wrote: > > > This is in preparation of switching to the generic dev PM mechanism. > > > Since the .s_power callback will be removed in the near future move the > > > powering into the .s_stream callback. So this driver no longer depends > > > on the .s_power mechanism. > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > > If you want to move to runtime_pm, I would implement it first and have > > s_power call the _resume and _suspend routines, as some platform > > drivers still use s_power() and some of them might depend on it. > > Do we really have platforms which depend on this? IMHO if that is the $ git grep "v4l2_subdev_call(.*, s_power" drivers/media/platform/ | cut -d : -f1 | uniq | wc -l 8 > case than we should fix those platfoms. Since new drivers shouldn't use > this callback anymore. Patches are clearly welcome I guess.. > > In my case, I worked on [1] and wondered why the sensor was enabled > before I told him to do so. Since I didn't implement the s_power() > callback, I had no chance to get enabled before. > I'm not sure I got this part > Can we please decide: > - Do we wanna get rid of the s_power() callback? I think that would be everyone's desire, but drivers have to be moved away from it > - If not, how do we handle those devices then with drivers not > implementing this callback? By maintaining compatibility. I suggested to move to runtime_pm() and wrap _resume/_suspend in s_power(). My understanding is that the two (runtime_pm/s_power) are mutually exclusive, but even if that was not the case, runtime_pm is reference counted, hence as long as calls are balanced this should work, right ? > > [1] https://lore.kernel.org/all/20220818143307.967150-1-m.felsch@pengutronix.de/ > > > It's a slippery slope.. I would love to get rid of s_power() but if > > any platform uses it with this sensor, it would stop working after > > this change. > > > > > --- > > > drivers/media/i2c/mt9m111.c | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c > > > index cd74c408e110..8e8ba5a8e6ea 100644 > > > --- a/drivers/media/i2c/mt9m111.c > > > +++ b/drivers/media/i2c/mt9m111.c > > > @@ -1065,7 +1065,6 @@ static const struct v4l2_ctrl_ops mt9m111_ctrl_ops = { > > > }; > > > > > > static const struct v4l2_subdev_core_ops mt9m111_subdev_core_ops = { > > > - .s_power = mt9m111_s_power, > > > .log_status = v4l2_ctrl_subdev_log_status, > > > .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > > > .unsubscribe_event = v4l2_event_subdev_unsubscribe, > > > @@ -1136,8 +1135,14 @@ static int mt9m111_enum_mbus_code(struct v4l2_subdev *sd, > > > static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable) > > > { > > > struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev); > > > + int ret; > > > > > > mt9m111->is_streaming = !!enable; > > > + > > > + ret = mt9m111_s_power(sd, enable); > > > + if (ret) > > > + return ret; > > > + > > > return 0; > > > } > > > > > > -- > > > 2.30.2 > > > > >
On 22-08-19, Jacopo Mondi wrote: > Hi Marco > > On Fri, Aug 19, 2022 at 09:18:32AM +0200, Marco Felsch wrote: > > Hi Jacopo, > > > > thanks for your fast feedback :) > > > > On 22-08-18, Jacopo Mondi wrote: > > > Hi Marco > > > > > > On Thu, Aug 18, 2022 at 04:47:12PM +0200, Marco Felsch wrote: > > > > This is in preparation of switching to the generic dev PM mechanism. > > > > Since the .s_power callback will be removed in the near future move the > > > > powering into the .s_stream callback. So this driver no longer depends > > > > on the .s_power mechanism. > > > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > > > > If you want to move to runtime_pm, I would implement it first and have > > > s_power call the _resume and _suspend routines, as some platform > > > drivers still use s_power() and some of them might depend on it. > > > > Do we really have platforms which depend on this? IMHO if that is the > > $ git grep "v4l2_subdev_call(.*, s_power" drivers/media/platform/ | cut -d : -f1 | uniq | wc -l > 8 > > > case than we should fix those platfoms. Since new drivers shouldn't use > > this callback anymore. > > Patches are clearly welcome I guess.. :) > > In my case, I worked on [1] and wondered why the sensor was enabled > > before I told him to do so. Since I didn't implement the s_power() > > callback, I had no chance to get enabled before. > > > > I'm not sure I got this part What I mean is, that the MT9M131 sensor gets enabled and immediately start sending frames before I told him to do so e.g. by calling s_stream(). This can confuse the downstream device. The only way to get enable the downstream device first is to add the s_power() callback. > > Can we please decide: > > - Do we wanna get rid of the s_power() callback? > > I think that would be everyone's desire, but drivers have to be moved > away from it > > > - If not, how do we handle those devices then with drivers not > > implementing this callback? > > By maintaining compatibility. I suggested to move to runtime_pm() and > wrap _resume/_suspend in s_power(). But then you're introducing new drivers with s_power() callbacks and so the behaviour isn't really changed. > My understanding is that the two (runtime_pm/s_power) are mutually > exclusive, but even if that was not the case, runtime_pm is reference > counted, hence as long as calls are balanced this should work, right ? Right but the s_power() behaviour is not changed and drivers still rely on it to work as right now. Regards, Marco
Hi Marco On Fri, Aug 19, 2022 at 10:06:26AM +0200, Marco Felsch wrote: > On 22-08-19, Jacopo Mondi wrote: > > Hi Marco > > > > On Fri, Aug 19, 2022 at 09:18:32AM +0200, Marco Felsch wrote: > > > Hi Jacopo, > > > > > > thanks for your fast feedback :) > > > > > > On 22-08-18, Jacopo Mondi wrote: > > > > Hi Marco > > > > > > > > On Thu, Aug 18, 2022 at 04:47:12PM +0200, Marco Felsch wrote: > > > > > This is in preparation of switching to the generic dev PM mechanism. > > > > > Since the .s_power callback will be removed in the near future move the > > > > > powering into the .s_stream callback. So this driver no longer depends > > > > > on the .s_power mechanism. > > > > > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > > > > > > If you want to move to runtime_pm, I would implement it first and have > > > > s_power call the _resume and _suspend routines, as some platform > > > > drivers still use s_power() and some of them might depend on it. > > > > > > Do we really have platforms which depend on this? IMHO if that is the > > > > $ git grep "v4l2_subdev_call(.*, s_power" drivers/media/platform/ | cut -d : -f1 | uniq | wc -l > > 8 > > > > > case than we should fix those platfoms. Since new drivers shouldn't use > > > this callback anymore. > > > > Patches are clearly welcome I guess.. > > :) > > > > In my case, I worked on [1] and wondered why the sensor was enabled > > > before I told him to do so. Since I didn't implement the s_power() > > > callback, I had no chance to get enabled before. > > > > > > > I'm not sure I got this part > > What I mean is, that the MT9M131 sensor gets enabled and immediately > start sending frames before I told him to do so e.g. by calling Why does this happen ? > s_stream(). This can confuse the downstream device. The only way to get > enable the downstream device first is to add the s_power() callback. > > > > Can we please decide: > > > - Do we wanna get rid of the s_power() callback? > > > > I think that would be everyone's desire, but drivers have to be moved > > away from it > > > > > - If not, how do we handle those devices then with drivers not > > > implementing this callback? > > > > By maintaining compatibility. I suggested to move to runtime_pm() and > > wrap _resume/_suspend in s_power(). > > But then you're introducing new drivers with s_power() callbacks and so > the behaviour isn't really changed. > I only meant in existing ones > > My understanding is that the two (runtime_pm/s_power) are mutually > > exclusive, but even if that was not the case, runtime_pm is reference > > counted, hence as long as calls are balanced this should work, right ? > > Right but the s_power() behaviour is not changed and drivers still rely > on it to work as right now. As long as we have bridge drivers using it, isn't this what we want ? > > Regards, > Marco
diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c index cd74c408e110..8e8ba5a8e6ea 100644 --- a/drivers/media/i2c/mt9m111.c +++ b/drivers/media/i2c/mt9m111.c @@ -1065,7 +1065,6 @@ static const struct v4l2_ctrl_ops mt9m111_ctrl_ops = { }; static const struct v4l2_subdev_core_ops mt9m111_subdev_core_ops = { - .s_power = mt9m111_s_power, .log_status = v4l2_ctrl_subdev_log_status, .subscribe_event = v4l2_ctrl_subdev_subscribe_event, .unsubscribe_event = v4l2_event_subdev_unsubscribe, @@ -1136,8 +1135,14 @@ static int mt9m111_enum_mbus_code(struct v4l2_subdev *sd, static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable) { struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev); + int ret; mt9m111->is_streaming = !!enable; + + ret = mt9m111_s_power(sd, enable); + if (ret) + return ret; + return 0; }
This is in preparation of switching to the generic dev PM mechanism. Since the .s_power callback will be removed in the near future move the powering into the .s_stream callback. So this driver no longer depends on the .s_power mechanism. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- drivers/media/i2c/mt9m111.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)