* [PATCH v3 1/9] dt-bindings: can: m_can: Add wakeup properties
2024-10-11 13:16 [PATCH v3 0/9] can: m_can: Add am62 wakeup support Markus Schneider-Pargmann
@ 2024-10-11 13:16 ` Markus Schneider-Pargmann
2024-10-11 14:43 ` Krzysztof Kozlowski
2024-10-11 13:16 ` [PATCH v3 2/9] dt-bindings: can: m_can: Add vio-supply Markus Schneider-Pargmann
` (7 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Markus Schneider-Pargmann @ 2024-10-11 13:16 UTC (permalink / raw)
To: Chandrasekar Ramakrishnan, Marc Kleine-Budde, Vincent Mailhol,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo
Cc: linux-can, netdev, devicetree, linux-kernel, linux-arm-kernel,
Matthias Schiffer, Vishal Mahaveer, Kevin Hilman, Dhruva Gole,
Markus Schneider-Pargmann
m_can can be a wakeup source on some devices. Especially on some of the
am62* SoCs pins, connected to m_can in the mcu, can be used to wakeup
the SoC.
The wakeup-source property defines on which devices m_can can be used
for wakeup.
The pins associated with m_can have to have a special configuration to
be able to wakeup the SoC. This configuration is described in the wakeup
pinctrl state while the default state describes the default
configuration.
Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
.../devicetree/bindings/net/can/bosch,m_can.yaml | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
index c4887522e8fe97c3947357b4dbd4ecf20ee8100a..0c1f9fa7371897d45539ead49c9d290fb4966f30 100644
--- a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
+++ b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
@@ -106,6 +106,22 @@ properties:
maximum: 32
minItems: 1
+ pinctrl-0:
+ description: Default pinctrl state
+
+ pinctrl-1:
+ description: Wakeup pinctrl state
+
+ pinctrl-names:
+ description:
+ When present should contain at least "default" describing the default pin
+ states. The second state called "wakeup" describes the pins in their
+ wakeup configuration required to exit sleep states.
+ minItems: 1
+ items:
+ - const: default
+ - const: wakeup
+
power-domains:
description:
Power domain provider node and an args specifier containing
@@ -122,6 +138,8 @@ properties:
minItems: 1
maxItems: 2
+ wakeup-source: true
+
required:
- compatible
- reg
--
2.45.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 1/9] dt-bindings: can: m_can: Add wakeup properties
2024-10-11 13:16 ` [PATCH v3 1/9] dt-bindings: can: m_can: Add wakeup properties Markus Schneider-Pargmann
@ 2024-10-11 14:43 ` Krzysztof Kozlowski
0 siblings, 0 replies; 19+ messages in thread
From: Krzysztof Kozlowski @ 2024-10-11 14:43 UTC (permalink / raw)
To: Markus Schneider-Pargmann
Cc: Chandrasekar Ramakrishnan, Marc Kleine-Budde, Vincent Mailhol,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo, linux-can, netdev, devicetree,
linux-kernel, linux-arm-kernel, Matthias Schiffer,
Vishal Mahaveer, Kevin Hilman, Dhruva Gole
On Fri, Oct 11, 2024 at 03:16:38PM +0200, Markus Schneider-Pargmann wrote:
> m_can can be a wakeup source on some devices. Especially on some of the
> am62* SoCs pins, connected to m_can in the mcu, can be used to wakeup
> the SoC.
>
> The wakeup-source property defines on which devices m_can can be used
> for wakeup.
>
> The pins associated with m_can have to have a special configuration to
> be able to wakeup the SoC. This configuration is described in the wakeup
> pinctrl state while the default state describes the default
> configuration.
>
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> ---
> .../devicetree/bindings/net/can/bosch,m_can.yaml | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 2/9] dt-bindings: can: m_can: Add vio-supply
2024-10-11 13:16 [PATCH v3 0/9] can: m_can: Add am62 wakeup support Markus Schneider-Pargmann
2024-10-11 13:16 ` [PATCH v3 1/9] dt-bindings: can: m_can: Add wakeup properties Markus Schneider-Pargmann
@ 2024-10-11 13:16 ` Markus Schneider-Pargmann
2024-10-11 14:44 ` Krzysztof Kozlowski
2024-10-11 13:16 ` [PATCH v3 3/9] can: m_can: Map WoL to device_set_wakeup_enable Markus Schneider-Pargmann
` (6 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Markus Schneider-Pargmann @ 2024-10-11 13:16 UTC (permalink / raw)
To: Chandrasekar Ramakrishnan, Marc Kleine-Budde, Vincent Mailhol,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo
Cc: linux-can, netdev, devicetree, linux-kernel, linux-arm-kernel,
Matthias Schiffer, Vishal Mahaveer, Kevin Hilman, Dhruva Gole,
Markus Schneider-Pargmann
The m_can unit can be integrated in different ways. For AM62 the unit is
integrated in different parts of the system (MCU or Main domain) and can
be powered by different external power sources. For examle on am62-lp-sk
mcu_mcan0 and mcu_mcan1 are powered through VDDSHV_CANUART by an
external regulator. To be able to describe these relationships, add a
vio-supply property to this binding.
Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
Documentation/devicetree/bindings/net/can/bosch,m_can.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
index 0c1f9fa7371897d45539ead49c9d290fb4966f30..e35cabce92c658c1b548cbac0940e16f7c2504ee 100644
--- a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
+++ b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
@@ -140,6 +140,10 @@ properties:
wakeup-source: true
+ vio-supply:
+ description: |
+ Reference to the main power supply of the unit.
+
required:
- compatible
- reg
--
2.45.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 2/9] dt-bindings: can: m_can: Add vio-supply
2024-10-11 13:16 ` [PATCH v3 2/9] dt-bindings: can: m_can: Add vio-supply Markus Schneider-Pargmann
@ 2024-10-11 14:44 ` Krzysztof Kozlowski
2024-10-11 19:01 ` Simon Horman
0 siblings, 1 reply; 19+ messages in thread
From: Krzysztof Kozlowski @ 2024-10-11 14:44 UTC (permalink / raw)
To: Markus Schneider-Pargmann
Cc: Chandrasekar Ramakrishnan, Marc Kleine-Budde, Vincent Mailhol,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo, linux-can, netdev, devicetree,
linux-kernel, linux-arm-kernel, Matthias Schiffer,
Vishal Mahaveer, Kevin Hilman, Dhruva Gole
On Fri, Oct 11, 2024 at 03:16:39PM +0200, Markus Schneider-Pargmann wrote:
> The m_can unit can be integrated in different ways. For AM62 the unit is
> integrated in different parts of the system (MCU or Main domain) and can
> be powered by different external power sources. For examle on am62-lp-sk
> mcu_mcan0 and mcu_mcan1 are powered through VDDSHV_CANUART by an
> external regulator. To be able to describe these relationships, add a
> vio-supply property to this binding.
>
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> ---
> Documentation/devicetree/bindings/net/can/bosch,m_can.yaml | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> index 0c1f9fa7371897d45539ead49c9d290fb4966f30..e35cabce92c658c1b548cbac0940e16f7c2504ee 100644
> --- a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> +++ b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> @@ -140,6 +140,10 @@ properties:
>
> wakeup-source: true
>
> + vio-supply:
> + description: |
If there is going to be new version: drop |
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 2/9] dt-bindings: can: m_can: Add vio-supply
2024-10-11 14:44 ` Krzysztof Kozlowski
@ 2024-10-11 19:01 ` Simon Horman
0 siblings, 0 replies; 19+ messages in thread
From: Simon Horman @ 2024-10-11 19:01 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Markus Schneider-Pargmann, Chandrasekar Ramakrishnan,
Marc Kleine-Budde, Vincent Mailhol, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Nishanth Menon, Vignesh Raghavendra, Tero Kristo,
linux-can, netdev, devicetree, linux-kernel, linux-arm-kernel,
Matthias Schiffer, Vishal Mahaveer, Kevin Hilman, Dhruva Gole
On Fri, Oct 11, 2024 at 04:44:00PM +0200, Krzysztof Kozlowski wrote:
> On Fri, Oct 11, 2024 at 03:16:39PM +0200, Markus Schneider-Pargmann wrote:
> > The m_can unit can be integrated in different ways. For AM62 the unit is
> > integrated in different parts of the system (MCU or Main domain) and can
> > be powered by different external power sources. For examle on am62-lp-sk
> > mcu_mcan0 and mcu_mcan1 are powered through VDDSHV_CANUART by an
> > external regulator. To be able to describe these relationships, add a
> > vio-supply property to this binding.
> >
> > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> > ---
> > Documentation/devicetree/bindings/net/can/bosch,m_can.yaml | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> > index 0c1f9fa7371897d45539ead49c9d290fb4966f30..e35cabce92c658c1b548cbac0940e16f7c2504ee 100644
> > --- a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> > +++ b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> > @@ -140,6 +140,10 @@ properties:
> >
> > wakeup-source: true
> >
> > + vio-supply:
> > + description: |
>
> If there is going to be new version: drop |
And likewise, correct the spelling of examle -> example.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 3/9] can: m_can: Map WoL to device_set_wakeup_enable
2024-10-11 13:16 [PATCH v3 0/9] can: m_can: Add am62 wakeup support Markus Schneider-Pargmann
2024-10-11 13:16 ` [PATCH v3 1/9] dt-bindings: can: m_can: Add wakeup properties Markus Schneider-Pargmann
2024-10-11 13:16 ` [PATCH v3 2/9] dt-bindings: can: m_can: Add vio-supply Markus Schneider-Pargmann
@ 2024-10-11 13:16 ` Markus Schneider-Pargmann
2024-10-11 18:59 ` Simon Horman
2024-10-11 13:16 ` [PATCH v3 4/9] can: m_can: Return ERR_PTR on error in allocation Markus Schneider-Pargmann
` (5 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Markus Schneider-Pargmann @ 2024-10-11 13:16 UTC (permalink / raw)
To: Chandrasekar Ramakrishnan, Marc Kleine-Budde, Vincent Mailhol,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo
Cc: linux-can, netdev, devicetree, linux-kernel, linux-arm-kernel,
Matthias Schiffer, Vishal Mahaveer, Kevin Hilman, Dhruva Gole,
Markus Schneider-Pargmann, Andrew Lunn
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;
+ 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
*/
--
2.45.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 3/9] can: m_can: Map WoL to device_set_wakeup_enable
2024-10-11 13:16 ` [PATCH v3 3/9] can: m_can: Map WoL to device_set_wakeup_enable Markus Schneider-Pargmann
@ 2024-10-11 18:59 ` Simon Horman
2024-10-15 6:32 ` Markus Schneider-Pargmann
0 siblings, 1 reply; 19+ messages in thread
From: Simon Horman @ 2024-10-11 18:59 UTC (permalink / raw)
To: Markus Schneider-Pargmann
Cc: Chandrasekar Ramakrishnan, Marc Kleine-Budde, Vincent Mailhol,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo, linux-can, netdev, devicetree,
linux-kernel, linux-arm-kernel, Matthias Schiffer,
Vishal Mahaveer, Kevin Hilman, Dhruva Gole, Andrew Lunn
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;
> +}
> +
...
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 3/9] can: m_can: Map WoL to device_set_wakeup_enable
2024-10-11 18:59 ` Simon Horman
@ 2024-10-15 6:32 ` Markus Schneider-Pargmann
0 siblings, 0 replies; 19+ messages in thread
From: Markus Schneider-Pargmann @ 2024-10-15 6:32 UTC (permalink / raw)
To: Simon Horman
Cc: Chandrasekar Ramakrishnan, Marc Kleine-Budde, Vincent Mailhol,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo, linux-can, netdev, devicetree,
linux-kernel, linux-arm-kernel, Matthias Schiffer,
Vishal Mahaveer, Kevin Hilman, Dhruva Gole, Andrew Lunn
[-- Attachment #1: Type: text/plain, Size: 2442 bytes --]
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;
> > +}
> > +
>
> ...
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 4/9] can: m_can: Return ERR_PTR on error in allocation
2024-10-11 13:16 [PATCH v3 0/9] can: m_can: Add am62 wakeup support Markus Schneider-Pargmann
` (2 preceding siblings ...)
2024-10-11 13:16 ` [PATCH v3 3/9] can: m_can: Map WoL to device_set_wakeup_enable Markus Schneider-Pargmann
@ 2024-10-11 13:16 ` Markus Schneider-Pargmann
2024-10-13 12:32 ` Vincent MAILHOL
2024-10-11 13:16 ` [PATCH v3 5/9] can: m_can: Support pinctrl wakeup state Markus Schneider-Pargmann
` (4 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Markus Schneider-Pargmann @ 2024-10-11 13:16 UTC (permalink / raw)
To: Chandrasekar Ramakrishnan, Marc Kleine-Budde, Vincent Mailhol,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo
Cc: linux-can, netdev, devicetree, linux-kernel, linux-arm-kernel,
Matthias Schiffer, Vishal Mahaveer, Kevin Hilman, Dhruva Gole,
Markus Schneider-Pargmann
We have more detailed error values available, return them in the core
driver and the calling drivers to return proper errors to callers.
Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
drivers/net/can/m_can/m_can.c | 6 +++---
drivers/net/can/m_can/m_can_pci.c | 4 ++--
drivers/net/can/m_can/m_can_platform.c | 4 ++--
drivers/net/can/m_can/tcan4x5x-core.c | 4 ++--
4 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 29accadc20de7e9efa509f14209cc62e599f03bb..5ab0bb3f1c71e7dc4d6144f7b9e8f58d0e0303fe 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -2355,7 +2355,7 @@ struct m_can_classdev *m_can_class_allocate_dev(struct device *dev,
sizeof(mram_config_vals) / 4);
if (ret) {
dev_err(dev, "Could not get Message RAM configuration.");
- goto out;
+ return ERR_PTR(ret);
}
if (dev->of_node && of_property_read_bool(dev->of_node, "wakeup-source"))
@@ -2370,7 +2370,7 @@ struct m_can_classdev *m_can_class_allocate_dev(struct device *dev,
net_dev = alloc_candev(sizeof_priv, tx_fifo_size);
if (!net_dev) {
dev_err(dev, "Failed to allocate CAN device");
- goto out;
+ return ERR_PTR(-ENOMEM);
}
class_dev = netdev_priv(net_dev);
@@ -2379,7 +2379,7 @@ struct m_can_classdev *m_can_class_allocate_dev(struct device *dev,
SET_NETDEV_DEV(net_dev, dev);
m_can_of_parse_mram(class_dev, mram_config_vals);
-out:
+
return class_dev;
}
EXPORT_SYMBOL_GPL(m_can_class_allocate_dev);
diff --git a/drivers/net/can/m_can/m_can_pci.c b/drivers/net/can/m_can/m_can_pci.c
index d72fe771dfc7aa768c728f817e67a87b49fd9974..05a01dfdbfbf18b74f796d2efc75e2be5cbb75ed 100644
--- a/drivers/net/can/m_can/m_can_pci.c
+++ b/drivers/net/can/m_can/m_can_pci.c
@@ -111,8 +111,8 @@ static int m_can_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
mcan_class = m_can_class_allocate_dev(&pci->dev,
sizeof(struct m_can_pci_priv));
- if (!mcan_class)
- return -ENOMEM;
+ if (IS_ERR(mcan_class))
+ return PTR_ERR(mcan_class);
priv = cdev_to_priv(mcan_class);
diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
index b832566efda042929486578fad1879c7ad4a0cff..40bd10f71f0e2fab847c40c5bd5f7d85d3d46712 100644
--- a/drivers/net/can/m_can/m_can_platform.c
+++ b/drivers/net/can/m_can/m_can_platform.c
@@ -87,8 +87,8 @@ static int m_can_plat_probe(struct platform_device *pdev)
mcan_class = m_can_class_allocate_dev(&pdev->dev,
sizeof(struct m_can_plat_priv));
- if (!mcan_class)
- return -ENOMEM;
+ if (IS_ERR(mcan_class))
+ return PTR_ERR(mcan_class);
priv = cdev_to_priv(mcan_class);
diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index 2f73bf3abad889c222f15c39a3d43de1a1cf5fbb..4c40b444727585b30df33a897c398e35e7592fb2 100644
--- a/drivers/net/can/m_can/tcan4x5x-core.c
+++ b/drivers/net/can/m_can/tcan4x5x-core.c
@@ -375,8 +375,8 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
mcan_class = m_can_class_allocate_dev(&spi->dev,
sizeof(struct tcan4x5x_priv));
- if (!mcan_class)
- return -ENOMEM;
+ if (IS_ERR(mcan_class))
+ return PTR_ERR(mcan_class);
ret = m_can_check_mram_cfg(mcan_class, TCAN4X5X_MRAM_SIZE);
if (ret)
--
2.45.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 4/9] can: m_can: Return ERR_PTR on error in allocation
2024-10-11 13:16 ` [PATCH v3 4/9] can: m_can: Return ERR_PTR on error in allocation Markus Schneider-Pargmann
@ 2024-10-13 12:32 ` Vincent MAILHOL
0 siblings, 0 replies; 19+ messages in thread
From: Vincent MAILHOL @ 2024-10-13 12:32 UTC (permalink / raw)
To: Markus Schneider-Pargmann
Cc: Chandrasekar Ramakrishnan, Marc Kleine-Budde, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo, linux-can, netdev, devicetree,
linux-kernel, linux-arm-kernel, Matthias Schiffer,
Vishal Mahaveer, Kevin Hilman, Dhruva Gole
Hi Markus,
Thanks for the patch.
On Fri. 11 Oct. 2024 at 22:19, Markus Schneider-Pargmann
<msp@baylibre.com> wrote:
> We have more detailed error values available, return them in the core
> driver and the calling drivers to return proper errors to callers.
>
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 5/9] can: m_can: Support pinctrl wakeup state
2024-10-11 13:16 [PATCH v3 0/9] can: m_can: Add am62 wakeup support Markus Schneider-Pargmann
` (3 preceding siblings ...)
2024-10-11 13:16 ` [PATCH v3 4/9] can: m_can: Return ERR_PTR on error in allocation Markus Schneider-Pargmann
@ 2024-10-11 13:16 ` Markus Schneider-Pargmann
2024-10-13 12:27 ` Vincent MAILHOL
2024-10-11 13:16 ` [PATCH v3 6/9] can: m_can: Add use of optional regulator Markus Schneider-Pargmann
` (3 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Markus Schneider-Pargmann @ 2024-10-11 13:16 UTC (permalink / raw)
To: Chandrasekar Ramakrishnan, Marc Kleine-Budde, Vincent Mailhol,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo
Cc: linux-can, netdev, devicetree, linux-kernel, linux-arm-kernel,
Matthias Schiffer, Vishal Mahaveer, Kevin Hilman, Dhruva Gole,
Markus Schneider-Pargmann
am62 requires a wakeup flag being set in pinctrl when mcan pins acts as
a wakeup source. Add support to select the wakeup state if WOL is
enabled.
Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
drivers/net/can/m_can/m_can.c | 60 +++++++++++++++++++++++++++++++++++++++++++
drivers/net/can/m_can/m_can.h | 4 +++
2 files changed, 64 insertions(+)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 5ab0bb3f1c71e7dc4d6144f7b9e8f58d0e0303fe..c56d61b0d20b05be36c95ec4a6651b0457883b66 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -2196,6 +2196,7 @@ static void m_can_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
static int m_can_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
{
struct m_can_classdev *cdev = netdev_priv(dev);
+ struct pinctrl_state *new_pinctrl_state = NULL;
bool wol_enable = !!wol->wolopts & WAKE_PHY;
int ret;
@@ -2212,7 +2213,28 @@ static int m_can_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
return ret;
}
+ if (wol_enable)
+ new_pinctrl_state = cdev->pinctrl_state_wakeup;
+ else
+ new_pinctrl_state = cdev->pinctrl_state_default;
+
+ if (IS_ERR_OR_NULL(new_pinctrl_state))
+ return 0;
+
+ ret = pinctrl_select_state(cdev->pinctrl, new_pinctrl_state);
+ if (ret) {
+ netdev_err(cdev->net, "Failed to select pinctrl state %pE\n",
+ ERR_PTR(ret));
+ goto err_wakeup_enable;
+ }
+
return 0;
+
+err_wakeup_enable:
+ /* Revert wakeup enable */
+ device_set_wakeup_enable(cdev->dev, !wol_enable);
+
+ return ret;
}
static const struct ethtool_ops m_can_ethtool_ops_coalescing = {
@@ -2380,7 +2402,45 @@ struct m_can_classdev *m_can_class_allocate_dev(struct device *dev,
m_can_of_parse_mram(class_dev, mram_config_vals);
+ class_dev->pinctrl = devm_pinctrl_get(dev);
+ if (IS_ERR(class_dev->pinctrl)) {
+ ret = PTR_ERR(class_dev->pinctrl);
+
+ if (ret != -ENODEV) {
+ dev_err_probe(dev, ret, "Failed to get pinctrl\n");
+ goto err_free_candev;
+ }
+
+ class_dev->pinctrl = NULL;
+ } else {
+ class_dev->pinctrl_state_wakeup =
+ pinctrl_lookup_state(class_dev->pinctrl, "wakeup");
+ if (IS_ERR(class_dev->pinctrl_state_wakeup)) {
+ ret = PTR_ERR(class_dev->pinctrl_state_wakeup);
+ ret = -EIO;
+
+ if (ret != -ENODEV) {
+ dev_err_probe(dev, ret, "Failed to lookup pinctrl wakeup state\n");
+ goto err_free_candev;
+ }
+
+ class_dev->pinctrl_state_wakeup = NULL;
+ } else {
+ class_dev->pinctrl_state_default =
+ pinctrl_lookup_state(class_dev->pinctrl, "default");
+ if (IS_ERR(class_dev->pinctrl_state_default)) {
+ ret = PTR_ERR(class_dev->pinctrl_state_default);
+ dev_err_probe(dev, ret, "Failed to lookup pinctrl default state\n");
+ goto err_free_candev;
+ }
+ }
+ }
+
return class_dev;
+
+err_free_candev:
+ free_candev(net_dev);
+ return ERR_PTR(ret);
}
EXPORT_SYMBOL_GPL(m_can_class_allocate_dev);
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index 92b2bd8628e6b31370f4accbc2e28f3b2257a71d..b75b0dd6ccc93973d0891daac07c92b61f81dc2a 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -126,6 +126,10 @@ struct m_can_classdev {
struct mram_cfg mcfg[MRAM_CFG_NUM];
struct hrtimer hrtimer;
+
+ struct pinctrl *pinctrl;
+ struct pinctrl_state *pinctrl_state_default;
+ struct pinctrl_state *pinctrl_state_wakeup;
};
struct m_can_classdev *m_can_class_allocate_dev(struct device *dev, int sizeof_priv);
--
2.45.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 5/9] can: m_can: Support pinctrl wakeup state
2024-10-11 13:16 ` [PATCH v3 5/9] can: m_can: Support pinctrl wakeup state Markus Schneider-Pargmann
@ 2024-10-13 12:27 ` Vincent MAILHOL
2024-10-15 9:29 ` Markus Schneider-Pargmann
0 siblings, 1 reply; 19+ messages in thread
From: Vincent MAILHOL @ 2024-10-13 12:27 UTC (permalink / raw)
To: Markus Schneider-Pargmann
Cc: Chandrasekar Ramakrishnan, Marc Kleine-Budde, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo, linux-can, netdev, devicetree,
linux-kernel, linux-arm-kernel, Matthias Schiffer,
Vishal Mahaveer, Kevin Hilman, Dhruva Gole
On Fri. 11 Oct. 2024 at 22:19, Markus Schneider-Pargmann
<msp@baylibre.com> wrote:
> am62 requires a wakeup flag being set in pinctrl when mcan pins acts as
> a wakeup source. Add support to select the wakeup state if WOL is
> enabled.
>
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> ---
> drivers/net/can/m_can/m_can.c | 60 +++++++++++++++++++++++++++++++++++++++++++
> drivers/net/can/m_can/m_can.h | 4 +++
> 2 files changed, 64 insertions(+)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 5ab0bb3f1c71e7dc4d6144f7b9e8f58d0e0303fe..c56d61b0d20b05be36c95ec4a6651b0457883b66 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -2196,6 +2196,7 @@ static void m_can_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
> static int m_can_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
> {
> struct m_can_classdev *cdev = netdev_priv(dev);
> + struct pinctrl_state *new_pinctrl_state = NULL;
> bool wol_enable = !!wol->wolopts & WAKE_PHY;
> int ret;
>
> @@ -2212,7 +2213,28 @@ static int m_can_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
> return ret;
> }
>
> + if (wol_enable)
> + new_pinctrl_state = cdev->pinctrl_state_wakeup;
> + else
> + new_pinctrl_state = cdev->pinctrl_state_default;
> +
> + if (IS_ERR_OR_NULL(new_pinctrl_state))
> + return 0;
> +
> + ret = pinctrl_select_state(cdev->pinctrl, new_pinctrl_state);
> + if (ret) {
> + netdev_err(cdev->net, "Failed to select pinctrl state %pE\n",
> + ERR_PTR(ret));
> + goto err_wakeup_enable;
> + }
> +
> return 0;
> +
> +err_wakeup_enable:
> + /* Revert wakeup enable */
> + device_set_wakeup_enable(cdev->dev, !wol_enable);
> +
> + return ret;
> }
>
> static const struct ethtool_ops m_can_ethtool_ops_coalescing = {
> @@ -2380,7 +2402,45 @@ struct m_can_classdev *m_can_class_allocate_dev(struct device *dev,
>
> m_can_of_parse_mram(class_dev, mram_config_vals);
>
> + class_dev->pinctrl = devm_pinctrl_get(dev);
> + if (IS_ERR(class_dev->pinctrl)) {
> + ret = PTR_ERR(class_dev->pinctrl);
> +
> + if (ret != -ENODEV) {
> + dev_err_probe(dev, ret, "Failed to get pinctrl\n");
> + goto err_free_candev;
> + }
> +
> + class_dev->pinctrl = NULL;
> + } else {
> + class_dev->pinctrl_state_wakeup =
> + pinctrl_lookup_state(class_dev->pinctrl, "wakeup");
> + if (IS_ERR(class_dev->pinctrl_state_wakeup)) {
> + ret = PTR_ERR(class_dev->pinctrl_state_wakeup);
> + ret = -EIO;
Here, ret is set twice, and the second time, it is unconditionally set
to -EIO...
> + if (ret != -ENODEV) {
... so isn't this check always true?
> + dev_err_probe(dev, ret, "Failed to lookup pinctrl wakeup state\n");
> + goto err_free_candev;
> + }
> +
> + class_dev->pinctrl_state_wakeup = NULL;
> + } else {
> + class_dev->pinctrl_state_default =
> + pinctrl_lookup_state(class_dev->pinctrl, "default");
> + if (IS_ERR(class_dev->pinctrl_state_default)) {
> + ret = PTR_ERR(class_dev->pinctrl_state_default);
> + dev_err_probe(dev, ret, "Failed to lookup pinctrl default state\n");
> + goto err_free_candev;
> + }
> + }
> + }
> +
> return class_dev;
> +
> +err_free_candev:
> + free_candev(net_dev);
> + return ERR_PTR(ret);
Here, you have three levels of nested if/else. It took me a bit of
effort to read and understand the logic. Wouldn't it be better to do
some early return at the end of each of the if branches in order to
remove the nesting? I am thinking of this:
class_dev->pinctrl = devm_pinctrl_get(dev);
if (IS_ERR(class_dev->pinctrl)) {
ret = PTR_ERR(class_dev->pinctrl);
if (ret != -ENODEV) {
dev_err_probe(dev, ret, "Failed to get pinctrl\n");
goto err_free_candev;
}
class_dev->pinctrl = NULL;
return class_dev;
}
class_dev->pinctrl_state_wakeup =
pinctrl_lookup_state(class_dev->pinctrl, "wakeup");
if (IS_ERR(class_dev->pinctrl_state_wakeup)) {
ret = PTR_ERR(class_dev->pinctrl_state_wakeup);
dev_err_probe(dev, ret, "Failed to lookup pinctrl
wakeup state\n");
goto err_free_candev;
}
class_dev->pinctrl_state_default =
pinctrl_lookup_state(class_dev->pinctrl, "default");
if (IS_ERR(class_dev->pinctrl_state_default)) {
ret = PTR_ERR(class_dev->pinctrl_state_default);
dev_err_probe(dev, ret, "Failed to lookup pinctrl
default state\n");
goto err_free_candev;
}
return class_dev;
err_free_candev:
free_candev(net_dev);
return ERR_PTR(ret);
> }
> EXPORT_SYMBOL_GPL(m_can_class_allocate_dev);
>
> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> index 92b2bd8628e6b31370f4accbc2e28f3b2257a71d..b75b0dd6ccc93973d0891daac07c92b61f81dc2a 100644
> --- a/drivers/net/can/m_can/m_can.h
> +++ b/drivers/net/can/m_can/m_can.h
> @@ -126,6 +126,10 @@ struct m_can_classdev {
> struct mram_cfg mcfg[MRAM_CFG_NUM];
>
> struct hrtimer hrtimer;
> +
> + struct pinctrl *pinctrl;
> + struct pinctrl_state *pinctrl_state_default;
> + struct pinctrl_state *pinctrl_state_wakeup;
> };
>
> struct m_can_classdev *m_can_class_allocate_dev(struct device *dev, int sizeof_priv);
>
> --
> 2.45.2
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 5/9] can: m_can: Support pinctrl wakeup state
2024-10-13 12:27 ` Vincent MAILHOL
@ 2024-10-15 9:29 ` Markus Schneider-Pargmann
0 siblings, 0 replies; 19+ messages in thread
From: Markus Schneider-Pargmann @ 2024-10-15 9:29 UTC (permalink / raw)
To: Vincent MAILHOL
Cc: Chandrasekar Ramakrishnan, Marc Kleine-Budde, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo, linux-can, netdev, devicetree,
linux-kernel, linux-arm-kernel, Matthias Schiffer,
Vishal Mahaveer, Kevin Hilman, Dhruva Gole
[-- Attachment #1: Type: text/plain, Size: 7214 bytes --]
On Sun, Oct 13, 2024 at 09:27:08PM GMT, Vincent MAILHOL wrote:
> On Fri. 11 Oct. 2024 at 22:19, Markus Schneider-Pargmann
> <msp@baylibre.com> wrote:
> > am62 requires a wakeup flag being set in pinctrl when mcan pins acts as
> > a wakeup source. Add support to select the wakeup state if WOL is
> > enabled.
> >
> > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> > ---
> > drivers/net/can/m_can/m_can.c | 60 +++++++++++++++++++++++++++++++++++++++++++
> > drivers/net/can/m_can/m_can.h | 4 +++
> > 2 files changed, 64 insertions(+)
> >
> > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > index 5ab0bb3f1c71e7dc4d6144f7b9e8f58d0e0303fe..c56d61b0d20b05be36c95ec4a6651b0457883b66 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -2196,6 +2196,7 @@ static void m_can_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
> > static int m_can_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
> > {
> > struct m_can_classdev *cdev = netdev_priv(dev);
> > + struct pinctrl_state *new_pinctrl_state = NULL;
> > bool wol_enable = !!wol->wolopts & WAKE_PHY;
> > int ret;
> >
> > @@ -2212,7 +2213,28 @@ static int m_can_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
> > return ret;
> > }
> >
> > + if (wol_enable)
> > + new_pinctrl_state = cdev->pinctrl_state_wakeup;
> > + else
> > + new_pinctrl_state = cdev->pinctrl_state_default;
> > +
> > + if (IS_ERR_OR_NULL(new_pinctrl_state))
> > + return 0;
> > +
> > + ret = pinctrl_select_state(cdev->pinctrl, new_pinctrl_state);
> > + if (ret) {
> > + netdev_err(cdev->net, "Failed to select pinctrl state %pE\n",
> > + ERR_PTR(ret));
> > + goto err_wakeup_enable;
> > + }
> > +
> > return 0;
> > +
> > +err_wakeup_enable:
> > + /* Revert wakeup enable */
> > + device_set_wakeup_enable(cdev->dev, !wol_enable);
> > +
> > + return ret;
> > }
> >
> > static const struct ethtool_ops m_can_ethtool_ops_coalescing = {
> > @@ -2380,7 +2402,45 @@ struct m_can_classdev *m_can_class_allocate_dev(struct device *dev,
> >
> > m_can_of_parse_mram(class_dev, mram_config_vals);
> >
> > + class_dev->pinctrl = devm_pinctrl_get(dev);
> > + if (IS_ERR(class_dev->pinctrl)) {
> > + ret = PTR_ERR(class_dev->pinctrl);
> > +
> > + if (ret != -ENODEV) {
> > + dev_err_probe(dev, ret, "Failed to get pinctrl\n");
> > + goto err_free_candev;
> > + }
> > +
> > + class_dev->pinctrl = NULL;
> > + } else {
> > + class_dev->pinctrl_state_wakeup =
> > + pinctrl_lookup_state(class_dev->pinctrl, "wakeup");
> > + if (IS_ERR(class_dev->pinctrl_state_wakeup)) {
> > + ret = PTR_ERR(class_dev->pinctrl_state_wakeup);
> > + ret = -EIO;
>
> Here, ret is set twice, and the second time, it is unconditionally set
> to -EIO...
Thank you, this was a left-over from testing this error path.
>
> > + if (ret != -ENODEV) {
>
> ... so isn't this check always true?
>
> > + dev_err_probe(dev, ret, "Failed to lookup pinctrl wakeup state\n");
> > + goto err_free_candev;
> > + }
> > +
> > + class_dev->pinctrl_state_wakeup = NULL;
> > + } else {
> > + class_dev->pinctrl_state_default =
> > + pinctrl_lookup_state(class_dev->pinctrl, "default");
> > + if (IS_ERR(class_dev->pinctrl_state_default)) {
> > + ret = PTR_ERR(class_dev->pinctrl_state_default);
> > + dev_err_probe(dev, ret, "Failed to lookup pinctrl default state\n");
> > + goto err_free_candev;
> > + }
> > + }
> > + }
> > +
> > return class_dev;
> > +
> > +err_free_candev:
> > + free_candev(net_dev);
> > + return ERR_PTR(ret);
>
> Here, you have three levels of nested if/else. It took me a bit of
> effort to read and understand the logic. Wouldn't it be better to do
> some early return at the end of each of the if branches in order to
> remove the nesting? I am thinking of this:
>
> class_dev->pinctrl = devm_pinctrl_get(dev);
> if (IS_ERR(class_dev->pinctrl)) {
> ret = PTR_ERR(class_dev->pinctrl);
>
> if (ret != -ENODEV) {
> dev_err_probe(dev, ret, "Failed to get pinctrl\n");
> goto err_free_candev;
> }
>
> class_dev->pinctrl = NULL;
> return class_dev;
> }
>
> class_dev->pinctrl_state_wakeup =
> pinctrl_lookup_state(class_dev->pinctrl, "wakeup");
> if (IS_ERR(class_dev->pinctrl_state_wakeup)) {
> ret = PTR_ERR(class_dev->pinctrl_state_wakeup);
> dev_err_probe(dev, ret, "Failed to lookup pinctrl
> wakeup state\n");
> goto err_free_candev;
> }
Thanks, yes you are right this is easier to read, but I don't want to do
it like this in the m_can_class_allocate_dev() function to keep the code
flow through the whole function and avoid early successful returns.
Instead I moved this to a separate function called
m_can_class_setup_optional_pinctrl(), doing it similarly to your
suggestion.
Best
Markus
>
> class_dev->pinctrl_state_default =
> pinctrl_lookup_state(class_dev->pinctrl, "default");
> if (IS_ERR(class_dev->pinctrl_state_default)) {
> ret = PTR_ERR(class_dev->pinctrl_state_default);
> dev_err_probe(dev, ret, "Failed to lookup pinctrl
> default state\n");
> goto err_free_candev;
> }
>
> return class_dev;
>
> err_free_candev:
> free_candev(net_dev);
> return ERR_PTR(ret);
>
> > }
> > EXPORT_SYMBOL_GPL(m_can_class_allocate_dev);
> >
> > diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> > index 92b2bd8628e6b31370f4accbc2e28f3b2257a71d..b75b0dd6ccc93973d0891daac07c92b61f81dc2a 100644
> > --- a/drivers/net/can/m_can/m_can.h
> > +++ b/drivers/net/can/m_can/m_can.h
> > @@ -126,6 +126,10 @@ struct m_can_classdev {
> > struct mram_cfg mcfg[MRAM_CFG_NUM];
> >
> > struct hrtimer hrtimer;
> > +
> > + struct pinctrl *pinctrl;
> > + struct pinctrl_state *pinctrl_state_default;
> > + struct pinctrl_state *pinctrl_state_wakeup;
> > };
> >
> > struct m_can_classdev *m_can_class_allocate_dev(struct device *dev, int sizeof_priv);
> >
> > --
> > 2.45.2
> >
> >
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 6/9] can: m_can: Add use of optional regulator
2024-10-11 13:16 [PATCH v3 0/9] can: m_can: Add am62 wakeup support Markus Schneider-Pargmann
` (4 preceding siblings ...)
2024-10-11 13:16 ` [PATCH v3 5/9] can: m_can: Support pinctrl wakeup state Markus Schneider-Pargmann
@ 2024-10-11 13:16 ` Markus Schneider-Pargmann
2024-10-13 12:34 ` Vincent MAILHOL
2024-10-11 13:16 ` [PATCH v3 7/9] arm64: dts: ti: k3-am62: Mark mcu_mcan0/1 as wakeup-source Markus Schneider-Pargmann
` (2 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Markus Schneider-Pargmann @ 2024-10-11 13:16 UTC (permalink / raw)
To: Chandrasekar Ramakrishnan, Marc Kleine-Budde, Vincent Mailhol,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo
Cc: linux-can, netdev, devicetree, linux-kernel, linux-arm-kernel,
Matthias Schiffer, Vishal Mahaveer, Kevin Hilman, Dhruva Gole,
Markus Schneider-Pargmann
Add support to use a regulator for the core. This is optional and used
to register the dependency on the regulator.
Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
drivers/net/can/m_can/m_can.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index c56d61b0d20b05be36c95ec4a6651b0457883b66..b009575354cf5f19e93950bb17d448f81609aae4 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -23,6 +23,7 @@
#include <linux/pinctrl/consumer.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
#include "m_can.h"
@@ -2383,6 +2384,11 @@ struct m_can_classdev *m_can_class_allocate_dev(struct device *dev,
if (dev->of_node && of_property_read_bool(dev->of_node, "wakeup-source"))
device_set_wakeup_capable(dev, true);
+ ret = devm_regulator_get_enable_optional(dev, "vio");
+ if (ret)
+ return ERR_PTR(
+ dev_err_probe(dev, ret, "Failed to get or enable optional regulator\n"));
+
/* Get TX FIFO size
* Defines the total amount of echo buffers for loopback
*/
--
2.45.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 6/9] can: m_can: Add use of optional regulator
2024-10-11 13:16 ` [PATCH v3 6/9] can: m_can: Add use of optional regulator Markus Schneider-Pargmann
@ 2024-10-13 12:34 ` Vincent MAILHOL
0 siblings, 0 replies; 19+ messages in thread
From: Vincent MAILHOL @ 2024-10-13 12:34 UTC (permalink / raw)
To: Markus Schneider-Pargmann
Cc: Chandrasekar Ramakrishnan, Marc Kleine-Budde, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo, linux-can, netdev, devicetree,
linux-kernel, linux-arm-kernel, Matthias Schiffer,
Vishal Mahaveer, Kevin Hilman, Dhruva Gole
On Fri. 11 Oct. 2024 at 22:20, Markus Schneider-Pargmann
<msp@baylibre.com> wrote:
> Add support to use a regulator for the core. This is optional and used
> to register the dependency on the regulator.
>
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 7/9] arm64: dts: ti: k3-am62: Mark mcu_mcan0/1 as wakeup-source
2024-10-11 13:16 [PATCH v3 0/9] can: m_can: Add am62 wakeup support Markus Schneider-Pargmann
` (5 preceding siblings ...)
2024-10-11 13:16 ` [PATCH v3 6/9] can: m_can: Add use of optional regulator Markus Schneider-Pargmann
@ 2024-10-11 13:16 ` Markus Schneider-Pargmann
2024-10-11 13:16 ` [PATCH v3 8/9] arm64: dts: ti: k3-am62a-mcu: " Markus Schneider-Pargmann
2024-10-11 13:16 ` [PATCH v3 9/9] arm64: dts: ti: k3-am62p-mcu: " Markus Schneider-Pargmann
8 siblings, 0 replies; 19+ messages in thread
From: Markus Schneider-Pargmann @ 2024-10-11 13:16 UTC (permalink / raw)
To: Chandrasekar Ramakrishnan, Marc Kleine-Budde, Vincent Mailhol,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo
Cc: linux-can, netdev, devicetree, linux-kernel, linux-arm-kernel,
Matthias Schiffer, Vishal Mahaveer, Kevin Hilman, Dhruva Gole,
Markus Schneider-Pargmann
mcu_mcan0 and mcu_mcan1 can be wakeup sources for the SoC. Mark them
accordingly in the devicetree.
Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
arch/arm64/boot/dts/ti/k3-am62-mcu.dtsi | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/ti/k3-am62-mcu.dtsi b/arch/arm64/boot/dts/ti/k3-am62-mcu.dtsi
index bb43a411f59b281df476afcb1a71b988ca27f002..e22177b9dfecb541e99b0807f8b79e7b878b6514 100644
--- a/arch/arm64/boot/dts/ti/k3-am62-mcu.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62-mcu.dtsi
@@ -160,6 +160,7 @@ mcu_mcan0: can@4e08000 {
clocks = <&k3_clks 188 6>, <&k3_clks 188 1>;
clock-names = "hclk", "cclk";
bosch,mram-cfg = <0x0 128 64 64 64 64 32 32>;
+ wakeup-source;
status = "disabled";
};
@@ -172,6 +173,7 @@ mcu_mcan1: can@4e18000 {
clocks = <&k3_clks 189 6>, <&k3_clks 189 1>;
clock-names = "hclk", "cclk";
bosch,mram-cfg = <0x0 128 64 64 64 64 32 32>;
+ wakeup-source;
status = "disabled";
};
};
--
2.45.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 8/9] arm64: dts: ti: k3-am62a-mcu: Mark mcu_mcan0/1 as wakeup-source
2024-10-11 13:16 [PATCH v3 0/9] can: m_can: Add am62 wakeup support Markus Schneider-Pargmann
` (6 preceding siblings ...)
2024-10-11 13:16 ` [PATCH v3 7/9] arm64: dts: ti: k3-am62: Mark mcu_mcan0/1 as wakeup-source Markus Schneider-Pargmann
@ 2024-10-11 13:16 ` Markus Schneider-Pargmann
2024-10-11 13:16 ` [PATCH v3 9/9] arm64: dts: ti: k3-am62p-mcu: " Markus Schneider-Pargmann
8 siblings, 0 replies; 19+ messages in thread
From: Markus Schneider-Pargmann @ 2024-10-11 13:16 UTC (permalink / raw)
To: Chandrasekar Ramakrishnan, Marc Kleine-Budde, Vincent Mailhol,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo
Cc: linux-can, netdev, devicetree, linux-kernel, linux-arm-kernel,
Matthias Schiffer, Vishal Mahaveer, Kevin Hilman, Dhruva Gole,
Markus Schneider-Pargmann
mcu_mcan0 and mcu_mcan1 can be wakeup sources for the SoC. Mark them
accordingly in the devicetree.
Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
arch/arm64/boot/dts/ti/k3-am62a-mcu.dtsi | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/ti/k3-am62a-mcu.dtsi b/arch/arm64/boot/dts/ti/k3-am62a-mcu.dtsi
index 0469c766b769e46068f23e0073f951aa094c456f..06361cfd7a8ee6f2acf2d15e8106087dd0f38008 100644
--- a/arch/arm64/boot/dts/ti/k3-am62a-mcu.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62a-mcu.dtsi
@@ -161,6 +161,7 @@ mcu_mcan0: can@4e08000 {
clocks = <&k3_clks 188 6>, <&k3_clks 188 1>;
clock-names = "hclk", "cclk";
bosch,mram-cfg = <0x0 128 64 64 64 64 32 32>;
+ wakeup-source;
status = "disabled";
};
@@ -173,6 +174,7 @@ mcu_mcan1: can@4e18000 {
clocks = <&k3_clks 189 6>, <&k3_clks 189 1>;
clock-names = "hclk", "cclk";
bosch,mram-cfg = <0x0 128 64 64 64 64 32 32>;
+ wakeup-source;
status = "disabled";
};
};
--
2.45.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 9/9] arm64: dts: ti: k3-am62p-mcu: Mark mcu_mcan0/1 as wakeup-source
2024-10-11 13:16 [PATCH v3 0/9] can: m_can: Add am62 wakeup support Markus Schneider-Pargmann
` (7 preceding siblings ...)
2024-10-11 13:16 ` [PATCH v3 8/9] arm64: dts: ti: k3-am62a-mcu: " Markus Schneider-Pargmann
@ 2024-10-11 13:16 ` Markus Schneider-Pargmann
8 siblings, 0 replies; 19+ messages in thread
From: Markus Schneider-Pargmann @ 2024-10-11 13:16 UTC (permalink / raw)
To: Chandrasekar Ramakrishnan, Marc Kleine-Budde, Vincent Mailhol,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo
Cc: linux-can, netdev, devicetree, linux-kernel, linux-arm-kernel,
Matthias Schiffer, Vishal Mahaveer, Kevin Hilman, Dhruva Gole,
Markus Schneider-Pargmann
From: Vibhore Vardhan <vibhore@ti.com>
mcu_mcan0 and mcu_mcan1 can be wakeup sources for the SoC. Mark them
accordingly in the devicetree. Based on the patch for AM62a.
Signed-off-by: Vibhore Vardhan <vibhore@ti.com>
Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
arch/arm64/boot/dts/ti/k3-am62p-j722s-common-mcu.dtsi | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/ti/k3-am62p-j722s-common-mcu.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-j722s-common-mcu.dtsi
index b33aff0d65c9def755f8dda9eb9feda7bc74e5c8..3afa17e6592f39387a667547835c90f95a9af351 100644
--- a/arch/arm64/boot/dts/ti/k3-am62p-j722s-common-mcu.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62p-j722s-common-mcu.dtsi
@@ -173,6 +173,7 @@ mcu_mcan0: can@4e08000 {
interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
interrupt-names = "int0", "int1";
+ wakeup-source;
status = "disabled";
};
@@ -188,6 +189,7 @@ mcu_mcan1: can@4e18000 {
interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
interrupt-names = "int0", "int1";
+ wakeup-source;
status = "disabled";
};
--
2.45.2
^ permalink raw reply related [flat|nested] 19+ messages in thread