On Tue, Mar 06, 2018 at 06:28:43PM +0900, Tomasz Figa wrote:
> On Tue, Mar 6, 2018 at 6:18 PM, Sakari Ailus
> <sakari.ailus@xxxxxxxxxxxxxxx> wrote:
> > On Tue, Mar 06, 2018 at 05:51:36PM +0900, Tomasz Figa wrote:
> >> On Tue, Mar 6, 2018 at 5:40 PM, Sakari Ailus
> >> <sakari.ailus@xxxxxxxxxxxxxxx> wrote:
> >> > Hi Tomasz and Andy,
> >> >
> >> > On Sat, Mar 03, 2018 at 12:43:59AM +0900, Tomasz Figa wrote:
> >> > ...
> >> >> > +static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
> >> >> > +{
> >> >> > + struct imx258 *imx258 =
> >> >> > + container_of(ctrl->handler, struct imx258, ctrl_handler);
> >> >> > + struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
> >> >> > + int ret = 0;
> >> >> > + /*
> >> >> > + * Applying V4L2 control value only happens
> >> >> > + * when power is up for streaming
> >> >> > + */
> >> >> > + if (pm_runtime_get_if_in_use(&client->dev) <= 0)
> >> >> > + return 0;
> >> >>
> >> >> I thought we decided to fix this to handle disabled runtime PM properly.
> >> >
> >> > Good point. I bet this is a problem in a few other drivers, too. How would
> >> > you fix that? Check for zero only?
> >> >
> >>
> >> bool need_runtime_put;
> >>
> >> ret = pm_runtime_get_if_in_use(&client->dev);
> >> if (ret <= 0 && ret != -EINVAL)
> >> return ret;
> >> need_runtime_put = ret > 0;
> >>
> >> // Do stuff ...
> >>
> >> if (need_runtime_put)
> >> pm_runtime_put(&client->dev);
> >>
> >> I don't like how ugly it is, but it appears to be the only way to
> >> handle this correctly.
> >
> > The driver enables runtime PM so if runtime PM is enabled in kernel
> > configuration, it is enabled here. In that case pm_runtime_get_if_in_use()
> > will return either 0 or 1. So as far as I can see, changing the lines to:
> >
> > if (!pm_runtime_get_if_in_use(&client->dev))
> > return 0;
> >
> > is enough.
>
> Right, my bad. Somehow I was convinced that enable status can change at
> runtime.
Good point. I guess in principle this could happen although I can't see a
reason to do so, other than to break things --- quoting
Documentation/power/runtime_pm.txt:
The user space can effectively disallow the driver of the device to
power manage it at run time by changing the value of its
/sys/devices/.../power/control attribute to "on", which causes
pm_runtime_forbid() to be called. In principle, this mechanism may
also be used by the driver to effectively turn off the runtime
power management of the device until the user space turns it on.
Namely, during the initialization the driver can make sure that the
runtime PM status of the device is 'active' and call
pm_runtime_forbid(). It should be noted, however, that if the user
space has already intentionally changed the value of
/sys/devices/.../power/control to "auto" to allow the driver to
power manage the device at run time, the driver may confuse it by
using pm_runtime_forbid() this way.
So that comes with a warning that things might not work well after doing
so.
What comes to the driver code, I still wouldn't complicate it by attempting
to make a driver work in such a case.
--
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
