Re: [PATCH] Input: st1232 - Support power supply regulators

[Mike Looijmans <mike.looijmans@topic.nl>]

Comment inlined below (mailserver injects signature halfway through the 
mail usually).


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
On 28-05-2022 06:47, Dmitry Torokhov wrote:
> Hi Mike,
>
> On Tue, May 24, 2022 at 10:12:16AM +0200, Mike Looijmans wrote:
>> Add support for the VDD and IOVDD power supply inputs. This allows the
>> chip to share its supplies with other components (e.g. panel) and manage
>> them.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>> ---
>>   .../input/touchscreen/sitronix,st1232.yaml    |  6 +++
>>   drivers/input/touchscreen/st1232.c            | 54 ++++++++++++++++---
>>   2 files changed, 52 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/sitronix,st1232.yaml b/Documentation/devicetree/bindings/input/touchscreen/sitronix,st1232.yaml
>> index 1d8ca19fd37a..240be8d49232 100644
>> --- a/Documentation/devicetree/bindings/input/touchscreen/sitronix,st1232.yaml
>> +++ b/Documentation/devicetree/bindings/input/touchscreen/sitronix,st1232.yaml
>> @@ -28,6 +28,12 @@ properties:
>>       description: A phandle to the reset GPIO
>>       maxItems: 1
>>   
>> +  vdd-supply:
>> +    description: Power supply regulator for the chip
>> +
>> +  vddio-supply:
>> +    description: Power supply regulator for the I2C bus
>> +
>>   required:
>>     - compatible
>>     - reg
>> diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
>> index e38ba3e4f183..d9c9f6f1f11a 100644
>> --- a/drivers/input/touchscreen/st1232.c
>> +++ b/drivers/input/touchscreen/st1232.c
>> @@ -44,6 +44,11 @@
>>   #define REG_XY_COORDINATES	0x12
>>   #define ST_TS_MAX_FINGERS	10
>>   
>> +enum st1232_regulators {
>> +	ST1232_REGULATOR_VDD,
>> +	ST1232_REGULATOR_IOVDD,
>> +};
>> +
>>   struct st_chip_info {
>>   	bool	have_z;
>>   	u16	max_area;
>> @@ -56,6 +61,7 @@ struct st1232_ts_data {
>>   	struct touchscreen_properties prop;
>>   	struct dev_pm_qos_request low_latency_req;
>>   	struct gpio_desc *reset_gpio;
>> +	struct regulator_bulk_data regulators[2];
>>   	const struct st_chip_info *chip_info;
>>   	int read_buf_len;
>>   	u8 *read_buf;
>> @@ -197,17 +203,36 @@ static irqreturn_t st1232_ts_irq_handler(int irq, void *dev_id)
>>   	return IRQ_HANDLED;
>>   }
>>   
>> -static void st1232_ts_power(struct st1232_ts_data *ts, bool poweron)
>> +static int st1232_ts_power_on(struct st1232_ts_data *ts)
>> +{
>> +	int err;
>> +
>> +	err = regulator_bulk_enable(ARRAY_SIZE(ts->regulators), ts->regulators);
>> +	if (err)
>> +		return err;
> Does it really make sense to try and handle regulators when reset gpio
> is not defined? Would it not be better to tie them to the presence of
> reset gpio to make sure we implement proper power on sequence?

I thought that's what we're doing here. The datasheet says 5ms between 
power-good and reset de-assert. Whether or not the hardware guys 
bothered to actually connect the reset is out of our hands. The 
regulator is not mandatory either, we'll get a dummy supply from the 
framework when not defined.

The main use case here is that if for example the panel and touchscreen 
share a power supply, they can now turn off the power supply when not in 
use.

>
>> +
>> +	usleep_range(5000, 6000);
>> +
>> +	if (ts->reset_gpio)
>> +		gpiod_set_value_cansleep(ts->reset_gpio, 0);
>> +
>> +	return 0;
>> +}
> Thanks.
>

-- 
Mike Looijmans