| Message ID | 20171208215419.30396-5-mylene.josserand@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Mylène, On Fri, Dec 08, 2017 at 10:54:18PM +0100, Mylène Josserand wrote: > Add the support of regulator to use them as VCC source. > > Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com> > --- > drivers/input/touchscreen/edt-ft5x06.c | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c > index c53a3d7239e7..44b0e04a8f35 100644 > --- a/drivers/input/touchscreen/edt-ft5x06.c > +++ b/drivers/input/touchscreen/edt-ft5x06.c > @@ -39,6 +39,7 @@ > #include <linux/input/mt.h> > #include <linux/input/touchscreen.h> > #include <linux/of_device.h> > +#include <linux/regulator/consumer.h> > > #define WORK_REGISTER_THRESHOLD 0x00 > #define WORK_REGISTER_REPORT_RATE 0x08 > @@ -91,6 +92,7 @@ struct edt_ft5x06_ts_data { > struct touchscreen_properties prop; > u16 num_x; > u16 num_y; > + struct regulator *supply; > > struct gpio_desc *reset_gpio; > struct gpio_desc *wake_gpio; > @@ -993,6 +995,23 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client, > > tsdata->max_support_points = chip_data->max_support_points; > > + tsdata->supply = devm_regulator_get_optional(&client->dev, "power"); I'd prefer if we used devm_regulator_get() instead. On a fully-constrained systems a missing regulator will be substituted with a dummy one that you can then use in regulator_enable() and regulator_disable(). The _optional regulator API is reserved for cases where some of chip functionality is optional and you can engage it by powering up parts of the chip. The reset is not such case: it is always present, but may not be exposed to the OS to control. I think the preference is to call the supply by the name is a datasheet, something like "vcc" or "vdd", etc. Thanks.
On Fri, Dec 08, 2017 at 05:16:29PM -0800, Dmitry Torokhov wrote: > Hi Mylène, > > On Fri, Dec 08, 2017 at 10:54:18PM +0100, Mylène Josserand wrote: > > Add the support of regulator to use them as VCC source. > > > > Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com> > > --- > > drivers/input/touchscreen/edt-ft5x06.c | 33 +++++++++++++++++++++++++++++++++ > > 1 file changed, 33 insertions(+) > > > > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c > > index c53a3d7239e7..44b0e04a8f35 100644 > > --- a/drivers/input/touchscreen/edt-ft5x06.c > > +++ b/drivers/input/touchscreen/edt-ft5x06.c > > @@ -39,6 +39,7 @@ > > #include <linux/input/mt.h> > > #include <linux/input/touchscreen.h> > > #include <linux/of_device.h> > > +#include <linux/regulator/consumer.h> > > > > #define WORK_REGISTER_THRESHOLD 0x00 > > #define WORK_REGISTER_REPORT_RATE 0x08 > > @@ -91,6 +92,7 @@ struct edt_ft5x06_ts_data { > > struct touchscreen_properties prop; > > u16 num_x; > > u16 num_y; > > + struct regulator *supply; > > > > struct gpio_desc *reset_gpio; > > struct gpio_desc *wake_gpio; > > @@ -993,6 +995,23 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client, > > > > tsdata->max_support_points = chip_data->max_support_points; > > > > + tsdata->supply = devm_regulator_get_optional(&client->dev, "power"); > > I'd prefer if we used devm_regulator_get() instead. On a > fully-constrained systems a missing regulator will be substituted with a > dummy one that you can then use in regulator_enable() and > regulator_disable(). The _optional regulator API is reserved for cases > where some of chip functionality is optional and you can engage it by > powering up parts of the chip. The reset is not such case: it is always > present, but may not be exposed to the OS to control. > > I think the preference is to call the supply by the name is a datasheet, > something like "vcc" or "vdd", etc. Looking at this some more, you need to make sure your new regulator support plays well with reset/wake GPIOs. I.e. you probably want to properly bring the chip out of reset in edt_ft5x06_ts_resume() and maybe driver the reset line low in suspend to make sure you do not leak into the unpowered chip? Also, please update Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt with the additional binding data and cc device tree folks and Rob Herring. Thanks.
diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c index c53a3d7239e7..44b0e04a8f35 100644 --- a/drivers/input/touchscreen/edt-ft5x06.c +++ b/drivers/input/touchscreen/edt-ft5x06.c @@ -39,6 +39,7 @@ #include <linux/input/mt.h> #include <linux/input/touchscreen.h> #include <linux/of_device.h> +#include <linux/regulator/consumer.h> #define WORK_REGISTER_THRESHOLD 0x00 #define WORK_REGISTER_REPORT_RATE 0x08 @@ -91,6 +92,7 @@ struct edt_ft5x06_ts_data { struct touchscreen_properties prop; u16 num_x; u16 num_y; + struct regulator *supply; struct gpio_desc *reset_gpio; struct gpio_desc *wake_gpio; @@ -993,6 +995,23 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client, tsdata->max_support_points = chip_data->max_support_points; + tsdata->supply = devm_regulator_get_optional(&client->dev, "power"); + if (IS_ERR(tsdata->supply)) { + error = PTR_ERR(tsdata->supply); + dev_err(&client->dev, "failed to request regulator: %d\n", + error); + return error; + }; + + if (tsdata->supply) { + error = regulator_enable(tsdata->supply); + if (error < 0) { + dev_err(&client->dev, "failed to enable supply: %d\n", + error); + return error; + } + } + tsdata->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH); if (IS_ERR(tsdata->reset_gpio)) { @@ -1122,20 +1141,34 @@ static int edt_ft5x06_ts_remove(struct i2c_client *client) static int __maybe_unused edt_ft5x06_ts_suspend(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); + struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client); if (device_may_wakeup(dev)) enable_irq_wake(client->irq); + if (tsdata->supply) + regulator_disable(tsdata->supply); + return 0; } 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)) disable_irq_wake(client->irq); + if (tsdata->supply) { + ret = regulator_enable(tsdata->supply); + if (ret < 0) { + dev_err(dev, "failed to enable supply: %d\n", ret); + return ret; + } + } + return 0; }
Add the support of regulator to use them as VCC source. Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com> --- drivers/input/touchscreen/edt-ft5x06.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)