Re: [Patch v2 5/7] media: i2c: ov2659: Add powerdown/reset gpio handling

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Benoit,

On Fri, Sep 20, 2019 at 11:55:29AM -0500, Benoit Parrot wrote:
...
> > > @@ -1400,6 +1440,18 @@ static int ov2659_probe(struct i2c_client *client)
> > >  	    ov2659->xvclk_frequency > 27000000)
> > >  		return -EINVAL;
> > >  
> > > +	/* Optional gpio don't fail if not present */
> > > +	ov2659->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
> > > +						    GPIOD_OUT_LOW);
> > > +	if (IS_ERR(ov2659->pwdn_gpio))
> > > +		return PTR_ERR(ov2659->pwdn_gpio);
> > > +
> > > +	/* Optional gpio don't fail if not present */
> > > +	ov2659->resetb_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> > > +						      GPIOD_OUT_HIGH);
> > > +	if (IS_ERR(ov2659->resetb_gpio))
> > > +		return PTR_ERR(ov2659->resetb_gpio);
> > > +
> > >  	v4l2_ctrl_handler_init(&ov2659->ctrls, 2);
> > >  	ov2659->link_frequency =
> > >  			v4l2_ctrl_new_std(&ov2659->ctrls, &ov2659_ctrl_ops,
> > > @@ -1445,6 +1497,9 @@ static int ov2659_probe(struct i2c_client *client)
> > >  	ov2659->frame_size = &ov2659_framesizes[2];
> > >  	ov2659->format_ctrl_regs = ov2659_formats[0].format_ctrl_regs;
> > >  
> > > +	pm_runtime_enable(&client->dev);
> > > +	pm_runtime_get_sync(&client->dev);
> > 
> > This makes the driver depend on runtime PM.
> 
> Obviously.
> Why? Is that bad?

Well, if it is, then it should be listed in driver's dependencies in
Kconfig.

> 
> > 
> > See e.g. the smiapp driver for an example how to make it work without. It
> > wasn't trivial. :I You won't need autosuspend.
> 
> I took a look at that driver, but I don't get your reference to being able
> to work without runtime pm!

The driver didn't need runtime PM, so it'd be nice to continue work
without.

What smiapp does is that it powers the sensor on first *without* runtime
PM, and then proceeds to set up runtime PM if it's available. The sensor
will only be powered off when the device is unbound with runtime PM
disabled.

Regarding the smiapp driver, you can replace pm_runtime_get_noresume() and
all the autoidle lines with pm_runtime_idle() call after
pm_runtime_enable() in the ov2659 driver.

> That driver looks pretty similar to ov7740.c which I used as a reference
> for this.

I guess in practice many sensor drivers don't work without it on DT-based
systems I'm afraid. :-( They should be fixed.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com