diff mbox series

[net-next,v2,2/4] net: ethernet: renesas: rswitch: Simplify struct phy * handling

Message ID 20230110050206.116110-3-yoshihiro.shimoda.uh@renesas.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: ethernet: renesas: rswitch: Modify initialization for SERDES and PHY | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 1 maintainers not CCed: s.shtylyov@omp.ru
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 86 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Yoshihiro Shimoda Jan. 10, 2023, 5:02 a.m. UTC 
Simplify struct phy *serdes handling by keeping the valiable in
the struct rswitch_device.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/net/ethernet/renesas/rswitch.c | 40 ++++++++++++--------------
 drivers/net/ethernet/renesas/rswitch.h |  1 +
 2 files changed, 19 insertions(+), 22 deletions(-)

Comments

Arun Ramadoss Jan. 10, 2023, 2:15 p.m. UTC | #1 
Hi Yoshihiro,
On Tue, 2023-01-10 at 14:02 +0900, Yoshihiro Shimoda wrote:
> Simplify struct phy *serdes handling by keeping the valiable in
> the struct rswitch_device.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/net/ethernet/renesas/rswitch.c | 40 ++++++++++++----------
> ----
>  drivers/net/ethernet/renesas/rswitch.h |  1 +
>  2 files changed, 19 insertions(+), 22 deletions(-)
> 
> 
>  
> -static int rswitch_serdes_set_params(struct rswitch_device *rdev)
> 
>  
>  static int rswitch_ether_port_init_one(struct rswitch_device *rdev)
> @@ -1299,6 +1290,10 @@ static int rswitch_ether_port_init_one(struct
> rswitch_device *rdev)
>  	if (err < 0)
>  		goto err_phylink_init;
>  
> +	err = rswitch_serdes_phy_get(rdev);
> +	if (err < 0)
> +		goto err_serdes_phy_get;

I think, we can use *err_serdes_set_params* instead of creating new
label err_serdes_phy_get, since the label is not doing any work.

> +
>  	err = rswitch_serdes_set_params(rdev);
>  	if (err < 0)
>  		goto err_serdes_set_params;
> @@ -1306,6 +1301,7 @@ static int rswitch_ether_port_init_one(struct
> rswitch_device *rdev)
>  	return 0;
>  
>  err_serdes_set_params:
> +err_serdes_phy_get:
>  	rswitch_phylink_deinit(rdev);
>  
>  err_phylink_init:
> diff --git a/drivers/net/ethernet/renesas/rswitch.h
> b/drivers/net/ethernet/renesas/rswitch.h
> index edbdd1b98d3d..d9a0be6666f5 100644
> --- a/drivers/net/ethernet/renesas/rswitch.h
> +++ b/drivers/net/ethernet/renesas/rswitch.h
> @@ -941,6 +941,7 @@ struct rswitch_device {
>  
>  	int port;
>  	struct rswitch_etha *etha;
> +	struct phy *serdes;
>  };
>  
>  struct rswitch_mfwd_mac_table_entry {
Yoshihiro Shimoda Jan. 12, 2023, 7:32 a.m. UTC | #2 
Hi Arun,

> From: Arun.Ramadoss@microchip.com, Sent: Tuesday, January 10, 2023 11:15 PM
> 
> Hi Yoshihiro,
> On Tue, 2023-01-10 at 14:02 +0900, Yoshihiro Shimoda wrote:
> > Simplify struct phy *serdes handling by keeping the valiable in
> > the struct rswitch_device.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/net/ethernet/renesas/rswitch.c | 40 ++++++++++++----------
> > ----
> >  drivers/net/ethernet/renesas/rswitch.h |  1 +
> >  2 files changed, 19 insertions(+), 22 deletions(-)
> >
> >
> >
> > -static int rswitch_serdes_set_params(struct rswitch_device *rdev)
> >
> >
> >  static int rswitch_ether_port_init_one(struct rswitch_device *rdev)
> > @@ -1299,6 +1290,10 @@ static int rswitch_ether_port_init_one(struct
> > rswitch_device *rdev)
> >  	if (err < 0)
> >  		goto err_phylink_init;
> >
> > +	err = rswitch_serdes_phy_get(rdev);
> > +	if (err < 0)
> > +		goto err_serdes_phy_get;
> 
> I think, we can use *err_serdes_set_params* instead of creating new
> label err_serdes_phy_get, since the label is not doing any work.

Thank you for your comment. However, this driver already has a similar
label and error handling like below:
---
        err = rswitch_gwca_request_irqs(priv);
        if (err < 0)
                goto err_gwca_request_irq;

        err = rswitch_gwca_hw_init(priv);
        if (err < 0)
                goto err_gwca_hw_init;
...
err_gwca_hw_init:
err_gwca_request_irq:
        rcar_gen4_ptp_unregister(priv->ptp_priv);
---

The err_ labels are related to the functions.
So, I think keeping same function names/label names is
better for readability.

Best regards,
Yoshihiro Shimoda

> > +
> >  	err = rswitch_serdes_set_params(rdev);
> >  	if (err < 0)
> >  		goto err_serdes_set_params;
> > @@ -1306,6 +1301,7 @@ static int rswitch_ether_port_init_one(struct
> > rswitch_device *rdev)
> >  	return 0;
> >
> >  err_serdes_set_params:
> > +err_serdes_phy_get:
> >  	rswitch_phylink_deinit(rdev);
> >
> >  err_phylink_init:
> > diff --git a/drivers/net/ethernet/renesas/rswitch.h
> > b/drivers/net/ethernet/renesas/rswitch.h
> > index edbdd1b98d3d..d9a0be6666f5 100644
> > --- a/drivers/net/ethernet/renesas/rswitch.h
> > +++ b/drivers/net/ethernet/renesas/rswitch.h
> > @@ -941,6 +941,7 @@ struct rswitch_device {
> >
> >  	int port;
> >  	struct rswitch_etha *etha;
> > +	struct phy *serdes;
> >  };
> >
> >  struct rswitch_mfwd_mac_table_entry {
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index 6441892636db..ca79ee168206 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -1235,49 +1235,40 @@  static void rswitch_phylink_deinit(struct rswitch_device *rdev)
 	phylink_destroy(rdev->phylink);
 }
 
-static int rswitch_serdes_set_params(struct rswitch_device *rdev)
+static int rswitch_serdes_phy_get(struct rswitch_device *rdev)
 {
 	struct device_node *port = rswitch_get_port_node(rdev);
 	struct phy *serdes;
-	int err;
 
 	serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL);
 	of_node_put(port);
 	if (IS_ERR(serdes))
 		return PTR_ERR(serdes);
+	rdev->serdes = serdes;
+
+	return 0;
+}
+
+static int rswitch_serdes_set_params(struct rswitch_device *rdev)
+{
+	int err;
 
-	err = phy_set_mode_ext(serdes, PHY_MODE_ETHERNET,
+	err = phy_set_mode_ext(rdev->serdes, PHY_MODE_ETHERNET,
 			       rdev->etha->phy_interface);
 	if (err < 0)
 		return err;
 
-	return phy_set_speed(serdes, rdev->etha->speed);
+	return phy_set_speed(rdev->serdes, rdev->etha->speed);
 }
 
 static int rswitch_serdes_init(struct rswitch_device *rdev)
 {
-	struct device_node *port = rswitch_get_port_node(rdev);
-	struct phy *serdes;
-
-	serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL);
-	of_node_put(port);
-	if (IS_ERR(serdes))
-		return PTR_ERR(serdes);
-
-	return phy_init(serdes);
+	return phy_init(rdev->serdes);
 }
 
 static int rswitch_serdes_deinit(struct rswitch_device *rdev)
 {
-	struct device_node *port = rswitch_get_port_node(rdev);
-	struct phy *serdes;
-
-	serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL);
-	of_node_put(port);
-	if (IS_ERR(serdes))
-		return PTR_ERR(serdes);
-
-	return phy_exit(serdes);
+	return phy_exit(rdev->serdes);
 }
 
 static int rswitch_ether_port_init_one(struct rswitch_device *rdev)
@@ -1299,6 +1290,10 @@  static int rswitch_ether_port_init_one(struct rswitch_device *rdev)
 	if (err < 0)
 		goto err_phylink_init;
 
+	err = rswitch_serdes_phy_get(rdev);
+	if (err < 0)
+		goto err_serdes_phy_get;
+
 	err = rswitch_serdes_set_params(rdev);
 	if (err < 0)
 		goto err_serdes_set_params;
@@ -1306,6 +1301,7 @@  static int rswitch_ether_port_init_one(struct rswitch_device *rdev)
 	return 0;
 
 err_serdes_set_params:
+err_serdes_phy_get:
 	rswitch_phylink_deinit(rdev);
 
 err_phylink_init:
diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h
index edbdd1b98d3d..d9a0be6666f5 100644
--- a/drivers/net/ethernet/renesas/rswitch.h
+++ b/drivers/net/ethernet/renesas/rswitch.h
@@ -941,6 +941,7 @@  struct rswitch_device {
 
 	int port;
 	struct rswitch_etha *etha;
+	struct phy *serdes;
 };
 
 struct rswitch_mfwd_mac_table_entry {