* [PATCH] [media] ov2640: make GPIOLIB an optional dependency @ 2017-04-19 11:49 Mauro Carvalho Chehab 2017-04-19 13:23 ` Pavel Machek 0 siblings, 1 reply; 13+ messages in thread From: Mauro Carvalho Chehab @ 2017-04-19 11:49 UTC (permalink / raw) Cc: Mauro Carvalho Chehab, Linux Media Mailing List, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Pavel Machek, Pali Rohár, Ramiro Oliveira, Todor Tomov, Robert Jarzmik, Steve Longerbeam, Guennadi Liakhovetski, Hugues Fruchet, Bhumika Goyal 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency 2017-04-19 11:49 [PATCH] [media] ov2640: make GPIOLIB an optional dependency Mauro Carvalho Chehab @ 2017-04-19 13:23 ` Pavel Machek 2017-04-19 14:03 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 13+ messages in thread From: Pavel Machek @ 2017-04-19 13:23 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Pali Rohár, Ramiro Oliveira, Todor Tomov, Robert Jarzmik, Steve Longerbeam, Guennadi Liakhovetski, Hugues Fruchet, Bhumika Goyal [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency 2017-04-19 13:23 ` Pavel Machek @ 2017-04-19 14:03 ` Mauro Carvalho Chehab 2017-04-21 6:33 ` Pavel Machek 0 siblings, 1 reply; 13+ messages in thread From: Mauro Carvalho Chehab @ 2017-04-19 14:03 UTC (permalink / raw) To: Pavel Machek Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Pali Rohár, Ramiro Oliveira, Todor Tomov, Robert Jarzmik, Steve Longerbeam, Guennadi Liakhovetski, Hugues Fruchet, Bhumika Goyal 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency 2017-04-19 14:03 ` Mauro Carvalho Chehab @ 2017-04-21 6:33 ` Pavel Machek 2017-04-21 14:39 ` Mauro Carvalho Chehab 2017-06-19 9:50 ` Hans Verkuil 0 siblings, 2 replies; 13+ messages in thread From: Pavel Machek @ 2017-04-21 6:33 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Pali Rohár, Ramiro Oliveira, Todor Tomov, Robert Jarzmik, Steve Longerbeam, Guennadi Liakhovetski, Hugues Fruchet, Bhumika Goyal [-- 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 --] ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency 2017-04-21 6:33 ` Pavel Machek @ 2017-04-21 14:39 ` Mauro Carvalho Chehab 2017-04-24 14:44 ` Sakari Ailus 2017-06-19 9:50 ` Hans Verkuil 1 sibling, 1 reply; 13+ messages in thread From: Mauro Carvalho Chehab @ 2017-04-21 14:39 UTC (permalink / raw) To: Pavel Machek Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Pali Rohár, Ramiro Oliveira, Todor Tomov, Robert Jarzmik, Steve Longerbeam, Guennadi Liakhovetski, Hugues Fruchet, Bhumika Goyal 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency 2017-04-21 14:39 ` Mauro Carvalho Chehab @ 2017-04-24 14:44 ` Sakari Ailus 2017-04-24 15:50 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 13+ messages in thread From: Sakari Ailus @ 2017-04-24 14:44 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Pavel Machek, Linux Media Mailing List, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Pali Rohár, Ramiro Oliveira, Todor Tomov, Robert Jarzmik, Steve Longerbeam, Guennadi Liakhovetski, Hugues Fruchet, Bhumika Goyal 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency 2017-04-24 14:44 ` Sakari Ailus @ 2017-04-24 15:50 ` Mauro Carvalho Chehab 2017-04-24 17:38 ` Sakari Ailus 0 siblings, 1 reply; 13+ messages in thread From: Mauro Carvalho Chehab @ 2017-04-24 15:50 UTC (permalink / raw) To: Sakari Ailus Cc: Pavel Machek, Linux Media Mailing List, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Pali Rohár, Ramiro Oliveira, Todor Tomov, Robert Jarzmik, Steve Longerbeam, Guennadi Liakhovetski, Hugues Fruchet, Bhumika Goyal 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; } ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency 2017-04-24 15:50 ` Mauro Carvalho Chehab @ 2017-04-24 17:38 ` Sakari Ailus 2017-04-24 17:50 ` Sakari Ailus 2017-04-25 2:05 ` Mauro Carvalho Chehab 0 siblings, 2 replies; 13+ messages in thread From: Sakari Ailus @ 2017-04-24 17:38 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Pavel Machek, Linux Media Mailing List, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Pali Rohár, Ramiro Oliveira, Todor Tomov, Robert Jarzmik, Steve Longerbeam, Guennadi Liakhovetski, Hugues Fruchet, Bhumika Goyal 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency 2017-04-24 17:38 ` Sakari Ailus @ 2017-04-24 17:50 ` Sakari Ailus 2017-04-25 2:05 ` Mauro Carvalho Chehab 1 sibling, 0 replies; 13+ messages in thread From: Sakari Ailus @ 2017-04-24 17:50 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Pavel Machek, Linux Media Mailing List, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Pali Rohár, Ramiro Oliveira, Todor Tomov, Robert Jarzmik, Steve Longerbeam, Guennadi Liakhovetski, Hugues Fruchet, Bhumika Goyal 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency 2017-04-24 17:38 ` Sakari Ailus 2017-04-24 17:50 ` Sakari Ailus @ 2017-04-25 2:05 ` Mauro Carvalho Chehab 2017-04-25 8:57 ` Sakari Ailus 1 sibling, 1 reply; 13+ messages in thread From: Mauro Carvalho Chehab @ 2017-04-25 2:05 UTC (permalink / raw) To: Sakari Ailus Cc: Pavel Machek, Linux Media Mailing List, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Pali Rohár, Ramiro Oliveira, Todor Tomov, Robert Jarzmik, Steve Longerbeam, Guennadi Liakhovetski, Hugues Fruchet, Bhumika Goyal 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency 2017-04-25 2:05 ` Mauro Carvalho Chehab @ 2017-04-25 8:57 ` Sakari Ailus 0 siblings, 0 replies; 13+ messages in thread From: Sakari Ailus @ 2017-04-25 8:57 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Pavel Machek, Linux Media Mailing List, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Pali Rohár, Ramiro Oliveira, Todor Tomov, Robert Jarzmik, Steve Longerbeam, Guennadi Liakhovetski, Hugues Fruchet, Bhumika Goyal 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency 2017-04-21 6:33 ` Pavel Machek 2017-04-21 14:39 ` Mauro Carvalho Chehab @ 2017-06-19 9:50 ` Hans Verkuil 2017-06-19 10:48 ` Pavel Machek 1 sibling, 1 reply; 13+ messages in thread From: Hans Verkuil @ 2017-06-19 9:50 UTC (permalink / raw) To: Pavel Machek, Mauro Carvalho Chehab Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Pali Rohár, Ramiro Oliveira, Todor Tomov, Robert Jarzmik, Steve Longerbeam, Guennadi Liakhovetski, Hugues Fruchet, Bhumika Goyal 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; > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [media] ov2640: make GPIOLIB an optional dependency 2017-06-19 9:50 ` Hans Verkuil @ 2017-06-19 10:48 ` Pavel Machek 0 siblings, 0 replies; 13+ messages in thread From: Pavel Machek @ 2017-06-19 10:48 UTC (permalink / raw) To: Hans Verkuil Cc: Mauro Carvalho Chehab, Linux Media Mailing List, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, Pali Rohár, Ramiro Oliveira, Todor Tomov, Robert Jarzmik, Steve Longerbeam, Guennadi Liakhovetski, Hugues Fruchet, Bhumika Goyal [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-06-19 10:48 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-19 11:49 [PATCH] [media] ov2640: make GPIOLIB an optional dependency Mauro Carvalho Chehab 2017-04-19 13:23 ` Pavel Machek 2017-04-19 14:03 ` Mauro Carvalho Chehab 2017-04-21 6:33 ` Pavel Machek 2017-04-21 14:39 ` Mauro Carvalho Chehab 2017-04-24 14:44 ` Sakari Ailus 2017-04-24 15:50 ` Mauro Carvalho Chehab 2017-04-24 17:38 ` Sakari Ailus 2017-04-24 17:50 ` Sakari Ailus 2017-04-25 2:05 ` Mauro Carvalho Chehab 2017-04-25 8:57 ` Sakari Ailus 2017-06-19 9:50 ` Hans Verkuil 2017-06-19 10:48 ` Pavel Machek
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.