Re: [PATCH v2 2/3] Input: ili210x - export ili251x version details via sysfs

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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@denx.de>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Joe Hung <joe_hung@ilitek.com>
> Cc: Luca Hsu <luca_hsu@ilitek.com>
> ---
> 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