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

[Mike Looijmans]

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@xxxxxxxxxxxxxxxxx
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@xxxxxxxx>
> > ---
> >   .../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