Re: [PATCH 4/4] media: mt9m111: remove .s_power callback

[Marco Felsch]

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@xxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxx/

> 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
> >
>