Re: [PATCH v4 8/9] Input: atmel_mxt_ts - add support for reference data

2016-06-20 Thread Hans Verkuil 
On 06/20/2016 06:18 PM, Nick Dyer wrote:
> On 20/06/2016 17:09, Hans Verkuil wrote:
>> On 06/17/2016 04:16 PM, Nick Dyer wrote:
>>> @@ -2325,11 +2344,20 @@ static int mxt_vidioc_querycap(struct file *file, 
>>> void *priv,
>>>  static int mxt_vidioc_enum_input(struct file *file, void *priv,
>>>struct v4l2_input *i)
>>>  {
>>> -   if (i->index > 0)
>>> +   if (i->index >= MXT_V4L_INPUT_MAX)
>>> return -EINVAL;
>>>  
>>> i->type = V4L2_INPUT_TYPE_TOUCH_SENSOR;
>>> -   strlcpy(i->name, "Mutual References", sizeof(i->name));
>>> +
>>> +   switch (i->index) {
>>> +   case MXT_V4L_INPUT_REFS:
>>> +   strlcpy(i->name, "Mutual References", sizeof(i->name));
>>> +   break;
>>> +   case MXT_V4L_INPUT_DELTAS:
>>> +   strlcpy(i->name, "Mutual Deltas", sizeof(i->name));
>>
>> I don't think this name is very clear. I have no idea how to interpret 
>> "Mutual"
>> in this context.
> 
> "Mutual" is a touch domain specific term, it means the delta value is for
> the capacitance between the horizontal and vertical lines at a particular
> "node" on the touchscreen matrix, see
> https://en.wikipedia.org/wiki/Touchscreen#Mutual_capacitance
> 
> I'll put in a comment.

As I mentioned in an earlier review, we need a v4l-touch interface description 
anyway.
I think it might be useful to describe some of these touch-specific terms there.
That way that could be a useful reference for end-users.

Nobody reads comments, but people do read the spec (well, I do).

Regards,

Hans

> 
>>
>>> +   break;
>>> +   }
>>> +
>>> return 0;
>>>  }
>>>  
>>> @@ -2337,12 +2365,16 @@ static int mxt_set_input(struct mxt_data *data, 
>>> unsigned int i)
>>>  {
>>> struct v4l2_pix_format *f = >dbg.format;
>>>  
>>> -   if (i > 0)
>>> +   if (i >= MXT_V4L_INPUT_MAX)
>>> return -EINVAL;
>>>  
>>> +   if (i == MXT_V4L_INPUT_DELTAS)
>>> +   f->pixelformat = V4L2_PIX_FMT_YS16;
>>> +   else
>>> +   f->pixelformat = V4L2_PIX_FMT_Y16;
>>
>> You probably need a V4L2_TOUCH_FMT_U16 or something for this as well. It 
>> certainly
>> needs to be documented.
> 
> OK, will change this.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 8/9] Input: atmel_mxt_ts - add support for reference data

2016-06-20 Thread Nick Dyer 
On 20/06/2016 17:09, Hans Verkuil wrote:
> On 06/17/2016 04:16 PM, Nick Dyer wrote:
>> @@ -2325,11 +2344,20 @@ static int mxt_vidioc_querycap(struct file *file, 
>> void *priv,
>>  static int mxt_vidioc_enum_input(struct file *file, void *priv,
>> struct v4l2_input *i)
>>  {
>> -if (i->index > 0)
>> +if (i->index >= MXT_V4L_INPUT_MAX)
>>  return -EINVAL;
>>  
>>  i->type = V4L2_INPUT_TYPE_TOUCH_SENSOR;
>> -strlcpy(i->name, "Mutual References", sizeof(i->name));
>> +
>> +switch (i->index) {
>> +case MXT_V4L_INPUT_REFS:
>> +strlcpy(i->name, "Mutual References", sizeof(i->name));
>> +break;
>> +case MXT_V4L_INPUT_DELTAS:
>> +strlcpy(i->name, "Mutual Deltas", sizeof(i->name));
> 
> I don't think this name is very clear. I have no idea how to interpret 
> "Mutual"
> in this context.

"Mutual" is a touch domain specific term, it means the delta value is for
the capacitance between the horizontal and vertical lines at a particular
"node" on the touchscreen matrix, see
https://en.wikipedia.org/wiki/Touchscreen#Mutual_capacitance

I'll put in a comment.

> 
>> +break;
>> +}
>> +
>>  return 0;
>>  }
>>  
>> @@ -2337,12 +2365,16 @@ static int mxt_set_input(struct mxt_data *data, 
>> unsigned int i)
>>  {
>>  struct v4l2_pix_format *f = >dbg.format;
>>  
>> -if (i > 0)
>> +if (i >= MXT_V4L_INPUT_MAX)
>>  return -EINVAL;
>>  
>> +if (i == MXT_V4L_INPUT_DELTAS)
>> +f->pixelformat = V4L2_PIX_FMT_YS16;
>> +else
>> +f->pixelformat = V4L2_PIX_FMT_Y16;
> 
> You probably need a V4L2_TOUCH_FMT_U16 or something for this as well. It 
> certainly
> needs to be documented.

OK, will change this.

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 8/9] Input: atmel_mxt_ts - add support for reference data

2016-06-20 Thread Hans Verkuil 
On 06/17/2016 04:16 PM, Nick Dyer wrote:
> There are different datatypes available from a maXTouch chip. Add
> support to retrieve reference data as well.
> 
> Signed-off-by: Nick Dyer 
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 58 
> 
>  1 file changed, 51 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c 
> b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 5281325d..bb01fb0 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -135,6 +135,7 @@ struct t9_range {
>  /* MXT_DEBUG_DIAGNOSTIC_T37 */
>  #define MXT_DIAGNOSTIC_PAGEUP0x01
>  #define MXT_DIAGNOSTIC_DELTAS0x10
> +#define MXT_DIAGNOSTIC_REFS  0x11
>  #define MXT_DIAGNOSTIC_SIZE  128
>  
>  #define MXT_FAMILY_1386  160
> @@ -249,6 +250,12 @@ struct mxt_dbg {
>   int input;
>  };
>  
> +enum v4l_dbg_inputs {
> + MXT_V4L_INPUT_DELTAS,
> + MXT_V4L_INPUT_REFS,
> + MXT_V4L_INPUT_MAX,
> +};
> +
>  static const struct v4l2_file_operations mxt_video_fops = {
>   .owner = THIS_MODULE,
>   .open = v4l2_fh_open,
> @@ -2273,6 +2280,7 @@ static void mxt_buffer_queue(struct vb2_buffer *vb)
>   struct mxt_data *data = vb2_get_drv_priv(vb->vb2_queue);
>   u16 *ptr;
>   int ret;
> + u8 mode;
>  
>   ptr = vb2_plane_vaddr(vb, 0);
>   if (!ptr) {
> @@ -2280,7 +2288,18 @@ static void mxt_buffer_queue(struct vb2_buffer *vb)
>   goto fault;
>   }
>  
> - ret = mxt_read_diagnostic_debug(data, MXT_DIAGNOSTIC_DELTAS, ptr);
> + switch (data->dbg.input) {
> + case MXT_V4L_INPUT_DELTAS:
> + default:
> + mode = MXT_DIAGNOSTIC_DELTAS;
> + break;
> +
> + case MXT_V4L_INPUT_REFS:
> + mode = MXT_DIAGNOSTIC_REFS;
> + break;
> + }
> +
> + ret = mxt_read_diagnostic_debug(data, mode, ptr);
>   if (ret)
>   goto fault;
>  
> @@ -2325,11 +2344,20 @@ static int mxt_vidioc_querycap(struct file *file, 
> void *priv,
>  static int mxt_vidioc_enum_input(struct file *file, void *priv,
>  struct v4l2_input *i)
>  {
> - if (i->index > 0)
> + if (i->index >= MXT_V4L_INPUT_MAX)
>   return -EINVAL;
>  
>   i->type = V4L2_INPUT_TYPE_TOUCH_SENSOR;
> - strlcpy(i->name, "Mutual References", sizeof(i->name));
> +
> + switch (i->index) {
> + case MXT_V4L_INPUT_REFS:
> + strlcpy(i->name, "Mutual References", sizeof(i->name));
> + break;
> + case MXT_V4L_INPUT_DELTAS:
> + strlcpy(i->name, "Mutual Deltas", sizeof(i->name));

I don't think this name is very clear. I have no idea how to interpret "Mutual"
in this context.

> + break;
> + }
> +
>   return 0;
>  }
>  
> @@ -2337,12 +2365,16 @@ static int mxt_set_input(struct mxt_data *data, 
> unsigned int i)
>  {
>   struct v4l2_pix_format *f = >dbg.format;
>  
> - if (i > 0)
> + if (i >= MXT_V4L_INPUT_MAX)
>   return -EINVAL;
>  
> + if (i == MXT_V4L_INPUT_DELTAS)
> + f->pixelformat = V4L2_PIX_FMT_YS16;
> + else
> + f->pixelformat = V4L2_PIX_FMT_Y16;

You probably need a V4L2_TOUCH_FMT_U16 or something for this as well. It 
certainly
needs to be documented.

Regards,

Hans

> +
>   f->width = data->xy_switch ? data->ysize : data->xsize;
>   f->height = data->xy_switch ? data->xsize : data->ysize;
> - f->pixelformat = V4L2_PIX_FMT_YS16;
>   f->field = V4L2_FIELD_NONE;
>   f->colorspace = V4L2_COLORSPACE_RAW;
>   f->bytesperline = f->width * sizeof(u16);
> @@ -2380,10 +2412,22 @@ static int mxt_vidioc_fmt(struct file *file, void 
> *priv, struct v4l2_format *f)
>  static int mxt_vidioc_enum_fmt(struct file *file, void *priv,
>struct v4l2_fmtdesc *fmt)
>  {
> - if (fmt->index > 0 || fmt->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> + if (fmt->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> + return -EINVAL;
> +
> + switch (fmt->index) {
> + case 0:
> + fmt->pixelformat = V4L2_PIX_FMT_Y16;
> + break;
> +
> + case 1:
> + fmt->pixelformat = V4L2_PIX_FMT_YS16;
> + break;
> +
> + default:
>   return -EINVAL;
> + }
>  
> - fmt->pixelformat = V4L2_PIX_FMT_YS16;
>   return 0;
>  }
>  
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html