Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1431950 > unrolled thread

Re: [PATCH v5 0/9] Output raw touch data via V4L2

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 0/9] Output raw touch data via V4L2 Hans Verkuil <hverkuil@xs4all.nl> - 2016-06-27 13:30 +0200
    Re: [PATCH v5 0/9] Output raw touch data via V4L2 Nick Dyer <nick.dyer@itdev.co.uk> - 2016-06-27 14:00 +0200
      Re: [PATCH v5 0/9] Output raw touch data via V4L2 Hans Verkuil <hverkuil@xs4all.nl> - 2016-06-27 14:30 +0200
        Re: [PATCH v5 0/9] Output raw touch data via V4L2 Nick Dyer <nick.dyer@itdev.co.uk> - 2016-06-27 15:00 +0200

#1431950 — Re: [PATCH v5 0/9] Output raw touch data via V4L2

On 06/23/2016 12:08 AM, Nick Dyer wrote:
> This is a series of patches to add output of raw touch diagnostic data via V4L2
> to the Atmel maXTouch and Synaptics RMI4 drivers.
> 
> It's a rewrite of the previous implementation which output via debugfs: it now
> uses a V4L2 device in a similar way to the sur40 driver.
> 
> We have a utility which can read the data and display it in a useful format:
>     https://github.com/ndyer/heatmap/commits/heatmap-v4l
> 
> These patches are also available from
>     https://github.com/ndyer/linux/commits/v4l-touch-2016-06-22
> 
> Changes in v5 (Hans Verkuil review):
> - Update v4l2-core:
>   - Add VFL_TYPE_TOUCH, V4L2_BUF_TYPE_TOUCH_CAPTURE and V4L2_CAP_TOUCH

The use of V4L2_CAP_TOUCH and V4L2_BUF_TYPE_TOUCH_CAPTURE is very inconsistent.
What is the rationale of adding V4L2_BUF_TYPE_TOUCH_CAPTURE? I can't remember
asking for it.

And wouldn't the use of V4L2_BUF_TYPE_TOUCH_CAPTURE break userspace for sur40?

I'm ambiguous towards having a V4L2_BUF_TYPE_TOUCH_CAPTURE, to be honest.

I would also recommend renaming V4L2_CAP_TOUCH to V4L2_CAP_TOUCH_CAPTURE.

I can imagine an embedded usb gadget device that outputs touch data to a PC.

Regards,

	Hans

>   - Change V4L2_INPUT_TYPE_TOUCH_SENSOR to V4L2_INPUT_TYPE_TOUCH
>   - Improve DocBook documentation
>   - Add FMT definitions for touch data
>   - Note this will need the latest version of the heatmap util
> - Synaptics RMI4 driver:
>   - Remove some less important non full frame report types
>   - Switch report type names to const char * array
>   - Move a static array to inside context struct
> - Split sur40 changes to a separate commit
> 
> Changes in v4:
> - Address nits from the input side in atmel_mxt_ts patches (Dmitry Torokhov)
> - Add Synaptics RMI4 F54 support patch
> 
> Changes in v3:
> - Address V4L2 review comments from Hans Verkuil
> - Run v4l-compliance and fix all issues - needs minor patch here:
>   https://github.com/ndyer/v4l-utils/commit/cf50469773f
> 
> Changes in v2:
> - Split pixfmt changes into separate commit and add DocBook
> - Introduce VFL_TYPE_TOUCH_SENSOR and /dev/v4l-touch
> - Remove "single node" support for now, it may be better to treat it as
>   metadata later
> - Explicitly set VFL_DIR_RX
> - Fix Kconfig
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [next] | [standalone]

#1431974

Hi Hans-

Thanks for reviewing this again in such detail.

On 27/06/2016 12:26, Hans Verkuil wrote:
> On 06/23/2016 12:08 AM, Nick Dyer wrote:
>> This is a series of patches to add output of raw touch diagnostic data via V4L2
>> to the Atmel maXTouch and Synaptics RMI4 drivers.
>>
>> It's a rewrite of the previous implementation which output via debugfs: it now
>> uses a V4L2 device in a similar way to the sur40 driver.
>>
>> We have a utility which can read the data and display it in a useful format:
>>     https://github.com/ndyer/heatmap/commits/heatmap-v4l
>>
>> These patches are also available from
>>     https://github.com/ndyer/linux/commits/v4l-touch-2016-06-22
>>
>> Changes in v5 (Hans Verkuil review):
>> - Update v4l2-core:
>>   - Add VFL_TYPE_TOUCH, V4L2_BUF_TYPE_TOUCH_CAPTURE and V4L2_CAP_TOUCH
> 
> The use of V4L2_CAP_TOUCH and V4L2_BUF_TYPE_TOUCH_CAPTURE is very inconsistent.
> What is the rationale of adding V4L2_BUF_TYPE_TOUCH_CAPTURE? I can't remember
> asking for it.

I am afraid that I missed updating atmel_mxt_ts from
V4L2_BUF_TYPE_VIDEO_CAPTURE to V4L2_BUF_TYPE_TOUCH_CAPTURE, which has
confused the situation.

Perhaps I read too much into your request that I look at the way that SDR
is treated. When I started going through the code paths in v4l2-core and
v4l2-compliance, it seemed cleaner to treat touch as completely separate,
hence introducing the new BUF_TYPE. I'm happy to try it without this.

> And wouldn't the use of V4L2_BUF_TYPE_TOUCH_CAPTURE break userspace for sur40?

I think it is likely, yes. And it looks like that would make Florian unhappy.

> I'm ambiguous towards having a V4L2_BUF_TYPE_TOUCH_CAPTURE, to be honest.
> 
> I would also recommend renaming V4L2_CAP_TOUCH to V4L2_CAP_TOUCH_CAPTURE.

Do you agree with the following changes:

- Rename V4L2_CAP_TOUCH to V4L2_CAP_TOUCH_CAPTURE.

- Touch devices should register both V4L2_CAP_VIDEO_CAPTURE and
V4L2_CAP_TOUCH_CAPTURE.

- Get rid of V4L2_BUF_TYPE_TOUCH_CAPTURE and use
V4L2_BUF_TYPE_VIDEO_CAPTURE. In v4l2-ioctl.c if we need to force particular
pix formats for touch, it will need to look at V4L2_CAP_TOUCH_CAPTURE.

Your other review comments look straightforward to address - thanks.

I should say, you can see my current changes to v4l2-compliance here:
https://github.com/ndyer/v4l-utils/commit/07e00c33

Should I post them along with the kernel patches next time?

> 
> I can imagine an embedded usb gadget device that outputs touch data to a PC.
> 
> Regards,
> 
> 	Hans

[toc] | [prev] | [next] | [standalone]

#1431992

On 06/27/2016 01:57 PM, Nick Dyer wrote:
> Hi Hans-
> 
> Thanks for reviewing this again in such detail.
> 
> On 27/06/2016 12:26, Hans Verkuil wrote:
>> On 06/23/2016 12:08 AM, Nick Dyer wrote:
>>> This is a series of patches to add output of raw touch diagnostic data via V4L2
>>> to the Atmel maXTouch and Synaptics RMI4 drivers.
>>>
>>> It's a rewrite of the previous implementation which output via debugfs: it now
>>> uses a V4L2 device in a similar way to the sur40 driver.
>>>
>>> We have a utility which can read the data and display it in a useful format:
>>>     https://github.com/ndyer/heatmap/commits/heatmap-v4l
>>>
>>> These patches are also available from
>>>     https://github.com/ndyer/linux/commits/v4l-touch-2016-06-22
>>>
>>> Changes in v5 (Hans Verkuil review):
>>> - Update v4l2-core:
>>>   - Add VFL_TYPE_TOUCH, V4L2_BUF_TYPE_TOUCH_CAPTURE and V4L2_CAP_TOUCH
>>
>> The use of V4L2_CAP_TOUCH and V4L2_BUF_TYPE_TOUCH_CAPTURE is very inconsistent.
>> What is the rationale of adding V4L2_BUF_TYPE_TOUCH_CAPTURE? I can't remember
>> asking for it.
> 
> I am afraid that I missed updating atmel_mxt_ts from
> V4L2_BUF_TYPE_VIDEO_CAPTURE to V4L2_BUF_TYPE_TOUCH_CAPTURE, which has
> confused the situation.
> 
> Perhaps I read too much into your request that I look at the way that SDR
> is treated. When I started going through the code paths in v4l2-core and
> v4l2-compliance, it seemed cleaner to treat touch as completely separate,
> hence introducing the new BUF_TYPE. I'm happy to try it without this.

Yeah, I didn't mean that you had to add a new BUF_TYPE. My remark was related to
ensuring that all occurrences in the spec where they talk about the various
/dev/video/radio/etc. devices are extended with v4l-touch as well.

> 
>> And wouldn't the use of V4L2_BUF_TYPE_TOUCH_CAPTURE break userspace for sur40?
> 
> I think it is likely, yes. And it looks like that would make Florian unhappy.
> 
>> I'm ambiguous towards having a V4L2_BUF_TYPE_TOUCH_CAPTURE, to be honest.
>>
>> I would also recommend renaming V4L2_CAP_TOUCH to V4L2_CAP_TOUCH_CAPTURE.
> 
> Do you agree with the following changes:
> 
> - Rename V4L2_CAP_TOUCH to V4L2_CAP_TOUCH_CAPTURE.
> 
> - Touch devices should register both V4L2_CAP_VIDEO_CAPTURE and
> V4L2_CAP_TOUCH_CAPTURE.
> 
> - Get rid of V4L2_BUF_TYPE_TOUCH_CAPTURE and use
> V4L2_BUF_TYPE_VIDEO_CAPTURE. In v4l2-ioctl.c if we need to force particular
> pix formats for touch, it will need to look at V4L2_CAP_TOUCH_CAPTURE.

Actually, I think we have two choices, depending on whether we use a
BUF_TYPE_TOUCH_CAPTURE or not.

1) If we go with a BUF_TYPE_TOUCH_CAPTURE, then:

- we need a V4L2_CAP_TOUCH_CAPTURE
- no V4L2_CAP_VIDEO_CAPTURE will be set (since that would indicate support for
  BUF_TYPE_VIDEO_CAPTURE, which we don't have anymore).
- new callbacks for g/s/try/enum_fmt_tch_cap should be added to v4l2-ioctl.h.

2) Alternatively, if we want to keep using BUF_TYPE_VIDEO_CAPTURE, then:

- we keep V4L2_CAP_TOUCH which is combined with CAP_VIDEO_CAPTURE (and perhaps
  VIDEO_OUTPUT in the future). The CAP_TOUCH just says that this is a touch
  device, not a video device, but otherwise it acts the same.

I'd go with 2, since I see no reason to add a new BUF_TYPE for this.
It acts exactly like video after all, with only a few restrictions (i.e. no
colorspace info or interlaced). And adding a new BUF_TYPE will likely break
the existing sur40 app.

> 
> Your other review comments look straightforward to address - thanks.
> 
> I should say, you can see my current changes to v4l2-compliance here:
> https://github.com/ndyer/v4l-utils/commit/07e00c33
> 
> Should I post them along with the kernel patches next time?

Yes, please.

Regards,

	Hans

> 
>>
>> I can imagine an embedded usb gadget device that outputs touch data to a PC.
>>
>> Regards,
>>
>> 	Hans
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [prev] | [next] | [standalone]

#1432007

On 27/06/2016 13:22, Hans Verkuil wrote:
> On 06/27/2016 01:57 PM, Nick Dyer wrote:
> 2) Alternatively, if we want to keep using BUF_TYPE_VIDEO_CAPTURE, then:
> 
> - we keep V4L2_CAP_TOUCH which is combined with CAP_VIDEO_CAPTURE (and perhaps
>   VIDEO_OUTPUT in the future). The CAP_TOUCH just says that this is a touch
>   device, not a video device, but otherwise it acts the same.
> 
> I'd go with 2, since I see no reason to add a new BUF_TYPE for this.
> It acts exactly like video after all, with only a few restrictions (i.e. no
> colorspace info or interlaced). And adding a new BUF_TYPE will likely break
> the existing sur40 app.

OK, I will rework with this approach.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web