input: touchscreen: silead: Add regulator support
Subsystems (4)
cover input: touchscreen: silead: Add regulator support
Hi Dmitri,
This one seems to have fallen through the cracks, can you pick it up please ?
Regards,
Hans
PATCH RESEND 1/1 input: touchscreen: silead: Add regulator support
On some tablets the touchscreen controller is powered by seperate regulators, add support for this.
--- .../bindings/input/touchscreen/silead_gsl1680.txt | 2 + drivers/input/touchscreen/silead.c | 51 ++++++++++++++++++++-- 2 files changed, 49 insertions(+), 4 deletions(-)
Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt +2 -0
@@ -22,6 +22,8 @@ Optional properties: - touchscreen-inverted-y : See touchscreen.txt - touchscreen-swapped-x-y : See touchscreen.txt - silead,max-fingers : maximum number of fingers the touchscreen can detect +- vddio-supply : regulator phandle for controller VDDIO +- avdd-supply : regulator phandle for controller AVDD Example:
drivers/input/touchscreen/silead.c +47 -4
90 lines not on this page · raw message
Signature · 1 line
2.9.3
Dmitry Torokhov
maintainerPATCH RESEND 1/1 input: touchscreen: silead: Add regulator support
Hi Hans,
Quoted 45 lines · Hans de Goede
On some tablets the touchscreen controller is powered by seperate regulators, add support for this.
Signed-off-by Hans de Goede <[email protected]>Acked-by Rob Herring <[email protected]>--- .../bindings/input/touchscreen/silead_gsl1680.txt | 2 + drivers/input/touchscreen/silead.c | 51 ++++++++++++++++++++-- 2 files changed, 49 insertions(+), 4 deletions(-)Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt +2
-0@@ -22,6 +22,8 @@ Optional properties: - touchscreen-inverted-y : See touchscreen.txt - touchscreen-swapped-x-y : See touchscreen.txt - silead,max-fingers : maximum number of fingers the touchscreen can detect +- vddio-supply : regulator phandle for controller VDDIO +- avdd-supply : regulator phandle for controller AVDD Example:drivers/input/touchscreen/silead.c +3
-0@@ -29,6 +29,7 @@#include <linux/input/touchscreen.h> #include <linux/pm.h> #include <linux/irq.h> +#include <linux/regulator/consumer.h> #include <asm/unaligned.h> @@ -72,6 +73,8 @@ enum silead_ts_power {struct silead_ts_data {struct i2c_client *client;struct gpio_desc *gpio_power; +struct regulator *vddio; +struct regulator *avdd;struct input_dev *input;char fw_name[64 ];struct touchscreen_properties prop;
drivers/input/touchscreen/silead.c static int silead_ts_probe(struct i2c_client *client, · line 475
Quoted 8 lines
drivers/input/touchscreen/silead.c +5
-0@@ -465,21 +468,52 @@ static int silead_ts_probe(struct i2c_client *client,if (client->irq <=0 )return -ENODEV ; + data->vddio =devm_regulator_get_optional (dev,"vddio" ); +if (IS_ERR (data->vddio)) { +if (PTR_ERR (data->vddio) == -EPROBE_DEFER ) +return -EPROBE_DEFER ; + data->vddio =NULL ;
Why do we ignore other errors? If there is an issue reported by regulator framework we should net be ignoring it.
Unless regulator is truly optional (i.e. chip can work with some functionality powered off) and not simply hidden (firmware takes care of powering up system), we should not be using regulator_get_optional():
if regulator is absent from ACPI/DT/etc, regulator framework will supply dummy regulator that you can enable/disable and not bothering checking whether it is NULL or not.
Also, please consider using devm_regulator_bulk_get().
Quoted 14 lines
drivers/input/touchscreen/silead.c +14
-0+ } + + data->avdd =devm_regulator_get_optional (dev,"avdd" ); +if (IS_ERR (data->avdd)) { +if (PTR_ERR (data->avdd) == -EPROBE_DEFER ) +return -EPROBE_DEFER ; + data->avdd =NULL ; + } + +/* +* Enable regulators at probe and disable them at remove, we need +* to keep the chip powered otherwise it forgets its firmware. +*/ +if (data->vddio) {
drivers/input/touchscreen/silead.c static int silead_ts_probe(struct i2c_client *client, · line 499
Quoted 10 lines
drivers/input/touchscreen/silead.c +10
-0+ error =regulator_enable (data->vddio); +if (error) +return error; + } + +if (data->avdd) { + error =regulator_enable (data->avdd); +if (error) + goto disable_vddio; + }
Use devm_add_action_or_reset() to work regulator_bulk_disable call into devm stream. As it is you are leaving regulators on on unbind/remove.
Quoted 47 lines
drivers/input/touchscreen/silead.c +15
-4+/* Power GPIO pin */ data->gpio_power =devm_gpiod_get_optional (dev,"power" ,GPIOD_OUT_LOW );if (IS_ERR (data->gpio_power)) {if (PTR_ERR (data->gpio_power) != -EPROBE_DEFER )dev_err (dev,"Shutdown GPIO request failed\n" ); -return PTR_ERR (data->gpio_power); + error =PTR_ERR (data->gpio_power); + goto disable_avdd; } error =silead_ts_setup (client);if (error) -return error; + goto disable_avdd; error =silead_ts_request_input_dev (data);if (error) -return error; + goto disable_avdd; error = devm_request_threaded_irq(dev, client->irq, NULL, silead_ts_threaded_irq_handler,@@ -487,10 +521,19 @@ static int silead_ts_probe(struct i2c_client *client,if (error) {if (error != -EPROBE_DEFER )dev_err (dev,"IRQ request failed %d\n" , error); -return error; + goto disable_avdd; }return 0 ; + +disable_avdd: +if (data->avdd) +regulator_disable (data->avdd); +disable_vddio: +if (data->vddio) +regulator_disable (data->vddio); + +return error; }static int __maybe_unused silead_ts_suspend (struct device *dev)Signature · 1 line
2.9.3
Thanks.
Signature · 1 line
Dmitry
PATCH RESEND 1/1 input: touchscreen: silead: Add regulator support
Hi,
Quoted 51 lines · Dmitry Torokhov
Hi Hans,
Quoted 48 lines · Hans de Goede
On some tablets the touchscreen controller is powered by seperate regulators, add support for this.
Signed-off-by Hans de Goede <[email protected]>Acked-by Rob Herring <[email protected]>--- .../bindings/input/touchscreen/silead_gsl1680.txt | 2 + drivers/input/touchscreen/silead.c | 51 ++++++++++++++++++++-- 2 files changed, 49 insertions(+), 4 deletions(-)Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt +2
-0@@ -22,6 +22,8 @@ Optional properties: - touchscreen-inverted-y : See touchscreen.txt - touchscreen-swapped-x-y : See touchscreen.txt - silead,max-fingers : maximum number of fingers the touchscreen can detect +- vddio-supply : regulator phandle for controller VDDIO +- avdd-supply : regulator phandle for controller AVDD Example:drivers/input/touchscreen/silead.c +3
-0@@ -29,6 +29,7 @@#include <linux/input/touchscreen.h> #include <linux/pm.h> #include <linux/irq.h> +#include <linux/regulator/consumer.h> #include <asm/unaligned.h> @@ -72,6 +73,8 @@ enum silead_ts_power {struct silead_ts_data {struct i2c_client *client;struct gpio_desc *gpio_power; +struct regulator *vddio; +struct regulator *avdd;struct input_dev *input;char fw_name[64 ];struct touchscreen_properties prop;@@ -465,21 +468,52 @@ static int silead_ts_probe(struct i2c_client *client,if (client->irq <=0 )return -ENODEV ;
Quoted 7 lines
drivers/input/touchscreen/silead.c static int silead_ts_probe(struct i2c_client *client, · line 475
Quoted 5 lines
drivers/input/touchscreen/silead.c +5
-0+ data->vddio =devm_regulator_get_optional (dev,"vddio" ); +if (IS_ERR (data->vddio)) { +if (PTR_ERR (data->vddio) == -EPROBE_DEFER ) +return -EPROBE_DEFER ; + data->vddio =NULL ;Why do we ignore other errors?
Other errors indicate that the regulator is not there specifically I think regulator_get_optional will return -ENOENT in that case.
Quoted 6 lines
If there is an issue reported by
regulator framework we should net be ignoring it.Unless regulator is truly optional (i.e. chip can work with some functionality powered off) and not simply hidden (firmware takes care of powering up system),
In most systems the vddio is simply hardwired to the always-on vcc-3.3V
Quoted 4 lines
we should not be using
regulator_get_optional():
if regulator is absent from ACPI/DT/etc, regulator framework will supply dummy regulator that you can enable/disable and not bothering checking whether it is NULL or not.
Right, the downside of that is that it prints a msg about this in dmesg which typically will lead to user questions.
Anyways if you prefer the non _optional variant I can do a v3 with that ...
Quoted 1 line
Also, please consider using
devm_regulator_bulk_get().
Hmm, so I would get:
In struct silead_ts_data:
struct regulator_bulk_data regulators[2];
And then in probe() do:
data->regulators[0].supply = "vddio"; data->regulators[1].supply = "avdd"; ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->regulators), data->regulators); if (ret) return ret;
And modify the enable / disable code to match.
Yes I can see how that is better / cleaner and it also answers the question whether or not to use get_optional.
Ok, I'll do a v2 switching to regulator_bulk_get.
Regards,
Hans
Quoted 79 lines
Quoted 15 lines
drivers/input/touchscreen/silead.c +14
-0+ } + + data->avdd =devm_regulator_get_optional (dev,"avdd" ); +if (IS_ERR (data->avdd)) { +if (PTR_ERR (data->avdd) == -EPROBE_DEFER ) +return -EPROBE_DEFER ; + data->avdd =NULL ; + } + +/* +* Enable regulators at probe and disable them at remove, we need +* to keep the chip powered otherwise it forgets its firmware. +*/ +if (data->vddio) {drivers/input/touchscreen/silead.c static int silead_ts_probe(struct i2c_client *client, · line 499
Quoted 10 lines
drivers/input/touchscreen/silead.c +10
-0+ error =regulator_enable (data->vddio); +if (error) +return error; + } + +if (data->avdd) { + error =regulator_enable (data->avdd); +if (error) + goto disable_vddio; + }Use
devm_add_action_or_reset()to workregulator_bulk_disablecall into devm stream. As it is you are leaving regulators on on unbind/remove.Quoted 45 lines
drivers/input/touchscreen/silead.c +15
-5+/* Power GPIO pin */ data->gpio_power =devm_gpiod_get_optional (dev,"power" ,GPIOD_OUT_LOW );if (IS_ERR (data->gpio_power)) {if (PTR_ERR (data->gpio_power) != -EPROBE_DEFER )dev_err (dev,"Shutdown GPIO request failed\n" ); -return PTR_ERR (data->gpio_power); + error =PTR_ERR (data->gpio_power); + goto disable_avdd; } error =silead_ts_setup (client);if (error) -return error; + goto disable_avdd; error =silead_ts_request_input_dev (data);if (error) -return error; + goto disable_avdd; error = devm_request_threaded_irq(dev, client->irq, NULL, silead_ts_threaded_irq_handler,@@ -487,10 +521,19 @@ static int silead_ts_probe(struct i2c_client *client,if (error) {if (error != -EPROBE_DEFER )dev_err (dev,"IRQ request failed %d\n" , error); -return error; + goto disable_avdd; }return 0 ; + +disable_avdd: +if (data->avdd) +regulator_disable (data->avdd); +disable_vddio: +if (data->vddio) +regulator_disable (data->vddio); + +return error; }static int __maybe_unused silead_ts_suspend (struct device *dev) --Quoted 2 lines
2.9.3
Thanks.