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.

Br, David