[PATCH v1 1/3] Input: ili210x - do not retrieve/print chip firmware version

[PATCH v1 1/3] Input: ili210x - do not retrieve/print chip firmware version

[Sven Van Asbroeck]

The driver's method to retrieve the firmware version on ili2117/
ili2118 chip flavours is incorrect. The firmware version register
address and layout are wrong.

The firmware version is not actually used anywhere inside or
outside this driver. There is a dev_dbg() print, but that is
only visible when the developer explicitly compiles in debug
support.

Don't make the code more complicated to preserve a feature that
no-one is using. Remove all code associated with chip firmware
version.

Link: https://lore.kernel.org/lkml/20191111181657.GA57214@dtor-ws/
Cc: Marek Vasut <marex@denx.de>
Cc: Adam Ford <aford173@gmail.com>
Cc: <linux-kernel@vger.kernel.org>
Cc: linux-input@vger.kernel.org
Tree: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=next
Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
 drivers/input/touchscreen/ili210x.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
index 0ed6014af6d7..a6feae5ce887 100644
--- a/drivers/input/touchscreen/ili210x.c
+++ b/drivers/input/touchscreen/ili210x.c
@@ -21,15 +21,8 @@
 /* Touchscreen commands */
 #define REG_TOUCHDATA		0x10
 #define REG_PANEL_INFO		0x20
-#define REG_FIRMWARE_VERSION	0x40
 #define REG_CALIBRATE		0xcc
 
-struct firmware_version {
-	u8 id;
-	u8 major;
-	u8 minor;
-} __packed;
-
 struct ili2xxx_chip {
 	int (*read_reg)(struct i2c_client *client, u8 reg,
 			void *buf, size_t len);
@@ -342,7 +335,6 @@ static int ili210x_i2c_probe(struct i2c_client *client,
 	struct ili210x *priv;
 	struct gpio_desc *reset_gpio;
 	struct input_dev *input;
-	struct firmware_version firmware;
 	int error;
 
 	dev_dbg(dev, "Probing for ILI210X I2C Touschreen driver");
@@ -389,15 +381,6 @@ static int ili210x_i2c_probe(struct i2c_client *client,
 	priv->chip = chip;
 	i2c_set_clientdata(client, priv);
 
-	/* Get firmware version */
-	error = chip->read_reg(client, REG_FIRMWARE_VERSION,
-			       &firmware, sizeof(firmware));
-	if (error) {
-		dev_err(dev, "Failed to get firmware version, err: %d\n",
-			error);
-		return error;
-	}
-
 	/* Setup input device */
 	input->name = "ILI210x Touchscreen";
 	input->id.bustype = BUS_I2C;
@@ -439,10 +422,6 @@ static int ili210x_i2c_probe(struct i2c_client *client,
 		return error;
 	}
 
-	dev_dbg(dev,
-		"ILI210x initialized (IRQ: %d), firmware version %d.%d.%d",
-		client->irq, firmware.id, firmware.major, firmware.minor);
-
 	return 0;
 }
 
-- 
2.17.1

[PATCH v1 2/3] Input: ili210x - add resolution to chip operations structure

[Sven Van Asbroeck]

Optionally allow the touch screen resolution to be set by adding
it to the chip operations structure. If it is omitted (left zero),
the resolution defaults to 64K. Which is the previously hard-coded
value.

Set the ili2117 resolution to 2048, as indicated in its datasheet.

Link: https://lore.kernel.org/lkml/20191111181657.GA57214@dtor-ws/
Cc: Marek Vasut <marex@denx.de>
Cc: Adam Ford <aford173@gmail.com>
Cc: <linux-kernel@vger.kernel.org>
Cc: linux-input@vger.kernel.org
Tree: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=next
Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
 drivers/input/touchscreen/ili210x.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
index a6feae5ce887..4321f0d676cc 100644
--- a/drivers/input/touchscreen/ili210x.c
+++ b/drivers/input/touchscreen/ili210x.c
@@ -31,6 +31,7 @@ struct ili2xxx_chip {
 				 unsigned int *x, unsigned int *y);
 	bool (*continue_polling)(const u8 *data, bool touch);
 	unsigned int max_touches;
+	unsigned int resolution;
 };
 
 struct ili210x {
@@ -160,6 +161,7 @@ static const struct ili2xxx_chip ili211x_chip = {
 	.parse_touch_data	= ili211x_touchdata_to_coords,
 	.continue_polling	= ili211x_decline_polling,
 	.max_touches		= 10,
+	.resolution		= 2048,
 };
 
 static int ili251x_read_reg(struct i2c_client *client,
@@ -336,6 +338,7 @@ static int ili210x_i2c_probe(struct i2c_client *client,
 	struct gpio_desc *reset_gpio;
 	struct input_dev *input;
 	int error;
+	unsigned int max_xy;
 
 	dev_dbg(dev, "Probing for ILI210X I2C Touschreen driver");
 
@@ -386,8 +389,12 @@ static int ili210x_i2c_probe(struct i2c_client *client,
 	input->id.bustype = BUS_I2C;
 
 	/* Multi touch */
-	input_set_abs_params(input, ABS_MT_POSITION_X, 0, 0xffff, 0, 0);
-	input_set_abs_params(input, ABS_MT_POSITION_Y, 0, 0xffff, 0, 0);
+	if (chip->resolution)
+		max_xy = chip->resolution - 1;
+	else
+		max_xy = 0xffff;
+	input_set_abs_params(input, ABS_MT_POSITION_X, 0, max_xy, 0, 0);
+	input_set_abs_params(input, ABS_MT_POSITION_Y, 0, max_xy, 0, 0);
 	touchscreen_parse_properties(input, true, &priv->prop);
 
 	error = input_mt_init_slots(input, priv->chip->max_touches,
-- 
2.17.1

[PATCH v1 3/3] Input: ili210x - optionally hide calibrate sysfs attribute

[Sven Van Asbroeck]

Only show the 'calibrate' sysfs attribute on chip flavours
which support calibration by writing to a calibration register.

Do this by adding a flag to the chip operations structure.

Link: https://lore.kernel.org/lkml/20191111181657.GA57214@dtor-ws/
Cc: Marek Vasut <marex@denx.de>
Cc: Adam Ford <aford173@gmail.com>
Cc: <linux-kernel@vger.kernel.org>
Cc: linux-input@vger.kernel.org
Tree: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=next
Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
 drivers/input/touchscreen/ili210x.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
index 4321f0d676cc..810770ad02e2 100644
--- a/drivers/input/touchscreen/ili210x.c
+++ b/drivers/input/touchscreen/ili210x.c
@@ -32,6 +32,7 @@ struct ili2xxx_chip {
 	bool (*continue_polling)(const u8 *data, bool touch);
 	unsigned int max_touches;
 	unsigned int resolution;
+	bool no_calibrate_reg;
 };
 
 struct ili210x {
@@ -162,6 +163,7 @@ static const struct ili2xxx_chip ili211x_chip = {
 	.continue_polling	= ili211x_decline_polling,
 	.max_touches		= 10,
 	.resolution		= 2048,
+	.no_calibrate_reg	= true,
 };
 
 static int ili251x_read_reg(struct i2c_client *client,
@@ -310,8 +312,19 @@ static struct attribute *ili210x_attributes[] = {
 	NULL,
 };
 
+static umode_t ili210x_calibrate_visible(struct kobject *kobj,
+					  struct attribute *attr, int index)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ili210x *priv = i2c_get_clientdata(client);
+
+	return !priv->chip->no_calibrate_reg;
+}
+
 static const struct attribute_group ili210x_attr_group = {
 	.attrs = ili210x_attributes,
+	.is_visible = ili210x_calibrate_visible,
 };
 
 static void ili210x_power_down(void *data)
-- 
2.17.1

Re: [PATCH v1 3/3] Input: ili210x - optionally hide calibrate sysfs attribute

[Dmitry Torokhov]

On Tue, Nov 12, 2019 at 11:44:29AM -0500, Sven Van Asbroeck wrote:
> Only show the 'calibrate' sysfs attribute on chip flavours
> which support calibration by writing to a calibration register.
> 
> Do this by adding a flag to the chip operations structure.
> 
> Link: https://lore.kernel.org/lkml/20191111181657.GA57214@dtor-ws/
> Cc: Marek Vasut <marex@denx.de>
> Cc: Adam Ford <aford173@gmail.com>
> Cc: <linux-kernel@vger.kernel.org>
> Cc: linux-input@vger.kernel.org
> Tree: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=next
> Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
> ---
>  drivers/input/touchscreen/ili210x.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
> index 4321f0d676cc..810770ad02e2 100644
> --- a/drivers/input/touchscreen/ili210x.c
> +++ b/drivers/input/touchscreen/ili210x.c
> @@ -32,6 +32,7 @@ struct ili2xxx_chip {
>  	bool (*continue_polling)(const u8 *data, bool touch);
>  	unsigned int max_touches;
>  	unsigned int resolution;
> +	bool no_calibrate_reg;

Please use positive logic and have chips opt-in into the calibration.

>  };
>  
>  struct ili210x {
> @@ -162,6 +163,7 @@ static const struct ili2xxx_chip ili211x_chip = {
>  	.continue_polling	= ili211x_decline_polling,
>  	.max_touches		= 10,
>  	.resolution		= 2048,
> +	.no_calibrate_reg	= true,
>  };
>  
>  static int ili251x_read_reg(struct i2c_client *client,
> @@ -310,8 +312,19 @@ static struct attribute *ili210x_attributes[] = {
>  	NULL,
>  };
>  
> +static umode_t ili210x_calibrate_visible(struct kobject *kobj,
> +					  struct attribute *attr, int index)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct ili210x *priv = i2c_get_clientdata(client);
> +
> +	return !priv->chip->no_calibrate_reg;
> +}
> +
>  static const struct attribute_group ili210x_attr_group = {
>  	.attrs = ili210x_attributes,
> +	.is_visible = ili210x_calibrate_visible,
>  };
>  
>  static void ili210x_power_down(void *data)
> -- 
> 2.17.1
> 

-- 
Dmitry

Re: [PATCH v1 2/3] Input: ili210x - add resolution to chip operations structure

[Dmitry Torokhov]

On Tue, Nov 12, 2019 at 11:44:28AM -0500, Sven Van Asbroeck wrote:
> Optionally allow the touch screen resolution to be set by adding
> it to the chip operations structure. If it is omitted (left zero),
> the resolution defaults to 64K. Which is the previously hard-coded
> value.
> 
> Set the ili2117 resolution to 2048, as indicated in its datasheet.
> 
> Link: https://lore.kernel.org/lkml/20191111181657.GA57214@dtor-ws/
> Cc: Marek Vasut <marex@denx.de>
> Cc: Adam Ford <aford173@gmail.com>
> Cc: <linux-kernel@vger.kernel.org>
> Cc: linux-input@vger.kernel.org
> Tree: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=next
> Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
> ---
>  drivers/input/touchscreen/ili210x.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
> index a6feae5ce887..4321f0d676cc 100644
> --- a/drivers/input/touchscreen/ili210x.c
> +++ b/drivers/input/touchscreen/ili210x.c
> @@ -31,6 +31,7 @@ struct ili2xxx_chip {
>  				 unsigned int *x, unsigned int *y);
>  	bool (*continue_polling)(const u8 *data, bool touch);
>  	unsigned int max_touches;
> +	unsigned int resolution;
>  };
>  
>  struct ili210x {
> @@ -160,6 +161,7 @@ static const struct ili2xxx_chip ili211x_chip = {
>  	.parse_touch_data	= ili211x_touchdata_to_coords,
>  	.continue_polling	= ili211x_decline_polling,
>  	.max_touches		= 10,
> +	.resolution		= 2048,
>  };
>  
>  static int ili251x_read_reg(struct i2c_client *client,
> @@ -336,6 +338,7 @@ static int ili210x_i2c_probe(struct i2c_client *client,
>  	struct gpio_desc *reset_gpio;
>  	struct input_dev *input;
>  	int error;
> +	unsigned int max_xy;
>  
>  	dev_dbg(dev, "Probing for ILI210X I2C Touschreen driver");
>  
> @@ -386,8 +389,12 @@ static int ili210x_i2c_probe(struct i2c_client *client,
>  	input->id.bustype = BUS_I2C;
>  
>  	/* Multi touch */
> -	input_set_abs_params(input, ABS_MT_POSITION_X, 0, 0xffff, 0, 0);
> -	input_set_abs_params(input, ABS_MT_POSITION_Y, 0, 0xffff, 0, 0);
> +	if (chip->resolution)
> +		max_xy = chip->resolution - 1;
> +	else
> +		max_xy = 0xffff;

	max_xy = (chip->resolution ?: 65536) - 1;

> +	input_set_abs_params(input, ABS_MT_POSITION_X, 0, max_xy, 0, 0);
> +	input_set_abs_params(input, ABS_MT_POSITION_Y, 0, max_xy, 0, 0);
>  	touchscreen_parse_properties(input, true, &priv->prop);
>  
>  	error = input_mt_init_slots(input, priv->chip->max_touches,
> -- 
> 2.17.1
> 

-- 
Dmitry

Re: [PATCH v1 1/3] Input: ili210x - do not retrieve/print chip firmware version

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3050 bytes --]

Hi,

On Tue, Nov 12, 2019 at 11:44:27AM -0500, Sven Van Asbroeck wrote:
> The driver's method to retrieve the firmware version on ili2117/
> ili2118 chip flavours is incorrect. The firmware version register
> address and layout are wrong.
> 
> The firmware version is not actually used anywhere inside or
> outside this driver. There is a dev_dbg() print, but that is
> only visible when the developer explicitly compiles in debug
> support.
> 
> Don't make the code more complicated to preserve a feature that
> no-one is using. Remove all code associated with chip firmware
> version.
> 
> Link: https://lore.kernel.org/lkml/20191111181657.GA57214@dtor-ws/
> Cc: Marek Vasut <marex@denx.de>
> Cc: Adam Ford <aford173@gmail.com>
> Cc: <linux-kernel@vger.kernel.org>
> Cc: linux-input@vger.kernel.org
> Tree: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=next
> Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
> ---

The firmware version check does one relevant thing: It tests
the I2C communication, which tends to be useful for board
bringup and development boards (which often allow to disconnect
(touch-)screens).

-- Sebastian

>  drivers/input/touchscreen/ili210x.c | 21 ---------------------
>  1 file changed, 21 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
> index 0ed6014af6d7..a6feae5ce887 100644
> --- a/drivers/input/touchscreen/ili210x.c
> +++ b/drivers/input/touchscreen/ili210x.c
> @@ -21,15 +21,8 @@
>  /* Touchscreen commands */
>  #define REG_TOUCHDATA		0x10
>  #define REG_PANEL_INFO		0x20
> -#define REG_FIRMWARE_VERSION	0x40
>  #define REG_CALIBRATE		0xcc
>  
> -struct firmware_version {
> -	u8 id;
> -	u8 major;
> -	u8 minor;
> -} __packed;
> -
>  struct ili2xxx_chip {
>  	int (*read_reg)(struct i2c_client *client, u8 reg,
>  			void *buf, size_t len);
> @@ -342,7 +335,6 @@ static int ili210x_i2c_probe(struct i2c_client *client,
>  	struct ili210x *priv;
>  	struct gpio_desc *reset_gpio;
>  	struct input_dev *input;
> -	struct firmware_version firmware;
>  	int error;
>  
>  	dev_dbg(dev, "Probing for ILI210X I2C Touschreen driver");
> @@ -389,15 +381,6 @@ static int ili210x_i2c_probe(struct i2c_client *client,
>  	priv->chip = chip;
>  	i2c_set_clientdata(client, priv);
>  
> -	/* Get firmware version */
> -	error = chip->read_reg(client, REG_FIRMWARE_VERSION,
> -			       &firmware, sizeof(firmware));
> -	if (error) {
> -		dev_err(dev, "Failed to get firmware version, err: %d\n",
> -			error);
> -		return error;
> -	}
> -
>  	/* Setup input device */
>  	input->name = "ILI210x Touchscreen";
>  	input->id.bustype = BUS_I2C;
> @@ -439,10 +422,6 @@ static int ili210x_i2c_probe(struct i2c_client *client,
>  		return error;
>  	}
>  
> -	dev_dbg(dev,
> -		"ILI210x initialized (IRQ: %d), firmware version %d.%d.%d",
> -		client->irq, firmware.id, firmware.major, firmware.minor);
> -
>  	return 0;
>  }
>  
> -- 
> 2.17.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 1/3] Input: ili210x - do not retrieve/print chip firmware version

[Dmitry Torokhov]

On Tue, Nov 12, 2019 at 11:44:27AM -0500, Sven Van Asbroeck wrote:
> The driver's method to retrieve the firmware version on ili2117/
> ili2118 chip flavours is incorrect. The firmware version register
> address and layout are wrong.
> 
> The firmware version is not actually used anywhere inside or
> outside this driver. There is a dev_dbg() print, but that is
> only visible when the developer explicitly compiles in debug
> support.
> 
> Don't make the code more complicated to preserve a feature that
> no-one is using. Remove all code associated with chip firmware
> version.
> 
> Link: https://lore.kernel.org/lkml/20191111181657.GA57214@dtor-ws/
> Cc: Marek Vasut <marex@denx.de>
> Cc: Adam Ford <aford173@gmail.com>
> Cc: <linux-kernel@vger.kernel.org>
> Cc: linux-input@vger.kernel.org
> Tree: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=next
> Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>

Applied, thank you.

> ---
>  drivers/input/touchscreen/ili210x.c | 21 ---------------------
>  1 file changed, 21 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
> index 0ed6014af6d7..a6feae5ce887 100644
> --- a/drivers/input/touchscreen/ili210x.c
> +++ b/drivers/input/touchscreen/ili210x.c
> @@ -21,15 +21,8 @@
>  /* Touchscreen commands */
>  #define REG_TOUCHDATA		0x10
>  #define REG_PANEL_INFO		0x20
> -#define REG_FIRMWARE_VERSION	0x40
>  #define REG_CALIBRATE		0xcc
>  
> -struct firmware_version {
> -	u8 id;
> -	u8 major;
> -	u8 minor;
> -} __packed;
> -
>  struct ili2xxx_chip {
>  	int (*read_reg)(struct i2c_client *client, u8 reg,
>  			void *buf, size_t len);
> @@ -342,7 +335,6 @@ static int ili210x_i2c_probe(struct i2c_client *client,
>  	struct ili210x *priv;
>  	struct gpio_desc *reset_gpio;
>  	struct input_dev *input;
> -	struct firmware_version firmware;
>  	int error;
>  
>  	dev_dbg(dev, "Probing for ILI210X I2C Touschreen driver");
> @@ -389,15 +381,6 @@ static int ili210x_i2c_probe(struct i2c_client *client,
>  	priv->chip = chip;
>  	i2c_set_clientdata(client, priv);
>  
> -	/* Get firmware version */
> -	error = chip->read_reg(client, REG_FIRMWARE_VERSION,
> -			       &firmware, sizeof(firmware));
> -	if (error) {
> -		dev_err(dev, "Failed to get firmware version, err: %d\n",
> -			error);
> -		return error;
> -	}
> -
>  	/* Setup input device */
>  	input->name = "ILI210x Touchscreen";
>  	input->id.bustype = BUS_I2C;
> @@ -439,10 +422,6 @@ static int ili210x_i2c_probe(struct i2c_client *client,
>  		return error;
>  	}
>  
> -	dev_dbg(dev,
> -		"ILI210x initialized (IRQ: %d), firmware version %d.%d.%d",
> -		client->irq, firmware.id, firmware.major, firmware.minor);
> -
>  	return 0;
>  }
>  
> -- 
> 2.17.1
> 

-- 
Dmitry

Re: [PATCH v1 1/3] Input: ili210x - do not retrieve/print chip firmware version

[Dmitry Torokhov]

On Wed, Nov 13, 2019 at 12:34:21AM +0100, Sebastian Reichel wrote:
> Hi,
> 
> On Tue, Nov 12, 2019 at 11:44:27AM -0500, Sven Van Asbroeck wrote:
> > The driver's method to retrieve the firmware version on ili2117/
> > ili2118 chip flavours is incorrect. The firmware version register
> > address and layout are wrong.
> > 
> > The firmware version is not actually used anywhere inside or
> > outside this driver. There is a dev_dbg() print, but that is
> > only visible when the developer explicitly compiles in debug
> > support.
> > 
> > Don't make the code more complicated to preserve a feature that
> > no-one is using. Remove all code associated with chip firmware
> > version.
> > 
> > Link: https://lore.kernel.org/lkml/20191111181657.GA57214@dtor-ws/
> > Cc: Marek Vasut <marex@denx.de>
> > Cc: Adam Ford <aford173@gmail.com>
> > Cc: <linux-kernel@vger.kernel.org>
> > Cc: linux-input@vger.kernel.org
> > Tree: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=next
> > Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
> > ---
> 
> The firmware version check does one relevant thing: It tests
> the I2C communication, which tends to be useful for board
> bringup and development boards (which often allow to disconnect
> (touch-)screens).

If/when this is needed I propose we add a separate "lite" xfer check,
similar to what elants_i2c and many other drivers are doing.

Thanks.

-- 
Dmitry