Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 27 May 2022 21:47:50 -0700
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     Mike Looijmans <mike.looijmans@topic.nl>
Cc:     linux-input@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, martink@posteo.de,
        geert+renesas@glider.be, john@metanate.com, hechtb@gmail.com
Subject: Re: [PATCH] Input: st1232 - Support power supply regulators
Message-ID: <YpGpdsAJicTqHbeY@google.com>
References: <20220524081216.8550-1-mike.looijmans@topic.nl>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220524081216.8550-1-mike.looijmans@topic.nl>
Precedence: bulk

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?

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

Thanks.

-- 
Dmitry