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
>>>>> <[email protected]> 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
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 5a0f04c..4702876 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -2637,8 +2637,11 @@ static int pch_gbe_probe(struct pci_dev *pdev,
adapter->hw.back = adapter;
adapter->hw.reg = pcim_iomap_table(pdev)[PCH_GBE_PCI_BAR];
adapter->pdata = (struct pch_gbe_privdata *)pci_id->driver_data;
- if (adapter->pdata && adapter->pdata->platform_init)
- adapter->pdata->platform_init(pdev);
+ if (adapter->pdata && adapter->pdata->platform_init) {
+ ret = adapter->pdata->platform_init(pdev);
+ if (ret)
+ goto err_free_netdev;
+ }
adapter->ptp_pdev = pci_get_bus_and_slot(adapter->pdev->bus->number,
PCI_DEVFN(12, 4));
@@ -2740,9 +2743,9 @@ static int pch_gbe_minnow_platform_init(struct pci_dev *pdev)
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);
- return ret;
+ dev_warn(&pdev->dev,
+ "ERR: Can't request PHY reset GPIO line '%d'\n", gpio);
+ return ret == -ENODEV ? ret : 0;
}
gpio_set_value(gpio, 0);
