| Message ID | 20221115221145.2550572-4-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] media: i2c: s5k6a3: switch to using gpiod API | expand |
On Tue, Nov 15, 2022 at 11:11 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > This patch switches the driver away from legacy gpio/of_gpio API to > gpiod API, and removes use of of_get_named_gpio_flags() which I want to > make private to gpiolib. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Tue, Nov 15, 2022 at 02:11:45PM -0800, Dmitry Torokhov wrote: > This patch switches the driver away from legacy gpio/of_gpio API to > gpiod API, and removes use of of_get_named_gpio_flags() which I want to > make private to gpiolib. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/media/i2c/s5c73m3/s5c73m3-core.c | 63 +++++++----------------- > drivers/media/i2c/s5c73m3/s5c73m3.h | 7 +-- > 2 files changed, 20 insertions(+), 50 deletions(-) > > diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c b/drivers/media/i2c/s5c73m3/s5c73m3-core.c > index 561c1a1583ac..f1e073ed5f99 100644 > --- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c > +++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c > @@ -10,12 +10,11 @@ > #include <linux/clk.h> > #include <linux/delay.h> > #include <linux/firmware.h> > -#include <linux/gpio.h> > +#include <linux/gpio/consumer.h> > #include <linux/i2c.h> > #include <linux/init.h> > #include <linux/media.h> > #include <linux/module.h> > -#include <linux/of_gpio.h> > #include <linux/of_graph.h> > #include <linux/regulator/consumer.h> > #include <linux/sizes.h> > @@ -1348,20 +1347,20 @@ static int s5c73m3_oif_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) > > static int s5c73m3_gpio_set_value(struct s5c73m3 *priv, int id, u32 val) > { > - if (!gpio_is_valid(priv->gpio[id].gpio)) > + if (!priv->gpio[id]) > return 0; > - gpio_set_value(priv->gpio[id].gpio, !!val); > + gpiod_set_value_cansleep(priv->gpio[id], val); > return 1; > } > > static int s5c73m3_gpio_assert(struct s5c73m3 *priv, int id) > { > - return s5c73m3_gpio_set_value(priv, id, priv->gpio[id].level); > + return s5c73m3_gpio_set_value(priv, id, 1); > } > > static int s5c73m3_gpio_deassert(struct s5c73m3 *priv, int id) > { > - return s5c73m3_gpio_set_value(priv, id, !priv->gpio[id].level); > + return s5c73m3_gpio_set_value(priv, id, 0); > } > > static int __s5c73m3_power_on(struct s5c73m3 *state) > @@ -1544,49 +1543,29 @@ static const struct v4l2_subdev_ops oif_subdev_ops = { > > static int s5c73m3_configure_gpios(struct s5c73m3 *state) > { > - static const char * const gpio_names[] = { > - "S5C73M3_STBY", "S5C73M3_RST" > - }; > + static const char * const name[] = { "standby", "xshutdown" }; > + static const char * const label[] = { "S5C73M3_STBY", "S5C73M3_RST" }; > struct i2c_client *c = state->i2c_client; > - struct s5c73m3_gpio *g = state->gpio; > + struct gpio_desc *gpio; > int ret, i; > > for (i = 0; i < GPIO_NUM; ++i) { > - unsigned int flags = GPIOF_DIR_OUT; > - if (g[i].level) > - flags |= GPIOF_INIT_HIGH; > - ret = devm_gpio_request_one(&c->dev, g[i].gpio, flags, > - gpio_names[i]); > + gpio = devm_gpiod_get(&c->dev, name[i], GPIOD_OUT_HIGH); > + ret = PTR_ERR_OR_ZERO(gpio); > if (ret) { > - v4l2_err(c, "failed to request gpio %s\n", > - gpio_names[i]); > + v4l2_err(c, "failed to request gpio %s: %d\n", > + name[i], ret); > return ret; > } > - } > - return 0; > -} > - > -static int s5c73m3_parse_gpios(struct s5c73m3 *state) > -{ > - static const char * const prop_names[] = { > - "standby-gpios", "xshutdown-gpios", > - }; > - struct device *dev = &state->i2c_client->dev; > - struct device_node *node = dev->of_node; > - int ret, i; > > - for (i = 0; i < GPIO_NUM; ++i) { > - enum of_gpio_flags of_flags; > - > - ret = of_get_named_gpio_flags(node, prop_names[i], > - 0, &of_flags); > - if (ret < 0) { > - dev_err(dev, "failed to parse %s DT property\n", > - prop_names[i]); > - return -EINVAL; > + ret = gpiod_set_consumer_name(gpio, label[i]); > + if (ret) { > + v4l2_err(c, "failed to set up name for gpio %s: %d\n", > + name[i], ret); > + return ret; > } > - state->gpio[i].gpio = ret; > - state->gpio[i].level = !(of_flags & OF_GPIO_ACTIVE_LOW); > + > + state->gpio[i] = gpio; > } > return 0; > } > @@ -1613,10 +1592,6 @@ static int s5c73m3_get_dt_data(struct s5c73m3 *state) > state->mclk_frequency); > } > > - ret = s5c73m3_parse_gpios(state); > - if (ret < 0) > - return -EINVAL; > - > node_ep = of_graph_get_next_endpoint(node, NULL); > if (!node_ep) { > dev_warn(dev, "no endpoint defined for node: %pOF\n", node); > diff --git a/drivers/media/i2c/s5c73m3/s5c73m3.h b/drivers/media/i2c/s5c73m3/s5c73m3.h > index d68528898249..9887d03fcdeb 100644 > --- a/drivers/media/i2c/s5c73m3/s5c73m3.h > +++ b/drivers/media/i2c/s5c73m3/s5c73m3.h > @@ -356,11 +356,6 @@ enum s5c73m3_gpio_id { > GPIO_NUM, > }; > > -struct s5c73m3_gpio { > - int gpio; > - int level; > -}; > - > enum s5c73m3_resolution_types { > RES_ISP, > RES_JPEG, > @@ -387,7 +382,7 @@ struct s5c73m3 { > u32 i2c_read_address; > > struct regulator_bulk_data supplies[S5C73M3_MAX_SUPPLIES]; > - struct s5c73m3_gpio gpio[GPIO_NUM]; > + struct gpio_desc *gpio[GPIO_NUM]; > > struct clk *clock; > > -- > 2.38.1.431.g37b22c650d-goog > Looks good to me. Reviewed-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
Hi Linus, On 17/11/2022 10:06, Linus Walleij wrote: > On Tue, Nov 15, 2022 at 11:11 PM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > >> This patch switches the driver away from legacy gpio/of_gpio API to >> gpiod API, and removes use of of_get_named_gpio_flags() which I want to >> make private to gpiolib. >> >> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> There are now two patches that do this switch to using the gpiod API: this one, and one from you: https://patchwork.linuxtv.org/project/linux-media/patch/20221108100604.1500909-1-linus.walleij@linaro.org/ Any preference? Regards, Hans
On 26/11/2022 11:34, Hans Verkuil wrote: > Hi Linus, > > On 17/11/2022 10:06, Linus Walleij wrote: >> On Tue, Nov 15, 2022 at 11:11 PM Dmitry Torokhov >> <dmitry.torokhov@gmail.com> wrote: >> >>> This patch switches the driver away from legacy gpio/of_gpio API to >>> gpiod API, and removes use of of_get_named_gpio_flags() which I want to >>> make private to gpiolib. >>> >>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> >> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > There are now two patches that do this switch to using the gpiod API: > this one, and one from you: > > https://patchwork.linuxtv.org/project/linux-media/patch/20221108100604.1500909-1-linus.walleij@linaro.org/ And a third one as well: https://patchwork.linuxtv.org/project/linux-media/patch/20221116194307.164543-1-mairacanal@riseup.net/ > > Any preference? > > Regards, > > Hans
On Sat, Nov 26, 2022 at 11:34 AM Hans Verkuil <hverkuil@xs4all.nl> wrote: > On 17/11/2022 10:06, Linus Walleij wrote: > > On Tue, Nov 15, 2022 at 11:11 PM Dmitry Torokhov > > <dmitry.torokhov@gmail.com> wrote: > > > >> This patch switches the driver away from legacy gpio/of_gpio API to > >> gpiod API, and removes use of of_get_named_gpio_flags() which I want to > >> make private to gpiolib. > >> > >> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > There are now two patches that do this switch to using the gpiod API: > this one, and one from you: Ooops we stepped on each others' toes again. Dmitry & I really like to fix these drivers :D sorry about that. > https://patchwork.linuxtv.org/project/linux-media/patch/20221108100604.1500909-1-linus.walleij@linaro.org/ > > Any preference? Not really, take the one you like best, they look functionally equivalent to me, just slightly different stylistic choices. Yours, Linus Walleij
diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c b/drivers/media/i2c/s5c73m3/s5c73m3-core.c index 561c1a1583ac..f1e073ed5f99 100644 --- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c +++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c @@ -10,12 +10,11 @@ #include <linux/clk.h> #include <linux/delay.h> #include <linux/firmware.h> -#include <linux/gpio.h> +#include <linux/gpio/consumer.h> #include <linux/i2c.h> #include <linux/init.h> #include <linux/media.h> #include <linux/module.h> -#include <linux/of_gpio.h> #include <linux/of_graph.h> #include <linux/regulator/consumer.h> #include <linux/sizes.h> @@ -1348,20 +1347,20 @@ static int s5c73m3_oif_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) static int s5c73m3_gpio_set_value(struct s5c73m3 *priv, int id, u32 val) { - if (!gpio_is_valid(priv->gpio[id].gpio)) + if (!priv->gpio[id]) return 0; - gpio_set_value(priv->gpio[id].gpio, !!val); + gpiod_set_value_cansleep(priv->gpio[id], val); return 1; } static int s5c73m3_gpio_assert(struct s5c73m3 *priv, int id) { - return s5c73m3_gpio_set_value(priv, id, priv->gpio[id].level); + return s5c73m3_gpio_set_value(priv, id, 1); } static int s5c73m3_gpio_deassert(struct s5c73m3 *priv, int id) { - return s5c73m3_gpio_set_value(priv, id, !priv->gpio[id].level); + return s5c73m3_gpio_set_value(priv, id, 0); } static int __s5c73m3_power_on(struct s5c73m3 *state) @@ -1544,49 +1543,29 @@ static const struct v4l2_subdev_ops oif_subdev_ops = { static int s5c73m3_configure_gpios(struct s5c73m3 *state) { - static const char * const gpio_names[] = { - "S5C73M3_STBY", "S5C73M3_RST" - }; + static const char * const name[] = { "standby", "xshutdown" }; + static const char * const label[] = { "S5C73M3_STBY", "S5C73M3_RST" }; struct i2c_client *c = state->i2c_client; - struct s5c73m3_gpio *g = state->gpio; + struct gpio_desc *gpio; int ret, i; for (i = 0; i < GPIO_NUM; ++i) { - unsigned int flags = GPIOF_DIR_OUT; - if (g[i].level) - flags |= GPIOF_INIT_HIGH; - ret = devm_gpio_request_one(&c->dev, g[i].gpio, flags, - gpio_names[i]); + gpio = devm_gpiod_get(&c->dev, name[i], GPIOD_OUT_HIGH); + ret = PTR_ERR_OR_ZERO(gpio); if (ret) { - v4l2_err(c, "failed to request gpio %s\n", - gpio_names[i]); + v4l2_err(c, "failed to request gpio %s: %d\n", + name[i], ret); return ret; } - } - return 0; -} - -static int s5c73m3_parse_gpios(struct s5c73m3 *state) -{ - static const char * const prop_names[] = { - "standby-gpios", "xshutdown-gpios", - }; - struct device *dev = &state->i2c_client->dev; - struct device_node *node = dev->of_node; - int ret, i; - for (i = 0; i < GPIO_NUM; ++i) { - enum of_gpio_flags of_flags; - - ret = of_get_named_gpio_flags(node, prop_names[i], - 0, &of_flags); - if (ret < 0) { - dev_err(dev, "failed to parse %s DT property\n", - prop_names[i]); - return -EINVAL; + ret = gpiod_set_consumer_name(gpio, label[i]); + if (ret) { + v4l2_err(c, "failed to set up name for gpio %s: %d\n", + name[i], ret); + return ret; } - state->gpio[i].gpio = ret; - state->gpio[i].level = !(of_flags & OF_GPIO_ACTIVE_LOW); + + state->gpio[i] = gpio; } return 0; } @@ -1613,10 +1592,6 @@ static int s5c73m3_get_dt_data(struct s5c73m3 *state) state->mclk_frequency); } - ret = s5c73m3_parse_gpios(state); - if (ret < 0) - return -EINVAL; - node_ep = of_graph_get_next_endpoint(node, NULL); if (!node_ep) { dev_warn(dev, "no endpoint defined for node: %pOF\n", node); diff --git a/drivers/media/i2c/s5c73m3/s5c73m3.h b/drivers/media/i2c/s5c73m3/s5c73m3.h index d68528898249..9887d03fcdeb 100644 --- a/drivers/media/i2c/s5c73m3/s5c73m3.h +++ b/drivers/media/i2c/s5c73m3/s5c73m3.h @@ -356,11 +356,6 @@ enum s5c73m3_gpio_id { GPIO_NUM, }; -struct s5c73m3_gpio { - int gpio; - int level; -}; - enum s5c73m3_resolution_types { RES_ISP, RES_JPEG, @@ -387,7 +382,7 @@ struct s5c73m3 { u32 i2c_read_address; struct regulator_bulk_data supplies[S5C73M3_MAX_SUPPLIES]; - struct s5c73m3_gpio gpio[GPIO_NUM]; + struct gpio_desc *gpio[GPIO_NUM]; struct clk *clock;
This patch switches the driver away from legacy gpio/of_gpio API to gpiod API, and removes use of of_get_named_gpio_flags() which I want to make private to gpiolib. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/media/i2c/s5c73m3/s5c73m3-core.c | 63 +++++++----------------- drivers/media/i2c/s5c73m3/s5c73m3.h | 7 +-- 2 files changed, 20 insertions(+), 50 deletions(-)