From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 78D80C3F68F for ; Thu, 30 Jan 2020 13:45:36 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 5944F20707 for ; Thu, 30 Jan 2020 13:45:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727348AbgA3Npf (ORCPT ); Thu, 30 Jan 2020 08:45:35 -0500 Received: from honk.sigxcpu.org ([24.134.29.49]:53374 "EHLO honk.sigxcpu.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726996AbgA3Npf (ORCPT ); Thu, 30 Jan 2020 08:45:35 -0500 Received: from localhost (localhost [127.0.0.1]) by honk.sigxcpu.org (Postfix) with ESMTP id 7528BFB03; Thu, 30 Jan 2020 14:45:33 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at honk.sigxcpu.org Received: from honk.sigxcpu.org ([127.0.0.1]) by localhost (honk.sigxcpu.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id bIifrS_4u1_D; Thu, 30 Jan 2020 14:45:31 +0100 (CET) Received: by bogon.sigxcpu.org (Postfix, from userid 1000) id B228540919; Thu, 30 Jan 2020 14:45:30 +0100 (CET) Date: Thu, 30 Jan 2020 14:45:30 +0100 From: Guido =?iso-8859-1?Q?G=FCnther?= To: Tomas Novotny Cc: Jonathan Cameron , Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald-Stadler , "Angus Ainslie (Purism)" , Marco Felsch , Thomas Gleixner , linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] iio: vncl4000: Enable runtime pm for vcnl4200/4040 Message-ID: <20200130134530.GA287278@bogon.m.sigxcpu.org> References: <65d5c7b562f9f7c17857310e8538afe0bb1b2ddb.1579531608.git.agx@sigxcpu.org> <20200120182853.37a724fa@tomas.local.tbs-biometrics.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200120182853.37a724fa@tomas.local.tbs-biometrics.cz> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Mon, Jan 20, 2020 at 06:28:53PM +0100, Tomas Novotny wrote: > Hi Guido, > > On Mon, 20 Jan 2020 16:01:24 +0100 > Guido Günther wrote: > > > This is modelled after the vcnl4035 driver. The vcnl4000 does not seem > > to have a way to save power so we just leave it running. > > To be precise - only the on-demand measurement is done for vcnl4000/4010/4020 > in this driver (so it is not running in the end). The periodic measurement > (only) is done for vcnl4040/4200. Fixed in the commit message. > > Note below. > > > Signed-off-by: Guido Günther > > --- > > drivers/iio/light/vcnl4000.c | 126 +++++++++++++++++++++++++++++++++-- > > 1 file changed, 120 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c > > index 8f198383626b..b0396a3f1915 100644 > > --- a/drivers/iio/light/vcnl4000.c > > +++ b/drivers/iio/light/vcnl4000.c > > @@ -22,6 +22,7 @@ > > #include > > #include > > #include > > +#include > > > > #include > > #include > > @@ -57,6 +58,8 @@ > > #define VCNL4000_AL_OD BIT(4) /* start on-demand ALS measurement */ > > #define VCNL4000_PS_OD BIT(3) /* start on-demand proximity measurement */ > > > > +#define VCNL4000_SLEEP_DELAY_MS 2000 /* before we enter pm_runtime_suspend */ > > + > > enum vcnl4000_device_ids { > > VCNL4000, > > VCNL4010, > > @@ -87,6 +90,7 @@ struct vcnl4000_chip_spec { > > int (*init)(struct vcnl4000_data *data); > > int (*measure_light)(struct vcnl4000_data *data, int *val); > > int (*measure_proximity)(struct vcnl4000_data *data, int *val); > > + int (*set_power_state)(struct vcnl4000_data *data, bool on); > > }; > > > > static const struct i2c_device_id vcnl4000_id[] = { > > @@ -99,6 +103,12 @@ static const struct i2c_device_id vcnl4000_id[] = { > > }; > > MODULE_DEVICE_TABLE(i2c, vcnl4000_id); > > > > +static int vcnl4000_set_power_state(struct vcnl4000_data *data, bool on) > > +{ > > + /* no suspend op */ > > + return 0; > > +} > > + > > static int vcnl4000_init(struct vcnl4000_data *data) > > { > > int ret, prod_id; > > @@ -127,9 +137,24 @@ static int vcnl4000_init(struct vcnl4000_data *data) > > data->al_scale = 250000; > > mutex_init(&data->vcnl4000_lock); > > > > + ret = data->chip_spec->set_power_state(data, true); > > + if (ret < 0) > > + return ret; > > + > > return 0; > > }; > > > > +static int vcnl4200_set_power_state(struct vcnl4000_data *data, bool on) > > +{ > > + u16 val = on ? 0 /* power on */ : 1 /* shut down */; > > + int ret; > > + > > + ret = i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, val); > > + if (ret < 0) > > + return ret; > > + return i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, val); > > +} > > + > > static int vcnl4200_init(struct vcnl4000_data *data) > > { > > int ret, id; > > @@ -155,11 +180,7 @@ static int vcnl4200_init(struct vcnl4000_data *data) > > > > data->rev = (ret >> 8) & 0xf; > > > > - /* Set defaults and enable both channels */ > > - ret = i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, 0); > > - if (ret < 0) > > - return ret; > > - ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, 0); > > + ret = data->chip_spec->set_power_state(data, true); > > if (ret < 0) > > return ret; > > > > @@ -291,24 +312,28 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = { > > .init = vcnl4000_init, > > .measure_light = vcnl4000_measure_light, > > .measure_proximity = vcnl4000_measure_proximity, > > + .set_power_state = vcnl4000_set_power_state, > > }, > > [VCNL4010] = { > > .prod = "VCNL4010/4020", > > .init = vcnl4000_init, > > .measure_light = vcnl4000_measure_light, > > .measure_proximity = vcnl4000_measure_proximity, > > + .set_power_state = vcnl4000_set_power_state, > > }, > > [VCNL4040] = { > > .prod = "VCNL4040", > > .init = vcnl4200_init, > > .measure_light = vcnl4200_measure_light, > > .measure_proximity = vcnl4200_measure_proximity, > > + .set_power_state = vcnl4200_set_power_state, > > }, > > [VCNL4200] = { > > .prod = "VCNL4200", > > .init = vcnl4200_init, > > .measure_light = vcnl4200_measure_light, > > .measure_proximity = vcnl4200_measure_proximity, > > + .set_power_state = vcnl4200_set_power_state, > > }, > > }; > > > > @@ -323,6 +348,23 @@ static const struct iio_chan_spec vcnl4000_channels[] = { > > } > > }; > > > > +static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data *data, bool on) > > +{ > > + struct device *dev = &data->client->dev; > > + int ret; > > + > > + if (on) { > > + ret = pm_runtime_get_sync(dev); > > + if (ret < 0) > > + pm_runtime_put_noidle(dev); > > + } else { > > + pm_runtime_mark_last_busy(dev); > > + ret = pm_runtime_put_autosuspend(dev); > > + } > > + > > + return ret; > > +} > > + > > static int vcnl4000_read_raw(struct iio_dev *indio_dev, > > struct iio_chan_spec const *chan, > > int *val, int *val2, long mask) > > @@ -332,6 +374,10 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev, > > > > switch (mask) { > > case IIO_CHAN_INFO_RAW: > > + ret = vcnl4000_set_pm_runtime_state(data, true); > > + if (ret < 0) > > + return ret; > > + > > switch (chan->type) { > > case IIO_LIGHT: > > ret = data->chip_spec->measure_light(data, val); > > @@ -346,6 +392,7 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev, > > default: > > ret = -EINVAL; > > } > > + vcnl4000_set_pm_runtime_state(data, false); > > return ret; > > case IIO_CHAN_INFO_SCALE: > > if (chan->type != IIO_LIGHT) > > @@ -394,7 +441,22 @@ static int vcnl4000_probe(struct i2c_client *client, > > indio_dev->name = VCNL4000_DRV_NAME; > > indio_dev->modes = INDIO_DIRECT_MODE; > > > > - return devm_iio_device_register(&client->dev, indio_dev); > > + ret = pm_runtime_set_active(&client->dev); > > + if (ret < 0) > > + goto fail_poweroff; > > + > > + ret = iio_device_register(indio_dev); > > + if (ret < 0) > > + goto fail_poweroff; > > + > > + pm_runtime_enable(&client->dev); > > + pm_runtime_set_autosuspend_delay(&client->dev, VCNL4000_SLEEP_DELAY_MS); > > + pm_runtime_use_autosuspend(&client->dev); > > + > > + return 0; > > +fail_poweroff: > > + data->chip_spec->set_power_state(data, false); > > + return ret; > > } > > > > static const struct of_device_id vcnl_4000_of_match[] = { > > @@ -422,13 +484,65 @@ static const struct of_device_id vcnl_4000_of_match[] = { > > }; > > MODULE_DEVICE_TABLE(of, vcnl_4000_of_match); > > > > +static int vcnl4000_remove(struct i2c_client *client) > > +{ > > + struct iio_dev *indio_dev = i2c_get_clientdata(client); > > + struct vcnl4000_data *data = iio_priv(indio_dev); > > + > > + pm_runtime_dont_use_autosuspend(&client->dev); > > + pm_runtime_disable(&client->dev); > > + iio_device_unregister(indio_dev); > > + pm_runtime_set_suspended(&client->dev); > > + > > + return data->chip_spec->set_power_state(data, false); > > +} > > + > > +static int __maybe_unused vcnl4000_runtime_suspend(struct device *dev) > > +{ > > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); > > + struct vcnl4000_data *data = iio_priv(indio_dev); > > + int ret; > > + > > + ret = data->chip_spec->set_power_state(data, false); > > + > > + return ret; > > +} > > + > > +static int __maybe_unused vcnl4000_runtime_resume(struct device *dev) > > +{ > > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); > > + struct vcnl4000_data *data = iio_priv(indio_dev); > > + unsigned int msecs; > > + int ret; > > + > > + ret = data->chip_spec->set_power_state(data, true); > > + if (ret < 0) > > + return ret; > > + > > + /* wait for 1 ALS integration cycle */ > > + msecs = data->vcnl4200_al.sampling_rate ? > > + ktime_to_ms(data->vcnl4200_al.sampling_rate) : 100; > > + msleep(ktime_to_ms(msecs)); > > What about setting the vcnl4200_{al,ps}.last_measurement to ktime_get() in > case of vcnl4040/4200? The sleeping will be handled in the respective channel > during measurement. And it will be much faster if you are interested in > proximity reading only (you will be blocked only for proximity's 4.2ms > instead of 54ms because of ambient light). Good point. By just moving that into vcnl4200_set_power_state this becomes even more readable. Cheers, -- Guido > > The sleeping for vcnl4000 is not needed, as there is currently on-demand > measurement only. > > BTW the second ktime_to_ms shouldn't be called. > > Tomas > > > + return 0; > > +} > > + > > +static const struct dev_pm_ops vcnl4000_pm_ops = { > > + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > > + pm_runtime_force_resume) > > + SET_RUNTIME_PM_OPS(vcnl4000_runtime_suspend, > > + vcnl4000_runtime_resume, NULL) > > +}; > > + > > static struct i2c_driver vcnl4000_driver = { > > .driver = { > > .name = VCNL4000_DRV_NAME, > > + .pm = &vcnl4000_pm_ops, > > .of_match_table = vcnl_4000_of_match, > > }, > > .probe = vcnl4000_probe, > > .id_table = vcnl4000_id, > > + .remove = vcnl4000_remove, > > }; > > > > module_i2c_driver(vcnl4000_driver); >