[PATCH 1/4] Input: exc3000: split MT event handling from IRQ handler

[PATCH 1/4] Input: exc3000: split MT event handling from IRQ handler

[Lucas Stach]

Split out the multitouch event handling into its own function to allow other
events to be handled in the IRQ handler without disturbing the MT handling.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/input/touchscreen/exc3000.c | 92 +++++++++++++++++------------
 1 file changed, 54 insertions(+), 38 deletions(-)

diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
index e007e2e8f626..3458d02310dd 100644
--- a/drivers/input/touchscreen/exc3000.c
+++ b/drivers/input/touchscreen/exc3000.c
@@ -58,6 +58,11 @@ static void exc3000_timer(struct timer_list *t)
 	input_sync(data->input);
 }
 
+static inline void exc3000_schedule_timer(struct exc3000_data *data)
+{
+	mod_timer(&data->timer, jiffies + msecs_to_jiffies(EXC3000_TIMEOUT_MS));
+}
+
 static int exc3000_read_frame(struct i2c_client *client, u8 *buf)
 {
 	int ret;
@@ -76,54 +81,35 @@ static int exc3000_read_frame(struct i2c_client *client, u8 *buf)
 	if (ret != EXC3000_LEN_FRAME)
 		return -EIO;
 
-	if (get_unaligned_le16(buf) != EXC3000_LEN_FRAME ||
-			buf[2] != EXC3000_MT_EVENT)
+	if (get_unaligned_le16(buf) != EXC3000_LEN_FRAME)
 		return -EINVAL;
 
 	return 0;
 }
 
-static int exc3000_read_data(struct i2c_client *client,
-			     u8 *buf, int *n_slots)
+static int exc3000_handle_mt_event(struct exc3000_data *data)
 {
-	int error;
-
-	error = exc3000_read_frame(client, buf);
-	if (error)
-		return error;
+	struct input_dev *input = data->input;
+	int ret, total_slots;
+	u8 *buf = data->buf;
 
-	*n_slots = buf[3];
-	if (!*n_slots || *n_slots > EXC3000_NUM_SLOTS)
-		return -EINVAL;
+	total_slots = buf[3];
+	if (!total_slots || total_slots > EXC3000_NUM_SLOTS) {
+		ret = -EINVAL;
+		goto out_fail;
+	}
 
-	if (*n_slots > EXC3000_SLOTS_PER_FRAME) {
+	if (total_slots > EXC3000_SLOTS_PER_FRAME) {
 		/* Read 2nd frame to get the rest of the contacts. */
-		error = exc3000_read_frame(client, buf + EXC3000_LEN_FRAME);
-		if (error)
-			return error;
+		ret = exc3000_read_frame(data->client, buf + EXC3000_LEN_FRAME);
+		if (ret)
+			goto out_fail;
 
 		/* 2nd chunk must have number of contacts set to 0. */
-		if (buf[EXC3000_LEN_FRAME + 3] != 0)
-			return -EINVAL;
-	}
-
-	return 0;
-}
-
-static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
-{
-	struct exc3000_data *data = dev_id;
-	struct input_dev *input = data->input;
-	u8 *buf = data->buf;
-	int slots, total_slots;
-	int error;
-
-	error = exc3000_read_data(data->client, buf, &total_slots);
-	if (error) {
-		/* Schedule a timer to release "stuck" contacts */
-		mod_timer(&data->timer,
-			  jiffies + msecs_to_jiffies(EXC3000_TIMEOUT_MS));
-		goto out;
+		if (buf[EXC3000_LEN_FRAME + 3] != 0) {
+			ret = -EINVAL;
+			goto out_fail;
+		}
 	}
 
 	/*
@@ -132,7 +118,7 @@ static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
 	del_timer_sync(&data->timer);
 
 	while (total_slots > 0) {
-		slots = min(total_slots, EXC3000_SLOTS_PER_FRAME);
+		int slots = min(total_slots, EXC3000_SLOTS_PER_FRAME);
 		exc3000_report_slots(input, &data->prop, buf + 4, slots);
 		total_slots -= slots;
 		buf += EXC3000_LEN_FRAME;
@@ -141,6 +127,36 @@ static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
 	input_mt_sync_frame(input);
 	input_sync(input);
 
+	return 0;
+
+out_fail:
+	/* Schedule a timer to release "stuck" contacts */
+	exc3000_schedule_timer(data);
+
+	return ret;
+}
+
+static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
+{
+	struct exc3000_data *data = dev_id;
+	u8 *buf = data->buf;
+	int ret;
+
+	ret = exc3000_read_frame(data->client, buf);
+	if (ret) {
+		/* Schedule a timer to release "stuck" contacts */
+		exc3000_schedule_timer(data);
+		goto out;
+	}
+
+	switch (buf[2]) {
+		case EXC3000_MT_EVENT:
+			exc3000_handle_mt_event(data);
+			break;
+		default:
+			break;
+	}
+
 out:
 	return IRQ_HANDLED;
 }
-- 
2.20.1

[PATCH 2/4] Input: exc3000: query and show type, model and firmware revision info

[Lucas Stach]

It's very useful to have this information in the log, as differences in
behavior can be tied to the controller firmware.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/input/touchscreen/exc3000.c | 116 ++++++++++++++++++++++++++--
 1 file changed, 110 insertions(+), 6 deletions(-)

diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
index 3458d02310dd..accee4fd1b97 100644
--- a/drivers/input/touchscreen/exc3000.c
+++ b/drivers/input/touchscreen/exc3000.c
@@ -22,7 +22,9 @@
 #define EXC3000_NUM_SLOTS		10
 #define EXC3000_SLOTS_PER_FRAME		5
 #define EXC3000_LEN_FRAME		66
+#define EXC3000_LEN_VENDOR_REQUEST	68
 #define EXC3000_LEN_POINT		10
+#define EXC3000_VENDOR_EVENT		3
 #define EXC3000_MT_EVENT		6
 #define EXC3000_TIMEOUT_MS		100
 
@@ -32,6 +34,9 @@ struct exc3000_data {
 	struct touchscreen_properties prop;
 	struct timer_list timer;
 	u8 buf[2 * EXC3000_LEN_FRAME];
+	struct mutex vendor_data_lock;
+	struct completion vendor_data_done;
+	char *type, *model, *fw_rev;
 };
 
 static void exc3000_report_slots(struct input_dev *input,
@@ -136,6 +141,13 @@ static int exc3000_handle_mt_event(struct exc3000_data *data)
 	return ret;
 }
 
+static int exc3000_handle_vendor_event(struct exc3000_data *data)
+{
+	complete(&data->vendor_data_done);
+
+	return 0;
+}
+
 static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
 {
 	struct exc3000_data *data = dev_id;
@@ -153,6 +165,9 @@ static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
 		case EXC3000_MT_EVENT:
 			exc3000_handle_mt_event(data);
 			break;
+		case EXC3000_VENDOR_EVENT:
+			exc3000_handle_vendor_event(data);
+			break;
 		default:
 			break;
 	}
@@ -161,6 +176,89 @@ static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int exc3000_vendor_data_request(struct exc3000_data *data, u8 *request,
+				       u8 request_len, u8 *response)
+{
+	u8 buf[EXC3000_LEN_VENDOR_REQUEST] = { 0x67, 0x00, 0x42, 0x00, 0x03 };
+	int ret;
+
+	mutex_lock(&data->vendor_data_lock);
+
+	reinit_completion(&data->vendor_data_done);
+
+	buf[5] = request_len;
+	memcpy(&buf[6], request, request_len);
+
+	ret = i2c_master_send(data->client, buf, EXC3000_LEN_VENDOR_REQUEST);
+	if (ret < 0)
+		goto out_unlock;
+
+	if (response) {
+		wait_for_completion(&data->vendor_data_done);
+		memcpy(response, &data->buf[4], data->buf[3]);
+		ret = data->buf[3];
+	}
+
+out_unlock:
+	mutex_unlock(&data->vendor_data_lock);
+
+	return ret;
+}
+static int exc3000_populate_device_info(struct exc3000_data *data)
+{
+	struct device *dev = &data->client->dev;
+	u8 response[EXC3000_LEN_FRAME];
+	int ret;
+
+	/* query type info */
+	ret = exc3000_vendor_data_request(data, (u8[]){0x46}, 1, response);
+	if (ret < 0)
+		return -ENODEV;
+
+	data->type = devm_kmemdup(dev, &response[1], ret - 1, GFP_KERNEL);
+
+	/* query model info */
+	ret = exc3000_vendor_data_request(data, (u8[]){0x45}, 1, response);
+	if (ret < 0)
+		return -ENODEV;
+
+	data->model = devm_kmemdup(dev, &response[1], ret - 1, GFP_KERNEL);
+
+	/* query bootloader info */
+	ret = exc3000_vendor_data_request(data,
+					  (u8[]){0x39, 0x02}, 2, response);
+	if (ret < 0)
+		return -ENODEV;
+
+	/*
+	 * If the bootloader version is non-zero then the device is in
+	 * bootloader mode and won't answer a query for the application FW
+	 * version, so we just use the bootloader version info.
+	 */
+	if (response[2] || response[3]) {
+		char bl_version[8];
+
+		snprintf(bl_version, 8, "%d.%d", response[2], response[3]);
+		data->fw_rev = devm_kmemdup(dev, bl_version,
+					    strlen(bl_version), GFP_KERNEL);
+	} else {
+		/* query application firmware version */
+		ret = exc3000_vendor_data_request(data,
+						  (u8[]){0x44}, 1, response);
+		if (ret < 0)
+			return -ENODEV;
+
+		data->fw_rev = devm_kmemdup(dev, &response[1],
+					    ret - 1, GFP_KERNEL);
+	}
+
+	dev_info(&data->client->dev,
+		 "found type %s, model %s, firmware revision %s",
+		 data->type, data->model, data->fw_rev);
+
+	return 0;
+}
+
 static int exc3000_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -174,6 +272,18 @@ static int exc3000_probe(struct i2c_client *client,
 
 	data->client = client;
 	timer_setup(&data->timer, exc3000_timer, 0);
+	mutex_init(&data->vendor_data_lock);
+	init_completion(&data->vendor_data_done);
+
+	error = devm_request_threaded_irq(&client->dev, client->irq,
+					  NULL, exc3000_interrupt, IRQF_ONESHOT,
+					  client->name, data);
+	if (error)
+		return error;
+
+	error = exc3000_populate_device_info(data);
+	if (error)
+		return error;
 
 	input = devm_input_allocate_device(&client->dev);
 	if (!input)
@@ -197,12 +307,6 @@ static int exc3000_probe(struct i2c_client *client,
 	if (error)
 		return error;
 
-	error = devm_request_threaded_irq(&client->dev, client->irq,
-					  NULL, exc3000_interrupt, IRQF_ONESHOT,
-					  client->name, data);
-	if (error)
-		return error;
-
 	return 0;
 }
 
-- 
2.20.1

Re: [PATCH 2/4] Input: exc3000: query and show type, model and firmware revision info

[Dmitry Torokhov]

Hi Lucas,

On Tue, Jan 07, 2020 at 06:17:38PM +0100, Lucas Stach wrote:
> It's very useful to have this information in the log, as differences in
> behavior can be tied to the controller firmware.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/input/touchscreen/exc3000.c | 116 ++++++++++++++++++++++++++--
>  1 file changed, 110 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
> index 3458d02310dd..accee4fd1b97 100644
> --- a/drivers/input/touchscreen/exc3000.c
> +++ b/drivers/input/touchscreen/exc3000.c
> @@ -22,7 +22,9 @@
>  #define EXC3000_NUM_SLOTS		10
>  #define EXC3000_SLOTS_PER_FRAME		5
>  #define EXC3000_LEN_FRAME		66
> +#define EXC3000_LEN_VENDOR_REQUEST	68
>  #define EXC3000_LEN_POINT		10
> +#define EXC3000_VENDOR_EVENT		3
>  #define EXC3000_MT_EVENT		6
>  #define EXC3000_TIMEOUT_MS		100
>  
> @@ -32,6 +34,9 @@ struct exc3000_data {
>  	struct touchscreen_properties prop;
>  	struct timer_list timer;
>  	u8 buf[2 * EXC3000_LEN_FRAME];
> +	struct mutex vendor_data_lock;
> +	struct completion vendor_data_done;
> +	char *type, *model, *fw_rev;
>  };
>  
>  static void exc3000_report_slots(struct input_dev *input,
> @@ -136,6 +141,13 @@ static int exc3000_handle_mt_event(struct exc3000_data *data)
>  	return ret;
>  }
>  
> +static int exc3000_handle_vendor_event(struct exc3000_data *data)
> +{
> +	complete(&data->vendor_data_done);
> +
> +	return 0;
> +}
> +
>  static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
>  {
>  	struct exc3000_data *data = dev_id;
> @@ -153,6 +165,9 @@ static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
>  		case EXC3000_MT_EVENT:
>  			exc3000_handle_mt_event(data);
>  			break;
> +		case EXC3000_VENDOR_EVENT:
> +			exc3000_handle_vendor_event(data);
> +			break;
>  		default:
>  			break;
>  	}
> @@ -161,6 +176,89 @@ static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static int exc3000_vendor_data_request(struct exc3000_data *data, u8 *request,
> +				       u8 request_len, u8 *response)
> +{
> +	u8 buf[EXC3000_LEN_VENDOR_REQUEST] = { 0x67, 0x00, 0x42, 0x00, 0x03 };
> +	int ret;
> +
> +	mutex_lock(&data->vendor_data_lock);
> +
> +	reinit_completion(&data->vendor_data_done);
> +
> +	buf[5] = request_len;
> +	memcpy(&buf[6], request, request_len);
> +
> +	ret = i2c_master_send(data->client, buf, EXC3000_LEN_VENDOR_REQUEST);
> +	if (ret < 0)
> +		goto out_unlock;
> +
> +	if (response) {
> +		wait_for_completion(&data->vendor_data_done);

You want some kind of timeout here to avoid hanging forever if device
does not respond properly.

> +		memcpy(response, &data->buf[4], data->buf[3]);
> +		ret = data->buf[3];
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&data->vendor_data_lock);
> +
> +	return ret;
> +}
> +static int exc3000_populate_device_info(struct exc3000_data *data)
> +{
> +	struct device *dev = &data->client->dev;
> +	u8 response[EXC3000_LEN_FRAME];
> +	int ret;
> +
> +	/* query type info */
> +	ret = exc3000_vendor_data_request(data, (u8[]){0x46}, 1, response);
> +	if (ret < 0)
> +		return -ENODEV;
> +
> +	data->type = devm_kmemdup(dev, &response[1], ret - 1, GFP_KERNEL);
> +
> +	/* query model info */
> +	ret = exc3000_vendor_data_request(data, (u8[]){0x45}, 1, response);
> +	if (ret < 0)
> +		return -ENODEV;
> +
> +	data->model = devm_kmemdup(dev, &response[1], ret - 1, GFP_KERNEL);
> +
> +	/* query bootloader info */
> +	ret = exc3000_vendor_data_request(data,
> +					  (u8[]){0x39, 0x02}, 2, response);
> +	if (ret < 0)
> +		return -ENODEV;
> +
> +	/*
> +	 * If the bootloader version is non-zero then the device is in
> +	 * bootloader mode and won't answer a query for the application FW
> +	 * version, so we just use the bootloader version info.
> +	 */
> +	if (response[2] || response[3]) {
> +		char bl_version[8];
> +
> +		snprintf(bl_version, 8, "%d.%d", response[2], response[3]);
> +		data->fw_rev = devm_kmemdup(dev, bl_version,
> +					    strlen(bl_version), GFP_KERNEL);

Error handing?

> +	} else {
> +		/* query application firmware version */
> +		ret = exc3000_vendor_data_request(data,
> +						  (u8[]){0x44}, 1, response);
> +		if (ret < 0)
> +			return -ENODEV;
> +
> +		data->fw_rev = devm_kmemdup(dev, &response[1],
> +					    ret - 1, GFP_KERNEL);

Error handling?

> +	}
> +
> +	dev_info(&data->client->dev,
> +		 "found type %s, model %s, firmware revision %s",
> +		 data->type, data->model, data->fw_rev);

dev_dbg()

> +
> +	return 0;
> +}
> +
>  static int exc3000_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -174,6 +272,18 @@ static int exc3000_probe(struct i2c_client *client,
>  
>  	data->client = client;
>  	timer_setup(&data->timer, exc3000_timer, 0);
> +	mutex_init(&data->vendor_data_lock);
> +	init_completion(&data->vendor_data_done);
> +
> +	error = devm_request_threaded_irq(&client->dev, client->irq,
> +					  NULL, exc3000_interrupt, IRQF_ONESHOT,
> +					  client->name, data);
> +	if (error)
> +		return error;

Since you moved it to be earlier than input device allocation I think it
will bomb if you boot when your finger touches the screen as it will try
to dereference NULL pointer.

It is usually OK to send events through allocated
but not yet registered input device, but I'd double-checked that
touchscreen_report_pos() API is also safe, and then moved input device
allocation to happen before you request interrupt.

> +
> +	error = exc3000_populate_device_info(data);
> +	if (error)
> +		return error;
>  
>  	input = devm_input_allocate_device(&client->dev);
>  	if (!input)
> @@ -197,12 +307,6 @@ static int exc3000_probe(struct i2c_client *client,
>  	if (error)
>  		return error;
>  
> -	error = devm_request_threaded_irq(&client->dev, client->irq,
> -					  NULL, exc3000_interrupt, IRQF_ONESHOT,
> -					  client->name, data);
> -	if (error)
> -		return error;
> -
>  	return 0;
>  }
>  
> -- 
> 2.20.1
> 

Thanks.

-- 
Dmitry

Re: [PATCH 2/4] Input: exc3000: query and show type, model and firmware revision info

[Lucas Stach]

Hi Dmitry,

On Mo, 2020-01-20 at 11:49 -0800, Dmitry Torokhov wrote:
> Hi Lucas,
> 
> On Tue, Jan 07, 2020 at 06:17:38PM +0100, Lucas Stach wrote:
> > It's very useful to have this information in the log, as differences in
> > behavior can be tied to the controller firmware.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/input/touchscreen/exc3000.c | 116 ++++++++++++++++++++++++++--
> >  1 file changed, 110 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
> > index 3458d02310dd..accee4fd1b97 100644
> > --- a/drivers/input/touchscreen/exc3000.c
> > +++ b/drivers/input/touchscreen/exc3000.c
> > @@ -22,7 +22,9 @@
> >  #define EXC3000_NUM_SLOTS		10
> >  #define EXC3000_SLOTS_PER_FRAME		5
> >  #define EXC3000_LEN_FRAME		66
> > +#define EXC3000_LEN_VENDOR_REQUEST	68
> >  #define EXC3000_LEN_POINT		10
> > +#define EXC3000_VENDOR_EVENT		3
> >  #define EXC3000_MT_EVENT		6
> >  #define EXC3000_TIMEOUT_MS		100
> >  
> > @@ -32,6 +34,9 @@ struct exc3000_data {
> >  	struct touchscreen_properties prop;
> >  	struct timer_list timer;
> >  	u8 buf[2 * EXC3000_LEN_FRAME];
> > +	struct mutex vendor_data_lock;
> > +	struct completion vendor_data_done;
> > +	char *type, *model, *fw_rev;
> >  };
> >  
> >  static void exc3000_report_slots(struct input_dev *input,
> > @@ -136,6 +141,13 @@ static int exc3000_handle_mt_event(struct exc3000_data *data)
> >  	return ret;
> >  }
> >  
> > +static int exc3000_handle_vendor_event(struct exc3000_data *data)
> > +{
> > +	complete(&data->vendor_data_done);
> > +
> > +	return 0;
> > +}
> > +
> >  static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
> >  {
> >  	struct exc3000_data *data = dev_id;
> > @@ -153,6 +165,9 @@ static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
> >  		case EXC3000_MT_EVENT:
> >  			exc3000_handle_mt_event(data);
> >  			break;
> > +		case EXC3000_VENDOR_EVENT:
> > +			exc3000_handle_vendor_event(data);
> > +			break;
> >  		default:
> >  			break;
> >  	}
> > @@ -161,6 +176,89 @@ static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
> >  	return IRQ_HANDLED;
> >  }
> >  
> > +static int exc3000_vendor_data_request(struct exc3000_data *data, u8 *request,
> > +				       u8 request_len, u8 *response)
> > +{
> > +	u8 buf[EXC3000_LEN_VENDOR_REQUEST] = { 0x67, 0x00, 0x42, 0x00, 0x03 };
> > +	int ret;
> > +
> > +	mutex_lock(&data->vendor_data_lock);
> > +
> > +	reinit_completion(&data->vendor_data_done);
> > +
> > +	buf[5] = request_len;
> > +	memcpy(&buf[6], request, request_len);
> > +
> > +	ret = i2c_master_send(data->client, buf, EXC3000_LEN_VENDOR_REQUEST);
> > +	if (ret < 0)
> > +		goto out_unlock;
> > +
> > +	if (response) {
> > +		wait_for_completion(&data->vendor_data_done);
> 
> You want some kind of timeout here to avoid hanging forever if device
> does not respond properly.
> 
> > +		memcpy(response, &data->buf[4], data->buf[3]);
> > +		ret = data->buf[3];
> > +	}
> > +
> > +out_unlock:
> > +	mutex_unlock(&data->vendor_data_lock);
> > +
> > +	return ret;
> > +}
> > +static int exc3000_populate_device_info(struct exc3000_data *data)
> > +{
> > +	struct device *dev = &data->client->dev;
> > +	u8 response[EXC3000_LEN_FRAME];
> > +	int ret;
> > +
> > +	/* query type info */
> > +	ret = exc3000_vendor_data_request(data, (u8[]){0x46}, 1, response);
> > +	if (ret < 0)
> > +		return -ENODEV;
> > +
> > +	data->type = devm_kmemdup(dev, &response[1], ret - 1, GFP_KERNEL);
> > +
> > +	/* query model info */
> > +	ret = exc3000_vendor_data_request(data, (u8[]){0x45}, 1, response);
> > +	if (ret < 0)
> > +		return -ENODEV;
> > +
> > +	data->model = devm_kmemdup(dev, &response[1], ret - 1, GFP_KERNEL);
> > +
> > +	/* query bootloader info */
> > +	ret = exc3000_vendor_data_request(data,
> > +					  (u8[]){0x39, 0x02}, 2, response);
> > +	if (ret < 0)
> > +		return -ENODEV;
> > +
> > +	/*
> > +	 * If the bootloader version is non-zero then the device is in
> > +	 * bootloader mode and won't answer a query for the application FW
> > +	 * version, so we just use the bootloader version info.
> > +	 */
> > +	if (response[2] || response[3]) {
> > +		char bl_version[8];
> > +
> > +		snprintf(bl_version, 8, "%d.%d", response[2], response[3]);
> > +		data->fw_rev = devm_kmemdup(dev, bl_version,
> > +					    strlen(bl_version), GFP_KERNEL);
> 
> Error handing?
> 
> > +	} else {
> > +		/* query application firmware version */
> > +		ret = exc3000_vendor_data_request(data,
> > +						  (u8[]){0x44}, 1, response);
> > +		if (ret < 0)
> > +			return -ENODEV;
> > +
> > +		data->fw_rev = devm_kmemdup(dev, &response[1],
> > +					    ret - 1, GFP_KERNEL);
> 
> Error handling?
> 
> > +	}
> > +
> > +	dev_info(&data->client->dev,
> > +		 "found type %s, model %s, firmware revision %s",
> > +		 data->type, data->model, data->fw_rev);
> 
> dev_dbg()

This one I kept as-is, as I really want to have this information in the
system log. It's a single line and carries quite important information
when trying to understand touchscreen issues, so IMHO the signal to
noise ratio is good enough to keep it at the info level.

All other comments have been fixed in v2.

Regards,
Lucas

> > +
> > +	return 0;
> > +}
> > +
> >  static int exc3000_probe(struct i2c_client *client,
> >  			 const struct i2c_device_id *id)
> >  {
> > @@ -174,6 +272,18 @@ static int exc3000_probe(struct i2c_client *client,
> >  
> >  	data->client = client;
> >  	timer_setup(&data->timer, exc3000_timer, 0);
> > +	mutex_init(&data->vendor_data_lock);
> > +	init_completion(&data->vendor_data_done);
> > +
> > +	error = devm_request_threaded_irq(&client->dev, client->irq,
> > +					  NULL, exc3000_interrupt, IRQF_ONESHOT,
> > +					  client->name, data);
> > +	if (error)
> > +		return error;
> 
> Since you moved it to be earlier than input device allocation I think it
> will bomb if you boot when your finger touches the screen as it will try
> to dereference NULL pointer.
> 
> It is usually OK to send events through allocated
> but not yet registered input device, but I'd double-checked that
> touchscreen_report_pos() API is also safe, and then moved input device
> allocation to happen before you request interrupt.
> 
> > +
> > +	error = exc3000_populate_device_info(data);
> > +	if (error)
> > +		return error;
> >  
> >  	input = devm_input_allocate_device(&client->dev);
> >  	if (!input)
> > @@ -197,12 +307,6 @@ static int exc3000_probe(struct i2c_client *client,
> >  	if (error)
> >  		return error;
> >  
> > -	error = devm_request_threaded_irq(&client->dev, client->irq,
> > -					  NULL, exc3000_interrupt, IRQF_ONESHOT,
> > -					  client->name, data);
> > -	if (error)
> > -		return error;
> > -
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.20.1
> > 
> 
> Thanks.
> 

[PATCH 3/4] Input: exc3000: expose type, model and firmware revision as sysfs attributes

[Lucas Stach]

This can be used by userspace to determine if a firmware update should be
started.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/input/touchscreen/exc3000.c | 54 +++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
index accee4fd1b97..ce83914d65ff 100644
--- a/drivers/input/touchscreen/exc3000.c
+++ b/drivers/input/touchscreen/exc3000.c
@@ -259,6 +259,47 @@ static int exc3000_populate_device_info(struct exc3000_data *data)
 	return 0;
 }
 
+static ssize_t exc3000_sysfs_type_show(struct device *dev,
+				       struct device_attribute *dattr,
+				       char *buf)
+{
+	struct exc3000_data *data = dev_get_drvdata(dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%s\n", data->type);
+}
+static DEVICE_ATTR(type, 0444, exc3000_sysfs_type_show, NULL);
+
+static ssize_t exc3000_sysfs_model_show(struct device *dev,
+					struct device_attribute *dattr,
+					char *buf)
+{
+	struct exc3000_data *data = dev_get_drvdata(dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%s\n", data->model);
+}
+static DEVICE_ATTR(model, 0444, exc3000_sysfs_model_show, NULL);
+
+static ssize_t exc3000_sysfs_fw_rev_show(struct device *dev,
+					 struct device_attribute *dattr,
+					 char *buf)
+{
+	struct exc3000_data *data = dev_get_drvdata(dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%s\n", data->fw_rev);
+}
+static DEVICE_ATTR(fw_rev, 0444, exc3000_sysfs_fw_rev_show, NULL);
+
+static struct attribute *exc3000_attrs[] = {
+	&dev_attr_type.attr,
+	&dev_attr_model.attr,
+	&dev_attr_fw_rev.attr,
+	NULL
+};
+
+static const struct attribute_group exc3000_attr_group = {
+	.attrs = exc3000_attrs,
+};
+
 static int exc3000_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -285,6 +326,11 @@ static int exc3000_probe(struct i2c_client *client,
 	if (error)
 		return error;
 
+	dev_set_drvdata(&client->dev, data);
+	error = sysfs_create_group(&client->dev.kobj, &exc3000_attr_group);
+	if (error)
+		return error;
+
 	input = devm_input_allocate_device(&client->dev);
 	if (!input)
 		return -ENOMEM;
@@ -310,6 +356,13 @@ static int exc3000_probe(struct i2c_client *client,
 	return 0;
 }
 
+int exc3000_remove(struct i2c_client *client)
+{
+	sysfs_remove_group(&client->dev.kobj, &exc3000_attr_group);
+
+	return 0;
+}
+
 static const struct i2c_device_id exc3000_id[] = {
 	{ "exc3000", 0 },
 	{ }
@@ -331,6 +384,7 @@ static struct i2c_driver exc3000_driver = {
 	},
 	.id_table	= exc3000_id,
 	.probe		= exc3000_probe,
+	.remove		= exc3000_remove,
 };
 
 module_i2c_driver(exc3000_driver);
-- 
2.20.1

Re: [PATCH 3/4] Input: exc3000: expose type, model and firmware revision as sysfs attributes

[Dmitry Torokhov]

On Tue, Jan 07, 2020 at 06:17:39PM +0100, Lucas Stach wrote:
> This can be used by userspace to determine if a firmware update should be
> started.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/input/touchscreen/exc3000.c | 54 +++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
> index accee4fd1b97..ce83914d65ff 100644
> --- a/drivers/input/touchscreen/exc3000.c
> +++ b/drivers/input/touchscreen/exc3000.c
> @@ -259,6 +259,47 @@ static int exc3000_populate_device_info(struct exc3000_data *data)
>  	return 0;
>  }
>  
> +static ssize_t exc3000_sysfs_type_show(struct device *dev,
> +				       struct device_attribute *dattr,
> +				       char *buf)
> +{
> +	struct exc3000_data *data = dev_get_drvdata(dev);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%s\n", data->type);
> +}
> +static DEVICE_ATTR(type, 0444, exc3000_sysfs_type_show, NULL);
> +
> +static ssize_t exc3000_sysfs_model_show(struct device *dev,
> +					struct device_attribute *dattr,
> +					char *buf)
> +{
> +	struct exc3000_data *data = dev_get_drvdata(dev);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%s\n", data->model);
> +}
> +static DEVICE_ATTR(model, 0444, exc3000_sysfs_model_show, NULL);
> +
> +static ssize_t exc3000_sysfs_fw_rev_show(struct device *dev,
> +					 struct device_attribute *dattr,
> +					 char *buf)
> +{
> +	struct exc3000_data *data = dev_get_drvdata(dev);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%s\n", data->fw_rev);
> +}
> +static DEVICE_ATTR(fw_rev, 0444, exc3000_sysfs_fw_rev_show, NULL);
> +
> +static struct attribute *exc3000_attrs[] = {
> +	&dev_attr_type.attr,
> +	&dev_attr_model.attr,
> +	&dev_attr_fw_rev.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group exc3000_attr_group = {
> +	.attrs = exc3000_attrs,
> +};
> +
>  static int exc3000_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -285,6 +326,11 @@ static int exc3000_probe(struct i2c_client *client,
>  	if (error)
>  		return error;
>  
> +	dev_set_drvdata(&client->dev, data);
> +	error = sysfs_create_group(&client->dev.kobj, &exc3000_attr_group);
> +	if (error)
> +		return error;

Please use devm_device_add_group() instead.

> +
>  	input = devm_input_allocate_device(&client->dev);
>  	if (!input)
>  		return -ENOMEM;
> @@ -310,6 +356,13 @@ static int exc3000_probe(struct i2c_client *client,
>  	return 0;
>  }
>  
> +int exc3000_remove(struct i2c_client *client)
> +{
> +	sysfs_remove_group(&client->dev.kobj, &exc3000_attr_group);
> +
> +	return 0;
> +}
> +
>  static const struct i2c_device_id exc3000_id[] = {
>  	{ "exc3000", 0 },
>  	{ }
> @@ -331,6 +384,7 @@ static struct i2c_driver exc3000_driver = {
>  	},
>  	.id_table	= exc3000_id,
>  	.probe		= exc3000_probe,
> +	.remove		= exc3000_remove,
>  };
>  
>  module_i2c_driver(exc3000_driver);
> -- 
> 2.20.1
> 

Thanks.

-- 
Dmitry

[PATCH 4/4] Input: exc3000: add firmware update support

[Lucas Stach]

This change allows the device firmware to be updated by putting a firmware
file in /lib/firmware and providing the name of the file via the update_fw
sysfs property. The driver will then flash the firmware image into the
controller internal storage and restart the controller to activate the new
firmware.

The implementation was done by looking at the the messages passed between
the controller and proprietary vendor update tool. Not every detail of the
protocol is totally well understood, so the implementation still has some
"monkey see, monkey do" parts, as far as they have been found to be required
for the update to succeed.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/input/touchscreen/exc3000.c | 248 +++++++++++++++++++++++++++-
 1 file changed, 246 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
index ce83914d65ff..f9a9820dc232 100644
--- a/drivers/input/touchscreen/exc3000.c
+++ b/drivers/input/touchscreen/exc3000.c
@@ -3,8 +3,8 @@
  * Driver for I2C connected EETI EXC3000 multiple touch controller
  *
  * Copyright (C) 2017 Ahmet Inan <inan@distec.de>
- *
- * minimal implementation based on egalax_ts.c and egalax_i2c.c
+ * Copyright (C) 2019 Pengutronix <kernel@pengutronix.de>
+ * Copyright (C) 2019 Zodiac Inflight Innovations
  */
 
 #include <linux/bitops.h>
@@ -18,6 +18,8 @@
 #include <linux/of.h>
 #include <linux/timer.h>
 #include <asm/unaligned.h>
+#include <linux/firmware.h>
+#include <linux/delay.h>
 
 #define EXC3000_NUM_SLOTS		10
 #define EXC3000_SLOTS_PER_FRAME		5
@@ -37,6 +39,7 @@ struct exc3000_data {
 	struct mutex vendor_data_lock;
 	struct completion vendor_data_done;
 	char *type, *model, *fw_rev;
+	int update_status;
 };
 
 static void exc3000_report_slots(struct input_dev *input,
@@ -215,6 +218,8 @@ static int exc3000_populate_device_info(struct exc3000_data *data)
 	if (ret < 0)
 		return -ENODEV;
 
+	if (data->type)
+		devm_kfree(dev, data->type);
 	data->type = devm_kmemdup(dev, &response[1], ret - 1, GFP_KERNEL);
 
 	/* query model info */
@@ -222,6 +227,8 @@ static int exc3000_populate_device_info(struct exc3000_data *data)
 	if (ret < 0)
 		return -ENODEV;
 
+	if (data->model)
+		devm_kfree(dev, data->model);
 	data->model = devm_kmemdup(dev, &response[1], ret - 1, GFP_KERNEL);
 
 	/* query bootloader info */
@@ -239,6 +246,8 @@ static int exc3000_populate_device_info(struct exc3000_data *data)
 		char bl_version[8];
 
 		snprintf(bl_version, 8, "%d.%d", response[2], response[3]);
+		if (data->fw_rev)
+			devm_kfree(dev, data->fw_rev);
 		data->fw_rev = devm_kmemdup(dev, bl_version,
 					    strlen(bl_version), GFP_KERNEL);
 	} else {
@@ -248,6 +257,8 @@ static int exc3000_populate_device_info(struct exc3000_data *data)
 		if (ret < 0)
 			return -ENODEV;
 
+		if (data->fw_rev)
+			devm_kfree(dev, data->fw_rev);
 		data->fw_rev = devm_kmemdup(dev, &response[1],
 					    ret - 1, GFP_KERNEL);
 	}
@@ -289,10 +300,243 @@ static ssize_t exc3000_sysfs_fw_rev_show(struct device *dev,
 }
 static DEVICE_ATTR(fw_rev, 0444, exc3000_sysfs_fw_rev_show, NULL);
 
+static void exc3000_generate_unlock_response(u8 *challenge, u8 *response)
+{
+	u8 op, rot, sum;
+	int i;
+
+	op = challenge[0] + challenge[3];
+	rot = challenge[1] + challenge[2];
+	sum = challenge[0] + challenge[1] + challenge[2] + challenge[3];
+
+	for (i = 0; i < 4; i++) {
+		if ((op >> i) & 0x1) {
+			response[i] = sum + challenge[(rot + i) & 0x3];
+		} else {
+			response[i] = sum - challenge[(rot + i) & 0x3];
+		}
+	}
+}
+
+static int exc3000_firmware_update(struct exc3000_data *data,
+				   const struct firmware *fw)
+{
+	struct device *dev = &data->client->dev;
+	u8 resp[EXC3000_LEN_FRAME];
+	int ret, i;
+
+	dev_info(dev, "starting firmware update\n");
+
+	/* 1: check device state */
+	ret = exc3000_vendor_data_request(data, (u8[]){0x39, 0x02}, 2, resp);
+	if (ret < 0)
+		goto out;
+
+	/* 2: switch state from app to bootloader mode if necessary */
+	if (!resp[2] && !resp[3]) {
+		u8 unlock_req[6] = { 0x3a, 0xfc };
+
+		dev_dbg(dev, "device in app mode, switching to bootloader\n");
+
+		/* 2.1 request unlock challenge */
+		ret = exc3000_vendor_data_request(data,
+						  (u8[]){0x3a, 0xfb}, 2, resp);
+		if (ret < 0)
+			goto out;
+
+		/* 2.2 generate and send response */
+		exc3000_generate_unlock_response(&resp[2], &unlock_req[2]);
+		ret = exc3000_vendor_data_request(data, unlock_req, 6, resp);
+		if (ret < 0)
+			goto out;
+
+		if (resp[2] != 0x01) {
+			dev_err(dev, "device unlock failed, aborting\n");
+			ret = -EINVAL;
+			goto out;
+		}
+
+		/* 2.3 unknown, but required and invariant data */
+		ret = exc3000_vendor_data_request(data,
+						  (u8[]){0x3a, 0xfe, 0x34,
+						         0x43, 0xcc}, 5, resp);
+		if (ret < 0)
+			goto out;
+
+		/* 2.4 reset controller */
+		ret = exc3000_vendor_data_request(data, (u8[]){0x3a, 0xff},
+						  2, NULL);
+		if (ret < 0)
+			goto out;
+
+		/* wait for controller init after reset */
+		msleep(500);
+
+		/* 2.5: check communication after reset */
+		ret = exc3000_vendor_data_request(data, (u8[]){0x39, 0x01},
+						  2, resp);
+		if (ret < 0)
+			goto out;
+
+		if (resp[1] != 0x02) {
+			dev_err(dev, "device ping request NACK, aborting\n");
+			ret = -EINVAL;
+			goto out;
+		}
+
+		/* 2.6: check device mode again */
+		ret = exc3000_vendor_data_request(data, (u8[]){0x39, 0x02},
+						  2, resp);
+		if (ret < 0)
+			goto out;
+
+		if (!resp[2] && !resp[3]) {
+			dev_err(dev, "device still app mode, aborting\n");
+			ret = -EINVAL;
+			goto out;
+		}
+	}
+
+	/* 3: start firmware upload */
+	dev_dbg(dev, "start firmware upload\n");
+	ret = exc3000_vendor_data_request(data, (u8[]){0x3a, 0x04}, 2, resp);
+	if (ret < 0)
+		goto out;
+
+	if (resp[2] != 0x01) {
+		dev_err(dev, "firmware update start NACK, aborting\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* 4: upload firmware */
+	for (i = 0x56; i < fw->size; i += 36) {
+		u8 fw_chunk[37] = { 0x3a, 0x01, fw->data[i],
+				    fw->data[i+1],fw->data[i+34] };
+
+		memcpy(&fw_chunk[5], &fw->data[i+2], 32);
+		ret = exc3000_vendor_data_request(data, fw_chunk, 37, resp);
+		if (ret < 0)
+			goto out;
+
+		if (resp[2] != fw->data[i] || resp[3] != fw->data[i+1] ||
+		    resp[4] != fw->data[i+34]) {
+			dev_err(dev,
+				"firmware update readback wrong, aborting\n");
+			ret = -EINVAL;
+			goto out;
+		}
+
+		data->update_status = DIV_ROUND_UP(i * 100, fw->size);
+	}
+
+	/* 5: end firmware upload */
+	ret = exc3000_vendor_data_request(data,
+					  (u8[]){0x3a, 0x05, fw->data[0x37],
+						fw->data[0x38], fw->data[0x39],
+						fw->data[0x1f], fw->data[0x20]},
+					  7, resp);
+	if (ret < 0)
+		goto out;
+
+	if (resp[2] != 0x01) {
+		dev_err(dev, "firmware update end NACK, aborting\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* 6: switch back to app mode */
+	ret = exc3000_vendor_data_request(data, (u8[]){0x3a, 0xff}, 2, NULL);
+	if (ret < 0)
+		goto out;
+
+	/* wait for controller init after reset */
+	msleep(500);
+
+	/* 7: check communication */
+	ret = exc3000_vendor_data_request(data, (u8[]){0x39, 0x01}, 2, resp);
+	if (ret < 0)
+		goto out;
+
+	if (resp[1] != 0x02) {
+		dev_err(dev, "device ping request NACK, aborting\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* 8: check if we are in app mode again */
+	ret = exc3000_vendor_data_request(data, (u8[]){0x39, 0x02}, 2, resp);
+	if (ret < 0)
+		goto out;
+
+	if (resp[2] || resp[3]) {
+		dev_err(dev, "device still bootloader mode, aborting\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	dev_info(dev, "firmware update complete\n");
+
+	exc3000_populate_device_info(data);
+
+	data->update_status = 0;
+
+	return 0;
+
+out:
+	data->update_status = ret;
+	return ret;
+}
+
+static ssize_t exc3000_update_fw_store(struct device *dev,
+				       struct device_attribute *dattr,
+				       const char *buf, size_t count)
+{
+	struct exc3000_data *data = dev_get_drvdata(dev);
+	char fw_name[NAME_MAX];
+	const struct firmware *fw;
+	size_t copy_count = count;
+	int ret;
+
+	if (count == 0 || count >= NAME_MAX)
+		return -EINVAL;
+
+	if (buf[count - 1] == '\0' || buf[count - 1] == '\n')
+		copy_count -= 1;
+
+	strncpy(fw_name, buf, copy_count);
+	fw_name[copy_count] = '\0';
+
+	ret = request_firmware(&fw, fw_name, dev);
+	if (ret)
+		return ret;
+
+	dev_info(dev, "Flashing %s\n", fw_name);
+
+	ret = exc3000_firmware_update(data, fw);
+
+	release_firmware(fw);
+
+	return ret ?: count;
+}
+static DEVICE_ATTR(update_fw, 0200, NULL, exc3000_update_fw_store);
+
+static ssize_t exc3000_update_fw_status_show(struct device *dev,
+					     struct device_attribute *dattr,
+					     char *buf)
+{
+	struct exc3000_data *data = dev_get_drvdata(dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", data->update_status);
+}
+static DEVICE_ATTR(update_fw_status, 0444, exc3000_update_fw_status_show, NULL);
+
 static struct attribute *exc3000_attrs[] = {
 	&dev_attr_type.attr,
 	&dev_attr_model.attr,
 	&dev_attr_fw_rev.attr,
+	&dev_attr_update_fw.attr,
+	&dev_attr_update_fw_status.attr,
 	NULL
 };
 
-- 
2.20.1

Re: [PATCH 4/4] Input: exc3000: add firmware update support

[Dmitry Torokhov]

On Tue, Jan 07, 2020 at 06:17:40PM +0100, Lucas Stach wrote:
> This change allows the device firmware to be updated by putting a firmware
> file in /lib/firmware and providing the name of the file via the update_fw
> sysfs property. The driver will then flash the firmware image into the
> controller internal storage and restart the controller to activate the new
> firmware.
> 
> The implementation was done by looking at the the messages passed between
> the controller and proprietary vendor update tool. Not every detail of the
> protocol is totally well understood, so the implementation still has some
> "monkey see, monkey do" parts, as far as they have been found to be required
> for the update to succeed.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/input/touchscreen/exc3000.c | 248 +++++++++++++++++++++++++++-
>  1 file changed, 246 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
> index ce83914d65ff..f9a9820dc232 100644
> --- a/drivers/input/touchscreen/exc3000.c
> +++ b/drivers/input/touchscreen/exc3000.c
> @@ -3,8 +3,8 @@
>   * Driver for I2C connected EETI EXC3000 multiple touch controller
>   *
>   * Copyright (C) 2017 Ahmet Inan <inan@distec.de>
> - *
> - * minimal implementation based on egalax_ts.c and egalax_i2c.c
> + * Copyright (C) 2019 Pengutronix <kernel@pengutronix.de>
> + * Copyright (C) 2019 Zodiac Inflight Innovations
>   */
>  
>  #include <linux/bitops.h>
> @@ -18,6 +18,8 @@
>  #include <linux/of.h>
>  #include <linux/timer.h>
>  #include <asm/unaligned.h>
> +#include <linux/firmware.h>
> +#include <linux/delay.h>
>  
>  #define EXC3000_NUM_SLOTS		10
>  #define EXC3000_SLOTS_PER_FRAME		5
> @@ -37,6 +39,7 @@ struct exc3000_data {
>  	struct mutex vendor_data_lock;
>  	struct completion vendor_data_done;
>  	char *type, *model, *fw_rev;
> +	int update_status;
>  };
>  
>  static void exc3000_report_slots(struct input_dev *input,
> @@ -215,6 +218,8 @@ static int exc3000_populate_device_info(struct exc3000_data *data)
>  	if (ret < 0)
>  		return -ENODEV;
>  
> +	if (data->type)
> +		devm_kfree(dev, data->type);
>  	data->type = devm_kmemdup(dev, &response[1], ret - 1, GFP_KERNEL);

This makes me uncomfortable, as this changes the release order of the
resources (newly re-allocated memory will be freed first). Please
reassure me that it will work fine WRT device removal (or,
alternatively, we will need devm_realloc() that would preserve devm
release ordering.

Thanks.

-- 
Dmitry

Re: [PATCH 4/4] Input: exc3000: add firmware update support

[Lucas Stach]

On Mo, 2020-01-20 at 11:54 -0800, Dmitry Torokhov wrote:
> On Tue, Jan 07, 2020 at 06:17:40PM +0100, Lucas Stach wrote:
> > This change allows the device firmware to be updated by putting a
> > firmware
> > file in /lib/firmware and providing the name of the file via the
> > update_fw
> > sysfs property. The driver will then flash the firmware image into
> > the
> > controller internal storage and restart the controller to activate
> > the new
> > firmware.
> > 
> > The implementation was done by looking at the the messages passed
> > between
> > the controller and proprietary vendor update tool. Not every detail
> > of the
> > protocol is totally well understood, so the implementation still
> > has some
> > "monkey see, monkey do" parts, as far as they have been found to be
> > required
> > for the update to succeed.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/input/touchscreen/exc3000.c | 248
> > +++++++++++++++++++++++++++-
> >  1 file changed, 246 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/input/touchscreen/exc3000.c
> > b/drivers/input/touchscreen/exc3000.c
> > index ce83914d65ff..f9a9820dc232 100644
> > --- a/drivers/input/touchscreen/exc3000.c
> > +++ b/drivers/input/touchscreen/exc3000.c
> > @@ -3,8 +3,8 @@
> >   * Driver for I2C connected EETI EXC3000 multiple touch controller
> >   *
> >   * Copyright (C) 2017 Ahmet Inan <inan@distec.de>
> > - *
> > - * minimal implementation based on egalax_ts.c and egalax_i2c.c
> > + * Copyright (C) 2019 Pengutronix <kernel@pengutronix.de>
> > + * Copyright (C) 2019 Zodiac Inflight Innovations
> >   */
> >  
> >  #include <linux/bitops.h>
> > @@ -18,6 +18,8 @@
> >  #include <linux/of.h>
> >  #include <linux/timer.h>
> >  #include <asm/unaligned.h>
> > +#include <linux/firmware.h>
> > +#include <linux/delay.h>
> >  
> >  #define EXC3000_NUM_SLOTS		10
> >  #define EXC3000_SLOTS_PER_FRAME		5
> > @@ -37,6 +39,7 @@ struct exc3000_data {
> >  	struct mutex vendor_data_lock;
> >  	struct completion vendor_data_done;
> >  	char *type, *model, *fw_rev;
> > +	int update_status;
> >  };
> >  
> >  static void exc3000_report_slots(struct input_dev *input,
> > @@ -215,6 +218,8 @@ static int exc3000_populate_device_info(struct
> > exc3000_data *data)
> >  	if (ret < 0)
> >  		return -ENODEV;
> >  
> > +	if (data->type)
> > +		devm_kfree(dev, data->type);
> >  	data->type = devm_kmemdup(dev, &response[1], ret - 1,
> > GFP_KERNEL);
> 
> This makes me uncomfortable, as this changes the release order of the
> resources (newly re-allocated memory will be freed first). Please
> reassure me that it will work fine WRT device removal (or,
> alternatively, we will need devm_realloc() that would preserve devm
> release ordering.

The cached information here is only used for the sysfs FW info
attributes. In v2 I split those out into a separate group, which gets
removed and registered again when refreshing this information after the
FW update. This keeps the devm ordering the same and fixes a possible
use-after-free through sysfs reads at the same time.

Regards,
Lucas

Re: [PATCH 1/4] Input: exc3000: split MT event handling from IRQ handler

[Chris Healy]

On both ARMv7 and ARMv8 platforms, full series is:

Tested-by: Chris Healy <cphealy@gmail.com>

On Tue, Jan 7, 2020 at 9:17 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Split out the multitouch event handling into its own function to allow other
> events to be handled in the IRQ handler without disturbing the MT handling.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/input/touchscreen/exc3000.c | 92 +++++++++++++++++------------
>  1 file changed, 54 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
> index e007e2e8f626..3458d02310dd 100644
> --- a/drivers/input/touchscreen/exc3000.c
> +++ b/drivers/input/touchscreen/exc3000.c
> @@ -58,6 +58,11 @@ static void exc3000_timer(struct timer_list *t)
>         input_sync(data->input);
>  }
>
> +static inline void exc3000_schedule_timer(struct exc3000_data *data)
> +{
> +       mod_timer(&data->timer, jiffies + msecs_to_jiffies(EXC3000_TIMEOUT_MS));
> +}
> +
>  static int exc3000_read_frame(struct i2c_client *client, u8 *buf)
>  {
>         int ret;
> @@ -76,54 +81,35 @@ static int exc3000_read_frame(struct i2c_client *client, u8 *buf)
>         if (ret != EXC3000_LEN_FRAME)
>                 return -EIO;
>
> -       if (get_unaligned_le16(buf) != EXC3000_LEN_FRAME ||
> -                       buf[2] != EXC3000_MT_EVENT)
> +       if (get_unaligned_le16(buf) != EXC3000_LEN_FRAME)
>                 return -EINVAL;
>
>         return 0;
>  }
>
> -static int exc3000_read_data(struct i2c_client *client,
> -                            u8 *buf, int *n_slots)
> +static int exc3000_handle_mt_event(struct exc3000_data *data)
>  {
> -       int error;
> -
> -       error = exc3000_read_frame(client, buf);
> -       if (error)
> -               return error;
> +       struct input_dev *input = data->input;
> +       int ret, total_slots;
> +       u8 *buf = data->buf;
>
> -       *n_slots = buf[3];
> -       if (!*n_slots || *n_slots > EXC3000_NUM_SLOTS)
> -               return -EINVAL;
> +       total_slots = buf[3];
> +       if (!total_slots || total_slots > EXC3000_NUM_SLOTS) {
> +               ret = -EINVAL;
> +               goto out_fail;
> +       }
>
> -       if (*n_slots > EXC3000_SLOTS_PER_FRAME) {
> +       if (total_slots > EXC3000_SLOTS_PER_FRAME) {
>                 /* Read 2nd frame to get the rest of the contacts. */
> -               error = exc3000_read_frame(client, buf + EXC3000_LEN_FRAME);
> -               if (error)
> -                       return error;
> +               ret = exc3000_read_frame(data->client, buf + EXC3000_LEN_FRAME);
> +               if (ret)
> +                       goto out_fail;
>
>                 /* 2nd chunk must have number of contacts set to 0. */
> -               if (buf[EXC3000_LEN_FRAME + 3] != 0)
> -                       return -EINVAL;
> -       }
> -
> -       return 0;
> -}
> -
> -static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
> -{
> -       struct exc3000_data *data = dev_id;
> -       struct input_dev *input = data->input;
> -       u8 *buf = data->buf;
> -       int slots, total_slots;
> -       int error;
> -
> -       error = exc3000_read_data(data->client, buf, &total_slots);
> -       if (error) {
> -               /* Schedule a timer to release "stuck" contacts */
> -               mod_timer(&data->timer,
> -                         jiffies + msecs_to_jiffies(EXC3000_TIMEOUT_MS));
> -               goto out;
> +               if (buf[EXC3000_LEN_FRAME + 3] != 0) {
> +                       ret = -EINVAL;
> +                       goto out_fail;
> +               }
>         }
>
>         /*
> @@ -132,7 +118,7 @@ static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
>         del_timer_sync(&data->timer);
>
>         while (total_slots > 0) {
> -               slots = min(total_slots, EXC3000_SLOTS_PER_FRAME);
> +               int slots = min(total_slots, EXC3000_SLOTS_PER_FRAME);
>                 exc3000_report_slots(input, &data->prop, buf + 4, slots);
>                 total_slots -= slots;
>                 buf += EXC3000_LEN_FRAME;
> @@ -141,6 +127,36 @@ static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
>         input_mt_sync_frame(input);
>         input_sync(input);
>
> +       return 0;
> +
> +out_fail:
> +       /* Schedule a timer to release "stuck" contacts */
> +       exc3000_schedule_timer(data);
> +
> +       return ret;
> +}
> +
> +static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
> +{
> +       struct exc3000_data *data = dev_id;
> +       u8 *buf = data->buf;
> +       int ret;
> +
> +       ret = exc3000_read_frame(data->client, buf);
> +       if (ret) {
> +               /* Schedule a timer to release "stuck" contacts */
> +               exc3000_schedule_timer(data);
> +               goto out;
> +       }
> +
> +       switch (buf[2]) {
> +               case EXC3000_MT_EVENT:
> +                       exc3000_handle_mt_event(data);
> +                       break;
> +               default:
> +                       break;
> +       }
> +
>  out:
>         return IRQ_HANDLED;
>  }
> --
> 2.20.1
>