Re: [PATCH] media: c8sectpfe: convert to gpio descriptors

[Andy Shevchenko]

On Mon, Jan 30, 2023 at 02:09:47PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@xxxxxxxx>
> 
> The gpio usage in the function is fairly straightforward,
> but the normal gpiod_get() interface cannot be used here
> since the gpio is referenced by a child node of the device.
> 
> Using devm_fwnode_gpiod_get_index() is the best alternative
> here.

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

One topic to discuss below (but I'm fine with this version).

> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> ---
>  .../st/sti/c8sectpfe/c8sectpfe-core.c         | 30 ++++++++-----------
>  .../st/sti/c8sectpfe/c8sectpfe-core.h         |  2 +-
>  2 files changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/platform/st/sti/c8sectpfe/c8sectpfe-core.c b/drivers/media/platform/st/sti/c8sectpfe/c8sectpfe-core.c
> index c38b62d4f1ae..86a2c77c5471 100644
> --- a/drivers/media/platform/st/sti/c8sectpfe/c8sectpfe-core.c
> +++ b/drivers/media/platform/st/sti/c8sectpfe/c8sectpfe-core.c
> @@ -22,7 +22,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> -#include <linux/of_gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/of_platform.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/pinctrl/pinctrl.h>
> @@ -812,30 +812,24 @@ static int c8sectpfe_probe(struct platform_device *pdev)
>  		}
>  		of_node_put(i2c_bus);
>  
> -		tsin->rst_gpio = of_get_named_gpio(child, "reset-gpios", 0);
> -
> -		ret = gpio_is_valid(tsin->rst_gpio);
> -		if (!ret) {
> -			dev_err(dev,
> -				"reset gpio for tsin%d not valid (gpio=%d)\n",
> -				tsin->tsin_id, tsin->rst_gpio);
> -			ret = -EINVAL;
> -			goto err_node_put;
> -		}
> -
> -		ret = devm_gpio_request_one(dev, tsin->rst_gpio,
> -					GPIOF_OUT_INIT_LOW, "NIM reset");
> +		tsin->rst_gpio = devm_fwnode_gpiod_get_index(dev,
> +							     of_node_to_fwnode(child),
> +							     "reset-gpios",
> +							     0, GPIOD_OUT_LOW,
> +							     "NIM reset");

> +		ret = PTR_ERR_OR_ZERO(tsin->rst_gpio);
>  		if (ret && ret != -EBUSY) {
> -			dev_err(dev, "Can't request tsin%d reset gpio\n"
> -				, fei->channel_data[index]->tsin_id);
> +			dev_err_probe(dev, ret,
> +				      "reset gpio for tsin%d not valid\n",
> +				      tsin->tsin_id);
>  			goto err_node_put;
>  		}
>  
>  		if (!ret) {

Can be 

	if (IS_ERR() && PTR_ERR() != -EBUSY) {
		ret = dev_err_probe(dev, PTR_ERR(), ...);
		...
	}

	if (!IS_ERR())

(Up to you)

But -EBUSY check seems strange to me. What was the motivation behind?
(As far as I can read the code the possibility to get this if and only
 if we have requested GPIO too early at initcall level. Would it be
 ever a possibility to get it in real life?)

>  			/* toggle reset lines */
> -			gpio_direction_output(tsin->rst_gpio, 0);
> +			gpiod_direction_output(tsin->rst_gpio, 0);
>  			usleep_range(3500, 5000);
> -			gpio_direction_output(tsin->rst_gpio, 1);
> +			gpiod_direction_output(tsin->rst_gpio, 1);
>  			usleep_range(3000, 5000);
>  		}
>  
> diff --git a/drivers/media/platform/st/sti/c8sectpfe/c8sectpfe-core.h b/drivers/media/platform/st/sti/c8sectpfe/c8sectpfe-core.h
> index c9d6021904cd..f2a6991e064e 100644
> --- a/drivers/media/platform/st/sti/c8sectpfe/c8sectpfe-core.h
> +++ b/drivers/media/platform/st/sti/c8sectpfe/c8sectpfe-core.h
> @@ -25,7 +25,7 @@ struct channel_info {
>  	int i2c;
>  	int dvb_card;
>  
> -	int rst_gpio;
> +	struct gpio_desc *rst_gpio;
>  
>  	struct i2c_adapter  *i2c_adapter;
>  	struct i2c_adapter  *tuner_i2c;
> -- 
> 2.39.0
> 

-- 
With Best Regards,
Andy Shevchenko