linux-can.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] can: dev: add transceiver capabilities to xilinx_can
@ 2023-04-17  8:52 Marcel Hellwig
  2023-05-22  7:24 ` git
  2023-05-22  7:58 ` Vincent MAILHOL
  0 siblings, 2 replies; 6+ messages in thread
From: Marcel Hellwig @ 2023-04-17  8:52 UTC (permalink / raw)
  To: Appana Durga Kedareswara rao, Naga Sureshkumar Relli, linux-can, netdev
  Cc: Marcel Hellwig, Marcel Hellwig

Currently the xilinx_can driver does not support adding a phy like the
"ti,tcan1043" to its devicetree.

This code makes it possible to add such phy, so that the kernel makes
sure that the PHY is in operational state, when the link is set to an
"up" state.

Signed-off-by: Marcel Hellwig <git@cookiesoft.de>
---
 drivers/net/can/xilinx_can.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 43c812ea1de0..6a5b805d579a 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -28,6 +28,7 @@
 #include <linux/types.h>
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
+#include <linux/phy/phy.h>
 #include <linux/pm_runtime.h>
 
 #define DRIVER_NAME	"xilinx_can"
@@ -215,6 +216,7 @@ struct xcan_priv {
 	struct clk *bus_clk;
 	struct clk *can_clk;
 	struct xcan_devtype_data devtype;
+	struct phy *transceiver;
 };
 
 /* CAN Bittiming constants as per Xilinx CAN specs */
@@ -1419,6 +1421,12 @@ static int xcan_open(struct net_device *ndev)
 	struct xcan_priv *priv = netdev_priv(ndev);
 	int ret;
 
+	ret = phy_power_on(priv->transceiver);
+	if (ret) {
+		netdev_err(ndev, "%s: phy_power_on failed(%d)\n", __func__, ret);
+		return ret;
+	}
+
 	ret = pm_runtime_get_sync(priv->dev);
 	if (ret < 0) {
 		netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
@@ -1461,6 +1469,7 @@ static int xcan_open(struct net_device *ndev)
 err_irq:
 	free_irq(ndev->irq, ndev);
 err:
+	phy_power_off(priv->transceiver);
 	pm_runtime_put(priv->dev);
 
 	return ret;
@@ -1482,6 +1491,7 @@ static int xcan_close(struct net_device *ndev)
 	free_irq(ndev->irq, ndev);
 	close_candev(ndev);
 
+	phy_power_off(priv->transceiver);
 	pm_runtime_put(priv->dev);
 
 	return 0;
@@ -1713,6 +1723,7 @@ static int xcan_probe(struct platform_device *pdev)
 {
 	struct net_device *ndev;
 	struct xcan_priv *priv;
+	struct phy *transceiver;
 	const struct of_device_id *of_id;
 	const struct xcan_devtype_data *devtype = &xcan_axi_data;
 	void __iomem *addr;
@@ -1843,6 +1854,14 @@ static int xcan_probe(struct platform_device *pdev)
 		goto err_free;
 	}
 
+	transceiver = devm_phy_optional_get(&pdev->dev, NULL);
+	if (IS_ERR(transceiver)) {
+		ret = PTR_ERR(transceiver);
+		dev_err_probe(&pdev->dev, ret, "failed to get phy\n");
+		goto err_free;
+	}
+	priv->transceiver = transceiver;
+
 	priv->write_reg = xcan_write_reg_le;
 	priv->read_reg = xcan_read_reg_le;
 
@@ -1869,6 +1888,7 @@ static int xcan_probe(struct platform_device *pdev)
 		goto err_disableclks;
 	}
 
+	of_can_transceiver(ndev);
 	pm_runtime_put(&pdev->dev);
 
 	if (priv->devtype.flags & XCAN_FLAG_CANFD_2) {
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] can: dev: add transceiver capabilities to xilinx_can
  2023-04-17  8:52 [PATCH] can: dev: add transceiver capabilities to xilinx_can Marcel Hellwig
@ 2023-05-22  7:24 ` git
  2023-05-22  8:00   ` Marc Kleine-Budde
  2023-05-22  7:58 ` Vincent MAILHOL
  1 sibling, 1 reply; 6+ messages in thread
From: git @ 2023-05-22  7:24 UTC (permalink / raw)
  To: Appana Durga Kedareswara rao, Naga Sureshkumar Relli, linux-can, netdev
  Cc: Marcel Hellwig

Hey everyone,

is there anything I can do to get this merged?
Is there anything missing?

Greetings,
Marcel

> Marcel Hellwig <git@cookiesoft.de> hat am 17.04.2023 10:52 CEST geschrieben:
> 
>  
> Currently the xilinx_can driver does not support adding a phy like the
> "ti,tcan1043" to its devicetree.
> 
> This code makes it possible to add such phy, so that the kernel makes
> sure that the PHY is in operational state, when the link is set to an
> "up" state.
> 
> Signed-off-by: Marcel Hellwig <git@cookiesoft.de>
> ---
>  drivers/net/can/xilinx_can.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 43c812ea1de0..6a5b805d579a 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -28,6 +28,7 @@
>  #include <linux/types.h>
>  #include <linux/can/dev.h>
>  #include <linux/can/error.h>
> +#include <linux/phy/phy.h>
>  #include <linux/pm_runtime.h>
>  
>  #define DRIVER_NAME	"xilinx_can"
> @@ -215,6 +216,7 @@ struct xcan_priv {
>  	struct clk *bus_clk;
>  	struct clk *can_clk;
>  	struct xcan_devtype_data devtype;
> +	struct phy *transceiver;
>  };
>  
>  /* CAN Bittiming constants as per Xilinx CAN specs */
> @@ -1419,6 +1421,12 @@ static int xcan_open(struct net_device *ndev)
>  	struct xcan_priv *priv = netdev_priv(ndev);
>  	int ret;
>  
> +	ret = phy_power_on(priv->transceiver);
> +	if (ret) {
> +		netdev_err(ndev, "%s: phy_power_on failed(%d)\n", __func__, ret);
> +		return ret;
> +	}
> +
>  	ret = pm_runtime_get_sync(priv->dev);
>  	if (ret < 0) {
>  		netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
> @@ -1461,6 +1469,7 @@ static int xcan_open(struct net_device *ndev)
>  err_irq:
>  	free_irq(ndev->irq, ndev);
>  err:
> +	phy_power_off(priv->transceiver);
>  	pm_runtime_put(priv->dev);
>  
>  	return ret;
> @@ -1482,6 +1491,7 @@ static int xcan_close(struct net_device *ndev)
>  	free_irq(ndev->irq, ndev);
>  	close_candev(ndev);
>  
> +	phy_power_off(priv->transceiver);
>  	pm_runtime_put(priv->dev);
>  
>  	return 0;
> @@ -1713,6 +1723,7 @@ static int xcan_probe(struct platform_device *pdev)
>  {
>  	struct net_device *ndev;
>  	struct xcan_priv *priv;
> +	struct phy *transceiver;
>  	const struct of_device_id *of_id;
>  	const struct xcan_devtype_data *devtype = &xcan_axi_data;
>  	void __iomem *addr;
> @@ -1843,6 +1854,14 @@ static int xcan_probe(struct platform_device *pdev)
>  		goto err_free;
>  	}
>  
> +	transceiver = devm_phy_optional_get(&pdev->dev, NULL);
> +	if (IS_ERR(transceiver)) {
> +		ret = PTR_ERR(transceiver);
> +		dev_err_probe(&pdev->dev, ret, "failed to get phy\n");
> +		goto err_free;
> +	}
> +	priv->transceiver = transceiver;
> +
>  	priv->write_reg = xcan_write_reg_le;
>  	priv->read_reg = xcan_read_reg_le;
>  
> @@ -1869,6 +1888,7 @@ static int xcan_probe(struct platform_device *pdev)
>  		goto err_disableclks;
>  	}
>  
> +	of_can_transceiver(ndev);
>  	pm_runtime_put(&pdev->dev);
>  
>  	if (priv->devtype.flags & XCAN_FLAG_CANFD_2) {
> -- 
> 2.34.1

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] can: dev: add transceiver capabilities to xilinx_can
  2023-04-17  8:52 [PATCH] can: dev: add transceiver capabilities to xilinx_can Marcel Hellwig
  2023-05-22  7:24 ` git
@ 2023-05-22  7:58 ` Vincent MAILHOL
  2023-05-22  8:03   ` Marc Kleine-Budde
  1 sibling, 1 reply; 6+ messages in thread
From: Vincent MAILHOL @ 2023-05-22  7:58 UTC (permalink / raw)
  To: Marcel Hellwig
  Cc: Appana Durga Kedareswara rao, Naga Sureshkumar Relli, linux-can,
	netdev, Marcel Hellwig

Hi Marcel,

Style check only (I am not yet familiar enough with the devicetree).

On Mon. 17 Apr. 2023 at 18:01, Marcel Hellwig <git@cookiesoft.de> wrote:
> Currently the xilinx_can driver does not support adding a phy like the
> "ti,tcan1043" to its devicetree.
>
> This code makes it possible to add such phy, so that the kernel makes
> sure that the PHY is in operational state, when the link is set to an
> "up" state.
>
> Signed-off-by: Marcel Hellwig <git@cookiesoft.de>
> ---
>  drivers/net/can/xilinx_can.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 43c812ea1de0..6a5b805d579a 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -28,6 +28,7 @@
>  #include <linux/types.h>
>  #include <linux/can/dev.h>
>  #include <linux/can/error.h>
> +#include <linux/phy/phy.h>
>  #include <linux/pm_runtime.h>
>
>  #define DRIVER_NAME    "xilinx_can"
> @@ -215,6 +216,7 @@ struct xcan_priv {
>         struct clk *bus_clk;
>         struct clk *can_clk;
>         struct xcan_devtype_data devtype;
> +       struct phy *transceiver;
>  };
>
>  /* CAN Bittiming constants as per Xilinx CAN specs */
> @@ -1419,6 +1421,12 @@ static int xcan_open(struct net_device *ndev)
>         struct xcan_priv *priv = netdev_priv(ndev);
>         int ret;
>
> +       ret = phy_power_on(priv->transceiver);
> +       if (ret) {
> +               netdev_err(ndev, "%s: phy_power_on failed(%d)\n", __func__, ret);

From the Linux kernel coding style:

  Printing numbers in parentheses (%d) adds no value and should be avoided.
  Link: https://www.kernel.org/doc/html/latest/process/coding-style.html#printing-kernel-messages

Also consider %pe to print the mnemotechnic instead of the value:

          netdev_err(ndev, "%s: phy_power_on failed: %pe\n", __func__,
ERR_PTR(ret));

> +               return ret;
> +       }
> +
>         ret = pm_runtime_get_sync(priv->dev);
>         if (ret < 0) {
>                 netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",

It is up to you, but bonus points if you send a clean-up patch to fix
the existing log messages.

> @@ -1461,6 +1469,7 @@ static int xcan_open(struct net_device *ndev)
>  err_irq:
>         free_irq(ndev->irq, ndev);
>  err:
> +       phy_power_off(priv->transceiver);
>         pm_runtime_put(priv->dev);
>
>         return ret;
> @@ -1482,6 +1491,7 @@ static int xcan_close(struct net_device *ndev)
>         free_irq(ndev->irq, ndev);
>         close_candev(ndev);
>
> +       phy_power_off(priv->transceiver);
>         pm_runtime_put(priv->dev);
>
>         return 0;
> @@ -1713,6 +1723,7 @@ static int xcan_probe(struct platform_device *pdev)
>  {
>         struct net_device *ndev;
>         struct xcan_priv *priv;
> +       struct phy *transceiver;
>         const struct of_device_id *of_id;
>         const struct xcan_devtype_data *devtype = &xcan_axi_data;
>         void __iomem *addr;
> @@ -1843,6 +1854,14 @@ static int xcan_probe(struct platform_device *pdev)
>                 goto err_free;
>         }
>
> +       transceiver = devm_phy_optional_get(&pdev->dev, NULL);
> +       if (IS_ERR(transceiver)) {
> +               ret = PTR_ERR(transceiver);
> +               dev_err_probe(&pdev->dev, ret, "failed to get phy\n");
> +               goto err_free;
> +       }
> +       priv->transceiver = transceiver;
> +
>         priv->write_reg = xcan_write_reg_le;
>         priv->read_reg = xcan_read_reg_le;
>
> @@ -1869,6 +1888,7 @@ static int xcan_probe(struct platform_device *pdev)
>                 goto err_disableclks;
>         }
>
> +       of_can_transceiver(ndev);
>         pm_runtime_put(&pdev->dev);
>
>         if (priv->devtype.flags & XCAN_FLAG_CANFD_2) {
> --
> 2.34.1
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] can: dev: add transceiver capabilities to xilinx_can
  2023-05-22  7:24 ` git
@ 2023-05-22  8:00   ` Marc Kleine-Budde
  2023-05-22  8:23     ` Marc Kleine-Budde
  0 siblings, 1 reply; 6+ messages in thread
From: Marc Kleine-Budde @ 2023-05-22  8:00 UTC (permalink / raw)
  To: git
  Cc: Appana Durga Kedareswara rao, Naga Sureshkumar Relli, linux-can,
	netdev, Marcel Hellwig

[-- Attachment #1: Type: text/plain, Size: 575 bytes --]

On 22.05.2023 09:24:17, git@cookiesoft.de wrote:
> Hey everyone,
> 
> is there anything I can do to get this merged?
> Is there anything missing?

Looks good, while applying I'v moved the phy_power_off() after the
pm_runtime_put() to make it symmetric with respect to xcan_open().

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] can: dev: add transceiver capabilities to xilinx_can
  2023-05-22  7:58 ` Vincent MAILHOL
@ 2023-05-22  8:03   ` Marc Kleine-Budde
  0 siblings, 0 replies; 6+ messages in thread
From: Marc Kleine-Budde @ 2023-05-22  8:03 UTC (permalink / raw)
  To: Vincent MAILHOL
  Cc: Marcel Hellwig, Appana Durga Kedareswara rao,
	Naga Sureshkumar Relli, linux-can, netdev, Marcel Hellwig

[-- Attachment #1: Type: text/plain, Size: 2561 bytes --]

On 22.05.2023 16:58:02, Vincent MAILHOL wrote:
> Hi Marcel,
> 
> Style check only (I am not yet familiar enough with the devicetree).
> 
> On Mon. 17 Apr. 2023 at 18:01, Marcel Hellwig <git@cookiesoft.de> wrote:
> > Currently the xilinx_can driver does not support adding a phy like the
> > "ti,tcan1043" to its devicetree.
> >
> > This code makes it possible to add such phy, so that the kernel makes
> > sure that the PHY is in operational state, when the link is set to an
> > "up" state.
> >
> > Signed-off-by: Marcel Hellwig <git@cookiesoft.de>
> > ---
> >  drivers/net/can/xilinx_can.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> > index 43c812ea1de0..6a5b805d579a 100644
> > --- a/drivers/net/can/xilinx_can.c
> > +++ b/drivers/net/can/xilinx_can.c
> > @@ -28,6 +28,7 @@
> >  #include <linux/types.h>
> >  #include <linux/can/dev.h>
> >  #include <linux/can/error.h>
> > +#include <linux/phy/phy.h>
> >  #include <linux/pm_runtime.h>
> >
> >  #define DRIVER_NAME    "xilinx_can"
> > @@ -215,6 +216,7 @@ struct xcan_priv {
> >         struct clk *bus_clk;
> >         struct clk *can_clk;
> >         struct xcan_devtype_data devtype;
> > +       struct phy *transceiver;
> >  };
> >
> >  /* CAN Bittiming constants as per Xilinx CAN specs */
> > @@ -1419,6 +1421,12 @@ static int xcan_open(struct net_device *ndev)
> >         struct xcan_priv *priv = netdev_priv(ndev);
> >         int ret;
> >
> > +       ret = phy_power_on(priv->transceiver);
> > +       if (ret) {
> > +               netdev_err(ndev, "%s: phy_power_on failed(%d)\n", __func__, ret);
> 
> From the Linux kernel coding style:
> 
>   Printing numbers in parentheses (%d) adds no value and should be avoided.
>   Link: https://www.kernel.org/doc/html/latest/process/coding-style.html#printing-kernel-messages
> 
> Also consider %pe to print the mnemotechnic instead of the value:
> 
>           netdev_err(ndev, "%s: phy_power_on failed: %pe\n", __func__,
> ERR_PTR(ret));

Good point. phy_power_on() already writes an error message:

| https://elixir.bootlin.com/linux/v6.3/source/drivers/phy/phy-core.c#L343

I'll remove that from this patch.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] can: dev: add transceiver capabilities to xilinx_can
  2023-05-22  8:00   ` Marc Kleine-Budde
@ 2023-05-22  8:23     ` Marc Kleine-Budde
  0 siblings, 0 replies; 6+ messages in thread
From: Marc Kleine-Budde @ 2023-05-22  8:23 UTC (permalink / raw)
  To: git
  Cc: Appana Durga Kedareswara rao, Naga Sureshkumar Relli, linux-can,
	netdev, Marcel Hellwig

[-- Attachment #1: Type: text/plain, Size: 779 bytes --]

On 22.05.2023 10:00:32, Marc Kleine-Budde wrote:
> On 22.05.2023 09:24:17, git@cookiesoft.de wrote:
> > Hey everyone,
> > 
> > is there anything I can do to get this merged?
> > Is there anything missing?
> 
> Looks good, while applying I'v moved the phy_power_off() after the
> pm_runtime_put() to make it symmetric with respect to xcan_open().

Also fixed that warning:

| drivers/net/can/xilinx_can.c:220: warning: Function parameter or member 'transceiver' not described in 'xcan_priv'

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-05-22  8:23 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-17  8:52 [PATCH] can: dev: add transceiver capabilities to xilinx_can Marcel Hellwig
2023-05-22  7:24 ` git
2023-05-22  8:00   ` Marc Kleine-Budde
2023-05-22  8:23     ` Marc Kleine-Budde
2023-05-22  7:58 ` Vincent MAILHOL
2023-05-22  8:03   ` Marc Kleine-Budde

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).