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, 10 Nov 2022 14:41:51 -0800
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     Balamanikandan Gunasundar 
        <balamanikandan.gunasundar@microchip.com>
Cc:     ludovic.desroches@microchip.com, ulf.hansson@linaro.org,
        nicolas.ferre@microchip.com, alexandre.belloni@bootlin.com,
        3chas3@gmail.com, linux-mmc@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-atm-general@lists.sourceforge.net, netdev@vger.kernel.org
Subject: Re: [PATCH] mmc: atmel-mci: Convert to gpio descriptors
Message-ID: <Y21+L01BcPQ35FYi@google.com>
References: <20221109043845.16617-1-balamanikandan.gunasundar@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221109043845.16617-1-balamanikandan.gunasundar@microchip.com>
Precedence: bulk

Hi Balamanikandan,

On Wed, Nov 09, 2022 at 10:08:45AM +0530, Balamanikandan Gunasundar wrote:
> Replace the legacy GPIO APIs with gpio descriptor consumer interface.
> 
> To maintain backward compatibility, we rely on the "cd-inverted"
> property to manage the invertion flag instead of GPIO property.
> 
> Signed-off-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com>
> ---
>  drivers/mmc/host/atmel-mci.c | 79 ++++++++++++++++++------------------
>  include/linux/atmel-mci.h    |  4 +-
>  2 files changed, 41 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
> index 67b2cd166e56..1df90966e104 100644
> --- a/drivers/mmc/host/atmel-mci.c
> +++ b/drivers/mmc/host/atmel-mci.c
> @@ -19,7 +19,8 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> -#include <linux/of_gpio.h>
> +#include <linux/irq.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/scatterlist.h>
>  #include <linux/seq_file.h>
> @@ -389,8 +390,8 @@ struct atmel_mci_slot {
>  #define ATMCI_CARD_NEED_INIT	1
>  #define ATMCI_SHUTDOWN		2
>  
> -	int			detect_pin;
> -	int			wp_pin;
> +	struct gpio_desc        *detect_pin;
> +	struct gpio_desc	*wp_pin;
>  	bool			detect_is_active_high;
>  
>  	struct timer_list	detect_timer;
> @@ -638,7 +639,11 @@ atmci_of_init(struct platform_device *pdev)
>  			pdata->slot[slot_id].bus_width = 1;
>  
>  		pdata->slot[slot_id].detect_pin =
> -			of_get_named_gpio(cnp, "cd-gpios", 0);
> +			devm_gpiod_get_from_of_node(&pdev->dev, cnp,
> +						    "cd-gpios",
> +						    0, GPIOD_IN, "cd-gpios");

As I mentioned in another email, please use devm_fwnode_gpiod_get()
instead of devm_gpiod_get_from_of_node() which is going away.

> +		if (IS_ERR(pdata->slot[slot_id].detect_pin))
> +			pdata->slot[slot_id].detect_pin = NULL;

I think it would be much better if we had proper error handling and did
something like:

		err = PTR_ERR_OR_ZERO(pdata->slot[slot_id].detect_pin);
		if (err) {
			if (err != -ENOENT)
				return ERR_PTR(err);
			pdata->slot[slot_id].detect_pin = NULL;
		}

This will help with proper deferral handling.

>  
>  		pdata->slot[slot_id].detect_is_active_high =
>  			of_property_read_bool(cnp, "cd-inverted");

Instead of doing gpiod_set_value_raw() below I would recommend handling
it here via gpiod_is_active_low() and gpiod_toggle_active_low() and
removing the flag from atmel_mci_slot structure.


> @@ -647,7 +652,11 @@ atmci_of_init(struct platform_device *pdev)
>  			of_property_read_bool(cnp, "non-removable");
>  
>  		pdata->slot[slot_id].wp_pin =
> -			of_get_named_gpio(cnp, "wp-gpios", 0);
> +			devm_gpiod_get_from_of_node(&pdev->dev, cnp,
> +						    "wp-gpios",
> +						    0, GPIOD_IN, "wp-gpios");
> +		if (IS_ERR(pdata->slot[slot_id].wp_pin))
> +			pdata->slot[slot_id].wp_pin = NULL;
>  	}
>  
>  	return pdata;
> @@ -1511,8 +1520,8 @@ static int atmci_get_ro(struct mmc_host *mmc)
>  	int			read_only = -ENOSYS;
>  	struct atmel_mci_slot	*slot = mmc_priv(mmc);
>  
> -	if (gpio_is_valid(slot->wp_pin)) {
> -		read_only = gpio_get_value(slot->wp_pin);
> +	if (slot->wp_pin) {
> +		read_only = gpiod_get_value(slot->wp_pin);

Consider using "cansleep" variants.

Thanks.

-- 
Dmitry