From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758835AbcCVOyK (ORCPT ); Tue, 22 Mar 2016 10:54:10 -0400 Received: from smtpoutz27.laposte.net ([194.117.213.102]:56134 "EHLO smtp.laposte.net" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752103AbcCVOx5 (ORCPT ); Tue, 22 Mar 2016 10:53:57 -0400 Message-ID: <56F15C7F.9060404@laposte.net> Date: Tue, 22 Mar 2016 15:53:51 +0100 From: Sebastian Frias User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: =?windows-1252?Q?Uwe_Kleine-K=F6nig?= , Sergei Shtylyov CC: "David S. Miller" , netdev@vger.kernel.org, lkml , mason , Daniel Mack Subject: Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB References: <56E99727.9040702@laposte.net> <56F05651.3030706@cogentembedded.com> <20160321204111.GB6191@pengutronix.de> In-Reply-To: <20160321204111.GB6191@pengutronix.de> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-VR-SrcIP: 83.142.147.193 X-VR-FullState: 0 X-VR-Score: -100 X-VR-Cause-1: gggruggvucftvghtrhhoucdtuddrfeekkedrudeggdeilecutefuodetggdotefrodftvfcurfhrohhf X-VR-Cause-2: ihhlvgemucfntefrqffuvffgnecuuegrihhlohhuthemucehtddtnecusecvtfgvtghiphhivghnthhs X-VR-Cause-3: ucdlqddutddtmdenucfjughrpefkfffhfgggvffufhgjtgfgsehtkegrtddtfeehnecuhfhrohhmpefu X-VR-Cause-4: vggsrghsthhirghnucfhrhhirghsuceoshhfkeegsehlrghpohhsthgvrdhnvghtqeenucfkphepkeef X-VR-Cause-5: rddugedvrddugeejrdduleefnecurfgrrhgrmhepmhhouggvpehsmhhtphhouhhtpdhhvghloheplgdu X-VR-Cause-6: jedvrddvjedrtddrvddugegnpdhinhgvthepkeefrddugedvrddugeejrdduleefpdhmrghilhhfrhho X-VR-Cause-7: mhepshhfkeegsehlrghpohhsthgvrdhnvghtpdhrtghpthhtohepuhdrkhhlvghinhgvqdhkohgvnhhi X-VR-Cause-8: ghesphgvnhhguhhtrhhonhhigidruggv X-VR-AvState: No X-VR-State: 0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 03/21/2016 09:41 PM, Uwe Kleine-König wrote: >> My patch basically gets rid of all this code. The thing that worries me >> is that the driver assumes that the reset singal is active low, despite what >> the GPIO specifier in the device tree has for the GPIO polarity. In fact, it >> will only work correctly if the specified has GPIO_ACTIVE_HIGH -- which is >> wrong because the reset signal is active low! > > Note that gpio descriptors handle the polarity just fine (i.e. the pin > is set to 0 after doing gpiod_set_value(1) if the gpio is active low). > Isn't that source of bugs? What about using some #define (or probably better, an enum)?, something like: gpiod_set_value(gpiod, GPIO_SET_VALUE_ACTIVE) gpiod_set_value(gpiod, GPIO_SET_VALUE_INACTIVE) gpiod_set_value(gpiod, GPIO_SET_VALUE_TRISTATE) then, somebody reading the code would have to stop and think what do these mean. IIUC, currently the "0" or "1" can easily be confused with the actual logical value of the GPIO. gpiod_set_value() could also return an int with the actual value it applied to the GPIO. For example: if gpiod is active low, gpiod_set_value(gpiod, GPIO_SET_VALUE_ACTIVE) would return 0; Conversely, if gpiod is active high, gpiod_set_value(gpiod, GPIO_SET_VALUE_ACTIVE) would return 1; Best regards, Sebastian > But having said that, the driver gets it wrong. > > The right sequence to reset a device using a gpio is: > > gpiod_set_value(priv->gpiod_reset, 1); > msleep(some_time); > gpiod_set_value(priv->gpiod_reset, 0); > > and if the gpio is active low, this should be specified in the device > tree. This was done wrong in 13a56b449325 (net: phy: at803x: Add support > for hardware reset). > > Best regards > Uwe >