Re: [PATCH] Input: goodix - use generic touchscreen_properties

Marcin Niestroj
2017-10-26T13:50:10Z

On 26.10.2017 15:02, Bastien Nocera wrote:
> On Thu, 2017-10-26 at 10:14 +0200, Marcin Niestroj wrote:
>> Hi Bastien,
>>
>> On 25.10.2017 21:42, Bastien Nocera wrote:
>>> Hey Marcin,
>>>
>>> On Wed, 2017-10-25 at 13:32 +0200, Marcin Niestroj wrote:
>>>> Use touchscreen_properties structure instead of implementing all
>>>> properties by our own. It allows to reuse generic code for
>>>> parsing
>>>> device-tree properties (which was implemented manually in the
>>>> driver
>>>> for now). Additionally, it allows us to report events using
>>>> generic
>>>> touchscreen_report_pos(), which automatically handles inverted
>>>> and
>>>> swapped axes.
>>>>
>>>> There was also bug in driver in touch position calculation, when
>>>> axes
>>>> were configured as inverted and swapped in the same time. This is
>>>> however fixed now, by using touchscreen_report_pos() function,
>>>> which
>>>> handles inversion+swapping correctly.
>>>
>>> <snip>
>>>> @@ -579,6 +568,7 @@ static int goodix_get_gpio_config(struct
>>>> goodix_ts_data *ts)
>>>> static void goodix_read_config(struct goodix_ts_data *ts)
>>>> {
>>>> u8 config[GOODIX_CONFIG_MAX_LENGTH];
>>>> + int x_max, y_max;
>>>> int error;
>>>>
>>>> error = goodix_i2c_read(ts->client, ts->chip-
>>>>> config_addr,
>>>> @@ -587,37 +577,34 @@ static void goodix_read_config(struct
>>>> goodix_ts_data *ts)
>>>> dev_warn(&ts->client->dev,
>>>> "Error reading config (%d), using
>>>> defaults\n",
>>>> error);
>>>> - ts->abs_x_max = GOODIX_MAX_WIDTH;
>>>> - ts->abs_y_max = GOODIX_MAX_HEIGHT;
>>>> - if (ts->swapped_x_y)
>>>> - swap(ts->abs_x_max, ts->abs_y_max);
>>>> + x_max = GOODIX_MAX_WIDTH;
>>>> + y_max = GOODIX_MAX_HEIGHT;
>>>
>>> When do you swap those out if necessary?
>>
>> Swapping axes is implemented in of_touchscreen.c. This includes
>> swapping
>> during event reporting, as well as during touchscreen width and
>> height
>> reporting during initialization.
>
> But this isn't swapped or rotated when the device's range is set, is
> it?
> + input_set_abs_params(ts->input_dev, ABS_MT_POSITION_X,
> + 0, x_max - 1, 0, 0);
> + input_set_abs_params(ts->input_dev, ABS_MT_POSITION_Y,
> + 0, y_max - 1, 0, 0);
>

Not sure I understand your question. You mean at this specific point
in code? No it is not. But this is how it should be done I think.
Swapping range (x_max, y_max) occurs at the end of
touchscreen_parse_properties().

> <snip>
>>>
>> You are right, I should mention this. I've searched through several
>> touchscreen drivers and I think this is how it should be. Am I right?
>> If you confirm, I will split it off in next patch version.
>>
>> Should we split fixing inverted+swapped axes as well? This is fixed
>> by
>> this patch right now, by reusing code in of_touchscreen.c. Below is a
>> patch, that fixes inversion+swapping, by not using of_touchscreen.c.
>> Should I add that to the patch set, so it could be backported to
>> stable
>> releases?
>
> That would be great, though I'm not sure whether the gt1151 support
> would get backported.
>

gt1151 patch should not be needed for that fix to apply cleanly.

>>> Looks good overall, but was this tested, and if so, on which
>>> device?
>>> Could you add a reference to the hardware used for testing in the
>>> commit log?
>>
>> I just have a custom hardware (prototype) with gt1151. Should I
>> mention
>> this in commit log as well?
>
> That would be useful, yes. The fact that it's a device-tree based
> device would also be good to mention.
>

Will do so.

--
Marcin Niestroj