[net-next v3 0/9] net: convert to devm_clk_get_enabled() and devm_clk_get_optional_enabled()

[net-next v3 0/9] net: convert to devm_clk_get_enabled() and devm_clk_get_optional_enabled()

[Yangtao Li]

This patchset simplify code w/ devm_clk_get*_enabled() API.

v3:
-convert to ERR_CAST
-move clock variables to keep lines longest to shortest
-Reduce the number of clk variables

v2:
-remove unused 'ret'
-fix incompatible-pointer-types
-get rid of amount of variables used
*** SUBJECT HERE ***

Yangtao Li (9):
  net: stmmac: dwmac-intel-plat: Convert to devm_clk_get_enabled()
  net: stmmac: platform: Convert to devm_clk_get_enabled() and
    devm_clk_get_optional_enabled()
  net: ethernet: cortina: Convert to devm_clk_get_enabled()
  net: mdio: hisi-femac: Convert to devm_clk_get_enabled()
  net: dsa: rzn1_a5psw: Convert to devm_clk_get_enabled()
  net: ethernet: broadcom: bcm63xx_enet: Convert to
    devm_clk_get_enabled()
  net: ethernet: marvell: mvneta: Convert to devm_clk_get_enabled()
  net: mvpp2: Convert to devm_clk_get_enabled() and
    devm_clk_get_optional_enabled()
  net: marvell: pxa168_eth: Convert to devm_clk_get_enabled()

 drivers/net/dsa/rzn1_a5psw.c                  | 22 +----
 drivers/net/ethernet/broadcom/bcm63xx_enet.c  | 47 +++-------
 drivers/net/ethernet/broadcom/bcm63xx_enet.h  |  6 --
 drivers/net/ethernet/cortina/gemini.c         | 25 ++----
 drivers/net/ethernet/marvell/mvneta_bm.c      | 16 ++--
 drivers/net/ethernet/marvell/mvneta_bm.h      |  1 -
 drivers/net/ethernet/marvell/mvpp2/mvpp2.h    |  7 --
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   | 89 +++++--------------
 drivers/net/ethernet/marvell/pxa168_eth.c     | 17 +---
 .../stmicro/stmmac/dwmac-intel-plat.c         | 11 +--
 .../ethernet/stmicro/stmmac/stmmac_platform.c | 35 ++------
 drivers/net/mdio/mdio-hisi-femac.c            | 18 ++--
 12 files changed, 73 insertions(+), 221 deletions(-)

-- 
2.39.0

[net-next v3 1/9] net: stmmac: dwmac-intel-plat: Convert to devm_clk_get_enabled()

[Yangtao Li]

Convert devm_clk_get(), clk_prepare_enable() to a single
call to devm_clk_get_enabled(), as this is exactly
what this function does.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-intel-plat.c    | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
index d68f0c4e7835..dcbae653ab8c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
@@ -104,12 +104,10 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
 
 		/* Enable TX clock */
 		if (dwmac->data->tx_clk_en) {
-			dwmac->tx_clk = devm_clk_get(&pdev->dev, "tx_clk");
+			dwmac->tx_clk = devm_clk_get_enabled(&pdev->dev, "tx_clk");
 			if (IS_ERR(dwmac->tx_clk))
 				return PTR_ERR(dwmac->tx_clk);
 
-			clk_prepare_enable(dwmac->tx_clk);
-
 			/* Check and configure TX clock rate */
 			rate = clk_get_rate(dwmac->tx_clk);
 			if (dwmac->data->tx_clk_rate &&
@@ -149,20 +147,15 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
 	}
 
 	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
-	if (ret) {
-		clk_disable_unprepare(dwmac->tx_clk);
+	if (ret)
 		return ret;
-	}
 
 	return 0;
 }
 
 static void intel_eth_plat_remove(struct platform_device *pdev)
 {
-	struct intel_dwmac *dwmac = get_stmmac_bsp_priv(&pdev->dev);
-
 	stmmac_pltfr_remove(pdev);
-	clk_disable_unprepare(dwmac->tx_clk);
 }
 
 static struct platform_driver intel_eth_plat_driver = {
-- 
2.39.0

Re: [net-next v3 1/9] net: stmmac: dwmac-intel-plat: Convert to devm_clk_get_enabled()

[Russell King (Oracle)]

On Tue, Aug 27, 2024 at 03:57:04AM -0600, Yangtao Li wrote:
>  	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
> -	if (ret) {
> -		clk_disable_unprepare(dwmac->tx_clk);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	return 0;

Please head off the next "cleanup" patch that someone has to review,
which will be to convert this to:

	return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);

When doing cleanups, don't _create_ new opportunities for cleanups.
Always try to write the best replacement code. This reduces the
burden on reviewers - and we need the burden on reviewers to be
minimised because there's relatively few of them compared to the
number of people generating patches.

-- 
*** please note that I probably will only be occasionally responsive
*** for an unknown period of time due to recent eye surgery making
*** reading quite difficult.

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [net-next v3 1/9] net: stmmac: dwmac-intel-plat: Convert to devm_clk_get_enabled()

[Simon Horman]

On Tue, Aug 27, 2024 at 03:57:04AM -0600, Yangtao Li wrote:
> Convert devm_clk_get(), clk_prepare_enable() to a single
> call to devm_clk_get_enabled(), as this is exactly
> what this function does.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-intel-plat.c    | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> index d68f0c4e7835..dcbae653ab8c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> @@ -104,12 +104,10 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
>  
>  		/* Enable TX clock */
>  		if (dwmac->data->tx_clk_en) {
> -			dwmac->tx_clk = devm_clk_get(&pdev->dev, "tx_clk");
> +			dwmac->tx_clk = devm_clk_get_enabled(&pdev->dev, "tx_clk");

As it looks like there will be a v4 anyway, a minor nit from my side:
IMHO, the line above could be trivially wrapped to keep it <= 80 columns wide,
which is still preferred by Networking code.

>  			if (IS_ERR(dwmac->tx_clk))
>  				return PTR_ERR(dwmac->tx_clk);
>  
> -			clk_prepare_enable(dwmac->tx_clk);
> -
>  			/* Check and configure TX clock rate */
>  			rate = clk_get_rate(dwmac->tx_clk);
>  			if (dwmac->data->tx_clk_rate &&

...

[net-next v3 2/9] net: stmmac: platform: Convert to devm_clk_get_enabled() and devm_clk_get_optional_enabled()

[Yangtao Li]

Use devm_clk_get_enabled() and devm_clk_get_optional_enabled()
to simplify code.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Suggested-by: Serge Semin <fancer.lancer@gmail.com>
---
v2:
-remove unused 'ret'
-fix incompatible-pointer-types

 .../ethernet/stmicro/stmmac/stmmac_platform.c | 35 +++++--------------
 1 file changed, 8 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index ad868e8d195d..4365afabf3c4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -415,8 +415,6 @@ static int stmmac_of_get_mac_mode(struct device_node *np)
 static void stmmac_remove_config_dt(struct platform_device *pdev,
 				    struct plat_stmmacenet_data *plat)
 {
-	clk_disable_unprepare(plat->stmmac_clk);
-	clk_disable_unprepare(plat->pclk);
 	of_node_put(plat->phy_node);
 	of_node_put(plat->mdio_node);
 }
@@ -436,7 +434,6 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
 	struct plat_stmmacenet_data *plat;
 	struct stmmac_dma_cfg *dma_cfg;
 	int phy_mode;
-	void *ret;
 	int rc;
 
 	plat = devm_kzalloc(&pdev->dev, sizeof(*plat), GFP_KERNEL);
@@ -615,21 +612,16 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
 
 	/* clock setup */
 	if (!of_device_is_compatible(np, "snps,dwc-qos-ethernet-4.10")) {
-		plat->stmmac_clk = devm_clk_get(&pdev->dev,
-						STMMAC_RESOURCE_NAME);
+		plat->stmmac_clk = devm_clk_get_enabled(&pdev->dev, STMMAC_RESOURCE_NAME);
 		if (IS_ERR(plat->stmmac_clk)) {
 			dev_warn(&pdev->dev, "Cannot get CSR clock\n");
 			plat->stmmac_clk = NULL;
 		}
-		clk_prepare_enable(plat->stmmac_clk);
 	}
 
-	plat->pclk = devm_clk_get_optional(&pdev->dev, "pclk");
-	if (IS_ERR(plat->pclk)) {
-		ret = plat->pclk;
-		goto error_pclk_get;
-	}
-	clk_prepare_enable(plat->pclk);
+	plat->pclk = devm_clk_get_optional_enabled(&pdev->dev, "pclk");
+	if (IS_ERR(plat->pclk))
+		return ERR_CAST(plat->pclk);
 
 	/* Fall-back to main clock in case of no PTP ref is passed */
 	plat->clk_ptp_ref = devm_clk_get(&pdev->dev, "ptp_ref");
@@ -644,26 +636,15 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
 
 	plat->stmmac_rst = devm_reset_control_get_optional(&pdev->dev,
 							   STMMAC_RESOURCE_NAME);
-	if (IS_ERR(plat->stmmac_rst)) {
-		ret = plat->stmmac_rst;
-		goto error_hw_init;
-	}
+	if (IS_ERR(plat->stmmac_rst))
+		return ERR_CAST(plat->stmmac_rst);
 
 	plat->stmmac_ahb_rst = devm_reset_control_get_optional_shared(
 							&pdev->dev, "ahb");
-	if (IS_ERR(plat->stmmac_ahb_rst)) {
-		ret = plat->stmmac_ahb_rst;
-		goto error_hw_init;
-	}
+	if (IS_ERR(plat->stmmac_ahb_rst))
+		return ERR_CAST(plat->stmmac_ahb_rst);
 
 	return plat;
-
-error_hw_init:
-	clk_disable_unprepare(plat->pclk);
-error_pclk_get:
-	clk_disable_unprepare(plat->stmmac_clk);
-
-	return ret;
 }
 
 static void devm_stmmac_remove_config_dt(void *data)
-- 
2.39.0

Re: [net-next v3 2/9] net: stmmac: platform: Convert to devm_clk_get_enabled() and devm_clk_get_optional_enabled()

[Simon Horman]

On Tue, Aug 27, 2024 at 03:57:05AM -0600, Yangtao Li wrote:
> Use devm_clk_get_enabled() and devm_clk_get_optional_enabled()
> to simplify code.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Suggested-by: Serge Semin <fancer.lancer@gmail.com>
> ---
> v2:
> -remove unused 'ret'
> -fix incompatible-pointer-types
> 
>  .../ethernet/stmicro/stmmac/stmmac_platform.c | 35 +++++--------------
>  1 file changed, 8 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index ad868e8d195d..4365afabf3c4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -415,8 +415,6 @@ static int stmmac_of_get_mac_mode(struct device_node *np)
>  static void stmmac_remove_config_dt(struct platform_device *pdev,
>  				    struct plat_stmmacenet_data *plat)
>  {
> -	clk_disable_unprepare(plat->stmmac_clk);
> -	clk_disable_unprepare(plat->pclk);
>  	of_node_put(plat->phy_node);
>  	of_node_put(plat->mdio_node);
>  }
> @@ -436,7 +434,6 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
>  	struct plat_stmmacenet_data *plat;
>  	struct stmmac_dma_cfg *dma_cfg;
>  	int phy_mode;
> -	void *ret;
>  	int rc;
>  
>  	plat = devm_kzalloc(&pdev->dev, sizeof(*plat), GFP_KERNEL);
> @@ -615,21 +612,16 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
>  
>  	/* clock setup */
>  	if (!of_device_is_compatible(np, "snps,dwc-qos-ethernet-4.10")) {
> -		plat->stmmac_clk = devm_clk_get(&pdev->dev,
> -						STMMAC_RESOURCE_NAME);
> +		plat->stmmac_clk = devm_clk_get_enabled(&pdev->dev, STMMAC_RESOURCE_NAME);

As it looks like there will be a v3 anyway, a minor nit from my side:
Please preserve the line wrapping so that the lines remain <= 80 columns wide,
which is still preferred by Networking code.

		plat->stmmac_clk = devm_clk_get_enabled(&pdev->dev,
							STMMAC_RESOURCE_NAME);

>  		if (IS_ERR(plat->stmmac_clk)) {
>  			dev_warn(&pdev->dev, "Cannot get CSR clock\n");
>  			plat->stmmac_clk = NULL;
>  		}
> -		clk_prepare_enable(plat->stmmac_clk);
>  	}
>  

...

Re: [net-next v3 2/9] net: stmmac: platform: Convert to devm_clk_get_enabled() and devm_clk_get_optional_enabled()

[Serge Semin]

On Tue, Aug 27, 2024 at 03:57:05AM -0600, Yangtao Li wrote:
> Use devm_clk_get_enabled() and devm_clk_get_optional_enabled()
> to simplify code.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Suggested-by: Serge Semin <fancer.lancer@gmail.com>

Can't remember that I suggested the entire change, but merely the
ERR_CAST() macro utilization. Anyway the patch now looks good:

Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

Thanks.
-Serge(y)

> ---
> v2:
> -remove unused 'ret'
> -fix incompatible-pointer-types
> 
>  .../ethernet/stmicro/stmmac/stmmac_platform.c | 35 +++++--------------
>  1 file changed, 8 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index ad868e8d195d..4365afabf3c4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -415,8 +415,6 @@ static int stmmac_of_get_mac_mode(struct device_node *np)
>  static void stmmac_remove_config_dt(struct platform_device *pdev,
>  				    struct plat_stmmacenet_data *plat)
>  {
> -	clk_disable_unprepare(plat->stmmac_clk);
> -	clk_disable_unprepare(plat->pclk);
>  	of_node_put(plat->phy_node);
>  	of_node_put(plat->mdio_node);
>  }
> @@ -436,7 +434,6 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
>  	struct plat_stmmacenet_data *plat;
>  	struct stmmac_dma_cfg *dma_cfg;
>  	int phy_mode;
> -	void *ret;
>  	int rc;
>  
>  	plat = devm_kzalloc(&pdev->dev, sizeof(*plat), GFP_KERNEL);
> @@ -615,21 +612,16 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
>  
>  	/* clock setup */
>  	if (!of_device_is_compatible(np, "snps,dwc-qos-ethernet-4.10")) {
> -		plat->stmmac_clk = devm_clk_get(&pdev->dev,
> -						STMMAC_RESOURCE_NAME);
> +		plat->stmmac_clk = devm_clk_get_enabled(&pdev->dev, STMMAC_RESOURCE_NAME);
>  		if (IS_ERR(plat->stmmac_clk)) {
>  			dev_warn(&pdev->dev, "Cannot get CSR clock\n");
>  			plat->stmmac_clk = NULL;
>  		}
> -		clk_prepare_enable(plat->stmmac_clk);
>  	}
>  
> -	plat->pclk = devm_clk_get_optional(&pdev->dev, "pclk");
> -	if (IS_ERR(plat->pclk)) {
> -		ret = plat->pclk;
> -		goto error_pclk_get;
> -	}
> -	clk_prepare_enable(plat->pclk);
> +	plat->pclk = devm_clk_get_optional_enabled(&pdev->dev, "pclk");
> +	if (IS_ERR(plat->pclk))
> +		return ERR_CAST(plat->pclk);
>  
>  	/* Fall-back to main clock in case of no PTP ref is passed */
>  	plat->clk_ptp_ref = devm_clk_get(&pdev->dev, "ptp_ref");
> @@ -644,26 +636,15 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
>  
>  	plat->stmmac_rst = devm_reset_control_get_optional(&pdev->dev,
>  							   STMMAC_RESOURCE_NAME);
> -	if (IS_ERR(plat->stmmac_rst)) {
> -		ret = plat->stmmac_rst;
> -		goto error_hw_init;
> -	}
> +	if (IS_ERR(plat->stmmac_rst))
> +		return ERR_CAST(plat->stmmac_rst);
>  
>  	plat->stmmac_ahb_rst = devm_reset_control_get_optional_shared(
>  							&pdev->dev, "ahb");
> -	if (IS_ERR(plat->stmmac_ahb_rst)) {
> -		ret = plat->stmmac_ahb_rst;
> -		goto error_hw_init;
> -	}
> +	if (IS_ERR(plat->stmmac_ahb_rst))
> +		return ERR_CAST(plat->stmmac_ahb_rst);
>  
>  	return plat;
> -
> -error_hw_init:
> -	clk_disable_unprepare(plat->pclk);
> -error_pclk_get:
> -	clk_disable_unprepare(plat->stmmac_clk);
> -
> -	return ret;
>  }
>  
>  static void devm_stmmac_remove_config_dt(void *data)
> -- 
> 2.39.0
> 

[net-next v3 3/9] net: ethernet: cortina: Convert to devm_clk_get_enabled()

[Yangtao Li]

Convert devm_clk_get(), clk_prepare_enable() to a single
call to devm_clk_get_enabled(), as this is exactly
what this function does.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
v3:
-move the local clock variables, keep lines longest to shortest

 drivers/net/ethernet/cortina/gemini.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 73e1c71c5092..5c86987c6fdf 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -109,7 +109,6 @@ struct gemini_ethernet_port {
 	struct device *dev;
 	void __iomem *dma_base;
 	void __iomem *gmac_base;
-	struct clk *pclk;
 	struct reset_control *reset;
 	int irq;
 	__le32 mac_addr[3];
@@ -2326,7 +2325,6 @@ static void gemini_port_remove(struct gemini_ethernet_port *port)
 		phy_disconnect(port->netdev->phydev);
 		unregister_netdev(port->netdev);
 	}
-	clk_disable_unprepare(port->pclk);
 	geth_cleanup_freeq(port->geth);
 }
 
@@ -2401,6 +2399,7 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
 	struct gemini_ethernet *geth;
 	struct net_device *netdev;
 	struct device *parent;
+	struct clk *pclk;
 	u8 mac[ETH_ALEN];
 	unsigned int id;
 	int irq;
@@ -2453,14 +2452,11 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
 	port->irq = irq;
 
 	/* Clock the port */
-	port->pclk = devm_clk_get(dev, "PCLK");
-	if (IS_ERR(port->pclk)) {
+	pclk = devm_clk_get_enabled(dev, "PCLK");
+	if (IS_ERR(pclk)) {
 		dev_err(dev, "no PCLK\n");
-		return PTR_ERR(port->pclk);
+		return PTR_ERR(pclk);
 	}
-	ret = clk_prepare_enable(port->pclk);
-	if (ret)
-		return ret;
 
 	/* Maybe there is a nice ethernet address we should use */
 	gemini_port_save_mac_addr(port);
@@ -2469,8 +2465,7 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
 	port->reset = devm_reset_control_get_exclusive(dev, NULL);
 	if (IS_ERR(port->reset)) {
 		dev_err(dev, "no reset\n");
-		ret = PTR_ERR(port->reset);
-		goto unprepare;
+		return PTR_ERR(port->reset);
 	}
 	reset_control_reset(port->reset);
 	usleep_range(100, 500);
@@ -2532,24 +2527,20 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
 					port_names[port->id],
 					port);
 	if (ret)
-		goto unprepare;
+		return ret;
 
 	ret = gmac_setup_phy(netdev);
 	if (ret) {
 		netdev_err(netdev,
 			   "PHY init failed\n");
-		goto unprepare;
+		return ret;
 	}
 
 	ret = register_netdev(netdev);
 	if (ret)
-		goto unprepare;
+		return ret;
 
 	return 0;
-
-unprepare:
-	clk_disable_unprepare(port->pclk);
-	return ret;
 }
 
 static void gemini_ethernet_port_remove(struct platform_device *pdev)
-- 
2.39.0

Re: [net-next v3 3/9] net: ethernet: cortina: Convert to devm_clk_get_enabled()

[Jonathan Cameron]

On Tue, 27 Aug 2024 03:57:06 -0600
Yangtao Li <frank.li@vivo.com> wrote:

> Convert devm_clk_get(), clk_prepare_enable() to a single
> call to devm_clk_get_enabled(), as this is exactly
> what this function does.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

I don't like the mixing of devm and non devm here.
Maybe better to use a devm_add_action_or_reset()
for geth_cleanup_freeq() as well.


> ---
> v3:
> -move the local clock variables, keep lines longest to shortest
> 
>  drivers/net/ethernet/cortina/gemini.c | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 73e1c71c5092..5c86987c6fdf 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -109,7 +109,6 @@ struct gemini_ethernet_port {
>  	struct device *dev;
>  	void __iomem *dma_base;
>  	void __iomem *gmac_base;
> -	struct clk *pclk;
>  	struct reset_control *reset;
>  	int irq;
>  	__le32 mac_addr[3];
> @@ -2326,7 +2325,6 @@ static void gemini_port_remove(struct gemini_ethernet_port *port)
>  		phy_disconnect(port->netdev->phydev);
>  		unregister_netdev(port->netdev);
>  	}
> -	clk_disable_unprepare(port->pclk);
>  	geth_cleanup_freeq(port->geth);
>  }
>  
> @@ -2401,6 +2399,7 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
>  	struct gemini_ethernet *geth;
>  	struct net_device *netdev;
>  	struct device *parent;
> +	struct clk *pclk;
>  	u8 mac[ETH_ALEN];
>  	unsigned int id;
>  	int irq;
> @@ -2453,14 +2452,11 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
>  	port->irq = irq;
>  
>  	/* Clock the port */
> -	port->pclk = devm_clk_get(dev, "PCLK");
> -	if (IS_ERR(port->pclk)) {
> +	pclk = devm_clk_get_enabled(dev, "PCLK");
> +	if (IS_ERR(pclk)) {
>  		dev_err(dev, "no PCLK\n");
> -		return PTR_ERR(port->pclk);
> +		return PTR_ERR(pclk);
>  	}
> -	ret = clk_prepare_enable(port->pclk);
> -	if (ret)
> -		return ret;
>  
>  	/* Maybe there is a nice ethernet address we should use */
>  	gemini_port_save_mac_addr(port);
> @@ -2469,8 +2465,7 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
>  	port->reset = devm_reset_control_get_exclusive(dev, NULL);
>  	if (IS_ERR(port->reset)) {
>  		dev_err(dev, "no reset\n");
> -		ret = PTR_ERR(port->reset);
> -		goto unprepare;
> +		return PTR_ERR(port->reset);
>  	}
>  	reset_control_reset(port->reset);
>  	usleep_range(100, 500);
> @@ -2532,24 +2527,20 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
>  					port_names[port->id],
>  					port);
>  	if (ret)
> -		goto unprepare;
> +		return ret;
>  
>  	ret = gmac_setup_phy(netdev);
>  	if (ret) {
>  		netdev_err(netdev,
>  			   "PHY init failed\n");
> -		goto unprepare;
> +		return ret;
>  	}
>  
>  	ret = register_netdev(netdev);
>  	if (ret)
> -		goto unprepare;
> +		return ret;
>  
>  	return 0;
> -
> -unprepare:
> -	clk_disable_unprepare(port->pclk);
> -	return ret;
>  }
>  
>  static void gemini_ethernet_port_remove(struct platform_device *pdev)

Re: [net-next v3 3/9] net: ethernet: cortina: Convert to devm_clk_get_enabled()

[Russell King (Oracle)]

On Tue, Aug 27, 2024 at 03:57:06AM -0600, Yangtao Li wrote:
>  	ret = register_netdev(netdev);
>  	if (ret)
> -		goto unprepare;
> +		return ret;
>  
>  	return 0;

Same comment as per patch 1. At this point, I'm going to stop reviewing
your patches (because I don't want to waste what little time I'm able
to spend in front of the screen raising comments against the same issue
throughout a patch set) and I ask you to do your own review of your
series for this pattern - and also consider where using
PTR_ERR_OR_ZERO() may also be appropriate in any of your patches. See
that function's documentation in linux/err.h.

Please wait at least 24 hours before reposting.

Thanks.

-- 
*** please note that I probably will only be occasionally responsive
*** for an unknown period of time due to recent eye surgery making
*** reading quite difficult.

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[net-next v3 4/9] net: mdio: hisi-femac: Convert to devm_clk_get_enabled()

[Yangtao Li]

Convert devm_clk_get(), clk_prepare_enable() to a single
call to devm_clk_get_enabled(), as this is exactly
what this function does.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 drivers/net/mdio/mdio-hisi-femac.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/net/mdio/mdio-hisi-femac.c b/drivers/net/mdio/mdio-hisi-femac.c
index 6703f626ee83..f6fcb9e11624 100644
--- a/drivers/net/mdio/mdio-hisi-femac.c
+++ b/drivers/net/mdio/mdio-hisi-femac.c
@@ -21,7 +21,6 @@
 #define BIT_WR_DATA_OFFSET	16
 
 struct hisi_femac_mdio_data {
-	struct clk *clk;
 	void __iomem *membase;
 };
 
@@ -74,6 +73,7 @@ static int hisi_femac_mdio_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	struct mii_bus *bus;
 	struct hisi_femac_mdio_data *data;
+	struct clk *clk;
 	int ret;
 
 	bus = mdiobus_alloc_size(sizeof(*data));
@@ -93,26 +93,20 @@ static int hisi_femac_mdio_probe(struct platform_device *pdev)
 		goto err_out_free_mdiobus;
 	}
 
-	data->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(data->clk)) {
-		ret = PTR_ERR(data->clk);
+	clk = devm_clk_get_prepared(&pdev->dev, NULL);
+	if (IS_ERR(clk)) {
+		ret = PTR_ERR(clk);
 		goto err_out_free_mdiobus;
 	}
 
-	ret = clk_prepare_enable(data->clk);
-	if (ret)
-		goto err_out_free_mdiobus;
-
 	ret = of_mdiobus_register(bus, np);
 	if (ret)
-		goto err_out_disable_clk;
+		goto err_out_free_mdiobus;
 
 	platform_set_drvdata(pdev, bus);
 
 	return 0;
 
-err_out_disable_clk:
-	clk_disable_unprepare(data->clk);
 err_out_free_mdiobus:
 	mdiobus_free(bus);
 	return ret;
@@ -121,10 +115,8 @@ static int hisi_femac_mdio_probe(struct platform_device *pdev)
 static void hisi_femac_mdio_remove(struct platform_device *pdev)
 {
 	struct mii_bus *bus = platform_get_drvdata(pdev);
-	struct hisi_femac_mdio_data *data = bus->priv;
 
 	mdiobus_unregister(bus);
-	clk_disable_unprepare(data->clk);
 	mdiobus_free(bus);
 }
 
-- 
2.39.0

Re: [net-next v3 4/9] net: mdio: hisi-femac: Convert to devm_clk_get_enabled()

[Jonathan Cameron]

On Tue, 27 Aug 2024 03:57:07 -0600
Yangtao Li <frank.li@vivo.com> wrote:

> Convert devm_clk_get(), clk_prepare_enable() to a single
> call to devm_clk_get_enabled(), as this is exactly
> what this function does.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>

Mixing an matching devm and otherwise can lead to subtle bugs
and generally makes the code harder to review

I'd also use devm_mdiobus_alloc_size() and devm_mdiobus_register()
and drop the remove() callback entirely.

I would not take this patch on its own as it causes a reordering
of cleanup we probably don't want.

As a general rule, single action devm cleanup series (so only using
one new type) fall into this trap. Much better to look at all the
improvements in each driver that are enabled by new infrastructure
rather than doing a single type change series like this one.

Thanks,

Jonathan


> ---
>  drivers/net/mdio/mdio-hisi-femac.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/mdio/mdio-hisi-femac.c b/drivers/net/mdio/mdio-hisi-femac.c
> index 6703f626ee83..f6fcb9e11624 100644
> --- a/drivers/net/mdio/mdio-hisi-femac.c
> +++ b/drivers/net/mdio/mdio-hisi-femac.c
> @@ -21,7 +21,6 @@
>  #define BIT_WR_DATA_OFFSET	16
>  
>  struct hisi_femac_mdio_data {
> -	struct clk *clk;
>  	void __iomem *membase;
>  };
>  
> @@ -74,6 +73,7 @@ static int hisi_femac_mdio_probe(struct platform_device *pdev)
>  	struct device_node *np = pdev->dev.of_node;
>  	struct mii_bus *bus;
>  	struct hisi_femac_mdio_data *data;
> +	struct clk *clk;
>  	int ret;
>  
>  	bus = mdiobus_alloc_size(sizeof(*data));
> @@ -93,26 +93,20 @@ static int hisi_femac_mdio_probe(struct platform_device *pdev)
>  		goto err_out_free_mdiobus;
>  	}
>  
> -	data->clk = devm_clk_get(&pdev->dev, NULL);
> -	if (IS_ERR(data->clk)) {
> -		ret = PTR_ERR(data->clk);
> +	clk = devm_clk_get_prepared(&pdev->dev, NULL);
> +	if (IS_ERR(clk)) {
> +		ret = PTR_ERR(clk);
>  		goto err_out_free_mdiobus;
>  	}
>  
> -	ret = clk_prepare_enable(data->clk);
> -	if (ret)
> -		goto err_out_free_mdiobus;
> -
>  	ret = of_mdiobus_register(bus, np);
>  	if (ret)
> -		goto err_out_disable_clk;
> +		goto err_out_free_mdiobus;
>  
>  	platform_set_drvdata(pdev, bus);
>  
>  	return 0;
>  
> -err_out_disable_clk:
> -	clk_disable_unprepare(data->clk);
>  err_out_free_mdiobus:
>  	mdiobus_free(bus);
>  	return ret;
> @@ -121,10 +115,8 @@ static int hisi_femac_mdio_probe(struct platform_device *pdev)
>  static void hisi_femac_mdio_remove(struct platform_device *pdev)
>  {
>  	struct mii_bus *bus = platform_get_drvdata(pdev);
> -	struct hisi_femac_mdio_data *data = bus->priv;
>  
>  	mdiobus_unregister(bus);
> -	clk_disable_unprepare(data->clk);
>  	mdiobus_free(bus);
>  }
>  

[net-next v3 5/9] net: dsa: rzn1_a5psw: Convert to devm_clk_get_enabled()

[Yangtao Li]

Convert devm_clk_get(), clk_prepare_enable() to a single
call to devm_clk_get_enabled(), as this is exactly
what this function does.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 drivers/net/dsa/rzn1_a5psw.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
index 92e032972b34..9627c6550a93 100644
--- a/drivers/net/dsa/rzn1_a5psw.c
+++ b/drivers/net/dsa/rzn1_a5psw.c
@@ -1228,35 +1228,27 @@ static int a5psw_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	a5psw->hclk = devm_clk_get(dev, "hclk");
+	a5psw->hclk = devm_clk_get_enabled(dev, "hclk");
 	if (IS_ERR(a5psw->hclk)) {
 		dev_err(dev, "failed get hclk clock\n");
 		ret = PTR_ERR(a5psw->hclk);
 		goto free_pcs;
 	}
 
-	a5psw->clk = devm_clk_get(dev, "clk");
+	a5psw->clk = devm_clk_get_enabled(dev, "clk");
 	if (IS_ERR(a5psw->clk)) {
 		dev_err(dev, "failed get clk_switch clock\n");
 		ret = PTR_ERR(a5psw->clk);
 		goto free_pcs;
 	}
 
-	ret = clk_prepare_enable(a5psw->clk);
-	if (ret)
-		goto free_pcs;
-
-	ret = clk_prepare_enable(a5psw->hclk);
-	if (ret)
-		goto clk_disable;
-
 	mdio = of_get_child_by_name(dev->of_node, "mdio");
 	if (of_device_is_available(mdio)) {
 		ret = a5psw_probe_mdio(a5psw, mdio);
 		if (ret) {
 			of_node_put(mdio);
 			dev_err(dev, "Failed to register MDIO: %d\n", ret);
-			goto hclk_disable;
+			goto free_pcs;
 		}
 	}
 
@@ -1272,15 +1264,11 @@ static int a5psw_probe(struct platform_device *pdev)
 	ret = dsa_register_switch(ds);
 	if (ret) {
 		dev_err(dev, "Failed to register DSA switch: %d\n", ret);
-		goto hclk_disable;
+		goto free_pcs;
 	}
 
 	return 0;
 
-hclk_disable:
-	clk_disable_unprepare(a5psw->hclk);
-clk_disable:
-	clk_disable_unprepare(a5psw->clk);
 free_pcs:
 	a5psw_pcs_free(a5psw);
 
@@ -1296,8 +1284,6 @@ static void a5psw_remove(struct platform_device *pdev)
 
 	dsa_unregister_switch(&a5psw->ds);
 	a5psw_pcs_free(a5psw);
-	clk_disable_unprepare(a5psw->hclk);
-	clk_disable_unprepare(a5psw->clk);
 }
 
 static void a5psw_shutdown(struct platform_device *pdev)
-- 
2.39.0

Re: [net-next v3 5/9] net: dsa: rzn1_a5psw: Convert to devm_clk_get_enabled()

[Geert Uytterhoeven]

On Tue, Aug 27, 2024 at 11:44 AM Yangtao Li <frank.li@vivo.com> wrote:
> Convert devm_clk_get(), clk_prepare_enable() to a single
> call to devm_clk_get_enabled(), as this is exactly
> what this function does.
>
> Signed-off-by: Yangtao Li <frank.li@vivo.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[net-next v3 6/9] net: ethernet: broadcom: bcm63xx_enet: Convert to devm_clk_get_enabled()

[Yangtao Li]

Convert devm_clk_get(), clk_prepare_enable() to a single
call to devm_clk_get_enabled(), as this is exactly
what this function does.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
v3:
-Reduce the number of clk variables

 drivers/net/ethernet/broadcom/bcm63xx_enet.c | 47 ++++++--------------
 drivers/net/ethernet/broadcom/bcm63xx_enet.h |  6 ---
 2 files changed, 13 insertions(+), 40 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcm63xx_enet.c b/drivers/net/ethernet/broadcom/bcm63xx_enet.c
index 3c0e3b9828be..dcc741837d50 100644
--- a/drivers/net/ethernet/broadcom/bcm63xx_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm63xx_enet.c
@@ -1718,6 +1718,7 @@ static int bcm_enet_probe(struct platform_device *pdev)
 	struct bcm63xx_enet_platform_data *pd;
 	int irq, irq_rx, irq_tx;
 	struct mii_bus *bus;
+	struct clk *clk;
 	int i, ret;
 
 	if (!bcm_enet_shared_base[0])
@@ -1752,14 +1753,11 @@ static int bcm_enet_probe(struct platform_device *pdev)
 	priv->irq_rx = irq_rx;
 	priv->irq_tx = irq_tx;
 
-	priv->mac_clk = devm_clk_get(&pdev->dev, "enet");
-	if (IS_ERR(priv->mac_clk)) {
-		ret = PTR_ERR(priv->mac_clk);
+	clk = devm_clk_get_enabled(&pdev->dev, "enet");
+	if (IS_ERR(clk)) {
+		ret = PTR_ERR(clk);
 		goto out;
 	}
-	ret = clk_prepare_enable(priv->mac_clk);
-	if (ret)
-		goto out;
 
 	/* initialize default and fetch platform data */
 	priv->rx_ring_size = BCMENET_DEF_RX_DESC;
@@ -1789,15 +1787,11 @@ static int bcm_enet_probe(struct platform_device *pdev)
 
 	if (priv->has_phy && !priv->use_external_mii) {
 		/* using internal PHY, enable clock */
-		priv->phy_clk = devm_clk_get(&pdev->dev, "ephy");
-		if (IS_ERR(priv->phy_clk)) {
-			ret = PTR_ERR(priv->phy_clk);
-			priv->phy_clk = NULL;
-			goto out_disable_clk_mac;
+		clk = devm_clk_get_enabled(&pdev->dev, "ephy");
+		if (IS_ERR(clk)) {
+			ret = PTR_ERR(clk);
+			goto out;
 		}
-		ret = clk_prepare_enable(priv->phy_clk);
-		if (ret)
-			goto out_disable_clk_mac;
 	}
 
 	/* do minimal hardware init to be able to probe mii bus */
@@ -1889,10 +1883,7 @@ static int bcm_enet_probe(struct platform_device *pdev)
 out_uninit_hw:
 	/* turn off mdc clock */
 	enet_writel(priv, 0, ENET_MIISC_REG);
-	clk_disable_unprepare(priv->phy_clk);
 
-out_disable_clk_mac:
-	clk_disable_unprepare(priv->mac_clk);
 out:
 	free_netdev(dev);
 	return ret;
@@ -1927,10 +1918,6 @@ static void bcm_enet_remove(struct platform_device *pdev)
 				       bcm_enet_mdio_write_mii);
 	}
 
-	/* disable hw block clocks */
-	clk_disable_unprepare(priv->phy_clk);
-	clk_disable_unprepare(priv->mac_clk);
-
 	free_netdev(dev);
 }
 
@@ -2648,6 +2635,7 @@ static int bcm_enetsw_probe(struct platform_device *pdev)
 	struct bcm63xx_enetsw_platform_data *pd;
 	struct resource *res_mem;
 	int ret, irq_rx, irq_tx;
+	struct clk *mac_clk;
 
 	if (!bcm_enet_shared_base[0])
 		return -EPROBE_DEFER;
@@ -2694,14 +2682,11 @@ static int bcm_enetsw_probe(struct platform_device *pdev)
 		goto out;
 	}
 
-	priv->mac_clk = devm_clk_get(&pdev->dev, "enetsw");
-	if (IS_ERR(priv->mac_clk)) {
-		ret = PTR_ERR(priv->mac_clk);
+	mac_clk = devm_clk_get_enabled(&pdev->dev, "enetsw");
+	if (IS_ERR(mac_clk)) {
+		ret = PTR_ERR(mac_clk);
 		goto out;
 	}
-	ret = clk_prepare_enable(priv->mac_clk);
-	if (ret)
-		goto out;
 
 	priv->rx_chan = 0;
 	priv->tx_chan = 1;
@@ -2720,7 +2705,7 @@ static int bcm_enetsw_probe(struct platform_device *pdev)
 
 	ret = register_netdev(dev);
 	if (ret)
-		goto out_disable_clk;
+		goto out;
 
 	netif_carrier_off(dev);
 	platform_set_drvdata(pdev, dev);
@@ -2729,8 +2714,6 @@ static int bcm_enetsw_probe(struct platform_device *pdev)
 
 	return 0;
 
-out_disable_clk:
-	clk_disable_unprepare(priv->mac_clk);
 out:
 	free_netdev(dev);
 	return ret;
@@ -2740,16 +2723,12 @@ static int bcm_enetsw_probe(struct platform_device *pdev)
 /* exit func, stops hardware and unregisters netdevice */
 static void bcm_enetsw_remove(struct platform_device *pdev)
 {
-	struct bcm_enet_priv *priv;
 	struct net_device *dev;
 
 	/* stop netdevice */
 	dev = platform_get_drvdata(pdev);
-	priv = netdev_priv(dev);
 	unregister_netdev(dev);
 
-	clk_disable_unprepare(priv->mac_clk);
-
 	free_netdev(dev);
 }
 
diff --git a/drivers/net/ethernet/broadcom/bcm63xx_enet.h b/drivers/net/ethernet/broadcom/bcm63xx_enet.h
index 78f1830fb3cb..e98838b8b92f 100644
--- a/drivers/net/ethernet/broadcom/bcm63xx_enet.h
+++ b/drivers/net/ethernet/broadcom/bcm63xx_enet.h
@@ -316,12 +316,6 @@ struct bcm_enet_priv {
 	/* lock mib update between userspace request and workqueue */
 	struct mutex mib_update_lock;
 
-	/* mac clock */
-	struct clk *mac_clk;
-
-	/* phy clock if internal phy is used */
-	struct clk *phy_clk;
-
 	/* network device reference */
 	struct net_device *net_dev;
 
-- 
2.39.0

Re: [net-next v3 6/9] net: ethernet: broadcom: bcm63xx_enet: Convert to devm_clk_get_enabled()

[Jonathan Cameron]

On Tue, 27 Aug 2024 03:57:09 -0600
Yangtao Li <frank.li@vivo.com> wrote:

> Convert devm_clk_get(), clk_prepare_enable() to a single
> call to devm_clk_get_enabled(), as this is exactly
> what this function does.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
Another one where this is mixing devm and not which makes care
hard to review and may introduce subtle bugs.

Use devm_alloc_etherdev() and devm_register_netdev()
and take all the cleanup handling managed.

Much simpler to review that way.

J
> ---
> v3:
> -Reduce the number of clk variables
> 
>  drivers/net/ethernet/broadcom/bcm63xx_enet.c | 47 ++++++--------------
>  drivers/net/ethernet/broadcom/bcm63xx_enet.h |  6 ---
>  2 files changed, 13 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bcm63xx_enet.c b/drivers/net/ethernet/broadcom/bcm63xx_enet.c
> index 3c0e3b9828be..dcc741837d50 100644
> --- a/drivers/net/ethernet/broadcom/bcm63xx_enet.c
> +++ b/drivers/net/ethernet/broadcom/bcm63xx_enet.c
> @@ -1718,6 +1718,7 @@ static int bcm_enet_probe(struct platform_device *pdev)
>  	struct bcm63xx_enet_platform_data *pd;
>  	int irq, irq_rx, irq_tx;
>  	struct mii_bus *bus;
> +	struct clk *clk;
>  	int i, ret;
>  
>  	if (!bcm_enet_shared_base[0])
> @@ -1752,14 +1753,11 @@ static int bcm_enet_probe(struct platform_device *pdev)
>  	priv->irq_rx = irq_rx;
>  	priv->irq_tx = irq_tx;
>  
> -	priv->mac_clk = devm_clk_get(&pdev->dev, "enet");
> -	if (IS_ERR(priv->mac_clk)) {
> -		ret = PTR_ERR(priv->mac_clk);
> +	clk = devm_clk_get_enabled(&pdev->dev, "enet");
> +	if (IS_ERR(clk)) {
> +		ret = PTR_ERR(clk);
>  		goto out;
>  	}
> -	ret = clk_prepare_enable(priv->mac_clk);
> -	if (ret)
> -		goto out;
>  
>  	/* initialize default and fetch platform data */
>  	priv->rx_ring_size = BCMENET_DEF_RX_DESC;
> @@ -1789,15 +1787,11 @@ static int bcm_enet_probe(struct platform_device *pdev)
>  
>  	if (priv->has_phy && !priv->use_external_mii) {
>  		/* using internal PHY, enable clock */
> -		priv->phy_clk = devm_clk_get(&pdev->dev, "ephy");
> -		if (IS_ERR(priv->phy_clk)) {
> -			ret = PTR_ERR(priv->phy_clk);
> -			priv->phy_clk = NULL;
> -			goto out_disable_clk_mac;
> +		clk = devm_clk_get_enabled(&pdev->dev, "ephy");
> +		if (IS_ERR(clk)) {
> +			ret = PTR_ERR(clk);
> +			goto out;
>  		}
> -		ret = clk_prepare_enable(priv->phy_clk);
> -		if (ret)
> -			goto out_disable_clk_mac;
>  	}
>  
>  	/* do minimal hardware init to be able to probe mii bus */
> @@ -1889,10 +1883,7 @@ static int bcm_enet_probe(struct platform_device *pdev)
>  out_uninit_hw:
>  	/* turn off mdc clock */
>  	enet_writel(priv, 0, ENET_MIISC_REG);
> -	clk_disable_unprepare(priv->phy_clk);
>  
> -out_disable_clk_mac:
> -	clk_disable_unprepare(priv->mac_clk);
>  out:
>  	free_netdev(dev);
>  	return ret;
> @@ -1927,10 +1918,6 @@ static void bcm_enet_remove(struct platform_device *pdev)
>  				       bcm_enet_mdio_write_mii);
>  	}
>  
> -	/* disable hw block clocks */
> -	clk_disable_unprepare(priv->phy_clk);
> -	clk_disable_unprepare(priv->mac_clk);
> -
>  	free_netdev(dev);
>  }
>  
> @@ -2648,6 +2635,7 @@ static int bcm_enetsw_probe(struct platform_device *pdev)
>  	struct bcm63xx_enetsw_platform_data *pd;
>  	struct resource *res_mem;
>  	int ret, irq_rx, irq_tx;
> +	struct clk *mac_clk;
>  
>  	if (!bcm_enet_shared_base[0])
>  		return -EPROBE_DEFER;
> @@ -2694,14 +2682,11 @@ static int bcm_enetsw_probe(struct platform_device *pdev)
>  		goto out;
>  	}
>  
> -	priv->mac_clk = devm_clk_get(&pdev->dev, "enetsw");
> -	if (IS_ERR(priv->mac_clk)) {
> -		ret = PTR_ERR(priv->mac_clk);
> +	mac_clk = devm_clk_get_enabled(&pdev->dev, "enetsw");
> +	if (IS_ERR(mac_clk)) {
> +		ret = PTR_ERR(mac_clk);
>  		goto out;
>  	}
> -	ret = clk_prepare_enable(priv->mac_clk);
> -	if (ret)
> -		goto out;
>  
>  	priv->rx_chan = 0;
>  	priv->tx_chan = 1;
> @@ -2720,7 +2705,7 @@ static int bcm_enetsw_probe(struct platform_device *pdev)
>  
>  	ret = register_netdev(dev);
>  	if (ret)
> -		goto out_disable_clk;
> +		goto out;
>  
>  	netif_carrier_off(dev);
>  	platform_set_drvdata(pdev, dev);
> @@ -2729,8 +2714,6 @@ static int bcm_enetsw_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> -out_disable_clk:
> -	clk_disable_unprepare(priv->mac_clk);
>  out:
>  	free_netdev(dev);
>  	return ret;
> @@ -2740,16 +2723,12 @@ static int bcm_enetsw_probe(struct platform_device *pdev)
>  /* exit func, stops hardware and unregisters netdevice */
>  static void bcm_enetsw_remove(struct platform_device *pdev)
>  {
> -	struct bcm_enet_priv *priv;
>  	struct net_device *dev;
>  
>  	/* stop netdevice */
>  	dev = platform_get_drvdata(pdev);
> -	priv = netdev_priv(dev);
>  	unregister_netdev(dev);
>  
> -	clk_disable_unprepare(priv->mac_clk);
> -
>  	free_netdev(dev);
>  }
>  
> diff --git a/drivers/net/ethernet/broadcom/bcm63xx_enet.h b/drivers/net/ethernet/broadcom/bcm63xx_enet.h
> index 78f1830fb3cb..e98838b8b92f 100644
> --- a/drivers/net/ethernet/broadcom/bcm63xx_enet.h
> +++ b/drivers/net/ethernet/broadcom/bcm63xx_enet.h
> @@ -316,12 +316,6 @@ struct bcm_enet_priv {
>  	/* lock mib update between userspace request and workqueue */
>  	struct mutex mib_update_lock;
>  
> -	/* mac clock */
> -	struct clk *mac_clk;
> -
> -	/* phy clock if internal phy is used */
> -	struct clk *phy_clk;
> -
>  	/* network device reference */
>  	struct net_device *net_dev;
>  

[net-next v3 7/9] net: ethernet: marvell: mvneta: Convert to devm_clk_get_enabled()

[Yangtao Li]

Convert devm_clk_get(), clk_prepare_enable() to a single
call to devm_clk_get_enabled(), as this is exactly
what this function does.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Reviewed-by: Marcin Wojtas <marcin.s.wojtas@gmail.com>
---
 drivers/net/ethernet/marvell/mvneta_bm.c | 16 +++++-----------
 drivers/net/ethernet/marvell/mvneta_bm.h |  1 -
 2 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta_bm.c b/drivers/net/ethernet/marvell/mvneta_bm.c
index 3f46a0fed048..bfd1ed12d98c 100644
--- a/drivers/net/ethernet/marvell/mvneta_bm.c
+++ b/drivers/net/ethernet/marvell/mvneta_bm.c
@@ -411,6 +411,7 @@ static int mvneta_bm_probe(struct platform_device *pdev)
 {
 	struct device_node *dn = pdev->dev.of_node;
 	struct mvneta_bm *priv;
+	struct clk *clk;
 	int err;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(struct mvneta_bm), GFP_KERNEL);
@@ -421,17 +422,14 @@ static int mvneta_bm_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->reg_base))
 		return PTR_ERR(priv->reg_base);
 
-	priv->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(priv->clk))
-		return PTR_ERR(priv->clk);
-	err = clk_prepare_enable(priv->clk);
-	if (err < 0)
-		return err;
+	clk = devm_clk_get_enabled(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
 
 	err = mvneta_bm_get_sram(dn, priv);
 	if (err < 0) {
 		dev_err(&pdev->dev, "failed to allocate internal memory\n");
-		goto err_clk;
+		return err;
 	}
 
 	priv->pdev = pdev;
@@ -452,8 +450,6 @@ static int mvneta_bm_probe(struct platform_device *pdev)
 
 err_sram:
 	mvneta_bm_put_sram(priv);
-err_clk:
-	clk_disable_unprepare(priv->clk);
 	return err;
 }
 
@@ -473,8 +469,6 @@ static void mvneta_bm_remove(struct platform_device *pdev)
 
 	/* Dectivate BM unit */
 	mvneta_bm_write(priv, MVNETA_BM_COMMAND_REG, MVNETA_BM_STOP_MASK);
-
-	clk_disable_unprepare(priv->clk);
 }
 
 static const struct of_device_id mvneta_bm_match[] = {
diff --git a/drivers/net/ethernet/marvell/mvneta_bm.h b/drivers/net/ethernet/marvell/mvneta_bm.h
index e47783ce77e0..396dced914aa 100644
--- a/drivers/net/ethernet/marvell/mvneta_bm.h
+++ b/drivers/net/ethernet/marvell/mvneta_bm.h
@@ -94,7 +94,6 @@ enum mvneta_bm_type {
 
 struct mvneta_bm {
 	void __iomem *reg_base;
-	struct clk *clk;
 	struct platform_device *pdev;
 
 	struct gen_pool *bppi_pool;
-- 
2.39.0

Re: [net-next v3 7/9] net: ethernet: marvell: mvneta: Convert to devm_clk_get_enabled()

[Jonathan Cameron]

On Tue, 27 Aug 2024 03:57:10 -0600
Yangtao Li <frank.li@vivo.com> wrote:

> Convert devm_clk_get(), clk_prepare_enable() to a single
> call to devm_clk_get_enabled(), as this is exactly
> what this function does.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Reviewed-by: Marcin Wojtas <marcin.s.wojtas@gmail.com>
LGTM
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[net-next v3 8/9] net: mvpp2: Convert to devm_clk_get_enabled() and devm_clk_get_optional_enabled()

[Yangtao Li]

Use devm_clk_get_enabled() and devm_clk_get_optional_enabled()
to simplify code.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Suggested-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Reviewed-by: Marcin Wojtas <marcin.s.wojtas@gmail.com>
---
v2:
-get rid of amount of variables used

 drivers/net/ethernet/marvell/mvpp2/mvpp2.h    |  7 --
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   | 89 +++++--------------
 2 files changed, 24 insertions(+), 72 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
index 9e02e4367bec..643a645e8097 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
@@ -1044,13 +1044,6 @@ struct mvpp2 {
 	 */
 	struct regmap *sysctrl_base;
 
-	/* Common clocks */
-	struct clk *pp_clk;
-	struct clk *gop_clk;
-	struct clk *mg_clk;
-	struct clk *mg_core_clk;
-	struct clk *axi_clk;
-
 	/* List of pointers to port structures */
 	int port_count;
 	struct mvpp2_port *port_list[MVPP2_MAX_PORTS];
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 2fe8bae4eb3c..0ca2daeb0f90 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -7561,56 +7561,32 @@ static int mvpp2_probe(struct platform_device *pdev)
 		priv->max_port_rxqs = 32;
 
 	if (dev_of_node(&pdev->dev)) {
-		priv->pp_clk = devm_clk_get(&pdev->dev, "pp_clk");
-		if (IS_ERR(priv->pp_clk))
-			return PTR_ERR(priv->pp_clk);
-		err = clk_prepare_enable(priv->pp_clk);
-		if (err < 0)
-			return err;
-
-		priv->gop_clk = devm_clk_get(&pdev->dev, "gop_clk");
-		if (IS_ERR(priv->gop_clk)) {
-			err = PTR_ERR(priv->gop_clk);
-			goto err_pp_clk;
-		}
-		err = clk_prepare_enable(priv->gop_clk);
-		if (err < 0)
-			goto err_pp_clk;
+		struct clk *clk;
 
-		if (priv->hw_version >= MVPP22) {
-			priv->mg_clk = devm_clk_get(&pdev->dev, "mg_clk");
-			if (IS_ERR(priv->mg_clk)) {
-				err = PTR_ERR(priv->mg_clk);
-				goto err_gop_clk;
-			}
+		clk = devm_clk_get_enabled(&pdev->dev, "pp_clk");
+		if (IS_ERR(clk))
+			return PTR_ERR(clk);
 
-			err = clk_prepare_enable(priv->mg_clk);
-			if (err < 0)
-				goto err_gop_clk;
+		/* Get system's tclk rate */
+		priv->tclk = clk_get_rate(clk);
 
-			priv->mg_core_clk = devm_clk_get_optional(&pdev->dev, "mg_core_clk");
-			if (IS_ERR(priv->mg_core_clk)) {
-				err = PTR_ERR(priv->mg_core_clk);
-				goto err_mg_clk;
-			}
+		clk = devm_clk_get_enabled(&pdev->dev, "gop_clk");
+		if (IS_ERR(clk))
+			return PTR_ERR(clk);
 
-			err = clk_prepare_enable(priv->mg_core_clk);
-			if (err < 0)
-				goto err_mg_clk;
-		}
+		if (priv->hw_version >= MVPP22) {
+			clk = devm_clk_get_enabled(&pdev->dev, "mg_clk");
+			if (IS_ERR(clk))
+				return PTR_ERR(clk);
 
-		priv->axi_clk = devm_clk_get_optional(&pdev->dev, "axi_clk");
-		if (IS_ERR(priv->axi_clk)) {
-			err = PTR_ERR(priv->axi_clk);
-			goto err_mg_core_clk;
+			clk = devm_clk_get_optional_enabled(&pdev->dev, "mg_core_clk");
+			if (IS_ERR(clk))
+				return PTR_ERR(clk);
 		}
 
-		err = clk_prepare_enable(priv->axi_clk);
-		if (err < 0)
-			goto err_mg_core_clk;
-
-		/* Get system's tclk rate */
-		priv->tclk = clk_get_rate(priv->pp_clk);
+		clk = devm_clk_get_optional_enabled(&pdev->dev, "axi_clk");
+		if (IS_ERR(clk))
+			return PTR_ERR(clk);
 	} else {
 		err = device_property_read_u32(&pdev->dev, "clock-frequency", &priv->tclk);
 		if (err) {
@@ -7622,7 +7598,7 @@ static int mvpp2_probe(struct platform_device *pdev)
 	if (priv->hw_version >= MVPP22) {
 		err = dma_set_mask(&pdev->dev, MVPP2_DESC_DMA_MASK);
 		if (err)
-			goto err_axi_clk;
+			return err;
 		/* Sadly, the BM pools all share the same register to
 		 * store the high 32 bits of their address. So they
 		 * must all have the same high 32 bits, which forces
@@ -7630,7 +7606,7 @@ static int mvpp2_probe(struct platform_device *pdev)
 		 */
 		err = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
 		if (err)
-			goto err_axi_clk;
+			return err;
 	}
 
 	/* Map DTS-active ports. Should be done before FIFO mvpp2_init */
@@ -7649,12 +7625,12 @@ static int mvpp2_probe(struct platform_device *pdev)
 	err = mvpp2_init(pdev, priv);
 	if (err < 0) {
 		dev_err(&pdev->dev, "failed to initialize controller\n");
-		goto err_axi_clk;
+		return err;
 	}
 
 	err = mvpp22_tai_probe(&pdev->dev, priv);
 	if (err < 0)
-		goto err_axi_clk;
+		return err;
 
 	/* Initialize ports */
 	device_for_each_child_node_scoped(&pdev->dev, port_fwnode) {
@@ -7665,8 +7641,7 @@ static int mvpp2_probe(struct platform_device *pdev)
 
 	if (priv->port_count == 0) {
 		dev_err(&pdev->dev, "no ports enabled\n");
-		err = -ENODEV;
-		goto err_axi_clk;
+		return -ENODEV;
 	}
 
 	/* Statistics must be gathered regularly because some of them (like
@@ -7698,16 +7673,6 @@ static int mvpp2_probe(struct platform_device *pdev)
 err_port_probe:
 	for (i = 0; i < priv->port_count; i++)
 		mvpp2_port_remove(priv->port_list[i]);
-err_axi_clk:
-	clk_disable_unprepare(priv->axi_clk);
-err_mg_core_clk:
-	clk_disable_unprepare(priv->mg_core_clk);
-err_mg_clk:
-	clk_disable_unprepare(priv->mg_clk);
-err_gop_clk:
-	clk_disable_unprepare(priv->gop_clk);
-err_pp_clk:
-	clk_disable_unprepare(priv->pp_clk);
 	return err;
 }
 
@@ -7745,12 +7710,6 @@ static void mvpp2_remove(struct platform_device *pdev)
 
 	if (!dev_of_node(&pdev->dev))
 		return;
-
-	clk_disable_unprepare(priv->axi_clk);
-	clk_disable_unprepare(priv->mg_core_clk);
-	clk_disable_unprepare(priv->mg_clk);
-	clk_disable_unprepare(priv->pp_clk);
-	clk_disable_unprepare(priv->gop_clk);
 }
 
 static const struct of_device_id mvpp2_match[] = {
-- 
2.39.0

Re: [net-next v3 8/9] net: mvpp2: Convert to devm_clk_get_enabled() and devm_clk_get_optional_enabled()

[Jonathan Cameron]

On Tue, 27 Aug 2024 03:57:11 -0600
Yangtao Li <frank.li@vivo.com> wrote:

> Use devm_clk_get_enabled() and devm_clk_get_optional_enabled()
> to simplify code.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Suggested-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Reviewed-by: Marcin Wojtas <marcin.s.wojtas@gmail.com>

>  
> @@ -7745,12 +7710,6 @@ static void mvpp2_remove(struct platform_device *pdev)
>  
>  	if (!dev_of_node(&pdev->dev))
>  		return;

Given this makes no difference any more, drop the above dev_of_node() check.

> -
> -	clk_disable_unprepare(priv->axi_clk);
> -	clk_disable_unprepare(priv->mg_core_clk);
> -	clk_disable_unprepare(priv->mg_clk);
> -	clk_disable_unprepare(priv->pp_clk);
> -	clk_disable_unprepare(priv->gop_clk);
>  }
>  
>  static const struct of_device_id mvpp2_match[] = {

Re: [net-next v3 8/9] net: mvpp2: Convert to devm_clk_get_enabled() and devm_clk_get_optional_enabled()

[Marcin Wojtas]

Hi Jonathan,

wt., 27 sie 2024 o 13:09 Jonathan Cameron
<Jonathan.Cameron@huawei.com> napisał(a):
>
> On Tue, 27 Aug 2024 03:57:11 -0600
> Yangtao Li <frank.li@vivo.com> wrote:
>
> > Use devm_clk_get_enabled() and devm_clk_get_optional_enabled()
> > to simplify code.
> >
> > Signed-off-by: Yangtao Li <frank.li@vivo.com>
> > Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > Suggested-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > Reviewed-by: Marcin Wojtas <marcin.s.wojtas@gmail.com>
>
> >
> > @@ -7745,12 +7710,6 @@ static void mvpp2_remove(struct platform_device *pdev)
> >
> >       if (!dev_of_node(&pdev->dev))
> >               return;
>
> Given this makes no difference any more, drop the above dev_of_node() check.
>

This check is to not execute the clk-related code when booting with
ACPI. It should remain as-is, unless the new devm_clk_get* api is
capable of not exploding in non-DT case. Can you confirm?

Best regards,
Marcin

> > -
> > -     clk_disable_unprepare(priv->axi_clk);
> > -     clk_disable_unprepare(priv->mg_core_clk);
> > -     clk_disable_unprepare(priv->mg_clk);
> > -     clk_disable_unprepare(priv->pp_clk);
> > -     clk_disable_unprepare(priv->gop_clk);
> >  }
> >
> >  static const struct of_device_id mvpp2_match[] = {
>

Re: [net-next v3 8/9] net: mvpp2: Convert to devm_clk_get_enabled() and devm_clk_get_optional_enabled()

[Geert Uytterhoeven]

Hi Marcin,

On Wed, Aug 28, 2024 at 8:26 AM Marcin Wojtas <marcin.s.wojtas@gmail.com> wrote:
> wt., 27 sie 2024 o 13:09 Jonathan Cameron
> <Jonathan.Cameron@huawei.com> napisał(a):
> > On Tue, 27 Aug 2024 03:57:11 -0600
> > Yangtao Li <frank.li@vivo.com> wrote:
> > > Use devm_clk_get_enabled() and devm_clk_get_optional_enabled()
> > > to simplify code.
> > >
> > > Signed-off-by: Yangtao Li <frank.li@vivo.com>
> > > Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > Suggested-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > Reviewed-by: Marcin Wojtas <marcin.s.wojtas@gmail.com>
> >
> > >
> > > @@ -7745,12 +7710,6 @@ static void mvpp2_remove(struct platform_device *pdev)
> > >
> > >       if (!dev_of_node(&pdev->dev))
> > >               return;
> >
> > Given this makes no difference any more, drop the above dev_of_node() check.
>
> This check is to not execute the clk-related code when booting with
> ACPI. It should remain as-is, unless the new devm_clk_get* api is
> capable of not exploding in non-DT case. Can you confirm?

As per the removals below, there is no code left in this function after
the check (i.e. the "else" part became empty).

> > > -
> > > -     clk_disable_unprepare(priv->axi_clk);
> > > -     clk_disable_unprepare(priv->mg_core_clk);
> > > -     clk_disable_unprepare(priv->mg_clk);
> > > -     clk_disable_unprepare(priv->pp_clk);
> > > -     clk_disable_unprepare(priv->gop_clk);
> > >  }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [net-next v3 8/9] net: mvpp2: Convert to devm_clk_get_enabled() and devm_clk_get_optional_enabled()

[Marcin Wojtas]

śr., 28 sie 2024 o 09:13 Geert Uytterhoeven <geert@linux-m68k.org> napisał(a):
>
> Hi Marcin,
>
> On Wed, Aug 28, 2024 at 8:26 AM Marcin Wojtas <marcin.s.wojtas@gmail.com> wrote:
> > wt., 27 sie 2024 o 13:09 Jonathan Cameron
> > <Jonathan.Cameron@huawei.com> napisał(a):
> > > On Tue, 27 Aug 2024 03:57:11 -0600
> > > Yangtao Li <frank.li@vivo.com> wrote:
> > > > Use devm_clk_get_enabled() and devm_clk_get_optional_enabled()
> > > > to simplify code.
> > > >
> > > > Signed-off-by: Yangtao Li <frank.li@vivo.com>
> > > > Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > > Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > > Suggested-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > > Reviewed-by: Marcin Wojtas <marcin.s.wojtas@gmail.com>
> > >
> > > >
> > > > @@ -7745,12 +7710,6 @@ static void mvpp2_remove(struct platform_device *pdev)
> > > >
> > > >       if (!dev_of_node(&pdev->dev))
> > > >               return;
> > >
> > > Given this makes no difference any more, drop the above dev_of_node() check.
> >
> > This check is to not execute the clk-related code when booting with
> > ACPI. It should remain as-is, unless the new devm_clk_get* api is
> > capable of not exploding in non-DT case. Can you confirm?
>
> As per the removals below, there is no code left in this function after
> the check (i.e. the "else" part became empty).
>
> > > > -
> > > > -     clk_disable_unprepare(priv->axi_clk);
> > > > -     clk_disable_unprepare(priv->mg_core_clk);
> > > > -     clk_disable_unprepare(priv->mg_clk);
> > > > -     clk_disable_unprepare(priv->pp_clk);
> > > > -     clk_disable_unprepare(priv->gop_clk);
> > > >  }
>
> Gr{oetje,eeting}s,
>
>                         Geert
>

You are right, I misread that it was mvpp2_probe :) I confirm, we can
remove in mvpp2_remove.

Best regards,
Marcin

Re: [net-next v3 8/9] net: mvpp2: Convert to devm_clk_get_enabled() and devm_clk_get_optional_enabled()

[Simon Horman]

On Tue, Aug 27, 2024 at 03:57:11AM -0600, Yangtao Li wrote:
> Use devm_clk_get_enabled() and devm_clk_get_optional_enabled()
> to simplify code.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Suggested-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Reviewed-by: Marcin Wojtas <marcin.s.wojtas@gmail.com>

...

> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 2fe8bae4eb3c..0ca2daeb0f90 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -7561,56 +7561,32 @@ static int mvpp2_probe(struct platform_device *pdev)

...

> -		priv->axi_clk = devm_clk_get_optional(&pdev->dev, "axi_clk");
> -		if (IS_ERR(priv->axi_clk)) {
> -			err = PTR_ERR(priv->axi_clk);
> -			goto err_mg_core_clk;
> +			clk = devm_clk_get_optional_enabled(&pdev->dev, "mg_core_clk");

As it looks like there will be a v3 anyway, a minor nit from my side:
IMHO, the line above could be trivially wrapped to keep it <= 80 columns wide,
which is still preferred by Networking code.

> +			if (IS_ERR(clk))
> +				return PTR_ERR(clk);
>  		}
>  
> -		err = clk_prepare_enable(priv->axi_clk);
> -		if (err < 0)
> -			goto err_mg_core_clk;
> -
> -		/* Get system's tclk rate */
> -		priv->tclk = clk_get_rate(priv->pp_clk);
> +		clk = devm_clk_get_optional_enabled(&pdev->dev, "axi_clk");
> +		if (IS_ERR(clk))
> +			return PTR_ERR(clk);

...

[net-next v3 9/9] net: marvell: pxa168_eth: Convert to devm_clk_get_enabled()

[Yangtao Li]

Convert devm_clk_get(), clk_prepare_enable() to a single
call to devm_clk_get_enabled(), as this is exactly
what this function does.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 drivers/net/ethernet/marvell/pxa168_eth.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
index 1a59c952aa01..bad91cc705e8 100644
--- a/drivers/net/ethernet/marvell/pxa168_eth.c
+++ b/drivers/net/ethernet/marvell/pxa168_eth.c
@@ -237,8 +237,6 @@ struct pxa168_eth_private {
 	struct timer_list timeout;
 	struct mii_bus *smi_bus;
 
-	/* clock */
-	struct clk *clk;
 	struct pxa168_eth_platform_data *pd;
 	/*
 	 * Ethernet controller base address.
@@ -1394,23 +1392,19 @@ static int pxa168_eth_probe(struct platform_device *pdev)
 
 	printk(KERN_NOTICE "PXA168 10/100 Ethernet Driver\n");
 
-	clk = devm_clk_get(&pdev->dev, NULL);
+	clk = devm_clk_get_enabled(&pdev->dev, NULL);
 	if (IS_ERR(clk)) {
 		dev_err(&pdev->dev, "Fast Ethernet failed to get clock\n");
-		return -ENODEV;
+		return PTR_ERR(clk);
 	}
-	clk_prepare_enable(clk);
 
 	dev = alloc_etherdev(sizeof(struct pxa168_eth_private));
-	if (!dev) {
-		err = -ENOMEM;
-		goto err_clk;
-	}
+	if (!dev)
+		return -ENOMEM;
 
 	platform_set_drvdata(pdev, dev);
 	pep = netdev_priv(dev);
 	pep->dev = dev;
-	pep->clk = clk;
 
 	pep->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pep->base)) {
@@ -1523,8 +1517,6 @@ static int pxa168_eth_probe(struct platform_device *pdev)
 	mdiobus_free(pep->smi_bus);
 err_netdev:
 	free_netdev(dev);
-err_clk:
-	clk_disable_unprepare(clk);
 	return err;
 }
 
@@ -1542,7 +1534,6 @@ static void pxa168_eth_remove(struct platform_device *pdev)
 	if (dev->phydev)
 		phy_disconnect(dev->phydev);
 
-	clk_disable_unprepare(pep->clk);
 	mdiobus_unregister(pep->smi_bus);
 	mdiobus_free(pep->smi_bus);
 	unregister_netdev(dev);
-- 
2.39.0

Re: [net-next v3 9/9] net: marvell: pxa168_eth: Convert to devm_clk_get_enabled()

[Jonathan Cameron]

On Tue, 27 Aug 2024 03:57:12 -0600
Yangtao Li <frank.li@vivo.com> wrote:

> Convert devm_clk_get(), clk_prepare_enable() to a single
> call to devm_clk_get_enabled(), as this is exactly
> what this function does.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
Ordering in here is already 'interesting' but I'd still look
at more devm_ calls for the mdio and netdev parts.

> ---
>  drivers/net/ethernet/marvell/pxa168_eth.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
> index 1a59c952aa01..bad91cc705e8 100644
> --- a/drivers/net/ethernet/marvell/pxa168_eth.c
> +++ b/drivers/net/ethernet/marvell/pxa168_eth.c
> @@ -237,8 +237,6 @@ struct pxa168_eth_private {
>  	struct timer_list timeout;
>  	struct mii_bus *smi_bus;
>  
> -	/* clock */
> -	struct clk *clk;
>  	struct pxa168_eth_platform_data *pd;
>  	/*
>  	 * Ethernet controller base address.
> @@ -1394,23 +1392,19 @@ static int pxa168_eth_probe(struct platform_device *pdev)
>  
>  	printk(KERN_NOTICE "PXA168 10/100 Ethernet Driver\n");
>  
> -	clk = devm_clk_get(&pdev->dev, NULL);
> +	clk = devm_clk_get_enabled(&pdev->dev, NULL);
>  	if (IS_ERR(clk)) {
>  		dev_err(&pdev->dev, "Fast Ethernet failed to get clock\n");
> -		return -ENODEV;
> +		return PTR_ERR(clk);
>  	}
> -	clk_prepare_enable(clk);
>  
>  	dev = alloc_etherdev(sizeof(struct pxa168_eth_private));
> -	if (!dev) {
> -		err = -ENOMEM;
> -		goto err_clk;
> -	}
> +	if (!dev)
> +		return -ENOMEM;
>  
>  	platform_set_drvdata(pdev, dev);
>  	pep = netdev_priv(dev);
>  	pep->dev = dev;
> -	pep->clk = clk;
>  
>  	pep->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(pep->base)) {
> @@ -1523,8 +1517,6 @@ static int pxa168_eth_probe(struct platform_device *pdev)
>  	mdiobus_free(pep->smi_bus);
>  err_netdev:
>  	free_netdev(dev);
> -err_clk:
> -	clk_disable_unprepare(clk);
>  	return err;
>  }
>  
> @@ -1542,7 +1534,6 @@ static void pxa168_eth_remove(struct platform_device *pdev)
>  	if (dev->phydev)
>  		phy_disconnect(dev->phydev);
>  
> -	clk_disable_unprepare(pep->clk);
>  	mdiobus_unregister(pep->smi_bus);
>  	mdiobus_free(pep->smi_bus);
>  	unregister_netdev(dev);