Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1340763 > unrolled thread
| Started by | Alexandre TORGUE <alexandre.torgue@gmail.com> |
|---|---|
| First post | 2016-02-23 16:20 +0100 |
| Last post | 2016-02-24 10:10 +0100 |
| Articles | 3 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH v2 1/4] net: ethernet: dwmac: add Ethernet glue logic for stm32 chip Alexandre TORGUE <alexandre.torgue@gmail.com> - 2016-02-23 16:20 +0100
Re: [PATCH v2 1/4] net: ethernet: dwmac: add Ethernet glue logic for stm32 chip Joachim Eastwood <manabian@gmail.com> - 2016-02-23 23:20 +0100
Re: [PATCH v2 1/4] net: ethernet: dwmac: add Ethernet glue logic for stm32 chip Alexandre Torgue <alexandre.torgue@gmail.com> - 2016-02-24 10:10 +0100
| From | Alexandre TORGUE <alexandre.torgue@gmail.com> |
|---|---|
| Date | 2016-02-23 16:20 +0100 |
| Subject | [PATCH v2 1/4] net: ethernet: dwmac: add Ethernet glue logic for stm32 chip |
| Message-ID | <r5mUO-7eb-33@gated-at.bofh.it> |
stm324xx family chips support Synopsys MAC 3.510 IP.
This patch adds settings for logical glue logic:
-clocks
-mode selection MII or RMII.
Signed-off-by: Alexandre TORGUE <alexandre.torgue@gmail.com>
diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index cec147d..f63bdcf 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -114,6 +114,18 @@ config DWMAC_SUNXI
This selects Allwinner SoC glue layer support for the
stmmac device driver. This driver is used for A20/A31
GMAC ethernet controller.
+
+config DWMAC_STM32
+ tristate "STM32 DWMAC support"
+ default ARCH_STM32
+ depends on OF && HAS_IOMEM
+ select MFD_SYSCON
+ ---help---
+ Support for ethernet controller on STM32 SOCs.
+
+ This selects STM32 SoC glue layer support for the stmmac
+ device driver. This driver is used on for the STM32 series
+ SOCs GMAC ethernet controller.
endif
config STMMAC_PCI
diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
index b390161..559086d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_DWMAC_ROCKCHIP) += dwmac-rk.o
obj-$(CONFIG_DWMAC_SOCFPGA) += dwmac-socfpga.o
obj-$(CONFIG_DWMAC_STI) += dwmac-sti.o
obj-$(CONFIG_DWMAC_SUNXI) += dwmac-sunxi.o
+obj-$(CONFIG_DWMAC_STM32) += dwmac-stm32.o
obj-$(CONFIG_DWMAC_GENERIC) += dwmac-generic.o
stmmac-platform-objs:= stmmac_platform.o
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
new file mode 100644
index 0000000..0b48ee7
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
@@ -0,0 +1,208 @@
+/*
+ * dwmac-stm32.c - DWMAC Specific Glue layer for STM32 MCU
+ *
+ * Copyright (C) Alexandre Torgue 2015
+ * Author: Alexandre Torgue <alexandre.torgue@gmail.com>
+ * License terms: GNU General Public License (GPL), version 2
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_net.h>
+#include <linux/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/stmmac.h>
+
+#include "stmmac_platform.h"
+
+#define MII_PHY_SEL_MASK BIT(23)
+
+struct stm32_dwmac {
+ int interface; /* MII interface */
+ struct clk *clk_tx;
+ struct clk *clk_rx;
+ u32 mode_reg; /* MAC glue-logic mode register */
+ struct regmap *regmap;
+ u32 speed;
+};
+
+static int stm32_dwmac_init(void *priv)
+{
+ 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;
+
+ clk_disable_unprepare(dwmac->clk_tx);
+ clk_disable_unprepare(dwmac->clk_rx);
+}
+
+static int stm32_dwmac_parse_data(struct stm32_dwmac *dwmac,
+ struct platform_device *pdev)
+{
+ 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;
+
+ 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);
+}
+
+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;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+SIMPLE_DEV_PM_OPS(stm32_dwmac_pm_ops, stm32_dwmac_suspend, stm32_dwmac_resume);
+
+static const struct of_device_id stm32_dwmac_match[] = {
+ { .compatible = "st,stm32-dwmac"},
+ { }
+};
+MODULE_DEVICE_TABLE(of, stm32_dwmac_match);
+
+static struct platform_driver stm32_dwmac_driver = {
+ .probe = stm32_dwmac_probe,
+ .remove = stm32_dwmac_remove,
+ .driver = {
+ .name = "stm32-dwmac",
+ .pm = &stm32_dwmac_pm_ops,
+ .of_match_table = stm32_dwmac_match,
+ },
+};
+module_platform_driver(stm32_dwmac_driver);
+
+MODULE_AUTHOR("Alexandre Torgue <alexandre.torgue@gmail.com>");
+MODULE_DESCRIPTION("STMicroelectronics MCU DWMAC Specific Glue layer");
+MODULE_LICENSE("GPL v2");
--
1.9.1
[toc] | [next] | [standalone]
| From | Joachim Eastwood <manabian@gmail.com> |
|---|---|
| Date | 2016-02-23 23:20 +0100 |
| Subject | Re: [PATCH v2 1/4] net: ethernet: dwmac: add Ethernet glue logic for stm32 chip |
| Message-ID | <r5ttf-3q9-7@gated-at.bofh.it> |
| In reply to | #1340763 |
Hi Alexandre,
On 23 February 2016 at 16:10, Alexandre TORGUE
<alexandre.torgue@gmail.com> wrote:
> stm324xx family chips support Synopsys MAC 3.510 IP.
> This patch adds settings for logical glue logic:
> -clocks
> -mode selection MII or RMII.
>
> Signed-off-by: Alexandre TORGUE <alexandre.torgue@gmail.com>
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> index cec147d..f63bdcf 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> @@ -114,6 +114,18 @@ config DWMAC_SUNXI
> This selects Allwinner SoC glue layer support for the
> stmmac device driver. This driver is used for A20/A31
> GMAC ethernet controller.
> +
> +config DWMAC_STM32
> + tristate "STM32 DWMAC support"
> + default ARCH_STM32
> + depends on OF && HAS_IOMEM
> + select MFD_SYSCON
> + ---help---
> + Support for ethernet controller on STM32 SOCs.
> +
> + This selects STM32 SoC glue layer support for the stmmac
> + device driver. This driver is used on for the STM32 series
> + SOCs GMAC ethernet controller.
> endif
>
> config STMMAC_PCI
> diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
> index b390161..559086d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
> +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_DWMAC_ROCKCHIP) += dwmac-rk.o
> obj-$(CONFIG_DWMAC_SOCFPGA) += dwmac-socfpga.o
> obj-$(CONFIG_DWMAC_STI) += dwmac-sti.o
> obj-$(CONFIG_DWMAC_SUNXI) += dwmac-sunxi.o
> +obj-$(CONFIG_DWMAC_STM32) += dwmac-stm32.o
Put them in alphabetic order. Same goes for the KConfig entry.
> obj-$(CONFIG_DWMAC_GENERIC) += dwmac-generic.o
> stmmac-platform-objs:= stmmac_platform.o
...
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> +struct stm32_dwmac {
> + int interface; /* MII interface */
> + struct clk *clk_tx;
> + struct clk *clk_rx;
> + u32 mode_reg; /* MAC glue-logic mode register */
> + struct regmap *regmap;
> + u32 speed;
> +};
> +
> +static int stm32_dwmac_init(void *priv)
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
[toc] | [prev] | [next] | [standalone]
| From | Alexandre Torgue <alexandre.torgue@gmail.com> |
|---|---|
| Date | 2016-02-24 10:10 +0100 |
| Subject | Re: [PATCH v2 1/4] net: ethernet: dwmac: add Ethernet glue logic for stm32 chip |
| Message-ID | <r5DCi-2ih-13@gated-at.bofh.it> |
| In reply to | #1341129 |
2016-02-23 23:16 GMT+01:00 Joachim Eastwood <manabian@gmail.com>:
> Hi Alexandre,
>
> On 23 February 2016 at 16:10, Alexandre TORGUE
> <alexandre.torgue@gmail.com> wrote:
>> stm324xx family chips support Synopsys MAC 3.510 IP.
>> This patch adds settings for logical glue logic:
>> -clocks
>> -mode selection MII or RMII.
>>
>> Signed-off-by: Alexandre TORGUE <alexandre.torgue@gmail.com>
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
>> index cec147d..f63bdcf 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
>> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
>> @@ -114,6 +114,18 @@ config DWMAC_SUNXI
>> This selects Allwinner SoC glue layer support for the
>> stmmac device driver. This driver is used for A20/A31
>> GMAC ethernet controller.
>> +
>> +config DWMAC_STM32
>> + tristate "STM32 DWMAC support"
>> + default ARCH_STM32
>> + depends on OF && HAS_IOMEM
>> + select MFD_SYSCON
>> + ---help---
>> + Support for ethernet controller on STM32 SOCs.
>> +
>> + This selects STM32 SoC glue layer support for the stmmac
>> + device driver. This driver is used on for the STM32 series
>> + SOCs GMAC ethernet controller.
>> endif
>>
>> config STMMAC_PCI
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
>> index b390161..559086d 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
>> +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
>> @@ -13,6 +13,7 @@ obj-$(CONFIG_DWMAC_ROCKCHIP) += dwmac-rk.o
>> obj-$(CONFIG_DWMAC_SOCFPGA) += dwmac-socfpga.o
>> obj-$(CONFIG_DWMAC_STI) += dwmac-sti.o
>> obj-$(CONFIG_DWMAC_SUNXI) += dwmac-sunxi.o
>> +obj-$(CONFIG_DWMAC_STM32) += dwmac-stm32.o
>
Hi Joachim,
> Put them in alphabetic order. Same goes for the KConfig entry.
Ok
>
>
>> obj-$(CONFIG_DWMAC_GENERIC) += dwmac-generic.o
>> stmmac-platform-objs:= stmmac_platform.o
> ...
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>> +struct stm32_dwmac {
>> + int interface; /* MII interface */
>> + struct clk *clk_tx;
>> + struct clk *clk_rx;
>> + u32 mode_reg; /* MAC glue-logic mode register */
>> + struct regmap *regmap;
>> + u32 speed;
>> +};
>> +
>> +static int stm32_dwmac_init(void *priv)
>
> 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().
Yes, interesting.
>
>
>> +{
>> + 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.
It was just to avoid redundant code, but yes there is not a big
interest to do 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.
ok
>
>> +{
>> + 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.
yes you are right.
>
>
>> + 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.
Ok catch.
>
> 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.
I will check.
>
>
>> +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?
Sorry no sens. I thought that it was better to avoid multiple return
but it this case it is stupid.
Best regards.
Alexandre
>
> This could be written:
> ret = stm32_dwmac_init(priv->plat->bsp_priv);
> if (ret)
> return ret;
>
> return stmmac_resume(ndev);
>
>
> regards,
> Joachim Eastwood
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web