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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190923061731.GZ5781@paasikivi.fi.intel.com>
Date:   Mon, 23 Sep 2019 09:17:32 +0300
From:   Sakari Ailus <sakari.ailus@...ux.intel.com>
To:     Benoit Parrot <bparrot@...com>
Cc:     Hans Verkuil <hverkuil@...all.nl>,
        Prabhakar Lad <prabhakar.csengg@...il.com>,
        linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [Patch v2 5/7] media: i2c: ov2659: Add powerdown/reset gpio
 handling

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@...ux.intel.com