Message-ID: <1477564458.2458.10.camel@hadess.net>
Subject: Re: [PATCH 3/4] Input: goodix - Tweak configuration to use passed
 in touchscreen resolution
From: Bastien Nocera <hadess@hadess.net>
To: Franklin S Cooper Jr <fcooper@ti.com>, dmitry.torokhov@gmail.com,
        robh+dt@kernel.org, octavian.purdila@intel.com, irina.tirdea@intel.com,
        merker@debian.org, linux-input@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        nsekhar@ti.com, nm@ti.com
Date: Thu, 27 Oct 2016 12:34:18 +0200
In-Reply-To: <20161020195917.20051-4-fcooper@ti.com>
References: <20161020195917.20051-1-fcooper@ti.com>
         <20161020195917.20051-4-fcooper@ti.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3712
Lines: 113

On Thu, 2016-10-20 at 14:59 -0500, Franklin S Cooper Jr wrote:
> Some goodix touchscreen controllers aren't programed properly to
> match the
> display panel it is used on. Although the defaults may largely work
> it is
> also likely that the screen resolution will be incorrect. Therefore,
> add support to allow via DT for the touchscreen resolution to be set
> and
> reprogram the controller to use this updated resolution.
> 
> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> ---
>  .../bindings/input/touchscreen/goodix.txt          |  2 ++
>  drivers/input/touchscreen/goodix.c                 | 26
> +++++++++++++++++++++-
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git
> a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> index c98757a..ebc7cb7 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> @@ -19,6 +19,8 @@ Optional properties:
>  			  interrupt gpio pin as output to reset the
> device.
>   - reset-gpios		: GPIO pin used for reset
>  
> + - touchscreen-size-x      : horizontal resolution of touchscreen
> (in pixels)
> + - touchscreen-size-y      : vertical resolution of touchscreen (in
> pixels)
>   - touchscreen-inverted-x  : X axis is inverted (boolean)
>   - touchscreen-inverted-y  : Y axis is inverted (boolean)
>   - touchscreen-swapped-x-y : X and Y axis are swapped (boolean)
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 01e12f8..c2428e1 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -496,6 +496,10 @@ static void goodix_tweak_config(struct
> goodix_ts_data *ts)
>  		return;
>  	}
>  
> +	/* Setting X and Y Resolution */
> +	put_unaligned_le16(ts->abs_x_max, &config[RESOLUTION_LOC]);
> +	put_unaligned_le16(ts->abs_y_max, &config[RESOLUTION_LOC +
> 2]);
> +
>  	check_sum = goodix_calculate_checksum(ts->cfg_len, config);
>  
>  	config[raw_cfg_len] = check_sum;
> @@ -669,6 +673,8 @@ static int goodix_request_input_dev(struct
> goodix_ts_data *ts)
>  static int goodix_configure_dev(struct goodix_ts_data *ts)
>  {
>  	int error;
> +	bool alter_config = false;
> +	u32 max_x, max_y;
>  
>  	ts->swapped_x_y = device_property_read_bool(&ts->client-
> >dev,
>  						    "touchscreen-
> swapped-x-y");
> @@ -676,9 +682,27 @@ static int goodix_configure_dev(struct
> goodix_ts_data *ts)
>  						   "touchscreen-
> inverted-x");
>  	ts->inverted_y = device_property_read_bool(&ts->client->dev,
>  						   "touchscreen-
> inverted-y");
> -

No need for that linefeed removal.

>  	goodix_read_config(ts);
>  
> +	if (device_property_present(&ts->client->dev, "touchscreen-
> size-x") &&

Is it expected that incomplete or invalid arguments do not generate
errors? I'd expect that the presence of just one of those properties,
or...

> +	    device_property_present(&ts->client->dev, "touchscreen-
> size-y")) {
> +
> +		device_property_read_u32(&ts->client->dev,
> "touchscreen-size-x",
> +				&max_x);
> +
> +		device_property_read_u32(&ts->client->dev,
> "touchscreen-size-y",
> +				&max_y);
> +
> +		if (max_x > 0 && max_y > 0) {

... invalid values for those properties would throw errors (either
warnings, or lower severity messages).

> +			ts->abs_x_max = max_x;
> +			ts->abs_y_max = max_y;
> +			alter_config = true;
> +		}
> +	}
> +
> +	if (alter_config)
> +		goodix_tweak_config(ts);
> +
>  	error = goodix_request_input_dev(ts);
>  	if (error)
>  		return error;