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]

Date:   Fri, 13 Jan 2023 20:43:13 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Gireesh.Hiremath@...bosch.com
Cc:     linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
        dmitry.torokhov@...il.com, Jonathan.Cameron@...wei.com,
        lis8215@...il.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

On Fri, Jan 13, 2023 at 06:25:38AM +0000, Gireesh.Hiremath@...bosch.com wrote:
> From: Gireesh Hiremath <Gireesh.Hiremath@...bosch.com>

Thank you for the patch, my comments below.

> switch to new 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.

...

>  	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;

...

> -		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.

...

>  	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?

> +		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;
>  	}

-- 
With Best Regards,
Andy Shevchenko