Re: [PATCH v3 6/6] Input: edt-ft5x06 - improve power management operations

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Marco,

On Thu, Jan 16, 2020 at 02:32:19PM +0100, Marco Felsch wrote:
> Hi Dmitry,
> 
> On 20-01-10 08:18, Marco Felsch wrote:
> > On 20-01-10 08:16, Marco Felsch wrote:
> > > Hi Dmitry,
> > > 
> > > On 20-01-09 17:09, Dmitry Torokhov wrote:
> > > > Hi Marco,
> > > > 
> > > > On Wed, Jan 08, 2020 at 12:10:50PM +0100, Marco Felsch wrote:
> > > > > +static int __maybe_unused edt_ft5x06_ts_resume(struct device *dev)
> > > > > +{
> > > > > +	struct i2c_client *client = to_i2c_client(dev);
> > > > > +	struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
> > > > > +	int ret;
> > > > > +
> > > > > +	if (device_may_wakeup(dev))
> > > > > +		return 0;
> > > > > +
> > > > > +	ret = regulator_enable(tsdata->vcc);
> > > > > +	if (ret)
> > > > > +		dev_warn(dev, "Failed to enable vcc\n");
> > > > 
> > > > I wonder if we should not return error here instead of continuing. If
> > > > device is not powered up properly we'll have hard time communicating
> > > > with it.
> > > 
> > > That's a reasonable point.
> > > 
> > > > The same is for suspend: maybe we should abort if we can't switch off
> > > > regulator or write to the device.
> > > 
> > > I have no strong opinion about that case but IMHO it's okay to go further
> > > if we can't switch it off. Instead we should print a warning.
> > 
> > I just noticed that we do that already.. So the suspend case should be
> > okay.
> 
> 
> Is it okay to check the return val for the resume case only? I want to
> prepare a v4 of this patch to get this done.

OK, I now remember my issues with power management in this driver. It
supports factory mode vs operational/normal mode, and updating register
settings at runtime. If you want to cut power off at suspend, then you
need to make sure you restore the mode and register settings at resume
time, not simply revert to normal mode.

Thanks.

-- 
Dmitry