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