| Message ID | 1471376124-31169-1-git-send-email-hdegoede@redhat.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Tue, Aug 16, 2016 at 09:35:24PM +0200, Hans de Goede wrote: > On some tablets the touchscreen controller is powered by seperate > regulators, add support for this. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > .../bindings/input/touchscreen/silead_gsl1680.txt | 2 + Acked-by: Rob Herring <robh@kernel.org> > drivers/input/touchscreen/silead.c | 51 ++++++++++++++++++++-- > 2 files changed, 49 insertions(+), 4 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Tue, Aug 16, 2016 at 09:35:24PM +0200, Hans de Goede wrote: > On some tablets the touchscreen controller is powered by seperate > regulators, add support for this. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > .../bindings/input/touchscreen/silead_gsl1680.txt | 2 + > drivers/input/touchscreen/silead.c | 51 ++++++++++++++++++++-- > 2 files changed, 49 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt > index 37a9370..65eb4c7 100644 > --- a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt > +++ b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt > @@ -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: > > diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c > index 7379fe1..04992c5 100644 > --- a/drivers/input/touchscreen/silead.c > +++ b/drivers/input/touchscreen/silead.c > @@ -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; > @@ -463,21 +466,52 @@ static int silead_ts_probe(struct i2c_client *client, > if (client->irq <= 0) > return -ENODEV; > > + data->vddio = devm_regulator_get_optional(dev, "vddio"); No need for devm_regulator_get_optional(), devm_regulator_get() will give you dummy regulator that you can enable/disable. regulator_get_optional() is only need if you have truly optional functionality in controller and you need to adjust behavior depending on presence of a regulator. > + if (IS_ERR(data->vddio)) { > + if (PTR_ERR(data->vddio) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + data->vddio = NULL; > + } > + > + 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) { > + error = regulator_enable(data->vddio); > + if (error) > + return error; > + } > + > + if (data->avdd) { > + error = regulator_enable(data->avdd); > + if (error) > + goto disable_vddio; > + } Hmm, so you are enabling regulators and leave them on. That is not great. I'd rather we powered the device up during probe(), but then shut it off and waited for ->open() to be called. And power it down in ->close(). You also need to handle it in suspend and resume. Thanks.
HI, On 22-08-16 23:11, Dmitry Torokhov wrote: > Hi Hans, > > On Tue, Aug 16, 2016 at 09:35:24PM +0200, Hans de Goede wrote: >> On some tablets the touchscreen controller is powered by seperate >> regulators, add support for this. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> .../bindings/input/touchscreen/silead_gsl1680.txt | 2 + >> drivers/input/touchscreen/silead.c | 51 ++++++++++++++++++++-- >> 2 files changed, 49 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt >> index 37a9370..65eb4c7 100644 >> --- a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt >> +++ b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt >> @@ -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: >> >> diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c >> index 7379fe1..04992c5 100644 >> --- a/drivers/input/touchscreen/silead.c >> +++ b/drivers/input/touchscreen/silead.c >> @@ -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; >> @@ -463,21 +466,52 @@ static int silead_ts_probe(struct i2c_client *client, >> if (client->irq <= 0) >> return -ENODEV; >> >> + data->vddio = devm_regulator_get_optional(dev, "vddio"); > > No need for devm_regulator_get_optional(), devm_regulator_get() will > give you dummy regulator that you can enable/disable. > regulator_get_optional() is only need if you have truly optional > functionality in controller and you need to adjust behavior depending on > presence of a regulator. I prefer using get_optional and not getting the dmesg output about using a dummy regulator, that tends to leads to users asking questions and dealing with not having the regulator is easy, but if you want me to drop the _optional bit and turn any errors into hard errors I can do that for v2. >> + if (IS_ERR(data->vddio)) { >> + if (PTR_ERR(data->vddio) == -EPROBE_DEFER) >> + return -EPROBE_DEFER; >> + data->vddio = NULL; >> + } >> + >> + 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) { >> + error = regulator_enable(data->vddio); >> + if (error) >> + return error; >> + } >> + >> + if (data->avdd) { >> + error = regulator_enable(data->avdd); >> + if (error) >> + goto disable_vddio; >> + } > > Hmm, so you are enabling regulators and leave them on. That is not > great. I'd rather we powered the device up during probe(), but then shut > it off and waited for ->open() to be called. And power it down in > ->close(). You also need to handle it in suspend and resume. The problem is the chip needs firmware (actually config data and lookup tables with info about the digitizer) and loading that takes aprox. 1 second so keeping the regulator on is better IMHO. As for power usage the device also has an enable pin, which we already drive high on open / drive low on close (and suspend/resume), with this pin low the current usage of the device is very minimal. Actually of the 20 tablets I've with a gsl chip only 2 have taken the trouble to hook it up to a separate regulator. All the others just have it hooked to their default 3.3V rail. So re-loading the firmware on open / resume would penalize 18 of these with a 1 second delay, for a very minimal current saving on 2 of these. IMHO, esp. adding 1s delay to resume is not acceptable. So all in all I would like you to take this as is :) But if you still want a v2 let me know. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt index 37a9370..65eb4c7 100644 --- a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt +++ b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt @@ -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: diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c index 7379fe1..04992c5 100644 --- a/drivers/input/touchscreen/silead.c +++ b/drivers/input/touchscreen/silead.c @@ -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; @@ -463,21 +466,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; + } + + 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) { + error = regulator_enable(data->vddio); + if (error) + return error; + } + + if (data->avdd) { + error = regulator_enable(data->avdd); + if (error) + goto disable_vddio; + } + /* Power GPIO pin */ data->gpio_power = 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, @@ -485,10 +519,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)
On some tablets the touchscreen controller is powered by seperate regulators, add support for this. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- .../bindings/input/touchscreen/silead_gsl1680.txt | 2 + drivers/input/touchscreen/silead.c | 51 ++++++++++++++++++++-- 2 files changed, 49 insertions(+), 4 deletions(-)