Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Received-SPF: Pass (protection.outlook.com: domain of in.bosch.com designates
 139.15.153.199 as permitted sender) receiver=protection.outlook.com;
 client-ip=139.15.153.199; helo=eop.bosch-org.com; pr=C
From:   <Gireesh.Hiremath@in.bosch.com>
To:     <andriy.shevchenko@linux.intel.com>
CC:     <linux-input@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <dmitry.torokhov@gmail.com>, <Jonathan.Cameron@huawei.com>,
        <lis8215@gmail.com>, <Gireesh.Hiremath@in.bosch.com>,
        <sjoerd.simons@collabora.co.uk>, <VinayKumar.Shettar@in.bosch.com>,
        <Govindaraji.Sivanantham@in.bosch.com>,
        <anaclaudia.dias@de.bosch.com>
Subject: Re: [PATCH] driver: input: matric-keypad: switch to gpiod API
Date:   Thu, 19 Jan 2023 11:47:36 +0000
Message-ID: <20230119114736.30257-1-Gireesh.Hiremath@in.bosch.com>
In-Reply-To: <20230113062538.1537-1-Gireesh.Hiremath@in.bosch.com>
References: <20230113062538.1537-1-Gireesh.Hiremath@in.bosch.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Jan 2023 11:47:49.3693
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 81f907e6-2b11-44ad-aa7c-08dafa12fd71
X-MS-Exchange-CrossTenant-Id: 0ae51e19-07c8-4e4b-bb6d-648ee58410f4
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=0ae51e19-07c8-4e4b-bb6d-648ee58410f4;Ip=[139.15.153.199];Helo=[eop.bosch-org.com]
X-MS-Exchange-CrossTenant-AuthSource: DBAEUR03FT007.eop-EUR03.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU2PR10MB7743
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,
        SPF_HELO_PASS,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Gireesh Hiremath <Gireesh.Hiremath@in.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