| Message ID | 20190917155808.27818-7-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | EDT-FT5x06 improvements | expand |
On Tue, Sep 17, 2019 at 05:58:08PM +0200, Marco Felsch wrote: > Currently the driver do not care about the regulator which supplies the > controller. This can lead into problems since we support (deep-)sleep > because the regulator can be turned off before we send the (deep-)sleep > command to the controller. Using a own regulator improves the power > consumption during sleep even more. > + tsdata->vdd = devm_regulator_get(&client->dev, "vdd"); > + if (IS_ERR(tsdata->vdd)) { > + error = PTR_ERR(tsdata->vdd); > + if (error == -EPROBE_DEFER) > + return error; Before it worked w/o regulator. You have to make it optional. > + dev_err(&client->dev, > + "Failed to request vdd-supply, error %d\n", error); > + return error; > + } > + > + error = regulator_enable(tsdata->vdd); > + if (error) { > + dev_err(&client->dev, > + "Failed to enable vdd-supply, error %d\n", error); > + return error; > + }
On 19-09-17 19:35, Andy Shevchenko wrote: > On Tue, Sep 17, 2019 at 05:58:08PM +0200, Marco Felsch wrote: > > Currently the driver do not care about the regulator which supplies the > > controller. This can lead into problems since we support (deep-)sleep > > because the regulator can be turned off before we send the (deep-)sleep > > command to the controller. Using a own regulator improves the power > > consumption during sleep even more. > > > + tsdata->vdd = devm_regulator_get(&client->dev, "vdd"); > > + if (IS_ERR(tsdata->vdd)) { > > + error = PTR_ERR(tsdata->vdd); > > + if (error == -EPROBE_DEFER) > > + return error; > > Before it worked w/o regulator. You have to make it optional. devm_regulator_get will return a dummy regulator if no one is specified so it is optional. Regards, Marco > > > + dev_err(&client->dev, > > + "Failed to request vdd-supply, error %d\n", error); > > + return error; > > + } > > + > > + error = regulator_enable(tsdata->vdd); > > + if (error) { > > + dev_err(&client->dev, > > + "Failed to enable vdd-supply, error %d\n", error); > > + return error; > > + } > > -- > With Best Regards, > Andy Shevchenko > > >
On Tue, Sep 17, 2019 at 07:35:36PM +0300, Andy Shevchenko wrote: > On Tue, Sep 17, 2019 at 05:58:08PM +0200, Marco Felsch wrote: > > Currently the driver do not care about the regulator which supplies the > > controller. This can lead into problems since we support (deep-)sleep > > because the regulator can be turned off before we send the (deep-)sleep > > command to the controller. Using a own regulator improves the power > > consumption during sleep even more. > > > + tsdata->vdd = devm_regulator_get(&client->dev, "vdd"); > > + if (IS_ERR(tsdata->vdd)) { > > + error = PTR_ERR(tsdata->vdd); > > + if (error == -EPROBE_DEFER) > > + return error; > > Before it worked w/o regulator. You have to make it optional. No, regulators are funky this way. If there isn't one declared in the device tree then a dummy is created automatically. Optional regulators are only to be used when parts of an IC can be powered up separately. Thanks.
On Tue, Sep 17, 2019 at 09:52:45AM -0700, Dmitry Torokhov wrote: > On Tue, Sep 17, 2019 at 07:35:36PM +0300, Andy Shevchenko wrote: > > On Tue, Sep 17, 2019 at 05:58:08PM +0200, Marco Felsch wrote: > > > Currently the driver do not care about the regulator which supplies the > > > controller. This can lead into problems since we support (deep-)sleep > > > because the regulator can be turned off before we send the (deep-)sleep > > > command to the controller. Using a own regulator improves the power > > > consumption during sleep even more. > > > > > + tsdata->vdd = devm_regulator_get(&client->dev, "vdd"); > > > + if (IS_ERR(tsdata->vdd)) { > > > + error = PTR_ERR(tsdata->vdd); > > > + if (error == -EPROBE_DEFER) > > > + return error; > > > > Before it worked w/o regulator. You have to make it optional. > > No, regulators are funky this way. If there isn't one declared in the > device tree then a dummy is created automatically. Optional regulators > are only to be used when parts of an IC can be powered up separately. It depends if platform has full constrains or not. For the former, indeed, you are right, for the latter, we will get -ENODEV.
On Tue, Sep 17, 2019 at 08:12:46PM +0300, Andy Shevchenko wrote: > On Tue, Sep 17, 2019 at 09:52:45AM -0700, Dmitry Torokhov wrote: > > On Tue, Sep 17, 2019 at 07:35:36PM +0300, Andy Shevchenko wrote: > > > On Tue, Sep 17, 2019 at 05:58:08PM +0200, Marco Felsch wrote: > > > > Currently the driver do not care about the regulator which supplies the > > > > controller. This can lead into problems since we support (deep-)sleep > > > > because the regulator can be turned off before we send the (deep-)sleep > > > > command to the controller. Using a own regulator improves the power > > > > consumption during sleep even more. > > > > > > > + tsdata->vdd = devm_regulator_get(&client->dev, "vdd"); > > > > + if (IS_ERR(tsdata->vdd)) { > > > > + error = PTR_ERR(tsdata->vdd); > > > > + if (error == -EPROBE_DEFER) > > > > + return error; > > > > > > Before it worked w/o regulator. You have to make it optional. > > > > No, regulators are funky this way. If there isn't one declared in the > > device tree then a dummy is created automatically. Optional regulators > > are only to be used when parts of an IC can be powered up separately. > > It depends if platform has full constrains or not. Full constraints is the default behavior. Do we even have platforms that are not? ACPI and DT are fully-constrained, so that leaves legacy boards... But there isn't a single one in the tree that uses this driver. Thanks.
On Tue, Sep 17, 2019 at 10:21:05AM -0700, Dmitry Torokhov wrote: > On Tue, Sep 17, 2019 at 08:12:46PM +0300, Andy Shevchenko wrote: > > On Tue, Sep 17, 2019 at 09:52:45AM -0700, Dmitry Torokhov wrote: > > > On Tue, Sep 17, 2019 at 07:35:36PM +0300, Andy Shevchenko wrote: > > > > On Tue, Sep 17, 2019 at 05:58:08PM +0200, Marco Felsch wrote: > > > > > Currently the driver do not care about the regulator which supplies the > > > > > controller. This can lead into problems since we support (deep-)sleep > > > > > because the regulator can be turned off before we send the (deep-)sleep > > > > > command to the controller. Using a own regulator improves the power > > > > > consumption during sleep even more. > > > > > > > > > + tsdata->vdd = devm_regulator_get(&client->dev, "vdd"); > > > > > + if (IS_ERR(tsdata->vdd)) { > > > > > + error = PTR_ERR(tsdata->vdd); > > > > > + if (error == -EPROBE_DEFER) > > > > > + return error; > > > > > > > > Before it worked w/o regulator. You have to make it optional. > > > > > > No, regulators are funky this way. If there isn't one declared in the > > > device tree then a dummy is created automatically. Optional regulators > > > are only to be used when parts of an IC can be powered up separately. > > > > It depends if platform has full constrains or not. > > Full constraints is the default behavior. Do we even have platforms that > are not? ACPI and DT are fully-constrained, so that leaves legacy > boards... But there isn't a single one in the tree that uses this > driver. Fine then!
diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c index b95ce5a7482d..6429e915ec94 100644 --- a/drivers/input/touchscreen/edt-ft5x06.c +++ b/drivers/input/touchscreen/edt-ft5x06.c @@ -27,6 +27,7 @@ #include <linux/module.h> #include <linux/property.h> #include <linux/ratelimit.h> +#include <linux/regulator/consumer.h> #include <linux/slab.h> #include <linux/uaccess.h> @@ -96,6 +97,7 @@ struct edt_ft5x06_ts_data { struct gpio_desc *reset_gpio; struct gpio_desc *wake_gpio; + struct regulator *vdd; #if defined(CONFIG_DEBUG_FS) struct dentry *debug_dir; @@ -1090,6 +1092,23 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client, return error; } + tsdata->vdd = devm_regulator_get(&client->dev, "vdd"); + if (IS_ERR(tsdata->vdd)) { + error = PTR_ERR(tsdata->vdd); + if (error == -EPROBE_DEFER) + return error; + dev_err(&client->dev, + "Failed to request vdd-supply, error %d\n", error); + return error; + } + + error = regulator_enable(tsdata->vdd); + if (error) { + dev_err(&client->dev, + "Failed to enable vdd-supply, error %d\n", error); + return error; + } + /* * We need to go this way to keep backward compatibility with old DT's * which may assume the default-on mechanism. @@ -1111,7 +1130,8 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client, input = devm_input_allocate_device(&client->dev); if (!input) { dev_err(&client->dev, "failed to allocate input device.\n"); - return -ENOMEM; + error = -ENOMEM; + goto err_reg_disable; } mutex_init(&tsdata->mutex); @@ -1122,7 +1142,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client, error = edt_ft5x06_ts_identify(client, tsdata, fw_version); if (error) { dev_err(&client->dev, "touchscreen probe failed\n"); - return error; + goto err_reg_disable; } edt_ft5x06_ts_set_regs(tsdata); @@ -1158,7 +1178,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client, INPUT_MT_DIRECT); if (error) { dev_err(&client->dev, "Unable to init MT slots.\n"); - return error; + goto err_reg_disable; } i2c_set_clientdata(client, tsdata); @@ -1173,16 +1193,16 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client, client->name, tsdata); if (error) { dev_err(&client->dev, "Unable to request touchscreen IRQ.\n"); - return error; + goto err_reg_disable; } error = devm_device_add_group(&client->dev, &edt_ft5x06_attr_group); if (error) - return error; + goto err_reg_disable; error = input_register_device(input); if (error) - return error; + goto err_reg_disable; edt_ft5x06_ts_prepare_debugfs(tsdata, dev_driver_string(&client->dev)); device_init_wakeup(&client->dev, en_wakeup); @@ -1194,6 +1214,10 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client, tsdata->reset_gpio ? desc_to_gpio(tsdata->reset_gpio) : -1); return 0; + +err_reg_disable: + regulator_disable(tsdata->vdd); + return error; } static int edt_ft5x06_ts_remove(struct i2c_client *client) @@ -1225,8 +1249,16 @@ static int __maybe_unused edt_ft5x06_ts_suspend(struct device *dev) ret = edt_ft5x06_register_write(tsdata, PMOD_REGISTER_OPMODE, PMOD_REGISTER_HIBERNATE); - if (ret) + if (ret) { dev_err(dev, "Failed to set hibernate mode\n"); + return ret; + } + + ret = regulator_disable(tsdata->vdd); + if (ret) { + dev_err(dev, "Failed to disable vdd\n"); + return ret; + } return ret; } @@ -1235,12 +1267,19 @@ 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 error; if (device_may_wakeup(dev)) { disable_irq_wake(client->irq); return 0; } + error = regulator_enable(tsdata->vdd); + if (error) { + dev_err(&client->dev, "Failed to enable vdd\n"); + return error; + } + /* Recover from hibernate mode if hardware supports it */ if (tsdata->wake_gpio) { gpiod_set_value_cansleep(tsdata->wake_gpio, 0);
Currently the driver do not care about the regulator which supplies the controller. This can lead into problems since we support (deep-)sleep because the regulator can be turned off before we send the (deep-)sleep command to the controller. Using a own regulator improves the power consumption during sleep even more. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- drivers/input/touchscreen/edt-ft5x06.c | 53 ++++++++++++++++++++++---- 1 file changed, 46 insertions(+), 7 deletions(-)