linux-kernel - Re: [PATCH] driver: input: matric-keypad: switch to gpiod API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230119114736.30257-1-Gireesh.Hiremath@in.bosch.com>
Date:   Thu, 19 Jan 2023 11:47:36 +0000
From:   <Gireesh.Hiremath@...bosch.com>
To:     <andriy.shevchenko@...ux.intel.com>
CC:     <linux-input@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <dmitry.torokhov@...il.com>, <Jonathan.Cameron@...wei.com>,
        <lis8215@...il.com>, <Gireesh.Hiremath@...bosch.com>,
        <sjoerd.simons@...labora.co.uk>, <VinayKumar.Shettar@...bosch.com>,
        <Govindaraji.Sivanantham@...bosch.com>,
        <anaclaudia.dias@...bosch.com>
Subject: Re: [PATCH] driver: input: matric-keypad: switch to gpiod API

From: Gireesh Hiremath <Gireesh.Hiremath@...bosch.com>

Hi Andy Shevchenko,

I will correct it as
>Thank you for the patch, my comments below.
>
>> switch to new gpio descriptor based API
Switch to GPIO descriptor based API.
>
>Please, respect English grammar and punctuation.
>
>Also, you have a typo in the Subject besides the fact that the template for
>Input subsystem is different. So prefix has to be changed as well.
and template as
Input: matrix_keypad - switch to gpiod API
>
>...
>
>>  	bool level_on = !pdata->active_low;
>>  
>>  	if (on) {
>> -		gpio_direction_output(pdata->col_gpios[col], level_on);
>> +		gpiod_direction_output(pdata->col_gpios[col], level_on);
>>  	} else {
>> -		gpio_set_value_cansleep(pdata->col_gpios[col], !level_on);
>> +		gpiod_set_value_cansleep(pdata->col_gpios[col], !level_on);
>>  	}
>
>I believe it's not so trivial. The GPIO descriptor already has encoded the
>level information and above one as below are not clear now.
>
>> -	return gpio_get_value_cansleep(pdata->row_gpios[row]) ?
>> +	return gpiod_get_value_cansleep(pdata->row_gpios[row]) ?
>>  			!pdata->active_low : pdata->active_low;
>
if GPIO from I2C or SPI IO expander, which may sleep, 
so it is safer to use the gpiod_set_value_cansleep() and
gpiod_get_value_cansleep().
>...
>
>> -		err = gpio_request(pdata->col_gpios[i], "matrix_kbd_col");
>> +		err = gpiod_direction_output(pdata->col_gpios[i], !pdata->active_low);
>
>>  	while (--i >= 0)
>> -		gpio_free(pdata->row_gpios[i]);
>> +		gpiod_put(pdata->row_gpios[i]);
>
>This looks like an incorrect change.
>
>>  	while (--i >= 0)
>> -		gpio_free(pdata->col_gpios[i]);
>> +		gpiod_put(pdata->col_gpios[i]);
>
>So does this.
>
>Since you dropped gpio_request() you need to drop gpio_free() as well,
>and not replace it.
gpio_request() equalent api gpiod_request() is alredy called inside gpiod_get_index(...).
gpiod_put() is required to free GPIO.
>
>...
>
>>  	for (i = 0; i < nrow; i++) {
>> -		ret = of_get_named_gpio(np, "row-gpios", i);
>> -		if (ret < 0)
>
>> -			return ERR_PTR(ret);
>
>(1)
>
>> -		gpios[i] = ret;
>> +		desc = gpiod_get_index(dev, "row", i, GPIOD_IN);
>> +		if (IS_ERR(desc))
>
>> +			return ERR_PTR(-EINVAL);
>
>Why?! How will it handle deferred probe, for example?
shall I update it as 
				return ERR_CAST(desc);
>
>> +		gpios[i] = desc;
>>  	}
>
>...
>
>>  	for (i = 0; i < ncol; i++) {
>> -		ret = of_get_named_gpio(np, "col-gpios", i);
>> -		if (ret < 0)
>> -			return ERR_PTR(ret);
>> -		gpios[nrow + i] = ret;
>> +		desc = gpiod_get_index(dev, "col", i, GPIOD_IN);
>> +		if (IS_ERR(desc))
>> +			return ERR_PTR(-EINVAL);
>
>Ditto.
>
>> +		gpios[nrow + i] = desc;
>>  	}

Thanks,
Gireesh Hiremath