Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S942313AbcJ0RDb (ORCPT ); Thu, 27 Oct 2016 13:03:31 -0400 Received: from arroyo.ext.ti.com ([198.47.19.12]:39283 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S941586AbcJ0RD2 (ORCPT ); Thu, 27 Oct 2016 13:03:28 -0400 Subject: Re: [PATCH 3/4] Input: goodix - Tweak configuration to use passed in touchscreen resolution To: Bastien Nocera , , , , , , , , , , References: <20161020195917.20051-1-fcooper@ti.com> <20161020195917.20051-4-fcooper@ti.com> <1477564458.2458.10.camel@hadess.net> From: Franklin S Cooper Jr Message-ID: <650a47a2-d854-f846-082b-acf5b2497cf5@ti.com> Date: Thu, 27 Oct 2016 12:03:10 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1477564458.2458.10.camel@hadess.net> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [128.247.83.33] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4022 Lines: 122 On 10/27/2016 05:34 AM, Bastien Nocera wrote: > On Thu, 2016-10-20 at 14:59 -0500, Franklin S Cooper Jr wrote: >> Some goodix touchscreen controllers aren't programed properly to >> match the >> display panel it is used on. Although the defaults may largely work >> it is >> also likely that the screen resolution will be incorrect. Therefore, >> add support to allow via DT for the touchscreen resolution to be set >> and >> reprogram the controller to use this updated resolution. >> >> Signed-off-by: Franklin S Cooper Jr >> --- >> .../bindings/input/touchscreen/goodix.txt | 2 ++ >> drivers/input/touchscreen/goodix.c | 26 >> +++++++++++++++++++++- >> 2 files changed, 27 insertions(+), 1 deletion(-) >> >> diff --git >> a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt >> b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt >> index c98757a..ebc7cb7 100644 >> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt >> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt >> @@ -19,6 +19,8 @@ Optional properties: >> interrupt gpio pin as output to reset the >> device. >> - reset-gpios : GPIO pin used for reset >> >> + - touchscreen-size-x : horizontal resolution of touchscreen >> (in pixels) >> + - touchscreen-size-y : vertical resolution of touchscreen (in >> pixels) >> - touchscreen-inverted-x : X axis is inverted (boolean) >> - touchscreen-inverted-y : Y axis is inverted (boolean) >> - touchscreen-swapped-x-y : X and Y axis are swapped (boolean) >> diff --git a/drivers/input/touchscreen/goodix.c >> b/drivers/input/touchscreen/goodix.c >> index 01e12f8..c2428e1 100644 >> --- a/drivers/input/touchscreen/goodix.c >> +++ b/drivers/input/touchscreen/goodix.c >> @@ -496,6 +496,10 @@ static void goodix_tweak_config(struct >> goodix_ts_data *ts) >> return; >> } >> >> + /* Setting X and Y Resolution */ >> + put_unaligned_le16(ts->abs_x_max, &config[RESOLUTION_LOC]); >> + put_unaligned_le16(ts->abs_y_max, &config[RESOLUTION_LOC + >> 2]); >> + >> check_sum = goodix_calculate_checksum(ts->cfg_len, config); >> >> config[raw_cfg_len] = check_sum; >> @@ -669,6 +673,8 @@ static int goodix_request_input_dev(struct >> goodix_ts_data *ts) >> static int goodix_configure_dev(struct goodix_ts_data *ts) >> { >> int error; >> + bool alter_config = false; >> + u32 max_x, max_y; >> >> ts->swapped_x_y = device_property_read_bool(&ts->client- >>> dev, >> "touchscreen- >> swapped-x-y"); >> @@ -676,9 +682,27 @@ static int goodix_configure_dev(struct >> goodix_ts_data *ts) >> "touchscreen- >> inverted-x"); >> ts->inverted_y = device_property_read_bool(&ts->client->dev, >> "touchscreen- >> inverted-y"); >> - > > No need for that linefeed removal. > >> goodix_read_config(ts); >> >> + if (device_property_present(&ts->client->dev, "touchscreen- >> size-x") && > > Is it expected that incomplete or invalid arguments do not generate > errors? I'd expect that the presence of just one of those properties, > or... Looking back at this there is technically nothing wrong with only wanting to set 1 property. So I will update this to decouple these two properties. > >> + device_property_present(&ts->client->dev, "touchscreen- >> size-y")) { >> + >> + device_property_read_u32(&ts->client->dev, >> "touchscreen-size-x", >> + &max_x); >> + >> + device_property_read_u32(&ts->client->dev, >> "touchscreen-size-y", >> + &max_y); >> + >> + if (max_x > 0 && max_y > 0) { > > ... invalid values for those properties would throw errors (either > warnings, or lower severity messages). I'll add a warning here indicating that the passed in value with be ignored. > >> + ts->abs_x_max = max_x; >> + ts->abs_y_max = max_y; >> + alter_config = true; >> + } >> + } >> + >> + if (alter_config) >> + goodix_tweak_config(ts); >> + >> error = goodix_request_input_dev(ts); >> if (error) >> return error;