From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> To: Arnd Bergmann <arnd@kernel.org> Cc: Patrice Chotard <patrice.chotard@foss.st.com>, Mauro Carvalho Chehab <mchehab@kernel.org>, Arnd Bergmann <arnd@arndb.de>, Hans Verkuil <hverkuil-cisco@xs4all.nl>, Hugues Fruchet <hugues.fruchet@st.com>, linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] media: c8sectpfe: convert to gpio descriptors Date: Mon, 30 Jan 2023 19:18:54 +0200 [thread overview] Message-ID: <Y9f7/q3aS5nlY7nJ@smile.fi.intel.com> (raw) In-Reply-To: <20230130131003.668888-1-arnd@kernel.org> On Mon, Jan 30, 2023 at 02:09:47PM +0100, Arnd Bergmann 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. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> One topic to discuss below (but I'm fine with this version). > 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", > + 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
WARNING: multiple messages have this Message-ID (diff)
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> To: Arnd Bergmann <arnd@kernel.org> Cc: Patrice Chotard <patrice.chotard@foss.st.com>, Mauro Carvalho Chehab <mchehab@kernel.org>, Arnd Bergmann <arnd@arndb.de>, Hans Verkuil <hverkuil-cisco@xs4all.nl>, Hugues Fruchet <hugues.fruchet@st.com>, linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] media: c8sectpfe: convert to gpio descriptors Date: Mon, 30 Jan 2023 19:18:54 +0200 [thread overview] Message-ID: <Y9f7/q3aS5nlY7nJ@smile.fi.intel.com> (raw) In-Reply-To: <20230130131003.668888-1-arnd@kernel.org> On Mon, Jan 30, 2023 at 02:09:47PM +0100, Arnd Bergmann 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. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> One topic to discuss below (but I'm fine with this version). > 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", > + 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-01-30 17:19 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-01-30 13:09 [PATCH] media: c8sectpfe: convert to gpio descriptors Arnd Bergmann 2023-01-30 13:09 ` Arnd Bergmann 2023-01-30 17:18 ` Andy Shevchenko [this message] 2023-01-30 17:18 ` Andy Shevchenko 2023-01-30 18:17 ` Arnd Bergmann 2023-01-30 18:17 ` Arnd Bergmann 2023-02-03 23:10 ` Dmitry Torokhov 2023-02-03 23:10 ` Dmitry Torokhov 2023-02-01 3:29 ` Dmitry Torokhov 2023-02-01 3:29 ` Dmitry Torokhov 2023-02-01 18:35 ` Andy Shevchenko 2023-02-01 18:35 ` Andy Shevchenko 2023-05-17 18:21 ` Sakari Ailus 2023-05-17 18:21 ` Sakari Ailus 2023-05-17 19:12 ` Dmitry Torokhov 2023-05-17 19:12 ` Dmitry Torokhov 2023-05-17 19:26 ` Sakari Ailus 2023-05-17 19:26 ` Sakari Ailus 2023-05-19 1:05 ` Dmitry Torokhov 2023-05-19 1:05 ` Dmitry Torokhov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=Y9f7/q3aS5nlY7nJ@smile.fi.intel.com \ --to=andriy.shevchenko@linux.intel.com \ --cc=arnd@arndb.de \ --cc=arnd@kernel.org \ --cc=hugues.fruchet@st.com \ --cc=hverkuil-cisco@xs4all.nl \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=mchehab@kernel.org \ --cc=patrice.chotard@foss.st.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.