[PATCH] [media] ov2640: make GPIOLIB an optional dependency

[PATCH] [media] ov2640: make GPIOLIB an optional dependency

[Mauro Carvalho Chehab]

As warned by kbuild test robot:
	warning: (VIDEO_EM28XX_V4L2) selects VIDEO_OV2640 which has unmet direct dependencies (MEDIA_SUPPORT && VIDEO_V4L2 && I2C && GPIOLIB && MEDIA_CAMERA_SUPPORT)

The em28xx driver can use ov2640, but it doesn't depend
(or use) the GPIOLIB in order to power off/on the sensor.

So, as we want to allow both usages with and without
GPIOLIB, make its dependency optional.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/i2c/Kconfig  | 2 +-
 drivers/media/i2c/ov2640.c | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 40bb4bdc51da..fd181c99ce11 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -519,7 +519,7 @@ config VIDEO_SMIAPP_PLL
 
 config VIDEO_OV2640
 	tristate "OmniVision OV2640 sensor support"
-	depends on VIDEO_V4L2 && I2C && GPIOLIB
+	depends on VIDEO_V4L2 && I2C
 	depends on MEDIA_CAMERA_SUPPORT
 	help
 	  This is a Video4Linux2 sensor-level driver for the OmniVision
diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
index d55ca37dc12f..9c00ed3543f8 100644
--- a/drivers/media/i2c/ov2640.c
+++ b/drivers/media/i2c/ov2640.c
@@ -743,13 +743,16 @@ static int ov2640_s_power(struct v4l2_subdev *sd, int on)
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov2640_priv *priv = to_ov2640(client);
 
-	gpiod_direction_output(priv->pwdn_gpio, !on);
+#ifdef CONFIG_GPIOLIB
+	if (priv->pwdn_gpio)
+		gpiod_direction_output(priv->pwdn_gpio, !on);
 	if (on && priv->resetb_gpio) {
 		/* Active the resetb pin to perform a reset pulse */
 		gpiod_direction_output(priv->resetb_gpio, 1);
 		usleep_range(3000, 5000);
 		gpiod_direction_output(priv->resetb_gpio, 0);
 	}
+#endif
 	return 0;
 }
 
-- 
2.9.3

Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2196 bytes --]

Hi!

> As warned by kbuild test robot:
> 	warning: (VIDEO_EM28XX_V4L2) selects VIDEO_OV2640 which has unmet direct dependencies (MEDIA_SUPPORT && VIDEO_V4L2 && I2C && GPIOLIB && MEDIA_CAMERA_SUPPORT)
> 
> The em28xx driver can use ov2640, but it doesn't depend
> (or use) the GPIOLIB in order to power off/on the sensor.
> 
> So, as we want to allow both usages with and without
> GPIOLIB, make its dependency optional.

Umm. The driver will not work too well with sensor powered off, no?
Will this result in some tricky-to-debug situations?

>  config VIDEO_OV2640
>  	tristate "OmniVision OV2640 sensor support"
> -	depends on VIDEO_V4L2 && I2C && GPIOLIB
> +	depends on VIDEO_V4L2 && I2C
>  	depends on MEDIA_CAMERA_SUPPORT
>  	help
>  	  This is a Video4Linux2 sensor-level driver for the
>  	OmniVision

Better solution would be for VIDEO_EM28XX_V4L2 to depend on GPIOLIB,
too, no? If not, should there be BUG_ON(priv->pwdn_gpio);
BUG_ON(priv->resetb_gpio);?


> diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> index d55ca37dc12f..9c00ed3543f8 100644
> --- a/drivers/media/i2c/ov2640.c
> +++ b/drivers/media/i2c/ov2640.c
> @@ -743,13 +743,16 @@ static int ov2640_s_power(struct v4l2_subdev *sd, int on)
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  	struct ov2640_priv *priv = to_ov2640(client);
>  
> -	gpiod_direction_output(priv->pwdn_gpio, !on);
> +#ifdef CONFIG_GPIOLIB
> +	if (priv->pwdn_gpio)
> +		gpiod_direction_output(priv->pwdn_gpio, !on);
>  	if (on && priv->resetb_gpio) {
>  		/* Active the resetb pin to perform a reset pulse */
>  		gpiod_direction_output(priv->resetb_gpio, 1);
>  		usleep_range(3000, 5000);
>  		gpiod_direction_output(priv->resetb_gpio, 0);
>  	}
> +#endif
>  	return 0;
>  }

What is going on there? Should that be

              gpiod_direction_output(priv->resetb_gpio, 1);
              usleep_range(3000, 5000);
              gpiod_set_value(priv->resetb_gpio, 0);

for readability's sake?

								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency

[Mauro Carvalho Chehab]

Em Wed, 19 Apr 2017 15:23:39 +0200
Pavel Machek <pavel@ucw.cz> escreveu:

> Hi!
> 
> > As warned by kbuild test robot:
> > 	warning: (VIDEO_EM28XX_V4L2) selects VIDEO_OV2640 which has unmet direct dependencies (MEDIA_SUPPORT && VIDEO_V4L2 && I2C && GPIOLIB && MEDIA_CAMERA_SUPPORT)
> > 
> > The em28xx driver can use ov2640, but it doesn't depend
> > (or use) the GPIOLIB in order to power off/on the sensor.
> > 
> > So, as we want to allow both usages with and without
> > GPIOLIB, make its dependency optional.  
> 
> Umm. The driver will not work too well with sensor powered off, no?
> Will this result in some tricky-to-debug situations?
> 
> >  config VIDEO_OV2640
> >  	tristate "OmniVision OV2640 sensor support"
> > -	depends on VIDEO_V4L2 && I2C && GPIOLIB
> > +	depends on VIDEO_V4L2 && I2C
> >  	depends on MEDIA_CAMERA_SUPPORT
> >  	help
> >  	  This is a Video4Linux2 sensor-level driver for the
> >  	OmniVision  
> 
> Better solution would be for VIDEO_EM28XX_V4L2 to depend on GPIOLIB,
> too, no? If not, should there be BUG_ON(priv->pwdn_gpio);
> BUG_ON(priv->resetb_gpio);?

Pavel,

The em28xx driver was added upstream several years the gpio driver. 
It controls GPIO using a different logic. It makes no sense to make
it dependent on GPIOLIB, except if someone converts it to use it.

Besides that, I won't doubt that, at least on some em28xx webcams,
the sensor is always on.

Converting it to use the gpiolib not an easy task, as it supports a
hundred different device models and several different types of devices:
webcams, analog TV, digital TV, hybrid devices (plus devices with FM
radio too).

Too much work for no gain and a high risk of regressions.


Thanks,
Mauro

Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1372 bytes --]

Hi!

> > Better solution would be for VIDEO_EM28XX_V4L2 to depend on GPIOLIB,
> > too, no? If not, should there be BUG_ON(priv->pwdn_gpio);
> > BUG_ON(priv->resetb_gpio);?
> 
> Pavel,
> 
> The em28xx driver was added upstream several years the gpio driver. 
> It controls GPIO using a different logic. It makes no sense to make
> it dependent on GPIOLIB, except if someone converts it to use it.

At least comment in the sourcecode...? Remove pwdn_gpio fields from
structure in !GPIOLIB case, because otherwise they are trap for the
programmer trying to understand what is going on?

Plus, something like this, because otherwise it is quite confusing?

Thanks,
								Pavel

diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
index 56de182..85620e1 100644
--- a/drivers/media/i2c/soc_camera/ov2640.c
+++ b/drivers/media/i2c/soc_camera/ov2640.c
@@ -1060,7 +1060,7 @@ static int ov2640_hw_reset(struct device *dev)
 		/* Active the resetb pin to perform a reset pulse */
 		gpiod_direction_output(priv->resetb_gpio, 1);
 		usleep_range(3000, 5000);
-		gpiod_direction_output(priv->resetb_gpio, 0);
+		gpiod_set_value(priv->resetb_gpio, 0);
 	}
 
 	return 0;

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency

[Mauro Carvalho Chehab]

Em Fri, 21 Apr 2017 08:33:12 +0200
Pavel Machek <pavel@ucw.cz> escreveu:

> Hi!
> 
> > > Better solution would be for VIDEO_EM28XX_V4L2 to depend on GPIOLIB,
> > > too, no? If not, should there be BUG_ON(priv->pwdn_gpio);
> > > BUG_ON(priv->resetb_gpio);?  
> > 
> > Pavel,
> > 
> > The em28xx driver was added upstream several years the gpio driver. 
> > It controls GPIO using a different logic. It makes no sense to make
> > it dependent on GPIOLIB, except if someone converts it to use it.  
> 
> At least comment in the sourcecode...? Remove pwdn_gpio fields from
> structure in !GPIOLIB case, because otherwise they are trap for the
> programmer trying to understand what is going on?


Sorry, I answered to another e-mail thread related to it. I assumed
that it was c/c to linux-media, but it is, in fact a private e-mail.

I can see two alternatives:

1) Restore old behavior, assuming that all drivers that use OV2640 will
have GPIOLIB enabled, with a patch like:

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index fd181c99ce11..4e834c36f7da 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -521,6 +521,7 @@ config VIDEO_OV2640
        tristate "OmniVision OV2640 sensor support"
        depends on VIDEO_V4L2 && I2C
        depends on MEDIA_CAMERA_SUPPORT
+       depends on GPIOLIB if OF
        help
          This is a Video4Linux2 sensor-level driver for the OmniVision
          OV2640 camera.

However, I was told that some OF drivers don't actually define the GPIO
pins.

So, the other option is:

2) Make the logic smarter for OF, with this change:


diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
index 4a2ae24f8722..8855c81a9e1f 100644
--- a/drivers/media/i2c/ov2640.c
+++ b/drivers/media/i2c/ov2640.c
@@ -1048,21 +1048,39 @@ static const struct v4l2_subdev_ops ov2640_subdev_ops = {
 static int ov2640_probe_dt(struct i2c_client *client,
 		struct ov2640_priv *priv)
 {
+	int ret;
+
 	/* Request the reset GPIO deasserted */
 	priv->resetb_gpio = devm_gpiod_get_optional(&client->dev, "resetb",
 			GPIOD_OUT_LOW);
-	if (!priv->resetb_gpio)
+	if (!priv->resetb_gpio) {
 		dev_dbg(&client->dev, "resetb gpio is not assigned!\n");
-	else if (IS_ERR(priv->resetb_gpio))
-		return PTR_ERR(priv->resetb_gpio);
+	} else {
+		ret = PTR_ERR(priv->resetb_gpio);
+
+		if (ret && ret != -ENOSYS) {
+			dev_dbg(&client->dev,
+				"Error %d while getting resetb gpio\n",
+				ret);
+			return ret;
+		}
+	}
 
 	/* Request the power down GPIO asserted */
 	priv->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "pwdn",
 			GPIOD_OUT_HIGH);
-	if (!priv->pwdn_gpio)
+	if (!priv->pwdn_gpio) {
 		dev_dbg(&client->dev, "pwdn gpio is not assigned!\n");
-	else if (IS_ERR(priv->pwdn_gpio))
-		return PTR_ERR(priv->pwdn_gpio);
+	} else {
+		ret = PTR_ERR(priv->pwdn_gpio);
+
+		if (ret && ret != -ENOSYS) {
+			dev_dbg(&client->dev,
+				"Error %d while getting pwdn gpio\n",
+				ret);
+			return ret;
+		}
+	}
 
 	return 0;
 }

For this to work, OF caller drivers will have to depend or select GPIOLIB,
if they need those GPIO pins.

IMHO, (2) is better, but I'd like to hear more opinions from the driver
authors that require the usage of ov2640 I2C driver.

> 
> Plus, something like this, because otherwise it is quite confusing?
> 
> Thanks,
> 								Pavel
> 
> diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
> index 56de182..85620e1 100644
> --- a/drivers/media/i2c/soc_camera/ov2640.c
> +++ b/drivers/media/i2c/soc_camera/ov2640.c
> @@ -1060,7 +1060,7 @@ static int ov2640_hw_reset(struct device *dev)
>  		/* Active the resetb pin to perform a reset pulse */
>  		gpiod_direction_output(priv->resetb_gpio, 1);
>  		usleep_range(3000, 5000);
> -		gpiod_direction_output(priv->resetb_gpio, 0);
> +		gpiod_set_value(priv->resetb_gpio, 0);
>  	}
>  
>  	return 0;
> 

That should be, IMHO, on a separate patch. Why are you changing just
one of the set commands there? Shouldn't it be, instead:

diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
index 8855c81a9e1f..4ec567569ba2 100644
--- a/drivers/media/i2c/ov2640.c
+++ b/drivers/media/i2c/ov2640.c
@@ -770,12 +770,12 @@ static int ov2640_s_power(struct v4l2_subdev *sd, int on)
 
 #ifdef CONFIG_GPIOLIB
        if (priv->pwdn_gpio)
-               gpiod_direction_output(priv->pwdn_gpio, !on);
+               gpiod_set_value(priv->pwdn_gpio, !on);
        if (on && priv->resetb_gpio) {
                /* Active the resetb pin to perform a reset pulse */
-               gpiod_direction_output(priv->resetb_gpio, 1);
+               gpiod_set_value(priv->resetb_gpio, 1);
                usleep_range(3000, 5000);
-               gpiod_direction_output(priv->resetb_gpio, 0);
+               gpiod_set_value(priv->resetb_gpio, 0);
        }
 #endif


Thanks,
Mauro

Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency

[Sakari Ailus]

Hi Mauro and others,

On Fri, Apr 21, 2017 at 11:39:42AM -0300, Mauro Carvalho Chehab wrote:
> Em Fri, 21 Apr 2017 08:33:12 +0200
> Pavel Machek <pavel@ucw.cz> escreveu:
> 
> > Hi!
> > 
> > > > Better solution would be for VIDEO_EM28XX_V4L2 to depend on GPIOLIB,
> > > > too, no? If not, should there be BUG_ON(priv->pwdn_gpio);
> > > > BUG_ON(priv->resetb_gpio);?  
> > > 
> > > Pavel,
> > > 
> > > The em28xx driver was added upstream several years the gpio driver. 
> > > It controls GPIO using a different logic. It makes no sense to make
> > > it dependent on GPIOLIB, except if someone converts it to use it.  
> > 
> > At least comment in the sourcecode...? Remove pwdn_gpio fields from
> > structure in !GPIOLIB case, because otherwise they are trap for the
> > programmer trying to understand what is going on?
> 
> 
> Sorry, I answered to another e-mail thread related to it. I assumed
> that it was c/c to linux-media, but it is, in fact a private e-mail.

I thought so, too! X-)

> 
> I can see two alternatives:
> 
> 1) Restore old behavior, assuming that all drivers that use OV2640 will
> have GPIOLIB enabled, with a patch like:
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index fd181c99ce11..4e834c36f7da 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -521,6 +521,7 @@ config VIDEO_OV2640
>         tristate "OmniVision OV2640 sensor support"
>         depends on VIDEO_V4L2 && I2C
>         depends on MEDIA_CAMERA_SUPPORT
> +       depends on GPIOLIB if OF
>         help
>           This is a Video4Linux2 sensor-level driver for the OmniVision
>           OV2640 camera.
> 
> However, I was told that some OF drivers don't actually define the GPIO
> pins.
> 
> So, the other option is:
> 
> 2) Make the logic smarter for OF, with this change:
> 
> 
> diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> index 4a2ae24f8722..8855c81a9e1f 100644
> --- a/drivers/media/i2c/ov2640.c
> +++ b/drivers/media/i2c/ov2640.c
> @@ -1048,21 +1048,39 @@ static const struct v4l2_subdev_ops ov2640_subdev_ops = {
>  static int ov2640_probe_dt(struct i2c_client *client,
>  		struct ov2640_priv *priv)
>  {
> +	int ret;
> +
>  	/* Request the reset GPIO deasserted */
>  	priv->resetb_gpio = devm_gpiod_get_optional(&client->dev, "resetb",
>  			GPIOD_OUT_LOW);
> -	if (!priv->resetb_gpio)
> +	if (!priv->resetb_gpio) {
>  		dev_dbg(&client->dev, "resetb gpio is not assigned!\n");
> -	else if (IS_ERR(priv->resetb_gpio))
> -		return PTR_ERR(priv->resetb_gpio);
> +	} else {
> +		ret = PTR_ERR(priv->resetb_gpio);
> +
> +		if (ret && ret != -ENOSYS) {
> +			dev_dbg(&client->dev,
> +				"Error %d while getting resetb gpio\n",
> +				ret);
> +			return ret;
> +		}
> +	}

This would work. I just wish it'd look nicer. :-)

How about something like:

ret = PTR_ERR(priv->reset_gpio);
if (!priv->reset_gpio) {
	dev_dbg("reset gpio is not assigned");
} else if (ret != -ENOSYS) {
	dev_dbg("error %d while getting reset gpio, ret);
	return ret;
}

I prefer this option as it's not dependent on the system firmware type.

>  
>  	/* Request the power down GPIO asserted */
>  	priv->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "pwdn",
>  			GPIOD_OUT_HIGH);
> -	if (!priv->pwdn_gpio)
> +	if (!priv->pwdn_gpio) {
>  		dev_dbg(&client->dev, "pwdn gpio is not assigned!\n");
> -	else if (IS_ERR(priv->pwdn_gpio))
> -		return PTR_ERR(priv->pwdn_gpio);
> +	} else {
> +		ret = PTR_ERR(priv->pwdn_gpio);
> +
> +		if (ret && ret != -ENOSYS) {
> +			dev_dbg(&client->dev,
> +				"Error %d while getting pwdn gpio\n",
> +				ret);
> +			return ret;
> +		}
> +	}
>  
>  	return 0;
>  }
> 
> For this to work, OF caller drivers will have to depend or select GPIOLIB,
> if they need those GPIO pins.
> 
> IMHO, (2) is better, but I'd like to hear more opinions from the driver
> authors that require the usage of ov2640 I2C driver.
> 
> > 
> > Plus, something like this, because otherwise it is quite confusing?
> > 
> > Thanks,
> > 								Pavel
> > 
> > diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
> > index 56de182..85620e1 100644
> > --- a/drivers/media/i2c/soc_camera/ov2640.c
> > +++ b/drivers/media/i2c/soc_camera/ov2640.c
> > @@ -1060,7 +1060,7 @@ static int ov2640_hw_reset(struct device *dev)
> >  		/* Active the resetb pin to perform a reset pulse */
> >  		gpiod_direction_output(priv->resetb_gpio, 1);
> >  		usleep_range(3000, 5000);
> > -		gpiod_direction_output(priv->resetb_gpio, 0);
> > +		gpiod_set_value(priv->resetb_gpio, 0);
> >  	}
> >  
> >  	return 0;
> > 
> 
> That should be, IMHO, on a separate patch. Why are you changing just
> one of the set commands there? Shouldn't it be, instead:
> 
> diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> index 8855c81a9e1f..4ec567569ba2 100644
> --- a/drivers/media/i2c/ov2640.c
> +++ b/drivers/media/i2c/ov2640.c
> @@ -770,12 +770,12 @@ static int ov2640_s_power(struct v4l2_subdev *sd, int on)
>  
>  #ifdef CONFIG_GPIOLIB
>         if (priv->pwdn_gpio)
> -               gpiod_direction_output(priv->pwdn_gpio, !on);
> +               gpiod_set_value(priv->pwdn_gpio, !on);

As long as the direction is first set somewhere... (I haven't checked.)

>         if (on && priv->resetb_gpio) {
>                 /* Active the resetb pin to perform a reset pulse */
> -               gpiod_direction_output(priv->resetb_gpio, 1);
> +               gpiod_set_value(priv->resetb_gpio, 1);
>                 usleep_range(3000, 5000);
> -               gpiod_direction_output(priv->resetb_gpio, 0);
> +               gpiod_set_value(priv->resetb_gpio, 0);
>         }
>  #endif

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency

[Mauro Carvalho Chehab]

Em Mon, 24 Apr 2017 17:44:02 +0300
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Hi Mauro and others,
> 
> On Fri, Apr 21, 2017 at 11:39:42AM -0300, Mauro Carvalho Chehab wrote:
> > Em Fri, 21 Apr 2017 08:33:12 +0200
> > Pavel Machek <pavel@ucw.cz> escreveu:
> >   
> > > Hi!
> > >   
> > > > > Better solution would be for VIDEO_EM28XX_V4L2 to depend on GPIOLIB,
> > > > > too, no? If not, should there be BUG_ON(priv->pwdn_gpio);
> > > > > BUG_ON(priv->resetb_gpio);?    
> > > > 
> > > > Pavel,
> > > > 
> > > > The em28xx driver was added upstream several years the gpio driver. 
> > > > It controls GPIO using a different logic. It makes no sense to make
> > > > it dependent on GPIOLIB, except if someone converts it to use it.    
> > > 
> > > At least comment in the sourcecode...? Remove pwdn_gpio fields from
> > > structure in !GPIOLIB case, because otherwise they are trap for the
> > > programmer trying to understand what is going on?  
> > 
> > 
> > Sorry, I answered to another e-mail thread related to it. I assumed
> > that it was c/c to linux-media, but it is, in fact a private e-mail.  
> 
> I thought so, too! X-)
> 
> > 
> > I can see two alternatives:
> > 
> > 1) Restore old behavior, assuming that all drivers that use OV2640 will
> > have GPIOLIB enabled, with a patch like:
> > 
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index fd181c99ce11..4e834c36f7da 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -521,6 +521,7 @@ config VIDEO_OV2640
> >         tristate "OmniVision OV2640 sensor support"
> >         depends on VIDEO_V4L2 && I2C
> >         depends on MEDIA_CAMERA_SUPPORT
> > +       depends on GPIOLIB if OF
> >         help
> >           This is a Video4Linux2 sensor-level driver for the OmniVision
> >           OV2640 camera.
> > 
> > However, I was told that some OF drivers don't actually define the GPIO
> > pins.
> > 
> > So, the other option is:
> > 
> > 2) Make the logic smarter for OF, with this change:
> > 
> > 
> > diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> > index 4a2ae24f8722..8855c81a9e1f 100644
> > --- a/drivers/media/i2c/ov2640.c
> > +++ b/drivers/media/i2c/ov2640.c
> > @@ -1048,21 +1048,39 @@ static const struct v4l2_subdev_ops ov2640_subdev_ops = {
> >  static int ov2640_probe_dt(struct i2c_client *client,
> >  		struct ov2640_priv *priv)
> >  {
> > +	int ret;
> > +
> >  	/* Request the reset GPIO deasserted */
> >  	priv->resetb_gpio = devm_gpiod_get_optional(&client->dev, "resetb",
> >  			GPIOD_OUT_LOW);
> > -	if (!priv->resetb_gpio)
> > +	if (!priv->resetb_gpio) {
> >  		dev_dbg(&client->dev, "resetb gpio is not assigned!\n");
> > -	else if (IS_ERR(priv->resetb_gpio))
> > -		return PTR_ERR(priv->resetb_gpio);
> > +	} else {
> > +		ret = PTR_ERR(priv->resetb_gpio);
> > +
> > +		if (ret && ret != -ENOSYS) {
> > +			dev_dbg(&client->dev,
> > +				"Error %d while getting resetb gpio\n",
> > +				ret);
> > +			return ret;
> > +		}
> > +	}  
> 
> This would work. I just wish it'd look nicer. :-)
> 
> How about something like:
> 
> ret = PTR_ERR(priv->reset_gpio);
> if (!priv->reset_gpio) {
> 	dev_dbg("reset gpio is not assigned");
> } else if (ret != -ENOSYS) {
> 	dev_dbg("error %d while getting reset gpio, ret);

It still need to test if ret == 0, as otherwise it will do the wrong thing
here. 


I guess the patch below does the trick and it is not ugly :-)


> 	return ret;
> }
> 
> I prefer this option as it's not dependent on the system firmware type.

Thanks,
Mauro

[PATCH] ov2640: print error if devm_*_optional*() fails

devm_gpiod_get_optional() can return -ENOSYS if GPIOLIB is
disabled, causing probe to fail. Warn the user if this
happens.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
index 4a2ae24f8722..8c87ed8306ea 100644
--- a/drivers/media/i2c/ov2640.c
+++ b/drivers/media/i2c/ov2640.c
@@ -765,17 +765,17 @@ static int ov2640_s_register(struct v4l2_subdev *sd,
 
 static int ov2640_s_power(struct v4l2_subdev *sd, int on)
 {
+#ifdef CONFIG_GPIOLIB
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov2640_priv *priv = to_ov2640(client);
 
-#ifdef CONFIG_GPIOLIB
 	if (priv->pwdn_gpio)
 		gpiod_direction_output(priv->pwdn_gpio, !on);
 	if (on && priv->resetb_gpio) {
 		/* Active the resetb pin to perform a reset pulse */
 		gpiod_direction_output(priv->resetb_gpio, 1);
 		usleep_range(3000, 5000);
-		gpiod_direction_output(priv->resetb_gpio, 0);
+		gpiod_set_value(priv->resetb_gpio, 0);
 	}
 #endif
 	return 0;
@@ -1048,21 +1048,35 @@ static const struct v4l2_subdev_ops ov2640_subdev_ops = {
 static int ov2640_probe_dt(struct i2c_client *client,
 		struct ov2640_priv *priv)
 {
+	int ret;
+
 	/* Request the reset GPIO deasserted */
 	priv->resetb_gpio = devm_gpiod_get_optional(&client->dev, "resetb",
 			GPIOD_OUT_LOW);
+
 	if (!priv->resetb_gpio)
 		dev_dbg(&client->dev, "resetb gpio is not assigned!\n");
-	else if (IS_ERR(priv->resetb_gpio))
-		return PTR_ERR(priv->resetb_gpio);
+
+	ret = PTR_ERR_OR_ZERO(priv->pwdn_gpio);
+	if (ret && ret != -ENOSYS) {
+		dev_dbg(&client->dev,
+			"Error %d while getting resetb gpio\n", ret);
+		return ret;
+	}
 
 	/* Request the power down GPIO asserted */
 	priv->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "pwdn",
 			GPIOD_OUT_HIGH);
+
 	if (!priv->pwdn_gpio)
 		dev_dbg(&client->dev, "pwdn gpio is not assigned!\n");
-	else if (IS_ERR(priv->pwdn_gpio))
-		return PTR_ERR(priv->pwdn_gpio);
+
+	ret = PTR_ERR_OR_ZERO(priv->pwdn_gpio);
+	if (ret && ret != -ENOSYS) {
+		dev_dbg(&client->dev,
+			"Error %d while getting pwdn gpio\n", ret);
+		return ret;
+	}
 
 	return 0;
 }

Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency

[Sakari Ailus]

Hi Mauro,

On Mon, Apr 24, 2017 at 12:50:36PM -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 24 Apr 2017 17:44:02 +0300
> Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> 
> > Hi Mauro and others,
> > 
> > On Fri, Apr 21, 2017 at 11:39:42AM -0300, Mauro Carvalho Chehab wrote:
> > > Em Fri, 21 Apr 2017 08:33:12 +0200
> > > Pavel Machek <pavel@ucw.cz> escreveu:
> > >   
> > > > Hi!
> > > >   
> > > > > > Better solution would be for VIDEO_EM28XX_V4L2 to depend on GPIOLIB,
> > > > > > too, no? If not, should there be BUG_ON(priv->pwdn_gpio);
> > > > > > BUG_ON(priv->resetb_gpio);?    
> > > > > 
> > > > > Pavel,
> > > > > 
> > > > > The em28xx driver was added upstream several years the gpio driver. 
> > > > > It controls GPIO using a different logic. It makes no sense to make
> > > > > it dependent on GPIOLIB, except if someone converts it to use it.    
> > > > 
> > > > At least comment in the sourcecode...? Remove pwdn_gpio fields from
> > > > structure in !GPIOLIB case, because otherwise they are trap for the
> > > > programmer trying to understand what is going on?  
> > > 
> > > 
> > > Sorry, I answered to another e-mail thread related to it. I assumed
> > > that it was c/c to linux-media, but it is, in fact a private e-mail.  
> > 
> > I thought so, too! X-)
> > 
> > > 
> > > I can see two alternatives:
> > > 
> > > 1) Restore old behavior, assuming that all drivers that use OV2640 will
> > > have GPIOLIB enabled, with a patch like:
> > > 
> > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > index fd181c99ce11..4e834c36f7da 100644
> > > --- a/drivers/media/i2c/Kconfig
> > > +++ b/drivers/media/i2c/Kconfig
> > > @@ -521,6 +521,7 @@ config VIDEO_OV2640
> > >         tristate "OmniVision OV2640 sensor support"
> > >         depends on VIDEO_V4L2 && I2C
> > >         depends on MEDIA_CAMERA_SUPPORT
> > > +       depends on GPIOLIB if OF
> > >         help
> > >           This is a Video4Linux2 sensor-level driver for the OmniVision
> > >           OV2640 camera.
> > > 
> > > However, I was told that some OF drivers don't actually define the GPIO
> > > pins.
> > > 
> > > So, the other option is:
> > > 
> > > 2) Make the logic smarter for OF, with this change:
> > > 
> > > 
> > > diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> > > index 4a2ae24f8722..8855c81a9e1f 100644
> > > --- a/drivers/media/i2c/ov2640.c
> > > +++ b/drivers/media/i2c/ov2640.c
> > > @@ -1048,21 +1048,39 @@ static const struct v4l2_subdev_ops ov2640_subdev_ops = {
> > >  static int ov2640_probe_dt(struct i2c_client *client,
> > >  		struct ov2640_priv *priv)
> > >  {
> > > +	int ret;
> > > +
> > >  	/* Request the reset GPIO deasserted */
> > >  	priv->resetb_gpio = devm_gpiod_get_optional(&client->dev, "resetb",
> > >  			GPIOD_OUT_LOW);
> > > -	if (!priv->resetb_gpio)
> > > +	if (!priv->resetb_gpio) {
> > >  		dev_dbg(&client->dev, "resetb gpio is not assigned!\n");
> > > -	else if (IS_ERR(priv->resetb_gpio))
> > > -		return PTR_ERR(priv->resetb_gpio);
> > > +	} else {
> > > +		ret = PTR_ERR(priv->resetb_gpio);
> > > +
> > > +		if (ret && ret != -ENOSYS) {
> > > +			dev_dbg(&client->dev,
> > > +				"Error %d while getting resetb gpio\n",
> > > +				ret);
> > > +			return ret;
> > > +		}
> > > +	}  
> > 
> > This would work. I just wish it'd look nicer. :-)
> > 
> > How about something like:
> > 
> > ret = PTR_ERR(priv->reset_gpio);
> > if (!priv->reset_gpio) {
> > 	dev_dbg("reset gpio is not assigned");
> > } else if (ret != -ENOSYS) {
> > 	dev_dbg("error %d while getting reset gpio, ret);
> 
> It still need to test if ret == 0, as otherwise it will do the wrong thing
> here. 

ret won't be zero here, that was checked above. You could check for just ret
though, it'd be easier to read that way.

> 
> 
> I guess the patch below does the trick and it is not ugly :-)
> 
> 
> > 	return ret;
> > }
> > 
> > I prefer this option as it's not dependent on the system firmware type.
> 
> Thanks,
> Mauro
> 
> [PATCH] ov2640: print error if devm_*_optional*() fails
> 
> devm_gpiod_get_optional() can return -ENOSYS if GPIOLIB is
> disabled, causing probe to fail. Warn the user if this
> happens.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> 
> diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> index 4a2ae24f8722..8c87ed8306ea 100644
> --- a/drivers/media/i2c/ov2640.c
> +++ b/drivers/media/i2c/ov2640.c
> @@ -765,17 +765,17 @@ static int ov2640_s_register(struct v4l2_subdev *sd,
>  
>  static int ov2640_s_power(struct v4l2_subdev *sd, int on)
>  {
> +#ifdef CONFIG_GPIOLIB
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  	struct ov2640_priv *priv = to_ov2640(client);
>  
> -#ifdef CONFIG_GPIOLIB
>  	if (priv->pwdn_gpio)
>  		gpiod_direction_output(priv->pwdn_gpio, !on);
>  	if (on && priv->resetb_gpio) {
>  		/* Active the resetb pin to perform a reset pulse */
>  		gpiod_direction_output(priv->resetb_gpio, 1);
>  		usleep_range(3000, 5000);
> -		gpiod_direction_output(priv->resetb_gpio, 0);
> +		gpiod_set_value(priv->resetb_gpio, 0);
>  	}
>  #endif
>  	return 0;
> @@ -1048,21 +1048,35 @@ static const struct v4l2_subdev_ops ov2640_subdev_ops = {
>  static int ov2640_probe_dt(struct i2c_client *client,
>  		struct ov2640_priv *priv)
>  {
> +	int ret;
> +
>  	/* Request the reset GPIO deasserted */
>  	priv->resetb_gpio = devm_gpiod_get_optional(&client->dev, "resetb",
>  			GPIOD_OUT_LOW);
> +
>  	if (!priv->resetb_gpio)
>  		dev_dbg(&client->dev, "resetb gpio is not assigned!\n");
> -	else if (IS_ERR(priv->resetb_gpio))
> -		return PTR_ERR(priv->resetb_gpio);
> +
> +	ret = PTR_ERR_OR_ZERO(priv->pwdn_gpio);

s/pwdn/resetb/

This works, but you're essentially checking the same thing twice. ret is
only zero iff priv->resetb_gpio is NULL.

> +	if (ret && ret != -ENOSYS) {
> +		dev_dbg(&client->dev,
> +			"Error %d while getting resetb gpio\n", ret);
> +		return ret;
> +	}
>  
>  	/* Request the power down GPIO asserted */
>  	priv->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "pwdn",
>  			GPIOD_OUT_HIGH);
> +
>  	if (!priv->pwdn_gpio)
>  		dev_dbg(&client->dev, "pwdn gpio is not assigned!\n");
> -	else if (IS_ERR(priv->pwdn_gpio))
> -		return PTR_ERR(priv->pwdn_gpio);
> +
> +	ret = PTR_ERR_OR_ZERO(priv->pwdn_gpio);
> +	if (ret && ret != -ENOSYS) {
> +		dev_dbg(&client->dev,
> +			"Error %d while getting pwdn gpio\n", ret);
> +		return ret;
> +	}
>  
>  	return 0;
>  }

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency

[Sakari Ailus]

On Mon, Apr 24, 2017 at 08:38:47PM +0300, Sakari Ailus wrote:
...
> ret won't be zero here, that was checked above. You could check for just ret
> though, it'd be easier to read that way.

I missed ret would have to have type long instead. How about:

ret = PTR_ERR(priv->reset_gpio);
if (!priv->reset_gpio) {
	dev_dbg("reset gpio is not assigned\n");
} else if (IS_ERR(priv->reset_gpio) && ret != -ENOSYS) {
	dev_dbg("error %d while getting reset gpio", ret);
	return ret;
}

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency

[Mauro Carvalho Chehab]

Em Mon, 24 Apr 2017 20:38:47 +0300
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Hi Mauro,
> 
> On Mon, Apr 24, 2017 at 12:50:36PM -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 24 Apr 2017 17:44:02 +0300
> > Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> >   
> > > Hi Mauro and others,
> > > 
> > > On Fri, Apr 21, 2017 at 11:39:42AM -0300, Mauro Carvalho Chehab wrote:  
> > > > Em Fri, 21 Apr 2017 08:33:12 +0200
> > > > Pavel Machek <pavel@ucw.cz> escreveu:
> > > >     
> > > > > Hi!
> > > > >     
> > > > > > > Better solution would be for VIDEO_EM28XX_V4L2 to depend on GPIOLIB,
> > > > > > > too, no? If not, should there be BUG_ON(priv->pwdn_gpio);
> > > > > > > BUG_ON(priv->resetb_gpio);?      
> > > > > > 
> > > > > > Pavel,
> > > > > > 
> > > > > > The em28xx driver was added upstream several years the gpio driver. 
> > > > > > It controls GPIO using a different logic. It makes no sense to make
> > > > > > it dependent on GPIOLIB, except if someone converts it to use it.      
> > > > > 
> > > > > At least comment in the sourcecode...? Remove pwdn_gpio fields from
> > > > > structure in !GPIOLIB case, because otherwise they are trap for the
> > > > > programmer trying to understand what is going on?    
> > > > 
> > > > 
> > > > Sorry, I answered to another e-mail thread related to it. I assumed
> > > > that it was c/c to linux-media, but it is, in fact a private e-mail.    
> > > 
> > > I thought so, too! X-)
> > >   
> > > > 
> > > > I can see two alternatives:
> > > > 
> > > > 1) Restore old behavior, assuming that all drivers that use OV2640 will
> > > > have GPIOLIB enabled, with a patch like:
> > > > 
> > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > index fd181c99ce11..4e834c36f7da 100644
> > > > --- a/drivers/media/i2c/Kconfig
> > > > +++ b/drivers/media/i2c/Kconfig
> > > > @@ -521,6 +521,7 @@ config VIDEO_OV2640
> > > >         tristate "OmniVision OV2640 sensor support"
> > > >         depends on VIDEO_V4L2 && I2C
> > > >         depends on MEDIA_CAMERA_SUPPORT
> > > > +       depends on GPIOLIB if OF
> > > >         help
> > > >           This is a Video4Linux2 sensor-level driver for the OmniVision
> > > >           OV2640 camera.
> > > > 
> > > > However, I was told that some OF drivers don't actually define the GPIO
> > > > pins.
> > > > 
> > > > So, the other option is:
> > > > 
> > > > 2) Make the logic smarter for OF, with this change:
> > > > 
> > > > 
> > > > diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> > > > index 4a2ae24f8722..8855c81a9e1f 100644
> > > > --- a/drivers/media/i2c/ov2640.c
> > > > +++ b/drivers/media/i2c/ov2640.c
> > > > @@ -1048,21 +1048,39 @@ static const struct v4l2_subdev_ops ov2640_subdev_ops = {
> > > >  static int ov2640_probe_dt(struct i2c_client *client,
> > > >  		struct ov2640_priv *priv)
> > > >  {
> > > > +	int ret;
> > > > +
> > > >  	/* Request the reset GPIO deasserted */
> > > >  	priv->resetb_gpio = devm_gpiod_get_optional(&client->dev, "resetb",
> > > >  			GPIOD_OUT_LOW);
> > > > -	if (!priv->resetb_gpio)
> > > > +	if (!priv->resetb_gpio) {
> > > >  		dev_dbg(&client->dev, "resetb gpio is not assigned!\n");
> > > > -	else if (IS_ERR(priv->resetb_gpio))
> > > > -		return PTR_ERR(priv->resetb_gpio);
> > > > +	} else {
> > > > +		ret = PTR_ERR(priv->resetb_gpio);
> > > > +
> > > > +		if (ret && ret != -ENOSYS) {
> > > > +			dev_dbg(&client->dev,
> > > > +				"Error %d while getting resetb gpio\n",
> > > > +				ret);
> > > > +			return ret;
> > > > +		}
> > > > +	}    
> > > 
> > > This would work. I just wish it'd look nicer. :-)
> > > 
> > > How about something like:
> > > 
> > > ret = PTR_ERR(priv->reset_gpio);
> > > if (!priv->reset_gpio) {
> > > 	dev_dbg("reset gpio is not assigned");
> > > } else if (ret != -ENOSYS) {
> > > 	dev_dbg("error %d while getting reset gpio, ret);  
> > 
> > It still need to test if ret == 0, as otherwise it will do the wrong thing
> > here.   
> 
> ret won't be zero here, that was checked above. You could check for just ret
> though, it'd be easier to read that way.
> 
> > 
> > 
> > I guess the patch below does the trick and it is not ugly :-)
> > 
> >   
> > > 	return ret;
> > > }
> > > 
> > > I prefer this option as it's not dependent on the system firmware type.  
> > 
> > Thanks,
> > Mauro
> > 
> > [PATCH] ov2640: print error if devm_*_optional*() fails
> > 
> > devm_gpiod_get_optional() can return -ENOSYS if GPIOLIB is
> > disabled, causing probe to fail. Warn the user if this
> > happens.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> > 
> > diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> > index 4a2ae24f8722..8c87ed8306ea 100644
> > --- a/drivers/media/i2c/ov2640.c
> > +++ b/drivers/media/i2c/ov2640.c
> > @@ -765,17 +765,17 @@ static int ov2640_s_register(struct v4l2_subdev *sd,
> >  
> >  static int ov2640_s_power(struct v4l2_subdev *sd, int on)
> >  {
> > +#ifdef CONFIG_GPIOLIB
> >  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> >  	struct ov2640_priv *priv = to_ov2640(client);
> >  
> > -#ifdef CONFIG_GPIOLIB
> >  	if (priv->pwdn_gpio)
> >  		gpiod_direction_output(priv->pwdn_gpio, !on);
> >  	if (on && priv->resetb_gpio) {
> >  		/* Active the resetb pin to perform a reset pulse */
> >  		gpiod_direction_output(priv->resetb_gpio, 1);
> >  		usleep_range(3000, 5000);
> > -		gpiod_direction_output(priv->resetb_gpio, 0);
> > +		gpiod_set_value(priv->resetb_gpio, 0);
> >  	}
> >  #endif
> >  	return 0;
> > @@ -1048,21 +1048,35 @@ static const struct v4l2_subdev_ops ov2640_subdev_ops = {
> >  static int ov2640_probe_dt(struct i2c_client *client,
> >  		struct ov2640_priv *priv)
> >  {
> > +	int ret;
> > +
> >  	/* Request the reset GPIO deasserted */
> >  	priv->resetb_gpio = devm_gpiod_get_optional(&client->dev, "resetb",
> >  			GPIOD_OUT_LOW);
> > +
> >  	if (!priv->resetb_gpio)
> >  		dev_dbg(&client->dev, "resetb gpio is not assigned!\n");
> > -	else if (IS_ERR(priv->resetb_gpio))
> > -		return PTR_ERR(priv->resetb_gpio);
> > +
> > +	ret = PTR_ERR_OR_ZERO(priv->pwdn_gpio);  
> 
> s/pwdn/resetb/

True.

> 
> This works, but you're essentially checking the same thing twice. ret is
> only zero iff priv->resetb_gpio is NULL.

No. That's the implementation for PTR_ERR_OR_ZERO:

static inline int __must_check PTR_ERR_OR_ZERO(__force const void *ptr)
{
	if (IS_ERR(ptr))
		return PTR_ERR(ptr);
	else
		return 0;
}

ret is zero if either priv->resetb_gpio is NULL or if it is a valid
pointer.

That is about the same as what you proposed on this code snippet:

> How about:
> 
> ret = PTR_ERR(priv->reset_gpio);
> if (!priv->reset_gpio) {
> 	dev_dbg("reset gpio is not assigned\n");
> } else if (IS_ERR(priv->reset_gpio) && ret != -ENOSYS) {
> 	dev_dbg("error %d while getting reset gpio", ret);
> 	return ret;
> }
> 


Thanks,
Mauro

Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency

[Sakari Ailus]

Hi Mauro,

On Mon, Apr 24, 2017 at 11:05:29PM -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 24 Apr 2017 20:38:47 +0300
> Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> 
> > Hi Mauro,
> > 
> > On Mon, Apr 24, 2017 at 12:50:36PM -0300, Mauro Carvalho Chehab wrote:
> > > Em Mon, 24 Apr 2017 17:44:02 +0300
> > > Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> > >   
> > > > Hi Mauro and others,
> > > > 
> > > > On Fri, Apr 21, 2017 at 11:39:42AM -0300, Mauro Carvalho Chehab wrote:  
> > > > > Em Fri, 21 Apr 2017 08:33:12 +0200
> > > > > Pavel Machek <pavel@ucw.cz> escreveu:
> > > > >     
> > > > > > Hi!
> > > > > >     
> > > > > > > > Better solution would be for VIDEO_EM28XX_V4L2 to depend on GPIOLIB,
> > > > > > > > too, no? If not, should there be BUG_ON(priv->pwdn_gpio);
> > > > > > > > BUG_ON(priv->resetb_gpio);?      
> > > > > > > 
> > > > > > > Pavel,
> > > > > > > 
> > > > > > > The em28xx driver was added upstream several years the gpio driver. 
> > > > > > > It controls GPIO using a different logic. It makes no sense to make
> > > > > > > it dependent on GPIOLIB, except if someone converts it to use it.      
> > > > > > 
> > > > > > At least comment in the sourcecode...? Remove pwdn_gpio fields from
> > > > > > structure in !GPIOLIB case, because otherwise they are trap for the
> > > > > > programmer trying to understand what is going on?    
> > > > > 
> > > > > 
> > > > > Sorry, I answered to another e-mail thread related to it. I assumed
> > > > > that it was c/c to linux-media, but it is, in fact a private e-mail.    
> > > > 
> > > > I thought so, too! X-)
> > > >   
> > > > > 
> > > > > I can see two alternatives:
> > > > > 
> > > > > 1) Restore old behavior, assuming that all drivers that use OV2640 will
> > > > > have GPIOLIB enabled, with a patch like:
> > > > > 
> > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > > index fd181c99ce11..4e834c36f7da 100644
> > > > > --- a/drivers/media/i2c/Kconfig
> > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > @@ -521,6 +521,7 @@ config VIDEO_OV2640
> > > > >         tristate "OmniVision OV2640 sensor support"
> > > > >         depends on VIDEO_V4L2 && I2C
> > > > >         depends on MEDIA_CAMERA_SUPPORT
> > > > > +       depends on GPIOLIB if OF
> > > > >         help
> > > > >           This is a Video4Linux2 sensor-level driver for the OmniVision
> > > > >           OV2640 camera.
> > > > > 
> > > > > However, I was told that some OF drivers don't actually define the GPIO
> > > > > pins.
> > > > > 
> > > > > So, the other option is:
> > > > > 
> > > > > 2) Make the logic smarter for OF, with this change:
> > > > > 
> > > > > 
> > > > > diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> > > > > index 4a2ae24f8722..8855c81a9e1f 100644
> > > > > --- a/drivers/media/i2c/ov2640.c
> > > > > +++ b/drivers/media/i2c/ov2640.c
> > > > > @@ -1048,21 +1048,39 @@ static const struct v4l2_subdev_ops ov2640_subdev_ops = {
> > > > >  static int ov2640_probe_dt(struct i2c_client *client,
> > > > >  		struct ov2640_priv *priv)
> > > > >  {
> > > > > +	int ret;
> > > > > +
> > > > >  	/* Request the reset GPIO deasserted */
> > > > >  	priv->resetb_gpio = devm_gpiod_get_optional(&client->dev, "resetb",
> > > > >  			GPIOD_OUT_LOW);
> > > > > -	if (!priv->resetb_gpio)
> > > > > +	if (!priv->resetb_gpio) {
> > > > >  		dev_dbg(&client->dev, "resetb gpio is not assigned!\n");
> > > > > -	else if (IS_ERR(priv->resetb_gpio))
> > > > > -		return PTR_ERR(priv->resetb_gpio);
> > > > > +	} else {
> > > > > +		ret = PTR_ERR(priv->resetb_gpio);
> > > > > +
> > > > > +		if (ret && ret != -ENOSYS) {
> > > > > +			dev_dbg(&client->dev,
> > > > > +				"Error %d while getting resetb gpio\n",
> > > > > +				ret);
> > > > > +			return ret;
> > > > > +		}
> > > > > +	}    
> > > > 
> > > > This would work. I just wish it'd look nicer. :-)
> > > > 
> > > > How about something like:
> > > > 
> > > > ret = PTR_ERR(priv->reset_gpio);
> > > > if (!priv->reset_gpio) {
> > > > 	dev_dbg("reset gpio is not assigned");
> > > > } else if (ret != -ENOSYS) {
> > > > 	dev_dbg("error %d while getting reset gpio, ret);  
> > > 
> > > It still need to test if ret == 0, as otherwise it will do the wrong thing
> > > here.   
> > 
> > ret won't be zero here, that was checked above. You could check for just ret
> > though, it'd be easier to read that way.
> > 
> > > 
> > > 
> > > I guess the patch below does the trick and it is not ugly :-)
> > > 
> > >   
> > > > 	return ret;
> > > > }
> > > > 
> > > > I prefer this option as it's not dependent on the system firmware type.  
> > > 
> > > Thanks,
> > > Mauro
> > > 
> > > [PATCH] ov2640: print error if devm_*_optional*() fails
> > > 
> > > devm_gpiod_get_optional() can return -ENOSYS if GPIOLIB is
> > > disabled, causing probe to fail. Warn the user if this
> > > happens.
> > > 
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> > > 
> > > diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> > > index 4a2ae24f8722..8c87ed8306ea 100644
> > > --- a/drivers/media/i2c/ov2640.c
> > > +++ b/drivers/media/i2c/ov2640.c
> > > @@ -765,17 +765,17 @@ static int ov2640_s_register(struct v4l2_subdev *sd,
> > >  
> > >  static int ov2640_s_power(struct v4l2_subdev *sd, int on)
> > >  {
> > > +#ifdef CONFIG_GPIOLIB
> > >  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> > >  	struct ov2640_priv *priv = to_ov2640(client);
> > >  
> > > -#ifdef CONFIG_GPIOLIB
> > >  	if (priv->pwdn_gpio)
> > >  		gpiod_direction_output(priv->pwdn_gpio, !on);
> > >  	if (on && priv->resetb_gpio) {
> > >  		/* Active the resetb pin to perform a reset pulse */
> > >  		gpiod_direction_output(priv->resetb_gpio, 1);
> > >  		usleep_range(3000, 5000);
> > > -		gpiod_direction_output(priv->resetb_gpio, 0);
> > > +		gpiod_set_value(priv->resetb_gpio, 0);
> > >  	}
> > >  #endif
> > >  	return 0;
> > > @@ -1048,21 +1048,35 @@ static const struct v4l2_subdev_ops ov2640_subdev_ops = {
> > >  static int ov2640_probe_dt(struct i2c_client *client,
> > >  		struct ov2640_priv *priv)
> > >  {
> > > +	int ret;
> > > +
> > >  	/* Request the reset GPIO deasserted */
> > >  	priv->resetb_gpio = devm_gpiod_get_optional(&client->dev, "resetb",
> > >  			GPIOD_OUT_LOW);
> > > +
> > >  	if (!priv->resetb_gpio)
> > >  		dev_dbg(&client->dev, "resetb gpio is not assigned!\n");
> > > -	else if (IS_ERR(priv->resetb_gpio))
> > > -		return PTR_ERR(priv->resetb_gpio);
> > > +
> > > +	ret = PTR_ERR_OR_ZERO(priv->pwdn_gpio);  
> > 
> > s/pwdn/resetb/
> 
> True.
> 
> > 
> > This works, but you're essentially checking the same thing twice. ret is
> > only zero iff priv->resetb_gpio is NULL.
> 
> No. That's the implementation for PTR_ERR_OR_ZERO:
> 
> static inline int __must_check PTR_ERR_OR_ZERO(__force const void *ptr)
> {
> 	if (IS_ERR(ptr))
> 		return PTR_ERR(ptr);
> 	else
> 		return 0;
> }
> 
> ret is zero if either priv->resetb_gpio is NULL or if it is a valid
> pointer.

Right. Indeed.

> 
> That is about the same as what you proposed on this code snippet:

Yeah, either should be good. With the above addressed,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> > How about:
> > 
> > ret = PTR_ERR(priv->reset_gpio);
> > if (!priv->reset_gpio) {
> > 	dev_dbg("reset gpio is not assigned\n");
> > } else if (IS_ERR(priv->reset_gpio) && ret != -ENOSYS) {
> > 	dev_dbg("error %d while getting reset gpio", ret);
> > 	return ret;
> > }
> > 
> 
> 

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency

[Hans Verkuil]

Hi Pavel,

I'm dropping this from patchwork since this no longer applies now that ov2640
has been moved out of soc_camera.

If you still want this (it is a reasonable patch), then please respin.

Regards,

	Hans

On 04/21/2017 08:33 AM, Pavel Machek wrote:
> Hi!
> 
>>> Better solution would be for VIDEO_EM28XX_V4L2 to depend on GPIOLIB,
>>> too, no? If not, should there be BUG_ON(priv->pwdn_gpio);
>>> BUG_ON(priv->resetb_gpio);?
>>
>> Pavel,
>>
>> The em28xx driver was added upstream several years the gpio driver.
>> It controls GPIO using a different logic. It makes no sense to make
>> it dependent on GPIOLIB, except if someone converts it to use it.
> 
> At least comment in the sourcecode...? Remove pwdn_gpio fields from
> structure in !GPIOLIB case, because otherwise they are trap for the
> programmer trying to understand what is going on?
> 
> Plus, something like this, because otherwise it is quite confusing?
> 
> Thanks,
> 								Pavel
> 
> diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
> index 56de182..85620e1 100644
> --- a/drivers/media/i2c/soc_camera/ov2640.c
> +++ b/drivers/media/i2c/soc_camera/ov2640.c
> @@ -1060,7 +1060,7 @@ static int ov2640_hw_reset(struct device *dev)
>   		/* Active the resetb pin to perform a reset pulse */
>   		gpiod_direction_output(priv->resetb_gpio, 1);
>   		usleep_range(3000, 5000);
> -		gpiod_direction_output(priv->resetb_gpio, 0);
> +		gpiod_set_value(priv->resetb_gpio, 0);
>   	}
>   
>   	return 0;
> 

Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 456 bytes --]

Hi!

> I'm dropping this from patchwork since this no longer applies now that ov2640
> has been moved out of soc_camera.
> 
> If you still want this (it is a reasonable patch), then please
> respin.

If I'm not mistaken, equivalent fix is already in
drivers/media/i2c/ov2640.c .

Thanks,
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]