Re: [PATCH v12 3/5] Input: goodix - add runtime power management support

[Bastien Nocera]

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 <irina.tirdea@xxxxxxxxx>
> ---
> 
<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 },