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 <[email protected]>
Cc: Adam Ford <[email protected]>
Cc: <[email protected]>
Cc: [email protected]
Tree: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=next
Signed-off-by: Sven Van Asbroeck <[email protected]>
---
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
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 <[email protected]>
Cc: Adam Ford <[email protected]>
Cc: <[email protected]>
Cc: [email protected]
Tree: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=next
Signed-off-by: Sven Van Asbroeck <[email protected]>
---
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
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 <[email protected]>
> Cc: Adam Ford <[email protected]>
> Cc: <[email protected]>
> Cc: [email protected]
> Tree: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=next
> Signed-off-by: Sven Van Asbroeck <[email protected]>
> ---
> 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
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 <[email protected]>
> Cc: Adam Ford <[email protected]>
> Cc: <[email protected]>
> Cc: [email protected]
> Tree: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=next
> Signed-off-by: Sven Van Asbroeck <[email protected]>
> ---
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
>
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 <[email protected]>
> Cc: Adam Ford <[email protected]>
> Cc: <[email protected]>
> Cc: [email protected]
> Tree: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=next
> Signed-off-by: Sven Van Asbroeck <[email protected]>
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
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 <[email protected]>
> > Cc: Adam Ford <[email protected]>
> > Cc: <[email protected]>
> > Cc: [email protected]
> > Tree: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=next
> > Signed-off-by: Sven Van Asbroeck <[email protected]>
> > ---
>
> 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