Hello everyone,
This is a V2 of the patch series that adds touchscreen support
(FocalTech EDT-FT5x06 Polytouch) for TBS A711 (Allwinner sun8i-a83t SoC).
Based on last linux-next (next-20171222).
Changes since v1:
- Remove patches 01 and 02 as Chen-Yu Tsai sent a similar patch:
https://patchwork.kernel.org/patch/10111431/
and it is merged on last next-20171222.
(See commit f066f46ce5a5 "ARM: dts: sun8i: a83t: Add I2C device nodes and pinmux settings")
- Update regulator according to Dmitry Torokhov's review: remove "optional"
suffix while retrieving the regulator, rename it into "vcc" instead of
"power" and add bindings documentation.
- Update device tree according to Maxime Ripard's review: remove the
label and rename the node.
- Squash patch 03 with patch 05 to add I2C0 and touchscreen's node
in one patch (see patch 02).
Patch 01: Add support for regulator in the FocalTech touchscreen driver
because A711 tablet is using a regulator to power-up the touchscreen.
Patch 02: Add i2c0 and touchscreen's node for A711 TBS tablet.
Thank you in advance for any review.
Best regards,
Mylène
Mylène Josserand (2):
Input: edt-ft5x06 - Add support for regulator
arm: dts: sun8i: a83t: a711: Add touchscreen node
.../bindings/input/touchscreen/edt-ft5x06.txt | 1 +
arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts | 16 +++++++++++
drivers/input/touchscreen/edt-ft5x06.c | 33 ++++++++++++++++++++++
3 files changed, 50 insertions(+)
--
2.11.0
Add the support of regulator to use it as VCC source.
Signed-off-by: Mylène Josserand <[email protected]>
---
.../bindings/input/touchscreen/edt-ft5x06.txt | 1 +
drivers/input/touchscreen/edt-ft5x06.c | 33 ++++++++++++++++++++++
2 files changed, 34 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
index 025cf8c9324a..48e975b9c1aa 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
@@ -30,6 +30,7 @@ Required properties:
Optional properties:
- reset-gpios: GPIO specification for the RESET input
- wake-gpios: GPIO specification for the WAKE input
+ - vcc-supply: Regulator that supplies the touchscreen
- pinctrl-names: should be "default"
- pinctrl-0: a phandle pointing to the pin settings for the
diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index c53a3d7239e7..5ee14a25a382 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 *vcc;
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->vcc = devm_regulator_get(&client->dev, "vcc");
+ if (IS_ERR(tsdata->vcc)) {
+ error = PTR_ERR(tsdata->vcc);
+ dev_err(&client->dev, "failed to request regulator: %d\n",
+ error);
+ return error;
+ };
+
+ if (tsdata->vcc) {
+ error = regulator_enable(tsdata->vcc);
+ if (error < 0) {
+ dev_err(&client->dev, "failed to enable vcc: %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->vcc)
+ regulator_disable(tsdata->vcc);
+
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->vcc) {
+ ret = regulator_enable(tsdata->vcc);
+ if (ret < 0) {
+ dev_err(dev, "failed to enable vcc: %d\n", ret);
+ return ret;
+ }
+ }
+
return 0;
}
--
2.11.0
Tha A711 tablet has a FocalTech EDT-FT5x06 Polytouch touchscreen.
It is connected via I2C0. The reset line is PD5, the interrupt
line is PL7 and the VCC supply is the ldo_io0 regulator.
Signed-off-by: Mylène Josserand <[email protected]>
---
arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts b/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
index a021ee6da396..7840f9aa9094 100644
--- a/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
+++ b/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
@@ -105,6 +105,22 @@
status = "okay";
};
+&i2c0 {
+ clock-frequency = <400000>;
+ status = "okay";
+
+ touchscreen@38 {
+ compatible = "edt,edt-ft5x06";
+ reg = <0x38>;
+ interrupt-parent = <&r_pio>;
+ interrupts = <0 7 IRQ_TYPE_EDGE_FALLING>;
+ reset-gpios = <&pio 3 5 GPIO_ACTIVE_LOW>;
+ vcc-supply = <®_ldo_io0>;
+ touchscreen-size-x = <1024>;
+ touchscreen-size-y = <600>;
+ };
+};
+
&mmc0 {
vmmc-supply = <®_dcdc1>;
pinctrl-names = "default";
--
2.11.0
Hello,
Le Thu, 28 Dec 2017 17:33:34 +0100,
Mylène Josserand <[email protected]> a écrit :
> Hello everyone,
>
> This is a V2 of the patch series that adds touchscreen support
> (FocalTech EDT-FT5x06 Polytouch) for TBS A711 (Allwinner sun8i-a83t SoC).
> Based on last linux-next (next-20171222).
>
> Changes since v1:
> - Remove patches 01 and 02 as Chen-Yu Tsai sent a similar patch:
> https://patchwork.kernel.org/patch/10111431/
> and it is merged on last next-20171222.
> (See commit f066f46ce5a5 "ARM: dts: sun8i: a83t: Add I2C device nodes and pinmux settings")
> - Update regulator according to Dmitry Torokhov's review: remove "optional"
> suffix while retrieving the regulator, rename it into "vcc" instead of
> "power" and add bindings documentation.
I notice that I forgot the second review of Dmitry about reset/wake
gpios so I will send a V3 with the modifications.
Thanks,
Mylène
> - Update device tree according to Maxime Ripard's review: remove the
> label and rename the node.
> - Squash patch 03 with patch 05 to add I2C0 and touchscreen's node
> in one patch (see patch 02).
>
> Patch 01: Add support for regulator in the FocalTech touchscreen driver
> because A711 tablet is using a regulator to power-up the touchscreen.
> Patch 02: Add i2c0 and touchscreen's node for A711 TBS tablet.
>
> Thank you in advance for any review.
> Best regards,
> Mylène
>
> Mylène Josserand (2):
> Input: edt-ft5x06 - Add support for regulator
> arm: dts: sun8i: a83t: a711: Add touchscreen node
>
> .../bindings/input/touchscreen/edt-ft5x06.txt | 1 +
> arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts | 16 +++++++++++
> drivers/input/touchscreen/edt-ft5x06.c | 33 ++++++++++++++++++++++
> 3 files changed, 50 insertions(+)
>
From: Fengguang Wu <[email protected]>
drivers/input/touchscreen/edt-ft5x06.c:1004:2-3: Unneeded semicolon
Remove unneeded semicolon.
Generated by: scripts/coccinelle/misc/semicolon.cocci
Fixes: 5969d946e8aa ("Input: edt-ft5x06 - Add support for regulator")
CC: Myl?ne Josserand <[email protected]>
Signed-off-by: Fengguang Wu <[email protected]>
---
edt-ft5x06.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1001,7 +1001,7 @@ static int edt_ft5x06_ts_probe(struct i2
dev_err(&client->dev, "failed to request regulator: %d\n",
error);
return error;
- };
+ }
if (tsdata->vcc) {
error = regulator_enable(tsdata->vcc);
Hi Myl?ne,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on v4.15-rc5 next-20171222]
[cannot apply to input/next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Myl-ne-Josserand/sun8i-a83t-Add-touchscreen-support-on-TBS-A711/20171230-091331
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
coccinelle warnings: (new ones prefixed by >>)
>> drivers/input/touchscreen/edt-ft5x06.c:1004:2-3: Unneeded semicolon
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Thu, Dec 28, 2017 at 05:33:35PM +0100, Myl?ne Josserand wrote:
> Add the support of regulator to use it as VCC source.
>
> Signed-off-by: Myl?ne Josserand <[email protected]>
> ---
> .../bindings/input/touchscreen/edt-ft5x06.txt | 1 +
> drivers/input/touchscreen/edt-ft5x06.c | 33 ++++++++++++++++++++++
> 2 files changed, 34 insertions(+)
Reviewed-by: Rob Herring <[email protected]>
Dmitry,
On Thu, Dec 28, 2017 at 05:33:35PM +0100, Myl?ne Josserand wrote:
> Add the support of regulator to use it as VCC source.
>
> Signed-off-by: Myl?ne Josserand <[email protected]>
Ping?
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Hi Mylène,
On Thu, Dec 28, 2017 at 8:33 AM, Mylène Josserand
<[email protected]> wrote:
> Add the support of regulator to use it as VCC source.
>
> Signed-off-by: Mylène Josserand <[email protected]>
> ---
> .../bindings/input/touchscreen/edt-ft5x06.txt | 1 +
> drivers/input/touchscreen/edt-ft5x06.c | 33 ++++++++++++++++++++++
> 2 files changed, 34 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> index 025cf8c9324a..48e975b9c1aa 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> @@ -30,6 +30,7 @@ Required properties:
> Optional properties:
> - reset-gpios: GPIO specification for the RESET input
> - wake-gpios: GPIO specification for the WAKE input
> + - vcc-supply: Regulator that supplies the touchscreen
>
> - pinctrl-names: should be "default"
> - pinctrl-0: a phandle pointing to the pin settings for the
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index c53a3d7239e7..5ee14a25a382 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 *vcc;
>
> 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->vcc = devm_regulator_get(&client->dev, "vcc");
> + if (IS_ERR(tsdata->vcc)) {
> + error = PTR_ERR(tsdata->vcc);
> + dev_err(&client->dev, "failed to request regulator: %d\n",
> + error);
> + return error;
> + };
As 0-day pounted out, this semicolon is not needed.
> +
> + if (tsdata->vcc) {
You do not need to check for non-NULL here, devm_regulator_get() wil
lnever give you a NULL. If regulator is not defined in DT/board
mappings, then dummy regulator will be provided. You can call
regulator_enable() and regulator_disable() and other regulator APIs
with dummy regulator.
> + error = regulator_enable(tsdata->vcc);
> + if (error < 0) {
> + dev_err(&client->dev, "failed to enable vcc: %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->vcc)
Same here.
> + regulator_disable(tsdata->vcc);
> +
> 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->vcc) {
And here.
> + ret = regulator_enable(tsdata->vcc);
> + if (ret < 0) {
> + dev_err(dev, "failed to enable vcc: %d\n", ret);
> + return ret;
> + }
Since power to the device may have been cut, I think you need to
restore the register settings to whatever it was (factory vs work
mode, threshold, gain and offset registers, etc, etc).
> + }
> +
> return 0;
> }
>
> --
> 2.11.0
>
Thanks.
--
Dmitry
Hi,
On Mon, 22 Jan 2018 09:42:08 -0800 Dmitry Torokhov wrote:
> Hi Mylène,
>
> On Thu, Dec 28, 2017 at 8:33 AM, Mylène Josserand
> <[email protected]> wrote:
> > Add the support of regulator to use it as VCC source.
> >
> > Signed-off-by: Mylène Josserand <[email protected]>
> > ---
> > .../bindings/input/touchscreen/edt-ft5x06.txt | 1 +
> > drivers/input/touchscreen/edt-ft5x06.c | 33 ++++++++++++++++++++++
> > 2 files changed, 34 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > index 025cf8c9324a..48e975b9c1aa 100644
> > --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > @@ -30,6 +30,7 @@ Required properties:
> > Optional properties:
> > - reset-gpios: GPIO specification for the RESET input
> > - wake-gpios: GPIO specification for the WAKE input
> > + - vcc-supply: Regulator that supplies the touchscreen
> >
> > - pinctrl-names: should be "default"
> > - pinctrl-0: a phandle pointing to the pin settings for the
> > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > index c53a3d7239e7..5ee14a25a382 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 *vcc;
> >
> > 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->vcc = devm_regulator_get(&client->dev, "vcc");
> > + if (IS_ERR(tsdata->vcc)) {
> > + error = PTR_ERR(tsdata->vcc);
> > + dev_err(&client->dev, "failed to request regulator: %d\n",
> > + error);
>
I would check for -EPROBE_DEFER here and omit the error message in this
case.
Lothar Waßmann
Hello Dimitry,
Thank you for the review!
Le Mon, 22 Jan 2018 09:42:08 -0800,
Dmitry Torokhov <[email protected]> a écrit :
> Hi Mylène,
>
> On Thu, Dec 28, 2017 at 8:33 AM, Mylène Josserand
> <[email protected]> wrote:
> > Add the support of regulator to use it as VCC source.
> >
> > Signed-off-by: Mylène Josserand <[email protected]>
> > ---
> > .../bindings/input/touchscreen/edt-ft5x06.txt | 1 +
> > drivers/input/touchscreen/edt-ft5x06.c | 33 ++++++++++++++++++++++
> > 2 files changed, 34 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > index 025cf8c9324a..48e975b9c1aa 100644
> > --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > @@ -30,6 +30,7 @@ Required properties:
> > Optional properties:
> > - reset-gpios: GPIO specification for the RESET input
> > - wake-gpios: GPIO specification for the WAKE input
> > + - vcc-supply: Regulator that supplies the touchscreen
> >
> > - pinctrl-names: should be "default"
> > - pinctrl-0: a phandle pointing to the pin settings for the
> > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > index c53a3d7239e7..5ee14a25a382 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 *vcc;
> >
> > 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->vcc = devm_regulator_get(&client->dev, "vcc");
> > + if (IS_ERR(tsdata->vcc)) {
> > + error = PTR_ERR(tsdata->vcc);
> > + dev_err(&client->dev, "failed to request regulator: %d\n",
> > + error);
> > + return error;
> > + };
>
> As 0-day pounted out, this semicolon is not needed.
Yes, thanks, I will fix that in next version.
>
> > +
> > + if (tsdata->vcc) {
>
> You do not need to check for non-NULL here, devm_regulator_get() wil
> lnever give you a NULL. If regulator is not defined in DT/board
> mappings, then dummy regulator will be provided. You can call
> regulator_enable() and regulator_disable() and other regulator APIs
> with dummy regulator.
Okay, thanks for the explanation, I will remove that.
>
> > + error = regulator_enable(tsdata->vcc);
> > + if (error < 0) {
> > + dev_err(&client->dev, "failed to enable vcc: %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->vcc)
>
> Same here.
yep
>
> > + regulator_disable(tsdata->vcc);
> > +
> > 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->vcc) {
>
> And here.
yep
>
> > + ret = regulator_enable(tsdata->vcc);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to enable vcc: %d\n", ret);
> > + return ret;
> > + }
>
> Since power to the device may have been cut, I think you need to
> restore the register settings to whatever it was (factory vs work
> mode, threshold, gain and offset registers, etc, etc).
Okay. Could you tell me how can I do that?
>
> > + }
> > +
> > return 0;
> > }
> >
> > --
> > 2.11.0
> >
>
> Thanks.
>
About your V2's review, you suggested to add support for wake/reset in
suspend/resume (that I forgot in this version). I wanted to add it but
with my board, I can't test suspend/resume. What should I do about
that?
If I send a V3 in next few days, do you think you will have time to
merge it for v4.16?
Thank you in advance,
Best regards,
Mylène
--
Mylène Josserand, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Hello Lothar,
Le Tue, 23 Jan 2018 09:04:14 +0100,
Lothar Waßmann <[email protected]> a écrit :
> Hi,
>
> On Mon, 22 Jan 2018 09:42:08 -0800 Dmitry Torokhov wrote:
> > Hi Mylène,
> >
> > On Thu, Dec 28, 2017 at 8:33 AM, Mylène Josserand
> > <[email protected]> wrote:
> > > Add the support of regulator to use it as VCC source.
> > >
> > > Signed-off-by: Mylène Josserand <[email protected]>
> > > ---
> > > .../bindings/input/touchscreen/edt-ft5x06.txt | 1 +
> > > drivers/input/touchscreen/edt-ft5x06.c | 33 ++++++++++++++++++++++
> > > 2 files changed, 34 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > index 025cf8c9324a..48e975b9c1aa 100644
> > > --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > @@ -30,6 +30,7 @@ Required properties:
> > > Optional properties:
> > > - reset-gpios: GPIO specification for the RESET input
> > > - wake-gpios: GPIO specification for the WAKE input
> > > + - vcc-supply: Regulator that supplies the touchscreen
> > >
> > > - pinctrl-names: should be "default"
> > > - pinctrl-0: a phandle pointing to the pin settings for the
> > > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > > index c53a3d7239e7..5ee14a25a382 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 *vcc;
> > >
> > > 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->vcc = devm_regulator_get(&client->dev, "vcc");
> > > + if (IS_ERR(tsdata->vcc)) {
> > > + error = PTR_ERR(tsdata->vcc);
> > > + dev_err(&client->dev, "failed to request regulator: %d\n",
> > > + error);
> >
> I would check for -EPROBE_DEFER here and omit the error message in this
> case.
>
>
> Lothar Waßmann
Sure, I will add this case, thank you for the review.
Best regards,
--
Mylène Josserand, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Tue, Jan 23, 2018 at 10:10:55AM +0100, Mylene Josserand wrote:
> Hello Dimitry,
>
> Thank you for the review!
>
> Le Mon, 22 Jan 2018 09:42:08 -0800,
> Dmitry Torokhov <[email protected]> a ?crit :
>
> > Hi Myl?ne,
> >
> > On Thu, Dec 28, 2017 at 8:33 AM, Myl?ne Josserand
> > <[email protected]> wrote:
> > > Add the support of regulator to use it as VCC source.
> > >
> > > Signed-off-by: Myl?ne Josserand <[email protected]>
> > > ---
> > > .../bindings/input/touchscreen/edt-ft5x06.txt | 1 +
> > > drivers/input/touchscreen/edt-ft5x06.c | 33 ++++++++++++++++++++++
> > > 2 files changed, 34 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > index 025cf8c9324a..48e975b9c1aa 100644
> > > --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > @@ -30,6 +30,7 @@ Required properties:
> > > Optional properties:
> > > - reset-gpios: GPIO specification for the RESET input
> > > - wake-gpios: GPIO specification for the WAKE input
> > > + - vcc-supply: Regulator that supplies the touchscreen
> > >
> > > - pinctrl-names: should be "default"
> > > - pinctrl-0: a phandle pointing to the pin settings for the
> > > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > > index c53a3d7239e7..5ee14a25a382 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 *vcc;
> > >
> > > 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->vcc = devm_regulator_get(&client->dev, "vcc");
> > > + if (IS_ERR(tsdata->vcc)) {
> > > + error = PTR_ERR(tsdata->vcc);
> > > + dev_err(&client->dev, "failed to request regulator: %d\n",
> > > + error);
> > > + return error;
> > > + };
> >
> > As 0-day pounted out, this semicolon is not needed.
>
> Yes, thanks, I will fix that in next version.
>
> >
> > > +
> > > + if (tsdata->vcc) {
> >
> > You do not need to check for non-NULL here, devm_regulator_get() wil
> > lnever give you a NULL. If regulator is not defined in DT/board
> > mappings, then dummy regulator will be provided. You can call
> > regulator_enable() and regulator_disable() and other regulator APIs
> > with dummy regulator.
>
> Okay, thanks for the explanation, I will remove that.
>
> >
> > > + error = regulator_enable(tsdata->vcc);
> > > + if (error < 0) {
> > > + dev_err(&client->dev, "failed to enable vcc: %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->vcc)
> >
> > Same here.
>
> yep
>
> >
> > > + regulator_disable(tsdata->vcc);
> > > +
> > > 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->vcc) {
> >
> > And here.
>
> yep
>
> >
> > > + ret = regulator_enable(tsdata->vcc);
> > > + if (ret < 0) {
> > > + dev_err(dev, "failed to enable vcc: %d\n", ret);
> > > + return ret;
> > > + }
> >
> > Since power to the device may have been cut, I think you need to
> > restore the register settings to whatever it was (factory vs work
> > mode, threshold, gain and offset registers, etc, etc).
>
> Okay. Could you tell me how can I do that?
If you take a look at edt_ft5x06_work_mode() at the end ther eis code to
restore parameters. You want to factor it out and apply to the device
when resuming. You probably also want to see what mode (factory/normal)
the device was before suspend and restore the previous mode on resume.
>
> >
> > > + }
> > > +
> > > return 0;
> > > }
> > >
> > > --
> > > 2.11.0
> > >
> >
> > Thanks.
> >
>
> About your V2's review, you suggested to add support for wake/reset in
> suspend/resume (that I forgot in this version). I wanted to add it but
> with my board, I can't test suspend/resume. What should I do about
> that?
See if Simon Budig <[email protected]> can help us here.
>
> If I send a V3 in next few days, do you think you will have time to
> merge it for v4.16?
It all depends on the patch shape...
Thanks.
--
Dmitry
Hello Dmitry,
Le Tue, 23 Jan 2018 09:58:29 -0800,
Dmitry Torokhov <[email protected]> a écrit :
> On Tue, Jan 23, 2018 at 10:10:55AM +0100, Mylene Josserand wrote:
> > Hello Dimitry,
> >
> > Thank you for the review!
> >
> > Le Mon, 22 Jan 2018 09:42:08 -0800,
> > Dmitry Torokhov <[email protected]> a écrit :
> >
> > > Hi Mylène,
> > >
> > > On Thu, Dec 28, 2017 at 8:33 AM, Mylène Josserand
> > > <[email protected]> wrote:
> > > > Add the support of regulator to use it as VCC source.
> > > >
> > > > Signed-off-by: Mylène Josserand <[email protected]>
> > > > ---
> > > > .../bindings/input/touchscreen/edt-ft5x06.txt | 1 +
> > > > drivers/input/touchscreen/edt-ft5x06.c | 33 ++++++++++++++++++++++
> > > > 2 files changed, 34 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > index 025cf8c9324a..48e975b9c1aa 100644
> > > > --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > > > @@ -30,6 +30,7 @@ Required properties:
> > > > Optional properties:
> > > > - reset-gpios: GPIO specification for the RESET input
> > > > - wake-gpios: GPIO specification for the WAKE input
> > > > + - vcc-supply: Regulator that supplies the touchscreen
> > > >
> > > > - pinctrl-names: should be "default"
> > > > - pinctrl-0: a phandle pointing to the pin settings for the
> > > > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > > > index c53a3d7239e7..5ee14a25a382 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 *vcc;
> > > >
> > > > 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->vcc = devm_regulator_get(&client->dev, "vcc");
> > > > + if (IS_ERR(tsdata->vcc)) {
> > > > + error = PTR_ERR(tsdata->vcc);
> > > > + dev_err(&client->dev, "failed to request regulator: %d\n",
> > > > + error);
> > > > + return error;
> > > > + };
> > >
> > > As 0-day pounted out, this semicolon is not needed.
> >
> > Yes, thanks, I will fix that in next version.
> >
> > >
> > > > +
> > > > + if (tsdata->vcc) {
> > >
> > > You do not need to check for non-NULL here, devm_regulator_get() wil
> > > lnever give you a NULL. If regulator is not defined in DT/board
> > > mappings, then dummy regulator will be provided. You can call
> > > regulator_enable() and regulator_disable() and other regulator APIs
> > > with dummy regulator.
> >
> > Okay, thanks for the explanation, I will remove that.
> >
> > >
> > > > + error = regulator_enable(tsdata->vcc);
> > > > + if (error < 0) {
> > > > + dev_err(&client->dev, "failed to enable vcc: %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->vcc)
> > >
> > > Same here.
> >
> > yep
> >
> > >
> > > > + regulator_disable(tsdata->vcc);
> > > > +
> > > > 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->vcc) {
> > >
> > > And here.
> >
> > yep
> >
> > >
> > > > + ret = regulator_enable(tsdata->vcc);
> > > > + if (ret < 0) {
> > > > + dev_err(dev, "failed to enable vcc: %d\n", ret);
> > > > + return ret;
> > > > + }
> > >
> > > Since power to the device may have been cut, I think you need to
> > > restore the register settings to whatever it was (factory vs work
> > > mode, threshold, gain and offset registers, etc, etc).
> >
> > Okay. Could you tell me how can I do that?
>
> If you take a look at edt_ft5x06_work_mode() at the end ther eis code to
> restore parameters. You want to factor it out and apply to the device
> when resuming. You probably also want to see what mode (factory/normal)
> the device was before suspend and restore the previous mode on resume.
Okay, thank you for your help.
About the factory/normal mode to be sure that I understand correctly,
in the resume, I should use something like "edt_ft5x06_debugfs_mode_set"
to check the "tsdata->factory_mode" and call "edt_ft5x06_factory_mode"
or "edt_ft5x06_work_mode", is not it?
As the current suspend/resume code is only enabling/disabling an irq, I
would like to understand exactly the modifications you would like.
>
> >
> > >
> > > > + }
> > > > +
> > > > return 0;
> > > > }
> > > >
> > > > --
> > > > 2.11.0
> > > >
> > >
> > > Thanks.
> > >
> >
> > About your V2's review, you suggested to add support for wake/reset in
> > suspend/resume (that I forgot in this version). I wanted to add it but
> > with my board, I can't test suspend/resume. What should I do about
> > that?
>
> See if Simon Budig <[email protected]> can help us here.
Thank you, I contacted Simon (added him in CC) and unfortunately, he
doesn't have a setup with working suspend/resume so he can't help me
on that.
My patches are ready (except for factory/normal mode, see above) but as
I can't test suspend/resume, I prefer not to send them without testing
it..
>
> >
> > If I send a V3 in next few days, do you think you will have time to
> > merge it for v4.16?
>
> It all depends on the patch shape...
Sure I understand.
Best regards,
--
Mylène Josserand, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com