[PATCH 1/2] dt-bindings: Input: exc3000 - Support power supply regulators

[PATCH 1/2] dt-bindings: Input: exc3000 - Support power supply regulators

[Mike Looijmans]

Add power supply regulator support to the exc3000 devices.
This provides the devicetree bindings.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

---

 .../devicetree/bindings/input/touchscreen/eeti,exc3000.yaml     | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/eeti,exc3000.yaml b/Documentation/devicetree/bindings/input/touchscreen/eeti,exc3000.yaml
index 007adbc89c14..9dc25d30a0a8 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/eeti,exc3000.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/eeti,exc3000.yaml
@@ -24,6 +24,8 @@ properties:
     maxItems: 1
   reset-gpios:
     maxItems: 1
+  vdd-supply:
+    description: Power supply regulator for the chip
   touchscreen-size-x: true
   touchscreen-size-y: true
   touchscreen-inverted-x: true
-- 
2.17.1


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topic.nl

Please consider the environment before printing this e-mail

[PATCH 2/2] Input: exc3000 - Support power supply regulators

[Mike Looijmans]

Add power supply regulator support to the exc3000 devices.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

---

 drivers/input/touchscreen/exc3000.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
index 4af4c1e5d0da..3e50af8a4a2d 100644
--- a/drivers/input/touchscreen/exc3000.c
+++ b/drivers/input/touchscreen/exc3000.c
@@ -18,6 +18,7 @@
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/regulator/consumer.h>
 #include <linux/sizes.h>
 #include <linux/timer.h>
 #include <asm/unaligned.h>
@@ -360,6 +361,12 @@ static int exc3000_probe(struct i2c_client *client)
 	if (IS_ERR(data->reset))
 		return PTR_ERR(data->reset);
 
+	/* For proper reset sequence, enable power while reset asserted */
+	error = devm_regulator_get_enable_optional(&client->dev, "vdd");
+	if (error && error != -ENODEV)
+		dev_err_probe(&client->dev, error,
+			      "failed to request vdd regulator\n");
+
 	if (data->reset) {
 		msleep(EXC3000_RESET_MS);
 		gpiod_set_value_cansleep(data->reset, 0);
-- 
2.17.1


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topic.nl

Please consider the environment before printing this e-mail

Re: [PATCH 1/2] dt-bindings: Input: exc3000 - Support power supply regulators

[Krzysztof Kozlowski]

On 03/07/2023 10:45, Mike Looijmans wrote:
> Add power supply regulator support to the exc3000 devices.
> This provides the devicetree bindings.

If there is going to be next version/resend, you could skip the last
sentence. It is a bit redundant.

In any case:

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

Re: [PATCH 2/2] Input: exc3000 - Support power supply regulators

[Dmitry Torokhov]

Hi Mike,

On Mon, Jul 03, 2023 at 10:45:36AM +0200, Mike Looijmans wrote:
> Add power supply regulator support to the exc3000 devices.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> 
> ---
> 
>  drivers/input/touchscreen/exc3000.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
> index 4af4c1e5d0da..3e50af8a4a2d 100644
> --- a/drivers/input/touchscreen/exc3000.c
> +++ b/drivers/input/touchscreen/exc3000.c
> @@ -18,6 +18,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/sizes.h>
>  #include <linux/timer.h>
>  #include <asm/unaligned.h>
> @@ -360,6 +361,12 @@ static int exc3000_probe(struct i2c_client *client)
>  	if (IS_ERR(data->reset))
>  		return PTR_ERR(data->reset);
>  
> +	/* For proper reset sequence, enable power while reset asserted */
> +	error = devm_regulator_get_enable_optional(&client->dev, "vdd");
> +	if (error && error != -ENODEV)
> +		dev_err_probe(&client->dev, error,
> +			      "failed to request vdd regulator\n");


If there is a regulator described in the firmware we should not continue
with initializing the device if we fail to grab/enable it. Think about
what happens if you get -EPROBE_DEFER here. You should return here.

Also, why are you using the _optional() variant? VDD is not an optional
for the controller. regulator_get_optional() is needed when you need to
alter the behavior of the device/driver depending on the presence of an
optional supply, whereas here it should work fine with a sub supply that
will be created if you simply call devm_regulator_get_enable() and there
is not regulator mentioned in DT/ACPI for the board.

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] Input: exc3000 - Support power supply regulators

[Mike Looijmans]

On 05-07-2023 23:49, Dmitry Torokhov wrote:
> Hi Mike,
>
> On Mon, Jul 03, 2023 at 10:45:36AM +0200, Mike Looijmans wrote:
>> Add power supply regulator support to the exc3000 devices.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>>
>> ---
>>
>>   drivers/input/touchscreen/exc3000.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
>> index 4af4c1e5d0da..3e50af8a4a2d 100644
>> --- a/drivers/input/touchscreen/exc3000.c
>> +++ b/drivers/input/touchscreen/exc3000.c
>> @@ -18,6 +18,7 @@
>>   #include <linux/interrupt.h>
>>   #include <linux/module.h>
>>   #include <linux/of.h>
>> +#include <linux/regulator/consumer.h>
>>   #include <linux/sizes.h>
>>   #include <linux/timer.h>
>>   #include <asm/unaligned.h>
>> @@ -360,6 +361,12 @@ static int exc3000_probe(struct i2c_client *client)
>>   	if (IS_ERR(data->reset))
>>   		return PTR_ERR(data->reset);
>>   
>> +	/* For proper reset sequence, enable power while reset asserted */
>> +	error = devm_regulator_get_enable_optional(&client->dev, "vdd");
>> +	if (error && error != -ENODEV)
>> +		dev_err_probe(&client->dev, error,
>> +			      "failed to request vdd regulator\n");
>
> If there is a regulator described in the firmware we should not continue
> with initializing the device if we fail to grab/enable it. Think about
> what happens if you get -EPROBE_DEFER here. You should return here.

True, good catch, I forgot the "return" here (which in all honesty was 
the reason for using dev_err_probe in the first place...).

> Also, why are you using the _optional() variant? VDD is not an optional
> for the controller. regulator_get_optional() is needed when you need to
> alter the behavior of the device/driver depending on the presence of an
> optional supply, whereas here it should work fine with a sub supply that
> will be created if you simply call devm_regulator_get_enable() and there
> is not regulator mentioned in DT/ACPI for the board.

Indeed, "optional" isn't correct here (I probably had the clock variant 
in mind), a dummy regulator is fine.

BTW, I also have a patch in store that tries to "recover" the device 
(using its reset when possible) if it isn't responding or if it causes 
I2C errors. Took several weeks uptime for this to occur spontaneously on 
our hardware, so I'll wait for this series so I can put that patch on 
top of this one.

-- 
Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl