Re: [PATCH] s5c73m3: adding gpiod support for the s5c73m3

[Andrzej Hajda <andrzej.hajda@xxxxxxxxx>]

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@xxxxxx>
> > ---
> >   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;
> >   };
> >