| Message ID | 20191112164429.11225-1-TheSven73@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c7dded5b2980de9811f47a1a576e0596a5a775ce |
| Headers | show |
| Series | [v1,1/3] Input: ili210x - do not retrieve/print chip firmware version | expand |
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 >
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 >
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.
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; }
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(-)