| Message ID | YWXmB3yHDeR9ORN7@fedora (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s5c73m3: adding gpiod support for the s5c73m3 | expand |
Hi Maíra, On 12/10/2021 21:46, Maíra Canal wrote: > Removing old gpiod interface and replacing it for the gpiod consumer > interface. Has this been tested? I feel a bit uncomfortable to merged this without knowing that it works. Andrzej, what do you think about this patch? Maíra, is there a specific reason why you made this patch? Regards, Hans > > Signed-off-by: Maíra Canal <maira.canal@usp.br> > --- > drivers/media/i2c/s5c73m3/s5c73m3-core.c | 25 +++++++++++++----------- > include/media/i2c/s5c73m3.h | 3 ++- > 2 files changed, 16 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c b/drivers/media/i2c/s5c73m3/s5c73m3-core.c > index e2b88c5e4f98..0c69a3fc7ebe 100644 > --- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c > +++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c > @@ -10,7 +10,7 @@ > #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> > @@ -1349,9 +1349,9 @@ 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].gpio) > return 0; > - gpio_set_value(priv->gpio[id].gpio, !!val); > + gpiod_set_value(priv->gpio[id].gpio, !!val); > return 1; > } > > @@ -1548,20 +1548,24 @@ static int s5c73m3_configure_gpios(struct s5c73m3 *state) > static const char * const gpio_names[] = { > "S5C73M3_STBY", "S5C73M3_RST" > }; > + static const char * const prop_names[] = { > + "standby", "xshutdown", > + }; > + > struct i2c_client *c = state->i2c_client; > struct s5c73m3_gpio *g = state->gpio; > - int ret, i; > + int i; > > for (i = 0; i < GPIO_NUM; ++i) { > - unsigned int flags = GPIOF_DIR_OUT; > + unsigned int flags = GPIOD_OUT_LOW; > if (g[i].level) > - flags |= GPIOF_INIT_HIGH; > - ret = devm_gpio_request_one(&c->dev, g[i].gpio, flags, > - gpio_names[i]); > - if (ret) { > + flags = GPIOD_OUT_HIGH; > + g[i].gpio = devm_gpiod_get_optional(&c->dev, prop_names[i], > + flags); > + if (IS_ERR(g[i].gpio)) { > v4l2_err(c, "failed to request gpio %s\n", > gpio_names[i]); > - return ret; > + return PTR_ERR(g[i].gpio); > } > } > return 0; > @@ -1586,7 +1590,6 @@ static int s5c73m3_parse_gpios(struct s5c73m3 *state) > prop_names[i]); > return -EINVAL; > } > - state->gpio[i].gpio = ret; > state->gpio[i].level = !(of_flags & OF_GPIO_ACTIVE_LOW); > } > return 0; > diff --git a/include/media/i2c/s5c73m3.h b/include/media/i2c/s5c73m3.h > index a51f1025ba1c..41e2235f0626 100644 > --- a/include/media/i2c/s5c73m3.h > +++ b/include/media/i2c/s5c73m3.h > @@ -17,6 +17,7 @@ > #ifndef MEDIA_S5C73M3__ > #define MEDIA_S5C73M3__ > > +#include <linux/gpio/consumer.h> > #include <linux/videodev2.h> > #include <media/v4l2-mediabus.h> > > @@ -26,7 +27,7 @@ > * @level: indicates active state of the @gpio > */ > struct s5c73m3_gpio { > - int gpio; > + struct gpio_desc *gpio; > int level; > }; > >
Hi Maíra, Hans, On 24.11.2021 12:25, Hans Verkuil wrote: > Hi Maíra, > > On 12/10/2021 21:46, Maíra Canal wrote: >> Removing old gpiod interface and replacing it for the gpiod consumer >> interface. > Has this been tested? I feel a bit uncomfortable to merged this without > knowing that it works. Andrzej, what do you think about this patch? This is step into good direction(thanks Maira), but I would suggest go further, all this gpio stuff in s5cc73m3 is obsolete. You could remove all the code which is already handled by gpiod framework: - s5c73m3_gpio, - s5c73m3_parse_gpios, - s5c73m3_gpio_set_value. Regards Andrzej > > Maíra, is there a specific reason why you made this patch? > > Regards, > > Hans > >> Signed-off-by: Maíra Canal <maira.canal@usp.br> >> --- >> drivers/media/i2c/s5c73m3/s5c73m3-core.c | 25 +++++++++++++----------- >> include/media/i2c/s5c73m3.h | 3 ++- >> 2 files changed, 16 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c b/drivers/media/i2c/s5c73m3/s5c73m3-core.c >> index e2b88c5e4f98..0c69a3fc7ebe 100644 >> --- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c >> +++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c >> @@ -10,7 +10,7 @@ >> #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> >> @@ -1349,9 +1349,9 @@ 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].gpio) >> return 0; >> - gpio_set_value(priv->gpio[id].gpio, !!val); >> + gpiod_set_value(priv->gpio[id].gpio, !!val); >> return 1; >> } >> >> @@ -1548,20 +1548,24 @@ static int s5c73m3_configure_gpios(struct s5c73m3 *state) >> static const char * const gpio_names[] = { >> "S5C73M3_STBY", "S5C73M3_RST" >> }; >> + static const char * const prop_names[] = { >> + "standby", "xshutdown", >> + }; >> + >> struct i2c_client *c = state->i2c_client; >> struct s5c73m3_gpio *g = state->gpio; >> - int ret, i; >> + int i; >> >> for (i = 0; i < GPIO_NUM; ++i) { >> - unsigned int flags = GPIOF_DIR_OUT; >> + unsigned int flags = GPIOD_OUT_LOW; >> if (g[i].level) >> - flags |= GPIOF_INIT_HIGH; >> - ret = devm_gpio_request_one(&c->dev, g[i].gpio, flags, >> - gpio_names[i]); >> - if (ret) { >> + flags = GPIOD_OUT_HIGH; >> + g[i].gpio = devm_gpiod_get_optional(&c->dev, prop_names[i], >> + flags); >> + if (IS_ERR(g[i].gpio)) { >> v4l2_err(c, "failed to request gpio %s\n", >> gpio_names[i]); >> - return ret; >> + return PTR_ERR(g[i].gpio); >> } >> } >> return 0; >> @@ -1586,7 +1590,6 @@ static int s5c73m3_parse_gpios(struct s5c73m3 *state) >> prop_names[i]); >> return -EINVAL; >> } >> - state->gpio[i].gpio = ret; >> state->gpio[i].level = !(of_flags & OF_GPIO_ACTIVE_LOW); >> } >> return 0; >> diff --git a/include/media/i2c/s5c73m3.h b/include/media/i2c/s5c73m3.h >> index a51f1025ba1c..41e2235f0626 100644 >> --- a/include/media/i2c/s5c73m3.h >> +++ b/include/media/i2c/s5c73m3.h >> @@ -17,6 +17,7 @@ >> #ifndef MEDIA_S5C73M3__ >> #define MEDIA_S5C73M3__ >> >> +#include <linux/gpio/consumer.h> >> #include <linux/videodev2.h> >> #include <media/v4l2-mediabus.h> >> >> @@ -26,7 +27,7 @@ >> * @level: indicates active state of the @gpio >> */ >> struct s5c73m3_gpio { >> - int gpio; >> + struct gpio_desc *gpio; >> int level; >> }; >> >>
diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c b/drivers/media/i2c/s5c73m3/s5c73m3-core.c index e2b88c5e4f98..0c69a3fc7ebe 100644 --- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c +++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c @@ -10,7 +10,7 @@ #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> @@ -1349,9 +1349,9 @@ 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].gpio) return 0; - gpio_set_value(priv->gpio[id].gpio, !!val); + gpiod_set_value(priv->gpio[id].gpio, !!val); return 1; } @@ -1548,20 +1548,24 @@ static int s5c73m3_configure_gpios(struct s5c73m3 *state) static const char * const gpio_names[] = { "S5C73M3_STBY", "S5C73M3_RST" }; + static const char * const prop_names[] = { + "standby", "xshutdown", + }; + struct i2c_client *c = state->i2c_client; struct s5c73m3_gpio *g = state->gpio; - int ret, i; + int i; for (i = 0; i < GPIO_NUM; ++i) { - unsigned int flags = GPIOF_DIR_OUT; + unsigned int flags = GPIOD_OUT_LOW; if (g[i].level) - flags |= GPIOF_INIT_HIGH; - ret = devm_gpio_request_one(&c->dev, g[i].gpio, flags, - gpio_names[i]); - if (ret) { + flags = GPIOD_OUT_HIGH; + g[i].gpio = devm_gpiod_get_optional(&c->dev, prop_names[i], + flags); + if (IS_ERR(g[i].gpio)) { v4l2_err(c, "failed to request gpio %s\n", gpio_names[i]); - return ret; + return PTR_ERR(g[i].gpio); } } return 0; @@ -1586,7 +1590,6 @@ static int s5c73m3_parse_gpios(struct s5c73m3 *state) prop_names[i]); return -EINVAL; } - state->gpio[i].gpio = ret; state->gpio[i].level = !(of_flags & OF_GPIO_ACTIVE_LOW); } return 0; diff --git a/include/media/i2c/s5c73m3.h b/include/media/i2c/s5c73m3.h index a51f1025ba1c..41e2235f0626 100644 --- a/include/media/i2c/s5c73m3.h +++ b/include/media/i2c/s5c73m3.h @@ -17,6 +17,7 @@ #ifndef MEDIA_S5C73M3__ #define MEDIA_S5C73M3__ +#include <linux/gpio/consumer.h> #include <linux/videodev2.h> #include <media/v4l2-mediabus.h> @@ -26,7 +27,7 @@ * @level: indicates active state of the @gpio */ struct s5c73m3_gpio { - int gpio; + struct gpio_desc *gpio; int level; };
Removing old gpiod interface and replacing it for the gpiod consumer interface. Signed-off-by: Maíra Canal <maira.canal@usp.br> --- drivers/media/i2c/s5c73m3/s5c73m3-core.c | 25 +++++++++++++----------- include/media/i2c/s5c73m3.h | 3 ++- 2 files changed, 16 insertions(+), 12 deletions(-)