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;
Date:   Thu, 19 Jan 2023 16:19:32 +0200
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Gireesh.Hiremath@in.bosch.com
Cc:     linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
        dmitry.torokhov@gmail.com, Jonathan.Cameron@huawei.com,
        lis8215@gmail.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
Message-ID: <Y8lRdC/B2VlDU5zB@smile.fi.intel.com>
References: <20230113062538.1537-1-Gireesh.Hiremath@in.bosch.com>
 <20230119114736.30257-1-Gireesh.Hiremath@in.bosch.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230119114736.30257-1-Gireesh.Hiremath@in.bosch.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Thu, Jan 19, 2023 at 11:47:36AM +0000, Gireesh.Hiremath@in.bosch.com wrote:

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

...to the GPIO...

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

OK!

...

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

No, my point is about active level (LOW or HIGH). It's encoded into
the descriptor in opposite to the plain GPIO number.

This change needs very careful understanding of the active level.

...

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

Yes, but you removed request call, so should remove the free.
The gpiod_put() should be at the same function as gpiod_get().

...

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

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