media: c8sectpfe: convert to gpio descriptors

From: Arnd Bergmann <arnd@arndb.de>

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.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 .../st/sti/c8sectpfe/c8sectpfe-core.c         | 30 ++++++++-----------
 .../st/sti/c8sectpfe/c8sectpfe-core.h         |  2 +-
 2 files changed, 13 insertions(+), 19 deletions(-)

On Mon, Jan 30, 2023 at 5:31 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> 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.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  .../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",

Wrong name.

> +                                                            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) {
>                         /* toggle reset lines */
> -                       gpio_direction_output(tsin->rst_gpio, 0);
> +                       gpiod_direction_output(tsin->rst_gpio, 0);

It is already set up as output, and you either need to use "raw" or
fix polarity.

>                         usleep_range(3500, 5000);
> -                       gpio_direction_output(tsin->rst_gpio, 1);
> +                       gpiod_direction_output(tsin->rst_gpio, 1);
>                         usleep_range(3000, 5000);
>                 }

Thanks.

On Tue, Jan 31, 2023 at 07:29:37PM -0800, Dmitry Torokhov wrote:
> On Mon, Jan 30, 2023 at 5:31 AM Arnd Bergmann <arnd@kernel.org> wrote:

...

> > +               tsin->rst_gpio = devm_fwnode_gpiod_get_index(dev,
> > +                                                            of_node_to_fwnode(child),

Actually, please use of_fwnode_handle() and not IRQ framework custom grown
duplicate.

> > +                                                            "reset-gpios",
> 
> Wrong name.
> 
> > +                                                            0, GPIOD_OUT_LOW,
> > +                                                            "NIM reset");

On Mon, Jan 30, 2023 at 10:19 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Mon, Jan 30, 2023, at 18:18, Andy Shevchenko wrote:
> > On Mon, Jan 30, 2023 at 02:09:47PM +0100, Arnd Bergmann wrote:
>
> >> +            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)
>
> I prefer the version that only has one PTR_ERR(), but
> either way is fine with me.
>
> > 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?)
>
> I noticed this part as being odd as well, no idea why the
> code is like this. I just left the logic unchanged here.

It could be they were trying to account for the possibility of the
reset line being shared between several blocks, and so the first one
to initialize would grab it and reset all chips, and the followers
would be skipping the reset logic.