Netdev List

* Re: [gpio:for-next 67/67] pch_gbe_main.c:undefined reference to `devm_gpio_request_one'
From: Darren Hart @ 2013-10-28  0:04 UTC (permalink / raw)
  To: David Cohen
  Cc: Linus Walleij, David S. Miller, netdev@vger.kernel.org,
	Fengguang Wu, Alexandre Courbot, linux-gpio@vger.kernel.org
In-Reply-To: <526CA546.7040406@linux.intel.com>

On Sat, 2013-10-26 at 22:31 -0700, David Cohen wrote:
> On 10/26/2013 02:15 PM, Darren Hart wrote:
> > On Sat, 2013-10-26 at 21:33 +0100, Darren Hart wrote:
> >> On Fri, 2013-10-25 at 14:25 -0700, David Cohen wrote:
> >>> On 10/25/2013 02:21 PM, David Cohen wrote:
> >>>> Hi Linus,
> >>>>
> >>>> On 10/25/2013 03:49 AM, Linus Walleij wrote:
> >>>>> On Fri, Oct 25, 2013 at 12:41 PM, Linus Walleij
> >>>>> <linus.walleij@linaro.org> wrote:
> >>>>>
> >>>>>>> I wouldn't object to adding a dependency to GPIO_PCH and GPIOLIB
> >>>>>>> unconditionally for PCH_GBE as GPIO_PCH is the same chip... but I don't
> >>>>>>> know if David Miller would be amenable to that.
> >>>>>>
> >>>>>> Well we should probably just stick a dependency to GPIOLIB in there.
> >>>>>>
> >>>>>> - It #includes <linux/gpio.h>
> >>>>>> - It uses gpiolib functions to do something vital
> >>>>>>
> >>>>>> It was just happy that dummy versions were slotted in until now.
> >>>>>
> >>>>> ...or maybe I'm just confused now?
> >>>>>
> >>>>> Should we just add a static inline stub of devm_gpio_request_one()?
> >>>>
> >>>> I am not familiar with the HW. But checking the code, platform
> >>>> initialization should fail with a dummy devm_gpio_request_one()
> >>>> implementation. IMO it makes more sense to depend on GPIOLIB.
> >>>
> >>> Actually, forget about it. Despite driver_data->platform_init() may
> >>> return error, probe() never checks for it. I think the driver must be
> >>> fixed, but in meanwhile a static inline stub seems reasonable.
> >>>
> >>
> >> Ah, that's a problem, and one I created :/ I'm traveling a bit through
> >> Europe atm for the conferences. I will try and have a look on the planes
> >> and add a check.
> > 
> > Ah, now I remember. The situation is this. If there is a cable plugged
> > into the jack, the PHY will not go to sleep. If there isn't, there is a
> > good chance it will go to sleep before the driver initializes. If it is
> > asleep, the scan for the PHY will fail. If it isn't, the scan will work
> > correctly and the device will be properly setup. The code currently
> > displays a dev error:
> > 
> > 	ret = devm_gpio_request_one(&pdev->dev, gpio, flags,
> > 				    "minnow_phy_reset");
> > 	if (ret) {
> > 		dev_err(&pdev->dev,
> > 			"ERR: Can't request PHY reset GPIO line '%d'\n", gpio);
> > 
> > But deliberately does not about the probe because there is a chance
> > things will work just fine. If they do not, the PHY detection code will
> > print display errors about a failure to communicate over RGMII, and the
> > device probe will fail from there.
> > 
> > This still seems like the correct approach to me. Does anyone disagree?
> 
> Considering this scenario it seems to be correct. But if
> devm_gpio_request_one() may fail for "unfriendly" reasons too, then
> it's incomplete.
> 
> > 
> > (we still need to sort out the GPIOLIB and GPIO_SCH dependency though of
> > course)
> 
> Maybe if GPIOLIB has the static inline stubs returning -ENODEV we could
> use a patch similar to the one attached here.
> 

I think you may have inverted your logic on the return?

+		dev_warn(&pdev->dev,
+			 "ERR: Can't request PHY reset GPIO line '%d'\n", gpio);
+		return ret == -ENODEV ? ret : 0;

Did you mean:

+		/* 
		 * Things may still work if the GPIO driver wasn't
		 * compiled in
		 */
+		return ret == -ENODEV ? 0 : ret;

The concern here of course would be someone added another GPIO
controller over i2c over the expansion connector or something similar
and did not enable the GPIO_SCH driver, then it could conceivably grab
the wrong GPIO pin.... or would those never map to GPIO 13?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

^ permalink raw reply