Re: [PATCH v2 02/11] Input: goodix - Make loading the config from disk independent from the GPIO setup

From: Hans de Goede <hdegoede@redhat.com>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Bastien Nocera <hadess@hadess.net>,
	linux-input@vger.kernel.org, Dmitry Mastykin <mastichi@gmail.com>
Subject: Re: [PATCH v2 02/11] Input: goodix - Make loading the config from disk independent from the GPIO setup
Date: Fri, 6 Mar 2020 10:01:51 +0100	[thread overview]
Message-ID: <c332be91-4487-fd8c-9a53-abf87ec32ecc@redhat.com> (raw)
In-Reply-To: <20200306040427.GC217608@dtor-ws>

Hi,

Thank you for the review.

On 3/6/20 5:04 AM, Dmitry Torokhov wrote:
> On Thu, Mar 05, 2020 at 11:01:23PM +0100, Hans de Goede wrote:
>> At least on X86 ACPI platforms it is not necessary to load the touchscreen
>> controller config from disk, if it needs to be loaded this has already been
>> done by the BIOS / UEFI firmware.
>>
>> Even on other (e.g. devicetree) platforms the config-loading as currently
>> done has the issue that the loaded cfg file is based on the controller
>> model, but the actual cfg is device specific, so the cfg files are not
>> part of linux-firmware and this can only work with a device specific OS
>> image which includes the cfg file.
>>
>> And we do not need access to the GPIOs at all to load the config, if we
>> do not have access we can still load the config.
>>
>> So all in all tying the decision to try to load the config from disk to
>> being able to access the GPIOs is not desirable. This commit adds a new
>> load_cfg_from_disk boolean to control the firmware loading instead.
>>
>> This commits sets the new bool to true when we set irq_pin_access_method
>> to IRQ_PIN_ACCESS_GPIO, so there are no functional changes.
>>
>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
>> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
>> Cc: Dmitry Mastykin <mastichi@gmail.com>
>> Reviewed-by: Bastien Nocera <hadess@hadess.net>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/input/touchscreen/goodix.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
>> index 9cfbcf3cbca8..28bb4385a54d 100644
>> --- a/drivers/input/touchscreen/goodix.c
>> +++ b/drivers/input/touchscreen/goodix.c
>> @@ -56,6 +56,7 @@ struct goodix_ts_data {
>>   	u16 id;
>>   	u16 version;
>>   	const char *cfg_name;
>> +	bool load_cfg_from_disk;
>>   	struct completion firmware_loading_complete;
>>   	unsigned long irq_flags;
>>   	enum goodix_irq_pin_access_method irq_pin_access_method;
>> @@ -654,8 +655,10 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
>>   
>>   	ts->gpiod_rst = gpiod;
>>   
>> -	if (ts->gpiod_int && ts->gpiod_rst)
>> +	if (ts->gpiod_int && ts->gpiod_rst) {
>> +		ts->load_cfg_from_disk = true;
>>   		ts->irq_pin_access_method = IRQ_PIN_ACCESS_GPIO;
>> +	}
>>   
>>   	return 0;
>>   }
>> @@ -952,7 +955,7 @@ static int goodix_ts_probe(struct i2c_client *client,
>>   
>>   	ts->chip = goodix_get_chip_data(ts->id);
>>   
>> -	if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_GPIO) {
>> +	if (ts->load_cfg_from_disk) {
>>   		/* update device config */
>>   		ts->cfg_name = devm_kasprintf(&client->dev, GFP_KERNEL,
>>   					      "goodix_%d_cfg.bin", ts->id);
>> @@ -983,7 +986,7 @@ static int goodix_ts_remove(struct i2c_client *client)
>>   {
>>   	struct goodix_ts_data *ts = i2c_get_clientdata(client);
>>   
>> -	if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_GPIO)
>> +	if (ts->load_cfg_from_disk)
>>   		wait_for_completion(&ts->firmware_loading_complete);
>>   
>>   	return 0;
>> @@ -1001,7 +1004,8 @@ static int __maybe_unused goodix_suspend(struct device *dev)
>>   		return 0;
>>   	}
>>   
>> -	wait_for_completion(&ts->firmware_loading_complete);
>> +	if (ts->load_cfg_from_disk)
>> +		wait_for_completion(&ts->firmware_loading_complete);
> 
> If you are detaching presence of GPIOs from firmware loading, then you
> need to move this wait higher, so we do not complete early if GPIOs are
> not there, but firmware is being loaded.

That is a good point, I've fixed this for v3. Do you have any other
remarks before I send out v3?

Regards,

Hans