[PATCH 0/5] input: rmi4/synaptics fixes

[PATCH 0/5] input: rmi4/synaptics fixes

[Hans Verkuil]

These are five patches that fix various issues I found when testing
the F54 support of my Lenovo X1 Carbon Gen 6 laptop.

The first three are pretty straightforward. The third patch is a
media documentation fix and, once Acked, it can go in either through
the media subsystem or the input subsystem, whatever is easiest.

The last two patches are more of an RFC quality:

I noticed that irq_find_mapping() could return 0, which causes
a kernel crash. I suspect that this patch just fixes the symptom
and not necessarily the actual cause. I can do more testing to see
if I can find the real cause. Hints of where to look would be
welcome.

The last patch is basically trial-and-error. When testing F54 I noticed
that only the first 32 bytes of the capture image were valid, everything
else was garbage. By deleting the line that increments rmiaddr it suddenly
started working, but I only found an old RMI4 spec and I have no idea
why this fix works.

I can't imagine that it failed when this F54 driver was first added,
so did something else break? Or is my Lenovo special in some way?

I can help with testing, but the https://github.com/ndyer/heatmap/commits/heatmap-v4l
is easy enough to use.

Regards,

	Hans

Hans Verkuil (5):
  input/mouse/synaptics: add LEN0091 support
  input/rmi4/rmi_f54: fix various V4L2 compliance problems
  pixfmt-tch-td16/tu16.rst: document that this is little endian
  input/rmi4/rmi_driver: check if irq_find_mapping returns 0
  input/rmi4/rmi_smbus.c: don't increment rmiaddr in
    rmi_smb_read_block()

 .../media/uapi/v4l/pixfmt-tch-td16.rst        | 34 +++++++++----------
 .../media/uapi/v4l/pixfmt-tch-tu16.rst        | 34 +++++++++----------
 drivers/input/mouse/synaptics.c               |  1 +
 drivers/input/rmi4/rmi_driver.c               |  8 +++--
 drivers/input/rmi4/rmi_f54.c                  | 15 +++++++-
 drivers/input/rmi4/rmi_smbus.c                |  1 -
 6 files changed, 55 insertions(+), 38 deletions(-)

-- 
2.23.0

[PATCH 1/5] input/mouse/synaptics: add LEN0091 support

[Hans Verkuil]

Some Lenovo X1 Carbon Gen 6 laptops report LEN0091. Add this
to the smbus_pnp_ids list.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/input/mouse/synaptics.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 56fae3472114..1ae6f8bba9ae 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -172,6 +172,7 @@ static const char * const smbus_pnp_ids[] = {
 	"LEN0071", /* T480 */
 	"LEN0072", /* X1 Carbon Gen 5 (2017) - Elan/ALPS trackpoint */
 	"LEN0073", /* X1 Carbon G5 (Elantech) */
+	"LEN0091", /* X1 Carbon 6 */
 	"LEN0092", /* X1 Carbon 6 */
 	"LEN0093", /* T480 */
 	"LEN0096", /* X280 */
-- 
2.23.0

Re: [PATCH 1/5] input/mouse/synaptics: add LEN0091 support

[Dmitry Torokhov]

On Tue, Nov 19, 2019 at 11:51:14AM +0100, Hans Verkuil wrote:
> Some Lenovo X1 Carbon Gen 6 laptops report LEN0091. Add this
> to the smbus_pnp_ids list.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Applied, thank you.

> ---
>  drivers/input/mouse/synaptics.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 56fae3472114..1ae6f8bba9ae 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -172,6 +172,7 @@ static const char * const smbus_pnp_ids[] = {
>  	"LEN0071", /* T480 */
>  	"LEN0072", /* X1 Carbon Gen 5 (2017) - Elan/ALPS trackpoint */
>  	"LEN0073", /* X1 Carbon G5 (Elantech) */
> +	"LEN0091", /* X1 Carbon 6 */
>  	"LEN0092", /* X1 Carbon 6 */
>  	"LEN0093", /* T480 */
>  	"LEN0096", /* X280 */
> -- 
> 2.23.0
> 

-- 
Dmitry

[PATCH 2/5] input/rmi4/rmi_f54: fix various V4L2 compliance problems

[Hans Verkuil]

The v4l2-compliance utility reported several V4L2 API compliance
issues:

- the sequence counter wasn't filled in
- the sequence counter wasn't reset to 0 at the start of streaming
- the returned field value wasn't set to V4L2_FIELD_NONE
- the timestamp wasn't set
- the payload size was undefined if an error was returned
- min_buffers_needed doesn't need to be initialized

Fix these issues.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/input/rmi4/rmi_f54.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
index 710b02595486..ebccab7a4834 100644
--- a/drivers/input/rmi4/rmi_f54.c
+++ b/drivers/input/rmi4/rmi_f54.c
@@ -116,6 +116,7 @@ struct f54_data {
 	struct video_device vdev;
 	struct vb2_queue queue;
 	struct mutex lock;
+	u32 sequence;
 	int input;
 	enum rmi_f54_report_type inputs[F54_MAX_REPORT_TYPE];
 };
@@ -290,6 +291,7 @@ static int rmi_f54_queue_setup(struct vb2_queue *q, unsigned int *nbuffers,
 
 static void rmi_f54_buffer_queue(struct vb2_buffer *vb)
 {
+	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
 	struct f54_data *f54 = vb2_get_drv_priv(vb->vb2_queue);
 	u16 *ptr;
 	enum vb2_buffer_state state;
@@ -298,6 +300,7 @@ static void rmi_f54_buffer_queue(struct vb2_buffer *vb)
 
 	mutex_lock(&f54->status_mutex);
 
+	vb2_set_plane_payload(vb, 0, 0);
 	reptype = rmi_f54_get_reptype(f54, f54->input);
 	if (reptype == F54_REPORT_NONE) {
 		state = VB2_BUF_STATE_ERROR;
@@ -344,14 +347,25 @@ static void rmi_f54_buffer_queue(struct vb2_buffer *vb)
 data_done:
 	mutex_unlock(&f54->data_mutex);
 done:
+	vb->timestamp = ktime_get_ns();
+	vbuf->field = V4L2_FIELD_NONE;
+	vbuf->sequence = f54->sequence++;
 	vb2_buffer_done(vb, state);
 	mutex_unlock(&f54->status_mutex);
 }
 
+static void rmi_f54_stop_streaming(struct vb2_queue *q)
+{
+	struct f54_data *f54 = vb2_get_drv_priv(q);
+
+	f54->sequence = 0;
+}
+
 /* V4L2 structures */
 static const struct vb2_ops rmi_f54_queue_ops = {
 	.queue_setup            = rmi_f54_queue_setup,
 	.buf_queue              = rmi_f54_buffer_queue,
+	.stop_streaming		= rmi_f54_stop_streaming,
 	.wait_prepare           = vb2_ops_wait_prepare,
 	.wait_finish            = vb2_ops_wait_finish,
 };
@@ -363,7 +377,6 @@ static const struct vb2_queue rmi_f54_queue = {
 	.ops = &rmi_f54_queue_ops,
 	.mem_ops = &vb2_vmalloc_memops,
 	.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC,
-	.min_buffers_needed = 1,
 };
 
 static int rmi_f54_vidioc_querycap(struct file *file, void *priv,
-- 
2.23.0

Re: [PATCH 2/5] input/rmi4/rmi_f54: fix various V4L2 compliance problems

[Lucas Stach]

On Di, 2019-11-19 at 11:51 +0100, Hans Verkuil wrote:
> The v4l2-compliance utility reported several V4L2 API compliance
> issues:
> 
> - the sequence counter wasn't filled in
> - the sequence counter wasn't reset to 0 at the start of streaming
> - the returned field value wasn't set to V4L2_FIELD_NONE
> - the timestamp wasn't set
> - the payload size was undefined if an error was returned
> - min_buffers_needed doesn't need to be initialized
> 
> Fix these issues.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de

> ---
>  drivers/input/rmi4/rmi_f54.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/rmi4/rmi_f54.c
> b/drivers/input/rmi4/rmi_f54.c
> index 710b02595486..ebccab7a4834 100644
> --- a/drivers/input/rmi4/rmi_f54.c
> +++ b/drivers/input/rmi4/rmi_f54.c
> @@ -116,6 +116,7 @@ struct f54_data {
>  	struct video_device vdev;
>  	struct vb2_queue queue;
>  	struct mutex lock;
> +	u32 sequence;
>  	int input;
>  	enum rmi_f54_report_type inputs[F54_MAX_REPORT_TYPE];
>  };
> @@ -290,6 +291,7 @@ static int rmi_f54_queue_setup(struct vb2_queue
> *q, unsigned int *nbuffers,
>  
>  static void rmi_f54_buffer_queue(struct vb2_buffer *vb)
>  {
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>  	struct f54_data *f54 = vb2_get_drv_priv(vb->vb2_queue);
>  	u16 *ptr;
>  	enum vb2_buffer_state state;
> @@ -298,6 +300,7 @@ static void rmi_f54_buffer_queue(struct
> vb2_buffer *vb)
>  
>  	mutex_lock(&f54->status_mutex);
>  
> +	vb2_set_plane_payload(vb, 0, 0);
>  	reptype = rmi_f54_get_reptype(f54, f54->input);
>  	if (reptype == F54_REPORT_NONE) {
>  		state = VB2_BUF_STATE_ERROR;
> @@ -344,14 +347,25 @@ static void rmi_f54_buffer_queue(struct
> vb2_buffer *vb)
>  data_done:
>  	mutex_unlock(&f54->data_mutex);
>  done:
> +	vb->timestamp = ktime_get_ns();
> +	vbuf->field = V4L2_FIELD_NONE;
> +	vbuf->sequence = f54->sequence++;
>  	vb2_buffer_done(vb, state);
>  	mutex_unlock(&f54->status_mutex);
>  }
>  
> +static void rmi_f54_stop_streaming(struct vb2_queue *q)
> +{
> +	struct f54_data *f54 = vb2_get_drv_priv(q);
> +
> +	f54->sequence = 0;
> +}
> +
>  /* V4L2 structures */
>  static const struct vb2_ops rmi_f54_queue_ops = {
>  	.queue_setup            = rmi_f54_queue_setup,
>  	.buf_queue              = rmi_f54_buffer_queue,
> +	.stop_streaming		= rmi_f54_stop_streaming,
>  	.wait_prepare           = vb2_ops_wait_prepare,
>  	.wait_finish            = vb2_ops_wait_finish,
>  };
> @@ -363,7 +377,6 @@ static const struct vb2_queue rmi_f54_queue = {
>  	.ops = &rmi_f54_queue_ops,
>  	.mem_ops = &vb2_vmalloc_memops,
>  	.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC,
> -	.min_buffers_needed = 1,
>  };
>  
>  static int rmi_f54_vidioc_querycap(struct file *file, void *priv,

Re: [PATCH 2/5] input/rmi4/rmi_f54: fix various V4L2 compliance problems

[Dmitry Torokhov]

On Tue, Nov 19, 2019 at 12:42:46PM +0100, Lucas Stach wrote:
> On Di, 2019-11-19 at 11:51 +0100, Hans Verkuil wrote:
> > The v4l2-compliance utility reported several V4L2 API compliance
> > issues:
> > 
> > - the sequence counter wasn't filled in
> > - the sequence counter wasn't reset to 0 at the start of streaming
> > - the returned field value wasn't set to V4L2_FIELD_NONE
> > - the timestamp wasn't set
> > - the payload size was undefined if an error was returned
> > - min_buffers_needed doesn't need to be initialized
> > 
> > Fix these issues.
> > 
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> Reviewed-by: Lucas Stach <l.stach@pengutronix.de

Applied, thank you.

> 
> > ---
> >  drivers/input/rmi4/rmi_f54.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/input/rmi4/rmi_f54.c
> > b/drivers/input/rmi4/rmi_f54.c
> > index 710b02595486..ebccab7a4834 100644
> > --- a/drivers/input/rmi4/rmi_f54.c
> > +++ b/drivers/input/rmi4/rmi_f54.c
> > @@ -116,6 +116,7 @@ struct f54_data {
> >  	struct video_device vdev;
> >  	struct vb2_queue queue;
> >  	struct mutex lock;
> > +	u32 sequence;
> >  	int input;
> >  	enum rmi_f54_report_type inputs[F54_MAX_REPORT_TYPE];
> >  };
> > @@ -290,6 +291,7 @@ static int rmi_f54_queue_setup(struct vb2_queue
> > *q, unsigned int *nbuffers,
> >  
> >  static void rmi_f54_buffer_queue(struct vb2_buffer *vb)
> >  {
> > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> >  	struct f54_data *f54 = vb2_get_drv_priv(vb->vb2_queue);
> >  	u16 *ptr;
> >  	enum vb2_buffer_state state;
> > @@ -298,6 +300,7 @@ static void rmi_f54_buffer_queue(struct
> > vb2_buffer *vb)
> >  
> >  	mutex_lock(&f54->status_mutex);
> >  
> > +	vb2_set_plane_payload(vb, 0, 0);
> >  	reptype = rmi_f54_get_reptype(f54, f54->input);
> >  	if (reptype == F54_REPORT_NONE) {
> >  		state = VB2_BUF_STATE_ERROR;
> > @@ -344,14 +347,25 @@ static void rmi_f54_buffer_queue(struct
> > vb2_buffer *vb)
> >  data_done:
> >  	mutex_unlock(&f54->data_mutex);
> >  done:
> > +	vb->timestamp = ktime_get_ns();
> > +	vbuf->field = V4L2_FIELD_NONE;
> > +	vbuf->sequence = f54->sequence++;
> >  	vb2_buffer_done(vb, state);
> >  	mutex_unlock(&f54->status_mutex);
> >  }
> >  
> > +static void rmi_f54_stop_streaming(struct vb2_queue *q)
> > +{
> > +	struct f54_data *f54 = vb2_get_drv_priv(q);
> > +
> > +	f54->sequence = 0;
> > +}
> > +
> >  /* V4L2 structures */
> >  static const struct vb2_ops rmi_f54_queue_ops = {
> >  	.queue_setup            = rmi_f54_queue_setup,
> >  	.buf_queue              = rmi_f54_buffer_queue,
> > +	.stop_streaming		= rmi_f54_stop_streaming,
> >  	.wait_prepare           = vb2_ops_wait_prepare,
> >  	.wait_finish            = vb2_ops_wait_finish,
> >  };
> > @@ -363,7 +377,6 @@ static const struct vb2_queue rmi_f54_queue = {
> >  	.ops = &rmi_f54_queue_ops,
> >  	.mem_ops = &vb2_vmalloc_memops,
> >  	.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC,
> > -	.min_buffers_needed = 1,
> >  };
> >  
> >  static int rmi_f54_vidioc_querycap(struct file *file, void *priv,
> 

-- 
Dmitry

[PATCH 3/5] pixfmt-tch-td16/tu16.rst: document that this is little endian

[Hans Verkuil]

Testing with the rmi_f54 driver on the Lenovo X1 Carbon 6th gen
laptop showed that the data is in little endian format. Update
the documentation accordingly.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 .../media/uapi/v4l/pixfmt-tch-td16.rst        | 34 +++++++++----------
 .../media/uapi/v4l/pixfmt-tch-tu16.rst        | 34 +++++++++----------
 2 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/Documentation/media/uapi/v4l/pixfmt-tch-td16.rst b/Documentation/media/uapi/v4l/pixfmt-tch-td16.rst
index 4031b175257c..6f1be873bec1 100644
--- a/Documentation/media/uapi/v4l/pixfmt-tch-td16.rst
+++ b/Documentation/media/uapi/v4l/pixfmt-tch-td16.rst
@@ -15,7 +15,7 @@ V4L2_TCH_FMT_DELTA_TD16 ('TD16')
 
 *man V4L2_TCH_FMT_DELTA_TD16(2)*
 
-16-bit signed Touch Delta
+16-bit signed little endian Touch Delta
 
 
 Description
@@ -37,38 +37,38 @@ Each cell is one byte.
     :widths:       2 1 1 1 1 1 1 1 1
 
     * - start + 0:
-      - D'\ :sub:`00high`
       - D'\ :sub:`00low`
-      - D'\ :sub:`01high`
+      - D'\ :sub:`00high`
       - D'\ :sub:`01low`
-      - D'\ :sub:`02high`
+      - D'\ :sub:`01high`
       - D'\ :sub:`02low`
-      - D'\ :sub:`03high`
+      - D'\ :sub:`02high`
       - D'\ :sub:`03low`
+      - D'\ :sub:`03high`
     * - start + 8:
-      - D'\ :sub:`10high`
       - D'\ :sub:`10low`
-      - D'\ :sub:`11high`
+      - D'\ :sub:`10high`
       - D'\ :sub:`11low`
-      - D'\ :sub:`12high`
+      - D'\ :sub:`11high`
       - D'\ :sub:`12low`
-      - D'\ :sub:`13high`
+      - D'\ :sub:`12high`
       - D'\ :sub:`13low`
+      - D'\ :sub:`13high`
     * - start + 16:
-      - D'\ :sub:`20high`
       - D'\ :sub:`20low`
-      - D'\ :sub:`21high`
+      - D'\ :sub:`20high`
       - D'\ :sub:`21low`
-      - D'\ :sub:`22high`
+      - D'\ :sub:`21high`
       - D'\ :sub:`22low`
-      - D'\ :sub:`23high`
+      - D'\ :sub:`22high`
       - D'\ :sub:`23low`
+      - D'\ :sub:`23high`
     * - start + 24:
-      - D'\ :sub:`30high`
       - D'\ :sub:`30low`
-      - D'\ :sub:`31high`
+      - D'\ :sub:`30high`
       - D'\ :sub:`31low`
-      - D'\ :sub:`32high`
+      - D'\ :sub:`31high`
       - D'\ :sub:`32low`
-      - D'\ :sub:`33high`
+      - D'\ :sub:`32high`
       - D'\ :sub:`33low`
+      - D'\ :sub:`33high`
diff --git a/Documentation/media/uapi/v4l/pixfmt-tch-tu16.rst b/Documentation/media/uapi/v4l/pixfmt-tch-tu16.rst
index 8278543be99a..cb3da6687a58 100644
--- a/Documentation/media/uapi/v4l/pixfmt-tch-tu16.rst
+++ b/Documentation/media/uapi/v4l/pixfmt-tch-tu16.rst
@@ -15,7 +15,7 @@ V4L2_TCH_FMT_TU16 ('TU16')
 
 *man V4L2_TCH_FMT_TU16(2)*
 
-16-bit unsigned raw touch data
+16-bit unsigned little endian raw touch data
 
 
 Description
@@ -36,38 +36,38 @@ Each cell is one byte.
     :widths:       2 1 1 1 1 1 1 1 1
 
     * - start + 0:
-      - R'\ :sub:`00high`
       - R'\ :sub:`00low`
-      - R'\ :sub:`01high`
+      - R'\ :sub:`00high`
       - R'\ :sub:`01low`
-      - R'\ :sub:`02high`
+      - R'\ :sub:`01high`
       - R'\ :sub:`02low`
-      - R'\ :sub:`03high`
+      - R'\ :sub:`02high`
       - R'\ :sub:`03low`
+      - R'\ :sub:`03high`
     * - start + 8:
-      - R'\ :sub:`10high`
       - R'\ :sub:`10low`
-      - R'\ :sub:`11high`
+      - R'\ :sub:`10high`
       - R'\ :sub:`11low`
-      - R'\ :sub:`12high`
+      - R'\ :sub:`11high`
       - R'\ :sub:`12low`
-      - R'\ :sub:`13high`
+      - R'\ :sub:`12high`
       - R'\ :sub:`13low`
+      - R'\ :sub:`13high`
     * - start + 16:
-      - R'\ :sub:`20high`
       - R'\ :sub:`20low`
-      - R'\ :sub:`21high`
+      - R'\ :sub:`20high`
       - R'\ :sub:`21low`
-      - R'\ :sub:`22high`
+      - R'\ :sub:`21high`
       - R'\ :sub:`22low`
-      - R'\ :sub:`23high`
+      - R'\ :sub:`22high`
       - R'\ :sub:`23low`
+      - R'\ :sub:`23high`
     * - start + 24:
-      - R'\ :sub:`30high`
       - R'\ :sub:`30low`
-      - R'\ :sub:`31high`
+      - R'\ :sub:`30high`
       - R'\ :sub:`31low`
-      - R'\ :sub:`32high`
+      - R'\ :sub:`31high`
       - R'\ :sub:`32low`
-      - R'\ :sub:`33high`
+      - R'\ :sub:`32high`
       - R'\ :sub:`33low`
+      - R'\ :sub:`33high`
-- 
2.23.0

Re: [PATCH 3/5] pixfmt-tch-td16/tu16.rst: document that this is little endian

[Hans Verkuil]

Hi Dmitry,

Is it OK with you I merge this patch via the media subsystem?

It's pretty independent of the other rmi4 patches in this series, so it makes
sense that I handle this one.

Regards,

	Hans

On 11/19/19 11:51 AM, Hans Verkuil wrote:
> Testing with the rmi_f54 driver on the Lenovo X1 Carbon 6th gen
> laptop showed that the data is in little endian format. Update
> the documentation accordingly.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  .../media/uapi/v4l/pixfmt-tch-td16.rst        | 34 +++++++++----------
>  .../media/uapi/v4l/pixfmt-tch-tu16.rst        | 34 +++++++++----------
>  2 files changed, 34 insertions(+), 34 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/pixfmt-tch-td16.rst b/Documentation/media/uapi/v4l/pixfmt-tch-td16.rst
> index 4031b175257c..6f1be873bec1 100644
> --- a/Documentation/media/uapi/v4l/pixfmt-tch-td16.rst
> +++ b/Documentation/media/uapi/v4l/pixfmt-tch-td16.rst
> @@ -15,7 +15,7 @@ V4L2_TCH_FMT_DELTA_TD16 ('TD16')
>  
>  *man V4L2_TCH_FMT_DELTA_TD16(2)*
>  
> -16-bit signed Touch Delta
> +16-bit signed little endian Touch Delta
>  
>  
>  Description
> @@ -37,38 +37,38 @@ Each cell is one byte.
>      :widths:       2 1 1 1 1 1 1 1 1
>  
>      * - start + 0:
> -      - D'\ :sub:`00high`
>        - D'\ :sub:`00low`
> -      - D'\ :sub:`01high`
> +      - D'\ :sub:`00high`
>        - D'\ :sub:`01low`
> -      - D'\ :sub:`02high`
> +      - D'\ :sub:`01high`
>        - D'\ :sub:`02low`
> -      - D'\ :sub:`03high`
> +      - D'\ :sub:`02high`
>        - D'\ :sub:`03low`
> +      - D'\ :sub:`03high`
>      * - start + 8:
> -      - D'\ :sub:`10high`
>        - D'\ :sub:`10low`
> -      - D'\ :sub:`11high`
> +      - D'\ :sub:`10high`
>        - D'\ :sub:`11low`
> -      - D'\ :sub:`12high`
> +      - D'\ :sub:`11high`
>        - D'\ :sub:`12low`
> -      - D'\ :sub:`13high`
> +      - D'\ :sub:`12high`
>        - D'\ :sub:`13low`
> +      - D'\ :sub:`13high`
>      * - start + 16:
> -      - D'\ :sub:`20high`
>        - D'\ :sub:`20low`
> -      - D'\ :sub:`21high`
> +      - D'\ :sub:`20high`
>        - D'\ :sub:`21low`
> -      - D'\ :sub:`22high`
> +      - D'\ :sub:`21high`
>        - D'\ :sub:`22low`
> -      - D'\ :sub:`23high`
> +      - D'\ :sub:`22high`
>        - D'\ :sub:`23low`
> +      - D'\ :sub:`23high`
>      * - start + 24:
> -      - D'\ :sub:`30high`
>        - D'\ :sub:`30low`
> -      - D'\ :sub:`31high`
> +      - D'\ :sub:`30high`
>        - D'\ :sub:`31low`
> -      - D'\ :sub:`32high`
> +      - D'\ :sub:`31high`
>        - D'\ :sub:`32low`
> -      - D'\ :sub:`33high`
> +      - D'\ :sub:`32high`
>        - D'\ :sub:`33low`
> +      - D'\ :sub:`33high`
> diff --git a/Documentation/media/uapi/v4l/pixfmt-tch-tu16.rst b/Documentation/media/uapi/v4l/pixfmt-tch-tu16.rst
> index 8278543be99a..cb3da6687a58 100644
> --- a/Documentation/media/uapi/v4l/pixfmt-tch-tu16.rst
> +++ b/Documentation/media/uapi/v4l/pixfmt-tch-tu16.rst
> @@ -15,7 +15,7 @@ V4L2_TCH_FMT_TU16 ('TU16')
>  
>  *man V4L2_TCH_FMT_TU16(2)*
>  
> -16-bit unsigned raw touch data
> +16-bit unsigned little endian raw touch data
>  
>  
>  Description
> @@ -36,38 +36,38 @@ Each cell is one byte.
>      :widths:       2 1 1 1 1 1 1 1 1
>  
>      * - start + 0:
> -      - R'\ :sub:`00high`
>        - R'\ :sub:`00low`
> -      - R'\ :sub:`01high`
> +      - R'\ :sub:`00high`
>        - R'\ :sub:`01low`
> -      - R'\ :sub:`02high`
> +      - R'\ :sub:`01high`
>        - R'\ :sub:`02low`
> -      - R'\ :sub:`03high`
> +      - R'\ :sub:`02high`
>        - R'\ :sub:`03low`
> +      - R'\ :sub:`03high`
>      * - start + 8:
> -      - R'\ :sub:`10high`
>        - R'\ :sub:`10low`
> -      - R'\ :sub:`11high`
> +      - R'\ :sub:`10high`
>        - R'\ :sub:`11low`
> -      - R'\ :sub:`12high`
> +      - R'\ :sub:`11high`
>        - R'\ :sub:`12low`
> -      - R'\ :sub:`13high`
> +      - R'\ :sub:`12high`
>        - R'\ :sub:`13low`
> +      - R'\ :sub:`13high`
>      * - start + 16:
> -      - R'\ :sub:`20high`
>        - R'\ :sub:`20low`
> -      - R'\ :sub:`21high`
> +      - R'\ :sub:`20high`
>        - R'\ :sub:`21low`
> -      - R'\ :sub:`22high`
> +      - R'\ :sub:`21high`
>        - R'\ :sub:`22low`
> -      - R'\ :sub:`23high`
> +      - R'\ :sub:`22high`
>        - R'\ :sub:`23low`
> +      - R'\ :sub:`23high`
>      * - start + 24:
> -      - R'\ :sub:`30high`
>        - R'\ :sub:`30low`
> -      - R'\ :sub:`31high`
> +      - R'\ :sub:`30high`
>        - R'\ :sub:`31low`
> -      - R'\ :sub:`32high`
> +      - R'\ :sub:`31high`
>        - R'\ :sub:`32low`
> -      - R'\ :sub:`33high`
> +      - R'\ :sub:`32high`
>        - R'\ :sub:`33low`
> +      - R'\ :sub:`33high`
> 

Re: [PATCH 3/5] pixfmt-tch-td16/tu16.rst: document that this is little endian

[Dmitry Torokhov]

Hi Hans,

On Sat, Nov 23, 2019 at 05:12:43PM +0100, Hans Verkuil wrote:
> Hi Dmitry,
> 
> Is it OK with you I merge this patch via the media subsystem?

Yes, absolutely. It does not touch any of the input bits so I actually
expected it to go through media tree.

Thanks!

-- 
Dmitry

[PATCH 4/5] input/rmi4/rmi_driver: check if irq_find_mapping returns 0

[Hans Verkuil]

The irq_find_mapping() function can return 0 when called in the
rmi_process_interrupt_requests() function.

This causes a kernel crash. Check for a 0 value and skip calling
handle_nested_irq() in that case.

This was tested with the F54 function enabled on a Lenovo X1 Carbon.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Fixes: 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution to irq_domain")
---
 drivers/input/rmi4/rmi_driver.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 772493b1f665..6085ec424a84 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -154,8 +154,12 @@ static int rmi_process_interrupt_requests(struct rmi_device *rmi_dev)
 	 */
 	mutex_unlock(&data->irq_mutex);
 
-	for_each_set_bit(i, data->irq_status, data->irq_count)
-		handle_nested_irq(irq_find_mapping(data->irqdomain, i));
+	for_each_set_bit(i, data->irq_status, data->irq_count) {
+		unsigned int irq = irq_find_mapping(data->irqdomain, i);
+
+		if (irq)
+			handle_nested_irq(irq);
+	}
 
 	if (data->input)
 		input_sync(data->input);
-- 
2.23.0

Re: [PATCH 4/5] input/rmi4/rmi_driver: check if irq_find_mapping returns 0

[Lucas Stach]

Hi Hans,

On Di, 2019-11-19 at 11:51 +0100, Hans Verkuil wrote:
> The irq_find_mapping() function can return 0 when called in the
> rmi_process_interrupt_requests() function.
> 
> This causes a kernel crash. Check for a 0 value and skip calling
> handle_nested_irq() in that case.
> 
> This was tested with the F54 function enabled on a Lenovo X1 Carbon.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Fixes: 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution to irq_domain")

This is already fixed upstream by 549766ac2ac1
"Input: synaptics-rmi4 - clear IRQ enables for F54"

Regards,
Lucas

> ---
>  drivers/input/rmi4/rmi_driver.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index 772493b1f665..6085ec424a84 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -154,8 +154,12 @@ static int rmi_process_interrupt_requests(struct rmi_device *rmi_dev)
>  	 */
>  	mutex_unlock(&data->irq_mutex);
>  
> -	for_each_set_bit(i, data->irq_status, data->irq_count)
> -		handle_nested_irq(irq_find_mapping(data->irqdomain, i));
> +	for_each_set_bit(i, data->irq_status, data->irq_count) {
> +		unsigned int irq = irq_find_mapping(data->irqdomain, i);
> +
> +		if (irq)
> +			handle_nested_irq(irq);
> +	}
>  
>  	if (data->input)
>  		input_sync(data->input);

Re: [PATCH 4/5] input/rmi4/rmi_driver: check if irq_find_mapping returns 0

[Hans Verkuil]

On 11/19/19 12:38 PM, Lucas Stach wrote:
> Hi Hans,
> 
> On Di, 2019-11-19 at 11:51 +0100, Hans Verkuil wrote:
>> The irq_find_mapping() function can return 0 when called in the
>> rmi_process_interrupt_requests() function.
>>
>> This causes a kernel crash. Check for a 0 value and skip calling
>> handle_nested_irq() in that case.
>>
>> This was tested with the F54 function enabled on a Lenovo X1 Carbon.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Fixes: 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution to irq_domain")
> 
> This is already fixed upstream by 549766ac2ac1
> "Input: synaptics-rmi4 - clear IRQ enables for F54"

Good news. I'm not subscribed to the linux-input ML, so I never saw that.

Ah, I now see that I'm missing a whole bunch of patches that were added
after v5.4-rc1. I'll test this again next week (I don't have access to my
Lenovo at the moment).

Regards,

	Hans

> 
> Regards,
> Lucas
> 
>> ---
>>  drivers/input/rmi4/rmi_driver.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
>> index 772493b1f665..6085ec424a84 100644
>> --- a/drivers/input/rmi4/rmi_driver.c
>> +++ b/drivers/input/rmi4/rmi_driver.c
>> @@ -154,8 +154,12 @@ static int rmi_process_interrupt_requests(struct rmi_device *rmi_dev)
>>  	 */
>>  	mutex_unlock(&data->irq_mutex);
>>  
>> -	for_each_set_bit(i, data->irq_status, data->irq_count)
>> -		handle_nested_irq(irq_find_mapping(data->irqdomain, i));
>> +	for_each_set_bit(i, data->irq_status, data->irq_count) {
>> +		unsigned int irq = irq_find_mapping(data->irqdomain, i);
>> +
>> +		if (irq)
>> +			handle_nested_irq(irq);
>> +	}
>>  
>>  	if (data->input)
>>  		input_sync(data->input);
> 

Re: [PATCH 4/5] input/rmi4/rmi_driver: check if irq_find_mapping returns 0

[Hans Verkuil]

On 11/19/19 12:43 PM, Hans Verkuil wrote:
> On 11/19/19 12:38 PM, Lucas Stach wrote:
>> Hi Hans,
>>
>> On Di, 2019-11-19 at 11:51 +0100, Hans Verkuil wrote:
>>> The irq_find_mapping() function can return 0 when called in the
>>> rmi_process_interrupt_requests() function.
>>>
>>> This causes a kernel crash. Check for a 0 value and skip calling
>>> handle_nested_irq() in that case.
>>>
>>> This was tested with the F54 function enabled on a Lenovo X1 Carbon.
>>>
>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>> Fixes: 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution to irq_domain")
>>
>> This is already fixed upstream by 549766ac2ac1
>> "Input: synaptics-rmi4 - clear IRQ enables for F54"
> 
> Good news. I'm not subscribed to the linux-input ML, so I never saw that.
> 
> Ah, I now see that I'm missing a whole bunch of patches that were added
> after v5.4-rc1. I'll test this again next week (I don't have access to my
> Lenovo at the moment).

Tested with v5.4-rc8 and I can confirm that this patch is not needed anymore
and can be dropped.

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
>>
>> Regards,
>> Lucas
>>
>>> ---
>>>  drivers/input/rmi4/rmi_driver.c | 8 ++++++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
>>> index 772493b1f665..6085ec424a84 100644
>>> --- a/drivers/input/rmi4/rmi_driver.c
>>> +++ b/drivers/input/rmi4/rmi_driver.c
>>> @@ -154,8 +154,12 @@ static int rmi_process_interrupt_requests(struct rmi_device *rmi_dev)
>>>  	 */
>>>  	mutex_unlock(&data->irq_mutex);
>>>  
>>> -	for_each_set_bit(i, data->irq_status, data->irq_count)
>>> -		handle_nested_irq(irq_find_mapping(data->irqdomain, i));
>>> +	for_each_set_bit(i, data->irq_status, data->irq_count) {
>>> +		unsigned int irq = irq_find_mapping(data->irqdomain, i);
>>> +
>>> +		if (irq)
>>> +			handle_nested_irq(irq);
>>> +	}
>>>  
>>>  	if (data->input)
>>>  		input_sync(data->input);
>>
> 

[PATCH 5/5] input/rmi4/rmi_smbus.c: don't increment rmiaddr in rmi_smb_read_block()

[Hans Verkuil]

This increment of rmi_smbus causes garbage to be returned.
The first read of SMB_MAX_COUNT bytes is fine, but after that
it is nonsense. Trial-and-error showed that by dropping the
increment of rmiaddr everything is fine and the F54 function
properly works.

Even going back to the original code when F54 was added, I
could not make it work without this patch. So I do not understand
how this ever worked.

My guess is that the same change is needed in rmi_smb_write_block,
but I wouldn't know how to test this.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/input/rmi4/rmi_smbus.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
index 2407ea43de59..79ecea5edacc 100644
--- a/drivers/input/rmi4/rmi_smbus.c
+++ b/drivers/input/rmi4/rmi_smbus.c
@@ -215,7 +215,6 @@ static int rmi_smb_read_block(struct rmi_transport_dev *xport, u16 rmiaddr,
 		/* prepare to read next block of bytes */
 		cur_len -= SMB_MAX_COUNT;
 		databuff += SMB_MAX_COUNT;
-		rmiaddr += SMB_MAX_COUNT;
 	}
 
 	retval = 0;
-- 
2.23.0

Re: [PATCH 5/5] input/rmi4/rmi_smbus.c: don't increment rmiaddr in rmi_smb_read_block()

[Lucas Stach]

Hi Hans,

On Di, 2019-11-19 at 11:51 +0100, Hans Verkuil wrote:
> This increment of rmi_smbus causes garbage to be returned.
> The first read of SMB_MAX_COUNT bytes is fine, but after that
> it is nonsense. Trial-and-error showed that by dropping the
> increment of rmiaddr everything is fine and the F54 function
> properly works.
> 
> Even going back to the original code when F54 was added, I
> could not make it work without this patch. So I do not understand
> how this ever worked.

My guess is that F54 has mostly been tested with touchscreens that are
connected to a regular i2c bus, not smbus. i2c has a separate transport
implementation in rmi4. Most of the other functions are probably
reading much smaller block data than F54.

Regards,
Lucas

> My guess is that the same change is needed in rmi_smb_write_block,
> but I wouldn't know how to test this.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/input/rmi4/rmi_smbus.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
> index 2407ea43de59..79ecea5edacc 100644
> --- a/drivers/input/rmi4/rmi_smbus.c
> +++ b/drivers/input/rmi4/rmi_smbus.c
> @@ -215,7 +215,6 @@ static int rmi_smb_read_block(struct rmi_transport_dev *xport, u16 rmiaddr,
>  		/* prepare to read next block of bytes */
>  		cur_len -= SMB_MAX_COUNT;
>  		databuff += SMB_MAX_COUNT;
> -		rmiaddr += SMB_MAX_COUNT;
>  	}
>  
>  	retval = 0;

Re: [PATCH 5/5] input/rmi4/rmi_smbus.c: don't increment rmiaddr in rmi_smb_read_block()

[Hans Verkuil]

Hi Lucas,

On 11/19/19 12:48 PM, Lucas Stach wrote:
> Hi Hans,
> 
> On Di, 2019-11-19 at 11:51 +0100, Hans Verkuil wrote:
>> This increment of rmi_smbus causes garbage to be returned.
>> The first read of SMB_MAX_COUNT bytes is fine, but after that
>> it is nonsense. Trial-and-error showed that by dropping the
>> increment of rmiaddr everything is fine and the F54 function
>> properly works.
>>
>> Even going back to the original code when F54 was added, I
>> could not make it work without this patch. So I do not understand
>> how this ever worked.
> 
> My guess is that F54 has mostly been tested with touchscreens that are
> connected to a regular i2c bus, not smbus. i2c has a separate transport
> implementation in rmi4. Most of the other functions are probably
> reading much smaller block data than F54.

That's my suspicion as well. I tried to configure my kernel so that it
would be using i2c instead of smbus, but I couldn't make it work. I'll
have to try again next week.

I only have Rev E of the RMI4 spec, which does not appear to document
the mapping table entry that rmi_smb_get_command_code() uses.

Do you (or someone else) have access to a newer version? If so, can you check
how this is supposed to work for reading large blocks over smbus?

With the current code it will create a new mapping entry for every 32
byte read. I suspect that that is not how it is supposed to work, but
without a spec this is just trial-and-error.

Regards,

	Hans

> 
> Regards,
> Lucas
> 
>> My guess is that the same change is needed in rmi_smb_write_block,
>> but I wouldn't know how to test this.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/input/rmi4/rmi_smbus.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
>> index 2407ea43de59..79ecea5edacc 100644
>> --- a/drivers/input/rmi4/rmi_smbus.c
>> +++ b/drivers/input/rmi4/rmi_smbus.c
>> @@ -215,7 +215,6 @@ static int rmi_smb_read_block(struct rmi_transport_dev *xport, u16 rmiaddr,
>>  		/* prepare to read next block of bytes */
>>  		cur_len -= SMB_MAX_COUNT;
>>  		databuff += SMB_MAX_COUNT;
>> -		rmiaddr += SMB_MAX_COUNT;
>>  	}
>>  
>>  	retval = 0;
> 

[PATCHv2 5/5] input/rmi4/rmi_smbus.c: don't increment rmiaddr

[Hans Verkuil]

This increment of rmi_smbus in rmi_smb_read/write_block() causes
garbage to be read/written.

The first read of SMB_MAX_COUNT bytes is fine, but after that
it is nonsense. Trial-and-error showed that by dropping the
increment of rmiaddr everything is fine and the F54 function
properly works.

I tried a hack with rmi_smb_write_block() as well (writing to the
same F54 touchpad data area, then reading it back), and that
suggests that there too the rmiaddr increment has to be dropped.
It makes sense that if it has to be dropped for read, then it has
to be dropped for write as well.

It looks like the initial work with F54 was done using i2c, not smbus,
and it seems nobody ever tested F54 with smbus. The other functions
all read/write less than SMB_MAX_COUNT as far as I can tell, so this
issue was never noticed with non-F54 functions.

With this change I can read out the touchpad data correctly on my
Lenovo X1 Carbon 6th Gen laptop.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/input/rmi4/rmi_smbus.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
index 2407ea43de59..b313c579914f 100644
--- a/drivers/input/rmi4/rmi_smbus.c
+++ b/drivers/input/rmi4/rmi_smbus.c
@@ -163,7 +163,6 @@ static int rmi_smb_write_block(struct rmi_transport_dev *xport, u16 rmiaddr,
 		/* prepare to write next block of bytes */
 		cur_len -= SMB_MAX_COUNT;
 		databuff += SMB_MAX_COUNT;
-		rmiaddr += SMB_MAX_COUNT;
 	}
 exit:
 	mutex_unlock(&rmi_smb->page_mutex);
@@ -215,7 +214,6 @@ static int rmi_smb_read_block(struct rmi_transport_dev *xport, u16 rmiaddr,
 		/* prepare to read next block of bytes */
 		cur_len -= SMB_MAX_COUNT;
 		databuff += SMB_MAX_COUNT;
-		rmiaddr += SMB_MAX_COUNT;
 	}

 	retval = 0;
-- 
2.24.0

Re: [PATCHv2 5/5] input/rmi4/rmi_smbus.c: don't increment rmiaddr

[Dmitry Torokhov]

On Sat, Nov 23, 2019 at 05:27:41PM +0100, Hans Verkuil wrote:
> This increment of rmi_smbus in rmi_smb_read/write_block() causes
> garbage to be read/written.
> 
> The first read of SMB_MAX_COUNT bytes is fine, but after that
> it is nonsense. Trial-and-error showed that by dropping the
> increment of rmiaddr everything is fine and the F54 function
> properly works.
> 
> I tried a hack with rmi_smb_write_block() as well (writing to the
> same F54 touchpad data area, then reading it back), and that
> suggests that there too the rmiaddr increment has to be dropped.
> It makes sense that if it has to be dropped for read, then it has
> to be dropped for write as well.
> 
> It looks like the initial work with F54 was done using i2c, not smbus,
> and it seems nobody ever tested F54 with smbus. The other functions
> all read/write less than SMB_MAX_COUNT as far as I can tell, so this
> issue was never noticed with non-F54 functions.
> 
> With this change I can read out the touchpad data correctly on my
> Lenovo X1 Carbon 6th Gen laptop.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Applied, thank you.

> ---
>  drivers/input/rmi4/rmi_smbus.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
> index 2407ea43de59..b313c579914f 100644
> --- a/drivers/input/rmi4/rmi_smbus.c
> +++ b/drivers/input/rmi4/rmi_smbus.c
> @@ -163,7 +163,6 @@ static int rmi_smb_write_block(struct rmi_transport_dev *xport, u16 rmiaddr,
>  		/* prepare to write next block of bytes */
>  		cur_len -= SMB_MAX_COUNT;
>  		databuff += SMB_MAX_COUNT;
> -		rmiaddr += SMB_MAX_COUNT;
>  	}
>  exit:
>  	mutex_unlock(&rmi_smb->page_mutex);
> @@ -215,7 +214,6 @@ static int rmi_smb_read_block(struct rmi_transport_dev *xport, u16 rmiaddr,
>  		/* prepare to read next block of bytes */
>  		cur_len -= SMB_MAX_COUNT;
>  		databuff += SMB_MAX_COUNT;
> -		rmiaddr += SMB_MAX_COUNT;
>  	}
> 
>  	retval = 0;
> -- 
> 2.24.0
> 
> 

-- 
Dmitry