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

[Gireesh.Hiremath]

From: Gireesh Hiremath <Gireesh.Hiremath@xxxxxxxxxxxx>

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