On Sat, 2016-09-10 at 20:57 +0300, Irina Tirdea wrote: > Add support for runtime power management so that the device is > turned off when not used (when the userspace holds no open > handles of the input device). The device uses autosuspend with a > default delay of 2 seconds, so the device will suspend if no > handles to it are open for 2 seconds. > > The runtime management support is only available if the gpio pins > are properly initialized from ACPI/DT. > > Signed-off-by: Irina Tirdea <[email protected]> > --- > <snip> > +static int goodix_set_power_state(struct goodix_ts_data *ts, bool > on)
I don't like having booleans for this sort of thing[1], as it's never clear whether "on" is "power state on" or "device is on". I'd rather you defined a 2 member enum, with meaningful names, so that the effect is clear when we're reading the caller. [1]: https://blog.ometer.com/2011/01/20/boolean-parameters-are-wrong/ > +{ > + int error; > + > + if (on) { > + error = pm_runtime_get_sync(&ts->client->dev); > + } else { > + pm_runtime_mark_last_busy(&ts->client->dev); > + error = pm_runtime_put_autosuspend(&ts->client- > >dev); > + } > + > + if (error < 0) { > + dev_err(&ts->client->dev, > + "failed to change power state to %d\n", on); > + if (on) > + pm_runtime_put_noidle(&ts->client->dev); > + > + return error; > + } > + > + return 0; > +} > + > static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 > *data) > { > int touch_num; > @@ -444,6 +474,10 @@ static ssize_t goodix_dump_config_show(struct > device *dev, > > wait_for_completion(&ts->firmware_loading_complete); > > + error = goodix_set_power_state(ts, true); > + if (error) > + return error; > + > error = goodix_i2c_read(ts->client, GOODIX_REG_CONFIG_DATA, > config, ts->cfg_len); > if (error) { > @@ -452,6 +486,8 @@ static ssize_t goodix_dump_config_show(struct > device *dev, > return error; > } > > + goodix_set_power_state(ts, false); > + > for (i = 0; i < ts->cfg_len; i++) > count += scnprintf(buf + count, PAGE_SIZE - count, > "%02x ", > config[i]); > @@ -548,11 +584,13 @@ static ssize_t goodix_esd_timeout_store(struct > device *dev, > return error; > > esd_timeout = atomic_read(&ts->esd_timeout); > - if (esd_timeout && !new_esd_timeout) > + if (esd_timeout && !new_esd_timeout && > + pm_runtime_active(&ts->client->dev)) > goodix_disable_esd(ts); > > atomic_set(&ts->esd_timeout, new_esd_timeout); > - if (!esd_timeout && new_esd_timeout) > + if (!esd_timeout && new_esd_timeout && > + pm_runtime_active(&ts->client->dev)) > goodix_enable_esd(ts); > > return count; > @@ -573,6 +611,34 @@ static const struct attribute_group > goodix_attr_group = { > .attrs = goodix_attrs, > }; > > +static int goodix_open(struct input_dev *input_dev) > +{ > + struct goodix_ts_data *ts = input_get_drvdata(input_dev); > + int error; > + > + if (!ts->gpiod_int || !ts->gpiod_rst) > + return 0; > + > + wait_for_completion(&ts->firmware_loading_complete); > + > + error = goodix_set_power_state(ts, true); > + if (error) > + return error; > + atomic_inc(&ts->open_count); > + return 0; > +} > + > +static void goodix_close(struct input_dev *input_dev) > +{ > + struct goodix_ts_data *ts = input_get_drvdata(input_dev); > + > + if (!ts->gpiod_int || !ts->gpiod_rst) > + return; > + > + goodix_set_power_state(ts, false); > + atomic_dec(&ts->open_count); > +} > + > /** > * goodix_get_gpio_config - Get GPIO config from ACPI/DT > * > @@ -754,6 +820,9 @@ static int goodix_request_input_dev(struct > goodix_ts_data *ts) > ts->input_dev->id.vendor = 0x0416; > ts->input_dev->id.product = ts->id; > ts->input_dev->id.version = ts->version; > + ts->input_dev->open = goodix_open; > + ts->input_dev->close = goodix_close; > + input_set_drvdata(ts->input_dev, ts); > > error = input_register_device(ts->input_dev); > if (error) { > @@ -808,7 +877,8 @@ static int goodix_configure_dev(struct > goodix_ts_data *ts) > * @ts: our goodix_ts_data pointer > * > * request_firmware_wait callback that finishes > - * initialization of the device. > + * initialization of the device. This will only be called > + * when ts->gpiod_int and ts->gpiod_rst are properly initialized. > */ > static void goodix_config_cb(const struct firmware *cfg, void *ctx) > { > @@ -828,6 +898,19 @@ static void goodix_config_cb(const struct > firmware *cfg, void *ctx) > > goodix_enable_esd(ts); > > + pm_runtime_set_autosuspend_delay(&ts->client->dev, > + GOODIX_AUTOSUSPEND_DELAY_MS > ); > + pm_runtime_use_autosuspend(&ts->client->dev); > + error = pm_runtime_set_active(&ts->client->dev); > + if (error) { > + dev_err(&ts->client->dev, "failed to set active: > %d\n", error); > + goto err_release_cfg; > + } > + pm_runtime_enable(&ts->client->dev); > + /* Must not suspend immediately after device initialization > */ > + pm_runtime_mark_last_busy(&ts->client->dev); > + pm_request_autosuspend(&ts->client->dev); > + > err_release_cfg: > release_firmware(cfg); > complete_all(&ts->firmware_loading_complete); > @@ -854,6 +937,7 @@ static int goodix_ts_probe(struct i2c_client > *client, > i2c_set_clientdata(client, ts); > init_completion(&ts->firmware_loading_complete); > INIT_DELAYED_WORK(&ts->esd_work, goodix_esd_work); > + mutex_init(&ts->mutex); > > error = goodix_get_gpio_config(ts); > if (error) > @@ -940,23 +1024,33 @@ static int goodix_ts_remove(struct i2c_client > *client) > > wait_for_completion(&ts->firmware_loading_complete); > > + pm_runtime_disable(&client->dev); > + pm_runtime_set_suspended(&client->dev); > + pm_runtime_put_noidle(&client->dev); > + > sysfs_remove_group(&client->dev.kobj, &goodix_attr_group); > goodix_disable_esd(ts); > > return 0; > } > > -static int __maybe_unused goodix_suspend(struct device *dev) > +static int __maybe_unused goodix_sleep(struct device *dev) > { > struct i2c_client *client = to_i2c_client(dev); > struct goodix_ts_data *ts = i2c_get_clientdata(client); > - int error; > + int error = 0; > > /* We need gpio pins to suspend/resume */ > if (!ts->gpiod_int || !ts->gpiod_rst) > return 0; > > wait_for_completion(&ts->firmware_loading_complete); > + > + mutex_lock(&ts->mutex); > + > + if (ts->suspended) > + goto out_error; > + > goodix_disable_esd(ts); > > /* Free IRQ as IRQ pin is used as output in the suspend > sequence */ > @@ -966,7 +1060,7 @@ static int __maybe_unused goodix_suspend(struct > device *dev) > error = gpiod_direction_output(ts->gpiod_int, 0); > if (error) { > goodix_request_irq(ts); > - return error; > + goto out_error; > } > > usleep_range(5000, 6000); > @@ -977,7 +1071,8 @@ static int __maybe_unused goodix_suspend(struct > device *dev) > dev_err(&ts->client->dev, "Screen off command > failed\n"); > gpiod_direction_input(ts->gpiod_int); > goodix_request_irq(ts); > - return -EAGAIN; > + error = -EAGAIN; > + goto out_error; > } > > /* > @@ -986,44 +1081,77 @@ static int __maybe_unused > goodix_suspend(struct device *dev) > * sooner, delay 58ms here. > */ > msleep(58); > + ts->suspended = true; > + mutex_unlock(&ts->mutex); > + > return 0; > + > +out_error: > + mutex_unlock(&ts->mutex); > + return error; > } > > -static int __maybe_unused goodix_resume(struct device *dev) > +static int __maybe_unused goodix_wakeup(struct device *dev) > { > struct i2c_client *client = to_i2c_client(dev); > struct goodix_ts_data *ts = i2c_get_clientdata(client); > - int error; > + int error = 0; > > if (!ts->gpiod_int || !ts->gpiod_rst) > return 0; > > + mutex_lock(&ts->mutex); > + > + if (!ts->suspended) > + goto out_error; > + > /* > * Exit sleep mode by outputting HIGH level to INT pin > * for 2ms~5ms. > */ > error = gpiod_direction_output(ts->gpiod_int, 1); > if (error) > - return error; > + goto out_error; > > usleep_range(2000, 5000); > > error = goodix_int_sync(ts); > if (error) > - return error; > + goto out_error; > > error = goodix_request_irq(ts); > if (error) > - return error; > + goto out_error; > > error = goodix_enable_esd(ts); > if (error) > - return error; > + goto out_error; > + > + ts->suspended = false; > + mutex_unlock(&ts->mutex); > > return 0; > + > +out_error: > + mutex_unlock(&ts->mutex); > + return error; > +} > + > +static int __maybe_unused goodix_resume(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct goodix_ts_data *ts = i2c_get_clientdata(client); > + > + if (!atomic_read(&ts->open_count)) > + return 0; > + > + return goodix_wakeup(dev); > } > > -static SIMPLE_DEV_PM_OPS(goodix_pm_ops, goodix_suspend, > goodix_resume); > +static const struct dev_pm_ops goodix_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(goodix_sleep, goodix_resume) > + SET_RUNTIME_PM_OPS(goodix_sleep, goodix_wakeup, NULL) > +}; > > static const struct i2c_device_id goodix_ts_id[] = { > { "GDIX1001:00", 0 },
