[PATCH 0/4] Input: goodix - Support dynamic reconfiguration

[PATCH 0/4] Input: goodix - Support dynamic reconfiguration

[Franklin S Cooper Jr]

Goodix touchscreen controller supports the ability to update its settings
at runtime. Currently, the driver supports this by loading a firmware
and reconfiguring the panel. However, this may lead to a large quantity
of firmwares being created and managed. This is a hassle since likely a
preexisting firmware or even the default configuration is good enough and
only a couple of changes need to be made.

This patchset adds the capability to dynamically alter the firmware
utilizing several new DT properties.

These new properties will allow a user to specify the touchscreen
resolution and also allow x and y coordinates to be interchanged. All of
this configuration changes are done dynamically by the driver and can work
using the default touchscreen controller configuration or even on top of
a firmware that was loaded.

Franklin S Cooper Jr (4):
  Input: goodix - Restructure cfg checksum function
  Input: goodix - Allow tweaking of configuration file dynamically
  Input: goodix - Tweak configuration to use passed in touchscreen
    resolution
  Input: goodix - Support interchanging x and y coordinates in hardware

 .../bindings/input/touchscreen/goodix.txt          |   4 +
 drivers/input/touchscreen/goodix.c                 | 109 +++++++++++++++++----
 2 files changed, 96 insertions(+), 17 deletions(-)

-- 
2.10.0

[PATCH 1/4] Input: goodix - Restructure cfg checksum function

[Franklin S Cooper Jr]

Split generation of checksum into is own function. Currently it is used
only to determine if the checksum for the user provided configuration file
is valid or not. However, support to dynamically alter the configuration
file will be supported and needs a way to calculate the updated file's
checksum. Therefore, by splitting calculating checksum into its own
function the code can be reused.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
 drivers/input/touchscreen/goodix.c | 42 +++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index e9d50bc..a43c8ca 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -305,35 +305,45 @@ static int goodix_request_irq(struct goodix_ts_data *ts)
 					 ts->irq_flags, ts->client->name, ts);
 }
 
+static int goodix_calculate_checksum(size_t size, const u8 *data)
+{
+	int i, raw_cfg_len;
+	u8 check_sum = 0;
+
+	raw_cfg_len = size - 2;
+	for (i = 0; i < raw_cfg_len; i++)
+		check_sum += data[i];
+	check_sum = (~check_sum) + 1;
+
+	return check_sum;
+}
 /**
  * goodix_check_cfg - Checks if config fw is valid
  *
  * @ts: goodix_ts_data pointer
  * @cfg: firmware config data
  */
-static int goodix_check_cfg(struct goodix_ts_data *ts,
-			    const struct firmware *cfg)
+static int goodix_check_cfg(struct goodix_ts_data *ts, size_t size,
+			    const u8 *data)
 {
-	int i, raw_cfg_len;
+	int raw_cfg_len;
 	u8 check_sum = 0;
 
-	if (cfg->size > GOODIX_CONFIG_MAX_LENGTH) {
+	if (size > GOODIX_CONFIG_MAX_LENGTH) {
 		dev_err(&ts->client->dev,
 			"The length of the config fw is not correct");
 		return -EINVAL;
 	}
 
-	raw_cfg_len = cfg->size - 2;
-	for (i = 0; i < raw_cfg_len; i++)
-		check_sum += cfg->data[i];
-	check_sum = (~check_sum) + 1;
-	if (check_sum != cfg->data[raw_cfg_len]) {
+	raw_cfg_len = size - 2;
+	check_sum = goodix_calculate_checksum(size, data);
+	if (check_sum != data[raw_cfg_len]) {
 		dev_err(&ts->client->dev,
 			"The checksum of the config fw is not correct");
 		return -EINVAL;
 	}
 
-	if (cfg->data[raw_cfg_len + 1] != 1) {
+	if (data[raw_cfg_len + 1] != 1) {
 		dev_err(&ts->client->dev,
 			"Config fw must have Config_Fresh register set");
 		return -EINVAL;
@@ -348,17 +358,17 @@ static int goodix_check_cfg(struct goodix_ts_data *ts,
  * @ts: goodix_ts_data pointer
  * @cfg: config firmware to write to device
  */
-static int goodix_send_cfg(struct goodix_ts_data *ts,
-			   const struct firmware *cfg)
+static int goodix_send_cfg(struct goodix_ts_data *ts, size_t size,
+			   const u8 *data)
 {
 	int error;
 
-	error = goodix_check_cfg(ts, cfg);
+	error = goodix_check_cfg(ts, size, data);
 	if (error)
 		return error;
 
-	error = goodix_i2c_write(ts->client, GOODIX_REG_CONFIG_DATA, cfg->data,
-				 cfg->size);
+	error = goodix_i2c_write(ts->client, GOODIX_REG_CONFIG_DATA, data,
+				 size);
 	if (error) {
 		dev_err(&ts->client->dev, "Failed to write config data: %d",
 			error);
@@ -670,7 +680,7 @@ static void goodix_config_cb(const struct firmware *cfg, void *ctx)
 
 	if (cfg) {
 		/* send device configuration to the firmware */
-		error = goodix_send_cfg(ts, cfg);
+		error = goodix_send_cfg(ts, cfg->size, cfg->data);
 		if (error)
 			goto err_release_cfg;
 	}
-- 
2.10.0

Re: [PATCH 1/4] Input: goodix - Restructure cfg checksum function

[Bastien Nocera]

On Thu, 2016-10-20 at 14:59 -0500, Franklin S Cooper Jr wrote:
> Split generation of checksum into is own function. Currently it is used

"its own".

> only to determine if the checksum for the user provided configuration file
> is valid or not. However, support to dynamically alter the configuration
> file will be supported and needs a way to calculate the updated file's
> checksum. Therefore, by splitting calculating checksum into its own
> function the code can be reused.
> 

After the fixup above:

Reviewed-by: Bastien Nocera <hadess@hadess.net>

[PATCH 2/4] Input: goodix - Allow tweaking of configuration file dynamically

[Franklin S Cooper Jr]

Some goodix touchscreen controllers don't have the correct configuration
firmware for the display panel it is attached to. Therefore, settings such
as touchscreen x and y size may need to be passed in via DT and have the
panel reprogrammed for the updated touchscreen resolution.

This patchset adds support for reading the current configuration firmware
on the panel and allowing it to be modified and rewritten back to the
device.

Currently this function is unused but later patches will make use of it.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
 drivers/input/touchscreen/goodix.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index a43c8ca..01e12f8 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -478,6 +478,34 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
 	return 0;
 }
 
+static void goodix_tweak_config(struct goodix_ts_data *ts)
+{
+	u8 config[GOODIX_CONFIG_MAX_LENGTH];
+	int error;
+	int raw_cfg_len;
+	u8 check_sum = 0;
+
+	raw_cfg_len = ts->cfg_len - 2;
+
+	error = goodix_i2c_read(ts->client, GOODIX_REG_CONFIG_DATA,
+				config, ts->cfg_len);
+	if (error) {
+		dev_warn(&ts->client->dev,
+			 "Error reading config (%d), avoid tweaking config\n",
+			 error);
+		return;
+	}
+
+	check_sum = goodix_calculate_checksum(ts->cfg_len, config);
+
+	config[raw_cfg_len] = check_sum;
+	config[raw_cfg_len + 1] = 1;
+
+	error = goodix_send_cfg(ts, ts->cfg_len, config);
+	if (error)
+		dev_warn(&ts->client->dev,
+			 "Error writing config (%d)\n", error);
+}
 /**
  * goodix_read_config - Read the embedded configuration of the panel
  *
-- 
2.10.0

Re: [PATCH 2/4] Input: goodix - Allow tweaking of configuration file dynamically

[Bastien Nocera]

On Thu, 2016-10-20 at 14:59 -0500, Franklin S Cooper Jr wrote:
> Some goodix touchscreen controllers don't have the correct
> configuration
> firmware for the display panel it is attached to. Therefore, settings
> such
> as touchscreen x and y size may need to be passed in via DT and have
> the
> panel reprogrammed for the updated touchscreen resolution.
> 
> This patchset adds support for reading the current configuration
> firmware
> on the panel and allowing it to be modified and rewritten back to the
> device.
> 
> Currently this function is unused but later patches will make use of
> it.
> 
> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> ---
>  drivers/input/touchscreen/goodix.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index a43c8ca..01e12f8 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -478,6 +478,34 @@ static int goodix_get_gpio_config(struct
> goodix_ts_data *ts)
>  	return 0;
>  }
>  
> +static void goodix_tweak_config(struct goodix_ts_data *ts)
> +{
> +	u8 config[GOODIX_CONFIG_MAX_LENGTH];
> +	int error;
> +	int raw_cfg_len;
> +	u8 check_sum = 0;
> +
> +	raw_cfg_len = ts->cfg_len - 2;
> +
> +	error = goodix_i2c_read(ts->client, GOODIX_REG_CONFIG_DATA,
> +				config, ts->cfg_len);
> +	if (error) {
> +		dev_warn(&ts->client->dev,
> +			 "Error reading config (%d), avoid tweaking
> config\n",
> +			 error);
> +		return;
> +	}

Please add a placeholder comment here, to go along with the commit
message explanation.

> +	check_sum = goodix_calculate_checksum(ts->cfg_len, config);
> +
> +	config[raw_cfg_len] = check_sum;
> +	config[raw_cfg_len + 1] = 1;
> +
> +	error = goodix_send_cfg(ts, ts->cfg_len, config);
> +	if (error)
> +		dev_warn(&ts->client->dev,
> +			 "Error writing config (%d)\n", error);
> +}
>  /**
>   * goodix_read_config - Read the embedded configuration of the panel
>   *

Re: [PATCH 2/4] Input: goodix - Allow tweaking of configuration file dynamically

[Franklin S Cooper Jr]

On 10/27/2016 05:33 AM, Bastien Nocera wrote:
> On Thu, 2016-10-20 at 14:59 -0500, Franklin S Cooper Jr wrote:
>> Some goodix touchscreen controllers don't have the correct
>> configuration
>> firmware for the display panel it is attached to. Therefore, settings
>> such
>> as touchscreen x and y size may need to be passed in via DT and have
>> the
>> panel reprogrammed for the updated touchscreen resolution.
>>
>> This patchset adds support for reading the current configuration
>> firmware
>> on the panel and allowing it to be modified and rewritten back to the
>> device.
>>
>> Currently this function is unused but later patches will make use of
>> it.
>>
>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>> ---
>>  drivers/input/touchscreen/goodix.c | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/input/touchscreen/goodix.c
>> b/drivers/input/touchscreen/goodix.c
>> index a43c8ca..01e12f8 100644
>> --- a/drivers/input/touchscreen/goodix.c
>> +++ b/drivers/input/touchscreen/goodix.c
>> @@ -478,6 +478,34 @@ static int goodix_get_gpio_config(struct
>> goodix_ts_data *ts)
>>  	return 0;
>>  }
>>  
>> +static void goodix_tweak_config(struct goodix_ts_data *ts)
>> +{
>> +	u8 config[GOODIX_CONFIG_MAX_LENGTH];
>> +	int error;
>> +	int raw_cfg_len;
>> +	u8 check_sum = 0;
>> +
>> +	raw_cfg_len = ts->cfg_len - 2;
>> +
>> +	error = goodix_i2c_read(ts->client, GOODIX_REG_CONFIG_DATA,
>> +				config, ts->cfg_len);
>> +	if (error) {
>> +		dev_warn(&ts->client->dev,
>> +			 "Error reading config (%d), avoid tweaking
>> config\n",
>> +			 error);
>> +		return;
>> +	}
> 
> Please add a placeholder comment here, to go along with the commit
> message explanation.

You just want a comment here stating that changes to the firmware should
be made here?
> 
>> +	check_sum = goodix_calculate_checksum(ts->cfg_len, config);
>> +
>> +	config[raw_cfg_len] = check_sum;
>> +	config[raw_cfg_len + 1] = 1;
>> +
>> +	error = goodix_send_cfg(ts, ts->cfg_len, config);
>> +	if (error)
>> +		dev_warn(&ts->client->dev,
>> +			 "Error writing config (%d)\n", error);
>> +}
>>  /**
>>   * goodix_read_config - Read the embedded configuration of the panel
>>   *

Re: [PATCH 2/4] Input: goodix - Allow tweaking of configuration file dynamically

[Bastien Nocera]

On Thu, 2016-10-27 at 11:58 -0500, Franklin S Cooper Jr wrote:
> 
> On 10/27/2016 05:33 AM, Bastien Nocera wrote:
> > On Thu, 2016-10-20 at 14:59 -0500, Franklin S Cooper Jr wrote:
> > > Some goodix touchscreen controllers don't have the correct
> > > configuration
> > > firmware for the display panel it is attached to. Therefore,
> > > settings
> > > such
> > > as touchscreen x and y size may need to be passed in via DT and
> > > have
> > > the
> > > panel reprogrammed for the updated touchscreen resolution.
> > > 
> > > This patchset adds support for reading the current configuration
> > > firmware
> > > on the panel and allowing it to be modified and rewritten back to
> > > the
> > > device.
> > > 
> > > Currently this function is unused but later patches will make use
> > > of
> > > it.
> > > 
> > > Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> > > ---
> > >  drivers/input/touchscreen/goodix.c | 28
> > > ++++++++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > > 
> > > diff --git a/drivers/input/touchscreen/goodix.c
> > > b/drivers/input/touchscreen/goodix.c
> > > index a43c8ca..01e12f8 100644
> > > --- a/drivers/input/touchscreen/goodix.c
> > > +++ b/drivers/input/touchscreen/goodix.c
> > > @@ -478,6 +478,34 @@ static int goodix_get_gpio_config(struct
> > > goodix_ts_data *ts)
> > >  	return 0;
> > >  }
> > >  
> > > +static void goodix_tweak_config(struct goodix_ts_data *ts)
> > > +{
> > > +	u8 config[GOODIX_CONFIG_MAX_LENGTH];
> > > +	int error;
> > > +	int raw_cfg_len;
> > > +	u8 check_sum = 0;
> > > +
> > > +	raw_cfg_len = ts->cfg_len - 2;
> > > +
> > > +	error = goodix_i2c_read(ts->client,
> > > GOODIX_REG_CONFIG_DATA,
> > > +				config, ts->cfg_len);
> > > +	if (error) {
> > > +		dev_warn(&ts->client->dev,
> > > +			 "Error reading config (%d), avoid
> > > tweaking
> > > config\n",
> > > +			 error);
> > > +		return;
> > > +	}
> > 
> > Please add a placeholder comment here, to go along with the commit
> > message explanation.
> 
> You just want a comment here stating that changes to the firmware
> should
> be made here?

Yes, at least for this intermediary patch. You can remove in in
subsequent patches when it's clearer that this is where things go.

> > > +	check_sum = goodix_calculate_checksum(ts->cfg_len,
> > > config);
> > > +
> > > +	config[raw_cfg_len] = check_sum;
> > > +	config[raw_cfg_len + 1] = 1;
> > > +
> > > +	error = goodix_send_cfg(ts, ts->cfg_len, config);
> > > +	if (error)
> > > +		dev_warn(&ts->client->dev,
> > > +			 "Error writing config (%d)\n", error);
> > > +}
> > >  /**
> > >   * goodix_read_config - Read the embedded configuration of the
> > > panel
> > >   *

[PATCH 3/4] Input: goodix - Tweak configuration to use passed in touchscreen resolution

[Franklin S Cooper Jr]

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");
-
 	goodix_read_config(ts);
 
+	if (device_property_present(&ts->client->dev, "touchscreen-size-x") &&
+	    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) {
+			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;
-- 
2.10.0

Re: [PATCH 3/4] Input: goodix - Tweak configuration to use passed in touchscreen resolution

[Rob Herring]

On Thu, Oct 20, 2016 at 02:59:16PM -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 ++

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/input/touchscreen/goodix.c                 | 26 +++++++++++++++++++++-
>  2 files changed, 27 insertions(+), 1 deletion(-)

Re: [PATCH 3/4] Input: goodix - Tweak configuration to use passed in touchscreen resolution

[Bastien Nocera]

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;

Re: [PATCH 3/4] Input: goodix - Tweak configuration to use passed in touchscreen resolution

[Franklin S Cooper Jr]

On 10/27/2016 05:34 AM, Bastien Nocera wrote:
> 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...

Looking back at this there is technically nothing wrong with only
wanting to set 1 property. So I will update this to decouple these two
properties.
> 
>> +	    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).

I'll add a warning here indicating that the passed in value with be ignored.
> 
>> +			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;

[PATCH 4/4] Input: goodix - Support interchanging x and y coordinates in hardware

[Franklin S Cooper Jr]

On systems with a fixed display/touchscreen orientation it is important to
pass in the "correct" x and y coordinates based on the orientation.
Currently, to support landscape and portrait touchscreen-swapped-x-y
simply does the following:

Assuming touchscreen is as follows:
X: 1280 Y:800 programmed in touchscreen controller and also interchange
bit cleared. Assuming ts mounted in portrait mode.

1280 (X)
------
|    |
|    | 800 (Y)
|    |
|    |
------

800 (Y)
------
|    |
|    | 1280 (X)
|    |
|    |
------

However, the above isn't really what we want especially in distros that
assumes a fixed orientation. In this case what we really want is to
interchange the x and y coordinates so the Y coordinate can return a max
value of 1280 and X can return a max value of 800.

800 (X)
------
|    |
|    | 1280 (Y)
|    |
|    |
------

Since the driver is limited to the value reported by the touchscreen
controller this issue can't be fixed purely in the driver. Therefore,
add a new DT property that supports interchanging X and Y coordinates
internally within the hardware.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
 .../devicetree/bindings/input/touchscreen/goodix.txt        |  2 ++
 drivers/input/touchscreen/goodix.c                          | 13 +++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index ebc7cb7..b8be2ab 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -25,6 +25,8 @@ Optional properties:
  - touchscreen-inverted-y  : Y axis is inverted (boolean)
  - touchscreen-swapped-x-y : X and Y axis are swapped (boolean)
                              (swapping is done after inverting the axis)
+ - touchscreen-inter-x-y   : X and Y maximum values programmed in the device
+                             are interchanged internally in hardware. (boolean)
 
 Example:
 
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index c2428e1..3f93375 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -39,6 +39,7 @@ struct goodix_ts_data {
 	bool swapped_x_y;
 	bool inverted_x;
 	bool inverted_y;
+	bool interchange_x_y;
 	unsigned int max_touch_num;
 	unsigned int int_trigger_type;
 	int cfg_len;
@@ -76,6 +77,9 @@ struct goodix_ts_data {
 #define MAX_CONTACTS_LOC	5
 #define TRIGGER_LOC		6
 
+/* Register Bits */
+#define XY_COORD_INTER		3
+
 static const unsigned long goodix_irq_flags[] = {
 	IRQ_TYPE_EDGE_RISING,
 	IRQ_TYPE_EDGE_FALLING,
@@ -500,6 +504,10 @@ static void goodix_tweak_config(struct goodix_ts_data *ts)
 	put_unaligned_le16(ts->abs_x_max, &config[RESOLUTION_LOC]);
 	put_unaligned_le16(ts->abs_y_max, &config[RESOLUTION_LOC + 2]);
 
+	if (ts->interchange_x_y)
+		config[TRIGGER_LOC] = config[TRIGGER_LOC] |
+				      (1 << XY_COORD_INTER);
+
 	check_sum = goodix_calculate_checksum(ts->cfg_len, config);
 
 	config[raw_cfg_len] = check_sum;
@@ -700,6 +708,11 @@ static int goodix_configure_dev(struct goodix_ts_data *ts)
 		}
 	}
 
+	ts->interchange_x_y = device_property_read_bool(&ts->client->dev,
+						    "touchscreen-inter-x-y");
+	if (ts->interchange_x_y)
+		alter_config = true;
+
 	if (alter_config)
 		goodix_tweak_config(ts);
 
-- 
2.10.0

Re: [PATCH 4/4] Input: goodix - Support interchanging x and y coordinates in hardware

[Rob Herring]

On Thu, Oct 20, 2016 at 02:59:17PM -0500, Franklin S Cooper Jr wrote:
> On systems with a fixed display/touchscreen orientation it is important to
> pass in the "correct" x and y coordinates based on the orientation.
> Currently, to support landscape and portrait touchscreen-swapped-x-y
> simply does the following:
> 
> Assuming touchscreen is as follows:
> X: 1280 Y:800 programmed in touchscreen controller and also interchange
> bit cleared. Assuming ts mounted in portrait mode.
> 
> 1280 (X)
> ------
> |    |
> |    | 800 (Y)
> |    |
> |    |
> ------
> 
> 800 (Y)
> ------
> |    |
> |    | 1280 (X)
> |    |
> |    |
> ------
> 
> However, the above isn't really what we want especially in distros that
> assumes a fixed orientation. In this case what we really want is to
> interchange the x and y coordinates so the Y coordinate can return a max
> value of 1280 and X can return a max value of 800.
> 
> 800 (X)
> ------
> |    |
> |    | 1280 (Y)
> |    |
> |    |
> ------
> 
> Since the driver is limited to the value reported by the touchscreen
> controller this issue can't be fixed purely in the driver. Therefore,
> add a new DT property that supports interchanging X and Y coordinates
> internally within the hardware.

I'm not sure I follow why existing properties don't cover this.

> 
> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> ---
>  .../devicetree/bindings/input/touchscreen/goodix.txt        |  2 ++
>  drivers/input/touchscreen/goodix.c                          | 13 +++++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> index ebc7cb7..b8be2ab 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> @@ -25,6 +25,8 @@ Optional properties:
>   - touchscreen-inverted-y  : Y axis is inverted (boolean)
>   - touchscreen-swapped-x-y : X and Y axis are swapped (boolean)
>                               (swapping is done after inverting the axis)
> + - touchscreen-inter-x-y   : X and Y maximum values programmed in the device
> +                             are interchanged internally in hardware. (boolean)

Minimally this should be vendor specific and have a vendor prefix I 
think.

Rob

Re: [PATCH 4/4] Input: goodix - Support interchanging x and y coordinates in hardware

[Bastien Nocera]

On Wed, 2016-10-26 at 18:18 -0500, Rob Herring wrote:
> On Thu, Oct 20, 2016 at 02:59:17PM -0500, Franklin S Cooper Jr wrote:
> > 
<snip>
> I'm not sure I follow why existing properties don't cover this.

Me neither. I certainly don't understand why the driver can't mangle
the data from the touchscreen all it wants. It's not like user-space is
talking to the touchscreen directly.

Re: [PATCH 4/4] Input: goodix - Support interchanging x and y coordinates in hardware

[Franklin S Cooper Jr]

On 10/27/2016 05:34 AM, Bastien Nocera wrote:
> On Wed, 2016-10-26 at 18:18 -0500, Rob Herring wrote:
>> On Thu, Oct 20, 2016 at 02:59:17PM -0500, Franklin S Cooper Jr wrote:
>>>
> <snip>
>> I'm not sure I follow why existing properties don't cover this.
> 
> Me neither. I certainly don't understand why the driver can't mangle
> the data from the touchscreen all it wants. It's not like user-space is
> talking to the touchscreen directly.
> 
Sorry the above could of been clearer.

Lets ignore talking about X and Y axis for a little bit since that
really depends on the default touchscreen config values and the way it
is mounted on the touchscreen. Now lets say when your interacting with
the touchscreen the touchscreen controller outputs a max value of 1280
when moving your finger horizontally and 800 when moving your finger
vertically.

<-1280->
------
|    |  ^
|    |  |
|    | 800
|    |  |
------  V

So no matter what your horizontal range is 0-1280 and your vertical
range is 0-800. Now based on the above diagram you can see that usually
you want the longer side to have a higher resolution. So you may want a
vertical range of 0-1280 and a horizontal range from 0-800 instead.

So lets add labels to the original diagram and assume that the x and y
axis from the driver/user-space perspective is as follows.
<-1280-> (X)
------
|    |  ^
|    |  |
|    | 800  (Y)
|    |  |
------  V

The only thing the driver (software) has the ability to do is change the
"orientation".

<-1280-> (Y)
------
|    |  ^
|    |  |
|    | 800  (X)
|    |  |
------  V

However, this doesn't change the resolution ie range of values in the
horizontal and vertical direction the touch screen controller will
report. Only the hardware can determine the resolution it will use. The
interchange bit I set essentially swaps the range that the controller is
currently programmed to use which in my first diagram the horizontal
range was 0-1280 and my vertical range is 0-800. So by setting this
interchange bit in hardware the horizontal range will now be 0-800 while
the vertical range will be 0-1280 which is what we want.

Does this clarify things? I messed up the second diagram in my commit
message which is probably what caused the confusion.

Re: [PATCH 4/4] Input: goodix - Support interchanging x and y coordinates in hardware

[Bastien Nocera]

On Thu, 2016-10-27 at 12:42 -0500, Franklin S Cooper Jr wrote:
> 
> On 10/27/2016 05:34 AM, Bastien Nocera wrote:
> > On Wed, 2016-10-26 at 18:18 -0500, Rob Herring wrote:
> > > On Thu, Oct 20, 2016 at 02:59:17PM -0500, Franklin S Cooper Jr
> > > wrote:
> > > > 
> > 
> > <snip>
> > > I'm not sure I follow why existing properties don't cover this.
> > 
> > Me neither. I certainly don't understand why the driver can't
> > mangle
> > the data from the touchscreen all it wants. It's not like user-
> > space is
> > talking to the touchscreen directly.
> > 
> 
> Sorry the above could of been clearer.
> 
> Lets ignore talking about X and Y axis for a little bit since that
> really depends on the default touchscreen config values and the way
> it
> is mounted on the touchscreen. Now lets say when your interacting
> with
> the touchscreen the touchscreen controller outputs a max value of
> 1280
> when moving your finger horizontally and 800 when moving your finger
> vertically.
> 
> <-1280->
> ------
> >    |  ^
> >    |  |
> >    | 800
> >    |  |
> 
> ------  V
> 
> So no matter what your horizontal range is 0-1280 and your vertical
> range is 0-800. Now based on the above diagram you can see that
> usually
> you want the longer side to have a higher resolution. So you may want
> a
> vertical range of 0-1280 and a horizontal range from 0-800 instead.
> 
> So lets add labels to the original diagram and assume that the x and
> y
> axis from the driver/user-space perspective is as follows.
> <-1280-> (X)
> ------
> >    |  ^
> >    |  |
> >    | 800  (Y)
> >    |  |
> 
> ------  V
> 
> The only thing the driver (software) has the ability to do is change
> the
> "orientation".
> 
> <-1280-> (Y)
> ------
> >    |  ^
> >    |  |
> >    | 800  (X)
> >    |  |
> 
> ------  V
> 
> However, this doesn't change the resolution ie range of values in the
> horizontal and vertical direction the touch screen controller will
> report. Only the hardware can determine the resolution it will use.
> The
> interchange bit I set essentially swaps the range that the controller
> is
> currently programmed to use which in my first diagram the horizontal
> range was 0-1280 and my vertical range is 0-800. So by setting this
> interchange bit in hardware the horizontal range will now be 0-800
> while
> the vertical range will be 0-1280 which is what we want.
> 
> Does this clarify things? I messed up the second diagram in my commit
> message which is probably what caused the confusion.

Looks to me that this should be fixed in the firmware configuration,
which is what Irina's patches allow doing.

If the goal of this series is implementing this, I wouldn't take any of
those patches until we figure out why the firmware config in those
devices isn't set properly.

Re: [PATCH 4/4] Input: goodix - Support interchanging x and y coordinates in hardware

[Franklin S Cooper Jr]

On 10/26/2016 06:18 PM, Rob Herring wrote:
> On Thu, Oct 20, 2016 at 02:59:17PM -0500, Franklin S Cooper Jr wrote:
>> On systems with a fixed display/touchscreen orientation it is important to
>> pass in the "correct" x and y coordinates based on the orientation.
>> Currently, to support landscape and portrait touchscreen-swapped-x-y
>> simply does the following:
>>
>> Assuming touchscreen is as follows:
>> X: 1280 Y:800 programmed in touchscreen controller and also interchange
>> bit cleared. Assuming ts mounted in portrait mode.
>>
>> 1280 (X)
>> ------
>> |    |
>> |    | 800 (Y)
>> |    |
>> |    |
>> ------
>>
>> 800 (Y)
>> ------
>> |    |
>> |    | 1280 (X)
>> |    |
>> |    |
>> ------
>>
>> However, the above isn't really what we want especially in distros that
>> assumes a fixed orientation. In this case what we really want is to
>> interchange the x and y coordinates so the Y coordinate can return a max
>> value of 1280 and X can return a max value of 800.
>>
>> 800 (X)
>> ------
>> |    |
>> |    | 1280 (Y)
>> |    |
>> |    |
>> ------
>>
>> Since the driver is limited to the value reported by the touchscreen
>> controller this issue can't be fixed purely in the driver. Therefore,
>> add a new DT property that supports interchanging X and Y coordinates
>> internally within the hardware.
> 
> I'm not sure I follow why existing properties don't cover this.
> 
>>
>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>> ---
>>  .../devicetree/bindings/input/touchscreen/goodix.txt        |  2 ++
>>  drivers/input/touchscreen/goodix.c                          | 13 +++++++++++++
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
>> index ebc7cb7..b8be2ab 100644
>> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
>> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
>> @@ -25,6 +25,8 @@ Optional properties:
>>   - touchscreen-inverted-y  : Y axis is inverted (boolean)
>>   - touchscreen-swapped-x-y : X and Y axis are swapped (boolean)
>>                               (swapping is done after inverting the axis)
>> + - touchscreen-inter-x-y   : X and Y maximum values programmed in the device
>> +                             are interchanged internally in hardware. (boolean)
> 
> Minimally this should be vendor specific and have a vendor prefix I 
> think.

Would "goodix,inter-x-y" work?
> 
> Rob
>