Re: [PATCH] spi: spi-mpc52xx: switch to using gpiod API

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Thu, Sep 22, 2022 at 09:51:37PM -0700, Dmitry Torokhov wrote:
> This switches the driver to use gpiod API instead of legacy gpio API,
> which will brings us close to removing of_get_gpio() and other
> OF-specific old APIs.
> 
> No functional change intended beyond some differences in error messages.

Fine to me:

Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

Thanks!

One nit-pick below.

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> ---
>  drivers/spi/spi-mpc52xx.c | 32 +++++++++++++-------------------
>  1 file changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/spi/spi-mpc52xx.c b/drivers/spi/spi-mpc52xx.c
> index 97cdd6545ee1..cb075c1acbee 100644
> --- a/drivers/spi/spi-mpc52xx.c
> +++ b/drivers/spi/spi-mpc52xx.c
> @@ -11,6 +11,7 @@
>   */
>  
>  #include <linux/module.h>
> +#include <linux/err.h>
>  #include <linux/errno.h>
>  #include <linux/of_platform.h>
>  #include <linux/interrupt.h>
> @@ -18,7 +19,6 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/spi/spi.h>
>  #include <linux/io.h>
> -#include <linux/of_gpio.h>
>  #include <linux/slab.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
> @@ -90,7 +90,7 @@ struct mpc52xx_spi {
>  	const u8 *tx_buf;
>  	int cs_change;
>  	int gpio_cs_count;
> -	unsigned int *gpio_cs;
> +	struct gpio_desc **gpio_cs;
>  };
>  
>  /*
> @@ -102,9 +102,10 @@ static void mpc52xx_spi_chipsel(struct mpc52xx_spi *ms, int value)
>  
>  	if (ms->gpio_cs_count > 0) {
>  		cs = ms->message->spi->chip_select;
> -		gpio_set_value(ms->gpio_cs[cs], value ? 0 : 1);
> -	} else
> +		gpiod_set_value(ms->gpio_cs[cs], value);
> +	} else {
>  		out_8(ms->regs + SPI_PORTDATA, value ? 0 : 0x08);
> +	}
>  }
>  
>  /*
> @@ -386,10 +387,10 @@ static int mpc52xx_spi_probe(struct platform_device *op)
>  {
>  	struct spi_master *master;
>  	struct mpc52xx_spi *ms;
> +	struct gpio_desc *gpio_cs;
>  	void __iomem *regs;
>  	u8 ctrl1;
>  	int rc, i = 0;
> -	int gpio_cs;
>  
>  	/* MMIO registers */
>  	dev_dbg(&op->dev, "probing mpc5200 SPI device\n");
> @@ -451,23 +452,16 @@ static int mpc52xx_spi_probe(struct platform_device *op)
>  		}
>  
>  		for (i = 0; i < ms->gpio_cs_count; i++) {
> -			gpio_cs = of_get_gpio(op->dev.of_node, i);
> -			if (!gpio_is_valid(gpio_cs)) {
> -				dev_err(&op->dev,
> -					"could not parse the gpio field in oftree\n");
> -				rc = -ENODEV;
> -				goto err_gpio;
> -			}
> -
> -			rc = gpio_request(gpio_cs, dev_name(&op->dev));
> +			gpio_cs = gpiod_get_index(&op->dev,
> +						  NULL, i, GPIOD_OUT_LOW);

More parameters can be placed on the previous line, but I think you can even
put everything on one line (it's only 85 characters long).

> +			rc = PTR_ERR_OR_ZERO(gpio_cs);
>  			if (rc) {
>  				dev_err(&op->dev,
> -					"can't request spi cs gpio #%d on gpio line %d\n",
> -					i, gpio_cs);
> +					"failed to get spi cs gpio #%d: %d\n",
> +					i, rc);
>  				goto err_gpio;
>  			}
>  
> -			gpio_direction_output(gpio_cs, 1);
>  			ms->gpio_cs[i] = gpio_cs;
>  		}
>  	}
> @@ -508,7 +502,7 @@ static int mpc52xx_spi_probe(struct platform_device *op)
>  	dev_err(&ms->master->dev, "initialization failed\n");
>   err_gpio:
>  	while (i-- > 0)
> -		gpio_free(ms->gpio_cs[i]);
> +		gpiod_put(ms->gpio_cs[i]);
>  
>  	kfree(ms->gpio_cs);
>   err_alloc_gpio:
> @@ -529,7 +523,7 @@ static int mpc52xx_spi_remove(struct platform_device *op)
>  	free_irq(ms->irq1, ms);
>  
>  	for (i = 0; i < ms->gpio_cs_count; i++)
> -		gpio_free(ms->gpio_cs[i]);
> +		gpiod_put(ms->gpio_cs[i]);
>  
>  	kfree(ms->gpio_cs);
>  	spi_unregister_master(master);
> -- 
> 2.37.3.998.g577e59143f-goog
> 
> 
> -- 
> Dmitry

-- 
With Best Regards,
Andy Shevchenko