On Sat, 2013-10-26 at 22:31 -0700, David Cohen wrote:

On 10/26/2013 02:15 PM, Darren Hart wrote:

quoted

On Sat, 2013-10-26 at 21:33 +0100, Darren Hart wrote:

quoted

On Fri, 2013-10-25 at 14:25 -0700, David Cohen wrote:

quoted

On 10/25/2013 02:21 PM, David Cohen wrote:

quoted

Hi Linus,

On 10/25/2013 03:49 AM, Linus Walleij wrote:

quoted

On Fri, Oct 25, 2013 at 12:41 PM, Linus Walleij
[off-list ref] wrote:

quoted

quoted

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.

quoted

(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