Message ID | 20170421063312.GA21434@amd (mailing list archive) |
---|---|
State | Obsoleted, archived |
Delegated to: | Guennadi Liakhovetski |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from <linux-media-owner@vger.kernel.org>) id 1d1S87-0003Ip-SM; Fri, 21 Apr 2017 06:33:19 +0000 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.84_2/mailfrontend-6) with esmtp id 1d1S85-0006ky-5f; Fri, 21 Apr 2017 08:33:19 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1035825AbdDUGdQ (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Fri, 21 Apr 2017 02:33:16 -0400 Received: from atrey.karlin.mff.cuni.cz ([195.113.26.193]:38326 "EHLO atrey.karlin.mff.cuni.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1035826AbdDUGdP (ORCPT <rfc822;linux-media@vger.kernel.org>); Fri, 21 Apr 2017 02:33:15 -0400 Received: by atrey.karlin.mff.cuni.cz (Postfix, from userid 512) id 8873481DCA; Fri, 21 Apr 2017 08:33:12 +0200 (CEST) Date: Fri, 21 Apr 2017 08:33:12 +0200 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 =?iso-8859-1?Q?Roh=E1r?= <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 Message-ID: <20170421063312.GA21434@amd> References: <a463ea990d2138ca93027b006be96a0324b77fe4.1492602584.git.mchehab@s-opensource.com> <20170419132339.GA31747@amd> <20170419110300.2dbbf784@vento.lan> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="0OAP2g/MAC+5xKAE" Content-Disposition: inline In-Reply-To: <20170419110300.2dbbf784@vento.lan> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2017.4.21.62716 X-PMX-Spam: Gauge=IIIIIIIII, Probability=9%, Report=' BODY_PARA_IS_SENTENCE_URL 0.1, MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, KNOWN_FREEWEB_URI 0.05, MSGID_ADDED_BY_MTA 0.05, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_1800_1899 0, BODY_SIZE_2000_LESS 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, INVALID_MSGID_NO_FQDN 0, IN_REP_TO 0, LEGITIMATE_SIGNS 0, MSG_THREAD 0, MULTIPLE_REAL_RCPTS 0, NO_URI_HTTPS 0, REFERENCES 0, URI_ENDS_IN_HTML 0, URI_WITH_PATH_ONLY 0, __ANY_URI 0, __ATTACHMENT_SIZE_0_10K 0, __BOUNCE_CHALLENGE_SUBJ 0, __BOUNCE_NDR_SUBJ_EXEMPT 0, __CC_NAME 0, __CC_NAME_DIFF_FROM_ACC 0, __CC_REAL_NAMES 0, __CD 0, __CP_URI_IN_BODY 0, __CT 0, __CTYPE_HAS_BOUNDARY 0, __CTYPE_MULTIPART 0, __FORWARDED_MSG 0, __HAS_ATTACHMENT 0, __HAS_ATTACHMENT1 0, __HAS_ATTACHMENT2 0, __HAS_CC_HDR 0, __HAS_FROM 0, __HAS_LIST_ID 0, __HAS_MSGID 0, __HAS_X_MAILING_LIST 0, __IN_REP_TO 0, __KNOWN_FREEWEB_URI2 0, __MIME_TEXT_P 0, __MIME_TEXT_P1 0, __MIME_TEXT_P2 0, __MIME_VERSION 0, __MULTIPLE_RCPTS_CC_X2 0, __MULTIPLE_URI_TEXT 0, __NO_HTML_TAG_RAW 0, __REFERENCES 0, __SANE_MSGID 0, __SUBJ_ALPHA_END 0, __SUBJ_ALPHA_NEGATE 0, __TO_MALFORMED_2 0, __TO_NAME 0, __TO_NAME_DIFF_FROM_ACC 0, __TO_REAL_NAMES 0, __URI_IN_BODY 0, __URI_NOT_IMG 0, __URI_NO_MAILTO 0, __URI_NS , __URI_WITH_PATH 0, __USER_AGENT 0' |
Commit Message
Pavel Machek
April 21, 2017, 6:33 a.m. UTC
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
Comments
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; >
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
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;