diff mbox series

[v3,3/9] can: m_can: Map WoL to device_set_wakeup_enable

Message ID 20241011-topic-mcan-wakeup-source-v6-12-v3-3-9752c714ad12@baylibre.com (mailing list archive)
State New, archived
Headers show
Series can: m_can: Add am62 wakeup support | expand

Commit Message

Markus Schneider-Pargmann Oct. 11, 2024, 1:16 p.m. UTC 
In some devices the pins of the m_can module can act as a wakeup source.
This patch helps do that by connecting the PHY_WAKE WoL option to
device_set_wakeup_enable. By marking this device as being wakeup
enabled, this setting can be used by platform code to decide which
sleep or poweroff mode to use.

Also this prepares the driver for the next patch in which the pinctrl
settings are changed depending on the desired wakeup source.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/can/m_can/m_can.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

Comments

Simon Horman Oct. 11, 2024, 6:59 p.m. UTC | #1 
On Fri, Oct 11, 2024 at 03:16:40PM +0200, Markus Schneider-Pargmann wrote:
> In some devices the pins of the m_can module can act as a wakeup source.
> This patch helps do that by connecting the PHY_WAKE WoL option to
> device_set_wakeup_enable. By marking this device as being wakeup
> enabled, this setting can be used by platform code to decide which
> sleep or poweroff mode to use.
> 
> Also this prepares the driver for the next patch in which the pinctrl
> settings are changed depending on the desired wakeup source.
> 
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/can/m_can/m_can.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index a978b960f1f1e1e8273216ff330ab789d0fd6d51..29accadc20de7e9efa509f14209cc62e599f03bb 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -2185,6 +2185,36 @@ static int m_can_set_coalesce(struct net_device *dev,
>  	return 0;
>  }
>  
> +static void m_can_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
> +{
> +	struct m_can_classdev *cdev = netdev_priv(dev);
> +
> +	wol->supported = device_can_wakeup(cdev->dev) ? WAKE_PHY : 0;
> +	wol->wolopts = device_may_wakeup(cdev->dev) ? WAKE_PHY : 0;
> +}
> +
> +static int m_can_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
> +{
> +	struct m_can_classdev *cdev = netdev_priv(dev);
> +	bool wol_enable = !!wol->wolopts & WAKE_PHY;

Hi Markus,

I suspect there is an order of operations issue here.
Should the line above be like this?

	bool wol_enable = !!(wol->wolopts & WAKE_PHY);

> +	int ret;
> +
> +	if ((wol->wolopts & WAKE_PHY) != wol->wolopts)
> +		return -EINVAL;
> +
> +	if (wol_enable == device_may_wakeup(cdev->dev))
> +		return 0;
> +
> +	ret = device_set_wakeup_enable(cdev->dev, wol_enable);
> +	if (ret) {
> +		netdev_err(cdev->net, "Failed to set wakeup enable %pE\n",
> +			   ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +

...
Markus Schneider-Pargmann Oct. 15, 2024, 6:32 a.m. UTC | #2 
On Fri, Oct 11, 2024 at 07:59:29PM GMT, Simon Horman wrote:
> On Fri, Oct 11, 2024 at 03:16:40PM +0200, Markus Schneider-Pargmann wrote:
> > In some devices the pins of the m_can module can act as a wakeup source.
> > This patch helps do that by connecting the PHY_WAKE WoL option to
> > device_set_wakeup_enable. By marking this device as being wakeup
> > enabled, this setting can be used by platform code to decide which
> > sleep or poweroff mode to use.
> > 
> > Also this prepares the driver for the next patch in which the pinctrl
> > settings are changed depending on the desired wakeup source.
> > 
> > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> >  drivers/net/can/m_can/m_can.c | 37 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> > 
> > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > index a978b960f1f1e1e8273216ff330ab789d0fd6d51..29accadc20de7e9efa509f14209cc62e599f03bb 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -2185,6 +2185,36 @@ static int m_can_set_coalesce(struct net_device *dev,
> >  	return 0;
> >  }
> >  
> > +static void m_can_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
> > +{
> > +	struct m_can_classdev *cdev = netdev_priv(dev);
> > +
> > +	wol->supported = device_can_wakeup(cdev->dev) ? WAKE_PHY : 0;
> > +	wol->wolopts = device_may_wakeup(cdev->dev) ? WAKE_PHY : 0;
> > +}
> > +
> > +static int m_can_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
> > +{
> > +	struct m_can_classdev *cdev = netdev_priv(dev);
> > +	bool wol_enable = !!wol->wolopts & WAKE_PHY;
> 
> Hi Markus,
> 
> I suspect there is an order of operations issue here.
> Should the line above be like this?
> 
> 	bool wol_enable = !!(wol->wolopts & WAKE_PHY);

Yes, you are absolutely right, thanks for spotting this.

Best
Markus

> 
> > +	int ret;
> > +
> > +	if ((wol->wolopts & WAKE_PHY) != wol->wolopts)
> > +		return -EINVAL;
> > +
> > +	if (wol_enable == device_may_wakeup(cdev->dev))
> > +		return 0;
> > +
> > +	ret = device_set_wakeup_enable(cdev->dev, wol_enable);
> > +	if (ret) {
> > +		netdev_err(cdev->net, "Failed to set wakeup enable %pE\n",
> > +			   ERR_PTR(ret));
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> 
> ...
diff mbox series

Patch

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index a978b960f1f1e1e8273216ff330ab789d0fd6d51..29accadc20de7e9efa509f14209cc62e599f03bb 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -2185,6 +2185,36 @@  static int m_can_set_coalesce(struct net_device *dev,
 	return 0;
 }
 
+static void m_can_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
+{
+	struct m_can_classdev *cdev = netdev_priv(dev);
+
+	wol->supported = device_can_wakeup(cdev->dev) ? WAKE_PHY : 0;
+	wol->wolopts = device_may_wakeup(cdev->dev) ? WAKE_PHY : 0;
+}
+
+static int m_can_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
+{
+	struct m_can_classdev *cdev = netdev_priv(dev);
+	bool wol_enable = !!wol->wolopts & WAKE_PHY;
+	int ret;
+
+	if ((wol->wolopts & WAKE_PHY) != wol->wolopts)
+		return -EINVAL;
+
+	if (wol_enable == device_may_wakeup(cdev->dev))
+		return 0;
+
+	ret = device_set_wakeup_enable(cdev->dev, wol_enable);
+	if (ret) {
+		netdev_err(cdev->net, "Failed to set wakeup enable %pE\n",
+			   ERR_PTR(ret));
+		return ret;
+	}
+
+	return 0;
+}
+
 static const struct ethtool_ops m_can_ethtool_ops_coalescing = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_RX_USECS_IRQ |
 		ETHTOOL_COALESCE_RX_MAX_FRAMES_IRQ |
@@ -2194,10 +2224,14 @@  static const struct ethtool_ops m_can_ethtool_ops_coalescing = {
 	.get_ts_info = ethtool_op_get_ts_info,
 	.get_coalesce = m_can_get_coalesce,
 	.set_coalesce = m_can_set_coalesce,
+	.get_wol = m_can_get_wol,
+	.set_wol = m_can_set_wol,
 };
 
 static const struct ethtool_ops m_can_ethtool_ops = {
 	.get_ts_info = ethtool_op_get_ts_info,
+	.get_wol = m_can_get_wol,
+	.set_wol = m_can_set_wol,
 };
 
 static int register_m_can_dev(struct m_can_classdev *cdev)
@@ -2324,6 +2358,9 @@  struct m_can_classdev *m_can_class_allocate_dev(struct device *dev,
 		goto out;
 	}
 
+	if (dev->of_node && of_property_read_bool(dev->of_node, "wakeup-source"))
+		device_set_wakeup_capable(dev, true);
+
 	/* Get TX FIFO size
 	 * Defines the total amount of echo buffers for loopback
 	 */