On Fri, Aug 27, 2021 at 11:12:57PM +0200, Marek Vasut wrote:
> The ili251x firmware protocol permits readout of firmware version,
> protocol version, mcu version and current mode (application, boot
> loader, forced update). These information are useful when updating
> the firmware on the il251x, e.g. to avoid updating the same firmware
> into the device multiple times. The locking is now necessary to avoid
> races between interrupt handler and the sysfs readouts.
>
> Note that the protocol differs considerably between the ili2xxx devices,
> this patch therefore implements this functionality only for ili251x that
> I can test.
>
> Signed-off-by: Marek Vasut <marex@xxxxxxx>
> Cc: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> Cc: Joe Hung <joe_hung@xxxxxxxxxx>
> Cc: Luca Hsu <luca_hsu@xxxxxxxxxx>
> ---
> V2: No change
>
> NOTE: Regarding checkpatch warnings, Consider renaming function(s)
> 'ili251x_firmware_version_show' to 'firmware_version_show',
> I considered it and decided against it, because grepping for
> ili251x in debug symbols gives far more accurate results than
> grepping for firmware_version.
Yep, I completely agree here. I wish checkpatch did not complain about
this.
> ---
> drivers/input/touchscreen/ili210x.c | 130 +++++++++++++++++++++++++++-
> 1 file changed, 127 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
> index c46553ecabe6..7790ad000dc1 100644
> --- a/drivers/input/touchscreen/ili210x.c
> +++ b/drivers/input/touchscreen/ili210x.c
> @@ -22,6 +22,12 @@
> /* Touchscreen commands */
> #define REG_TOUCHDATA 0x10
> #define REG_PANEL_INFO 0x20
> +#define REG_FIRMWARE_VERSION 0x40
> +#define REG_PROTOCOL_VERSION 0x42
> +#define REG_KERNEL_VERSION 0x61
> +#define REG_IC_MODE 0xc0
> +#define REG_IC_MODE_AP 0x5a
> +#define REG_IC_MODE_BL 0x55
> #define REG_CALIBRATE 0xcc
>
> struct ili2xxx_chip {
> @@ -43,6 +49,7 @@ struct ili210x {
> struct input_dev *input;
> struct gpio_desc *reset_gpio;
> struct touchscreen_properties prop;
> + struct mutex lock;
> const struct ili2xxx_chip *chip;
> bool stop;
> };
> @@ -307,7 +314,9 @@ static irqreturn_t ili210x_irq(int irq, void *irq_data)
> int error;
>
> do {
> + mutex_lock(&priv->lock);
> error = chip->get_touch_data(client, touchdata);
> + mutex_unlock(&priv->lock);
> if (error) {
> dev_err(&client->dev,
> "Unable to get touch data: %d\n", error);
> @@ -351,6 +360,108 @@ static int ili251x_firmware_update_resolution(struct device *dev)
> return 0;
> }
>
> +static ssize_t ili251x_firmware_version_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct ili210x *priv = i2c_get_clientdata(client);
> + u8 fw[8];
> + int ret;
> +
> + /* Get firmware version */
> + mutex_lock(&priv->lock);
> + ret = priv->chip->read_reg(client, REG_FIRMWARE_VERSION,
> + &fw, sizeof(fw));
> + mutex_unlock(&priv->lock);
Could we query firmware version and mode at probe time (and also later
after firmware update attempt) so that we do not need to introduce
locking against interrupt handler?
> +
> + if (ret)
> + return ret;
> +
> + if (!ret) {
> + ret = scnprintf(buf, PAGE_SIZE,
> + "%02x%02x.%02x%02x.%02x%02x.%02x%02x\n",
> + fw[0], fw[1], fw[2], fw[3],
> + fw[4], fw[5], fw[6], fw[7]);
> + }
I think there is sysfs_emit() that is preferred now.
Thanks.
--
Dmitry
