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

From: Pavel Machek <pavel@ucw.cz>
To: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Cc: "Linux Media Mailing List" <linux-media@vger.kernel.org>,
	"Mauro Carvalho Chehab" <mchehab@infradead.org>,
	"Hans Verkuil" <hans.verkuil@cisco.com>,
	"Sakari Ailus" <sakari.ailus@linux.intel.com>,
	"Pali Rohár" <pali.rohar@gmail.com>,
	"Ramiro Oliveira" <Ramiro.Oliveira@synopsys.com>,
	"Todor Tomov" <todor.tomov@linaro.org>,
	"Robert Jarzmik" <robert.jarzmik@free.fr>,
	"Steve Longerbeam" <slongerbeam@gmail.com>,
	"Guennadi Liakhovetski" <g.liakhovetski@gmx.de>,
	"Hugues Fruchet" <hugues.fruchet@st.com>,
	"Bhumika Goyal" <bhumirks@gmail.com>
Subject: Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency
Date: Wed, 19 Apr 2017 15:23:39 +0200	[thread overview]
Message-ID: <20170419132339.GA31747@amd> (raw)
In-Reply-To: <a463ea990d2138ca93027b006be96a0324b77fe4.1492602584.git.mchehab@s-opensource.com>

[-- 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 --]