* [PATCH resend 0/1] input: touchscreen: silead: Add regulator support @ 2016-11-14 14:45 Hans de Goede 2016-11-14 14:45 ` [PATCH resend] " Hans de Goede 0 siblings, 1 reply; 4+ messages in thread From: Hans de Goede @ 2016-11-14 14:45 UTC (permalink / raw) To: linux-arm-kernel Hi Dmitri, This one seems to have fallen through the cracks, can you pick it up please ? Regards, Hans ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH resend] input: touchscreen: silead: Add regulator support 2016-11-14 14:45 [PATCH resend 0/1] input: touchscreen: silead: Add regulator support Hans de Goede @ 2016-11-14 14:45 ` Hans de Goede 2016-11-14 18:50 ` Dmitry Torokhov 0 siblings, 1 reply; 4+ messages in thread From: Hans de Goede @ 2016-11-14 14:45 UTC (permalink / raw) To: linux-arm-kernel On some tablets the touchscreen controller is powered by seperate regulators, add support for this. Signed-off-by: Hans de Goede <hdegoede@redhat.com> Acked-by: Rob Herring <robh@kernel.org> --- .../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 e844c3f..b726823 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 f502c84..c6a1ae9 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; @@ -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; + } + + 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 = 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) -- 2.9.3 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH resend] input: touchscreen: silead: Add regulator support 2016-11-14 14:45 ` [PATCH resend] " Hans de Goede @ 2016-11-14 18:50 ` Dmitry Torokhov 2016-11-15 10:56 ` Hans de Goede 0 siblings, 1 reply; 4+ messages in thread From: Dmitry Torokhov @ 2016-11-14 18:50 UTC (permalink / raw) To: linux-arm-kernel Hi Hans, On Mon, Nov 14, 2016 at 03:45:02PM +0100, 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> > Acked-by: Rob Herring <robh@kernel.org> > --- > .../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 e844c3f..b726823 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 f502c84..c6a1ae9 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; > @@ -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(). > + } > + > + 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; > + } 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. > + > /* 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) > -- > 2.9.3 > Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH resend] input: touchscreen: silead: Add regulator support 2016-11-14 18:50 ` Dmitry Torokhov @ 2016-11-15 10:56 ` Hans de Goede 0 siblings, 0 replies; 4+ messages in thread From: Hans de Goede @ 2016-11-15 10:56 UTC (permalink / raw) To: linux-arm-kernel Hi, On 14-11-16 19:50, Dmitry Torokhov wrote: > Hi Hans, > > On Mon, Nov 14, 2016 at 03:45:02PM +0100, 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> >> Acked-by: Rob Herring <robh@kernel.org> >> --- >> .../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 e844c3f..b726823 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 f502c84..c6a1ae9 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; >> @@ -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? Other errors indicate that the regulator is not there specifically I think regulator_get_optional will return -ENOENT in that case. > 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 > 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 ... > 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 > >> + } >> + >> + 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; >> + } > > 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. > >> + >> /* 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) >> -- >> 2.9.3 >> > > Thanks. > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-11-15 10:56 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-14 14:45 [PATCH resend 0/1] input: touchscreen: silead: Add regulator support Hans de Goede 2016-11-14 14:45 ` [PATCH resend] " Hans de Goede 2016-11-14 18:50 ` Dmitry Torokhov 2016-11-15 10:56 ` Hans de Goede
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).