On Mon, Nov 30, 2015 at 6:21 PM, Paul Burton <paul.burton@xxxxxxxxxx> wrote:
> Introduce support for retrieving the PHY reset GPIO from device tree,
> which will be used on the MIPS Boston development board. This requires
> support for probe deferral in order to work correctly, since the order
> of device probe is not guaranteed & typically the EG20T GPIO controller
> device will be probed after the ethernet MAC.
>
> Signed-off-by: Paul Burton <paul.burton@xxxxxxxxxx>
> ---
>
> .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 33 +++++++++++++++++++++-
> 1 file changed, 32 insertions(+), 1 deletion(-)
>
> 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 824ff9e..f2a9a38 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
> @@ -23,6 +23,8 @@
> #include <linux/net_tstamp.h>
> #include <linux/ptp_classify.h>
> #include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/of_gpio.h>
>
> #define DRV_VERSION "1.01"
> const char pch_driver_version[] = DRV_VERSION;
> @@ -2594,13 +2596,41 @@ static void pch_gbe_remove(struct pci_dev *pdev)
> free_netdev(netdev);
> }
>
> +static int pch_gbe_parse_dt(struct pci_dev *pdev,
> + struct pch_gbe_privdata **pdata)
Why not to return pdata as it done in many other drivers?
You have ERR_PTR() macro to pass errors.
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct gpio_desc *gpio;
> +
> + if (!config_enabled(CONFIG_OF) || !np)
Before I saw IS_ENABLED(). Is this one a preferable new API?
> + return 0;
> +
> + if (!*pdata)
> + *pdata = devm_kzalloc(&pdev->dev, sizeof(**pdata), GFP_KERNEL);
> + if (!*pdata)
> + return -ENOMEM;
> +
> + gpio = devm_gpiod_get(&pdev->dev, "phy-reset", GPIOD_ASIS);
> + if (IS_ERR(gpio))
> + return PTR_ERR(gpio);
> +
> + (*pdata)->phy_reset_gpio = gpio;
> + return 0;
> +}
> +
> static int pch_gbe_probe(struct pci_dev *pdev,
> const struct pci_device_id *pci_id)
> {
> struct net_device *netdev;
> struct pch_gbe_adapter *adapter;
> + struct pch_gbe_privdata *pdata;
> int ret;
>
> + pdata = (struct pch_gbe_privdata *)pci_id->driver_data;
> + ret = pch_gbe_parse_dt(pdev, &pdata);
So, I didn;t see anything related to dt in that function.
Maybe you just call it always? In that case remove check for np.
> + if (ret)
> + goto err_out;
> +
> ret = pcim_enable_device(pdev);
> if (ret)
> return ret;
> @@ -2638,7 +2668,7 @@ static int pch_gbe_probe(struct pci_dev *pdev,
> adapter->pdev = 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;
> + adapter->pdata = pdata;
> if (adapter->pdata && adapter->pdata->platform_init)
> adapter->pdata->platform_init(pdev, pdata);
>
> @@ -2729,6 +2759,7 @@ err_free_adapter:
> pch_gbe_hal_phy_hw_reset(&adapter->hw);
> err_free_netdev:
> free_netdev(netdev);
> +err_out:
Redundant.
> return ret;
> }
For now it's a common practice to mix styles in probe due to usage of
devres API.
--
With Best Regards,
Andy Shevchenko
