Re: [PATCH v4 9/9] Input: synaptics-rmi4 - add support for F54 diagnostics

[Nick Dyer]

On 20/06/2016 17:20, Hans Verkuil wrote:
> On 06/17/2016 04:16 PM, Nick Dyer wrote:
>> +static int rmi_f54_vidioc_enum_input(struct file *file, void *priv,
>> +				     struct v4l2_input *i)
>> +{
>> +	struct f54_data *f54 = video_drvdata(file);
>> +	enum f54_report_type reptype;
>> +
>> +	reptype = rmi_f54_get_reptype(f54, i->index);
>> +	if (reptype == F54_REPORT_NONE)
>> +		return -EINVAL;
>> +
>> +	i->type = V4L2_INPUT_TYPE_TOUCH_SENSOR;
>> +	strlcpy(i->name, rmi_f54_reptype_str(reptype), sizeof(i->name));
> 
> Hmm, this doesn't feel right, but I don't have enough knowledge to decide if
> using inputs for this is the right approach.
> 
> One thing that strikes me as odd is that both F54_8BIT_IMAGE and F54_16BIT_IMAGE
> both seem to return signed 16 bit samples. I would expect this to result in
> different pixel formats.

Yes, you're right. I've had a look at this area now and fixed it up. I've
added pixel formats for the various types of touch data so we will never
emit anything that's not under V4L2_TCH_FMT.

I've also worked on adding the DocBook documentation that was missing,
cribbing from SDR as you suggested. I think I've found all the places it
was needed now.

> I have no idea what all these inputs mean. Are they all actually needed?
> Would this perhaps be better implemented through a menu control?

These are the various types of tuning or fault finding data that users
expect to be able to access via the RMI4 F54 diagnostics function. Although
I would have to do some digging to give an detailed use case for each one.
Due to the way that F54 is implemented, only one data source can be
selected at once. So it seemed fairly reasonable to me to map it to inputs.

> I generally go by the philosophy that if I can't understand it, then it is
> likely that others won't either :-)

No problem.