Hi Alexandre,

On 23 February 2016 at 16:10, Alexandre TORGUE
[off-list ref] wrote:

Put them in alphabetic order. Same goes for the KConfig entry.

 obj-$(CONFIG_DWMAC_GENERIC)    += dwmac-generic.o
 stmmac-platform-objs:= stmmac_platform.o

...

If you used 'struct stm32_dwmac *' instead of 'void *' you could skip
the local variable assignment.

Even better; you could pass 'struct plat_stmmacenet_data *' and use
it's 'interface' member to set the phy mode. Then you could drop the
interface member in your priv data struct and remove of_get_phy_mode()
in stm32_dwmac_parse_data().

+{
+       struct stm32_dwmac *dwmac = priv;
+       struct regmap *regmap = dwmac->regmap;
+       int ret, iface = dwmac->interface;
+       u32 reg = dwmac->mode_reg;
+       u32 val;
+
+       ret = clk_prepare_enable(dwmac->clk_tx);
+       if (ret)
+               goto out;
+
+       ret = clk_prepare_enable(dwmac->clk_rx);
+       if (ret)
+               goto out_disable_clk_tx;
+
+       val = (iface == PHY_INTERFACE_MODE_MII) ? 0 : 1;
+       ret = regmap_update_bits(regmap, reg, MII_PHY_SEL_MASK, val);
+       if (ret)
+               goto out_disable_clk_tx_rx;
+
+       return 0;
+
+out_disable_clk_tx_rx:
+       clk_disable_unprepare(dwmac->clk_rx);
+out_disable_clk_tx:
+       clk_disable_unprepare(dwmac->clk_tx);
+out:
+       return ret;
+}
+
+static void stm32_dwmac_exit(void *priv)
+{
+       struct stm32_dwmac *dwmac = priv;

Again; instead of 'void *' use 'struct stm32_dwmac *' to avoid the
local assignment.

+
+       clk_disable_unprepare(dwmac->clk_tx);
+       clk_disable_unprepare(dwmac->clk_rx);
+}

To be honest I really don't see the point in having a function with
just two other function calls in it. Consider dropping the function
altogether and place the clk_disable_unprepare() calls where it's
called from. If you still want to keep it, please put a more
descriptive name on it.

+static int stm32_dwmac_parse_data(struct stm32_dwmac *dwmac,
+                                 struct platform_device *pdev)

Since you are only interested in *dev and not *pdev you could pass a
'struct dev *' instead.

+{
+       struct device *dev = &pdev->dev;
+       struct device_node *np = dev->of_node;
+       struct regmap *regmap;
+       int err;
+
+       /*  Get TX/RX clocks */
+       dwmac->clk_tx = devm_clk_get(dev, "tx-clk");
+       if (IS_ERR(dwmac->clk_tx)) {
+               dev_warn(dev, "No tx clock provided...\n");
+               dwmac->clk_tx = NULL;
+       }
+       dwmac->clk_rx = devm_clk_get(dev, "rx-clk");
+       if (IS_ERR(dwmac->clk_rx)) {
+               dev_warn(dev, "No rx clock provided...\n");
+               dwmac->clk_rx = NULL;
+       }
+
+       /* Get mode register */
+       regmap = syscon_regmap_lookup_by_phandle(np, "st,syscon");
+       if (IS_ERR(regmap))
+               return PTR_ERR(regmap);
+
+       err = of_property_read_u32_index(np, "st,syscon", 1, &dwmac->mode_reg);
+       if (err) {
+               dev_err(dev, "Can't get sysconfig mode offset (%d)\n", err);
+               return err;
+       }
+
+       dwmac->interface = of_get_phy_mode(np);
+       dwmac->regmap = regmap;

Why the temporary local regmap variable?

Assigning dwmac->regmap with syscon_regmap_lookup_by_phandle() should
not exceed 80 chars if that is what you are worried about.

+       return 0;
+}
+
+static int stm32_dwmac_probe(struct platform_device *pdev)
+{
+       struct plat_stmmacenet_data *plat_dat;
+       struct stmmac_resources stmmac_res;
+       struct stm32_dwmac *dwmac;
+       int ret;
+
+       ret = stmmac_get_platform_resources(pdev, &stmmac_res);
+       if (ret)
+               return ret;
+
+       plat_dat = stmmac_probe_config_dt(pdev, &stmmac_res.mac);
+       if (IS_ERR(plat_dat))
+               return PTR_ERR(plat_dat);
+
+       dwmac = devm_kzalloc(&pdev->dev, sizeof(*dwmac), GFP_KERNEL);
+       if (!dwmac)
+               return -ENOMEM;
+
+       ret = stm32_dwmac_parse_data(dwmac, pdev);
+       if (ret) {
+               dev_err(&pdev->dev, "Unable to parse OF data\n");
+               return ret;
+       }
+
+       plat_dat->bsp_priv = dwmac;
+
+       ret = stm32_dwmac_init(plat_dat->bsp_priv);
+       if (ret)
+               return ret;
+
+       return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);

Note that stmmac_dvr_probe() can fail and if so you should disable
your tx/rx clks before you return.

Consider putting the clk_prepare_enable() directly here and use goto
labels for the clean up like most other drivers do in probe.

Also if you put regmap_update_bits() for phy mode above the
clk_prepare_enable() calls you remove one of the gotos.
I assume you don't need to enable tx/rx clock before you write to syscon.

+static int stm32_dwmac_remove(struct platform_device *pdev)
+{
+       struct net_device *ndev = platform_get_drvdata(pdev);
+       struct stmmac_priv *priv = netdev_priv(ndev);
+       int ret = stmmac_dvr_remove(ndev);
+
+       stm32_dwmac_exit(priv->plat->bsp_priv);
+
+       return ret;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int stm32_dwmac_suspend(struct device *dev)
+{
+       struct net_device *ndev = dev_get_drvdata(dev);
+       struct stmmac_priv *priv = netdev_priv(ndev);
+       int ret;
+
+       ret = stmmac_suspend(ndev);
+       stm32_dwmac_exit(priv->plat->bsp_priv);
+
+       return ret;
+}
+
+static int stm32_dwmac_resume(struct device *dev)
+{
+       struct net_device *ndev = dev_get_drvdata(dev);
+       struct stmmac_priv *priv = netdev_priv(ndev);
+       int ret;
+
+       ret = stm32_dwmac_init(priv->plat->bsp_priv);
+       if (ret)
+               goto out_regmap;
+
+       ret = stmmac_resume(ndev);
+
+out_regmap:
+       return ret;

Why the goto?

This could be written:
    ret = stm32_dwmac_init(priv->plat->bsp_priv);
    if (ret)
       return ret;

    return stmmac_resume(ndev);


regards,
Joachim Eastwood