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 | 68 +++++++++++++++++++++++++++++++++++++++++++
 drivers/net/can/m_can/m_can.h |  4 +++
 2 files changed, 72 insertions(+)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 5a4e0ad07e9ecc82de5f1f606707f3380d3679fc..c539375005f71c88fd1f7d1a885ce890ce0e9327 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 = {
@@ -2340,6 +2362,44 @@ int m_can_class_get_clocks(struct m_can_classdev *cdev)
 }
 EXPORT_SYMBOL_GPL(m_can_class_get_clocks);
 
+static int m_can_class_setup_optional_pinctrl(struct m_can_classdev *class_dev)
+{
+	struct device *dev = class_dev->dev;
+	int ret;
+
+	class_dev->pinctrl = devm_pinctrl_get(dev);
+	if (IS_ERR(class_dev->pinctrl)) {
+		ret = PTR_ERR(class_dev->pinctrl);
+		class_dev->pinctrl = NULL;
+
+		if (ret == -ENODEV)
+			return 0;
+
+		return dev_err_probe(dev, ret, "Failed to get pinctrl\n");
+	}
+
+	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);
+		class_dev->pinctrl_state_wakeup = NULL;
+
+		if (ret == -ENODEV)
+			return 0;
+
+		return dev_err_probe(dev, ret, "Failed to lookup pinctrl wakeup state\n");
+	}
+
+	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);
+		return dev_err_probe(dev, ret, "Failed to lookup pinctrl default state\n");
+	}
+
+	return 0;
+}
+
 struct m_can_classdev *m_can_class_allocate_dev(struct device *dev,
 						int sizeof_priv)
 {
@@ -2380,7 +2440,15 @@ struct m_can_classdev *m_can_class_allocate_dev(struct device *dev,
 
 	m_can_of_parse_mram(class_dev, mram_config_vals);
 
+	ret = m_can_class_setup_optional_pinctrl(class_dev);
+	if (ret)
+		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

Hi Markus,

This is a nice improvement from the v3.

On Wed. 16 Oct. 2024 at 04: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 | 68 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/net/can/m_can/m_can.h |  4 +++
>  2 files changed, 72 insertions(+)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 5a4e0ad07e9ecc82de5f1f606707f3380d3679fc..c539375005f71c88fd1f7d1a885ce890ce0e9327 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 = {
> @@ -2340,6 +2362,44 @@ int m_can_class_get_clocks(struct m_can_classdev *cdev)
>  }
>  EXPORT_SYMBOL_GPL(m_can_class_get_clocks);
>
> +static int m_can_class_setup_optional_pinctrl(struct m_can_classdev *class_dev)
> +{
> +       struct device *dev = class_dev->dev;
> +       int ret;
> +
> +       class_dev->pinctrl = devm_pinctrl_get(dev);
> +       if (IS_ERR(class_dev->pinctrl)) {
> +               ret = PTR_ERR(class_dev->pinctrl);
> +               class_dev->pinctrl = NULL;
> +
> +               if (ret == -ENODEV)
> +                       return 0;
> +
> +               return dev_err_probe(dev, ret, "Failed to get pinctrl\n");
> +       }
> +
> +       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);
> +               class_dev->pinctrl_state_wakeup = NULL;
> +
> +               if (ret == -ENODEV)
> +                       return 0;
> +
> +               return dev_err_probe(dev, ret, "Failed to lookup pinctrl wakeup state\n");
> +       }
> +
> +       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);

Sorry if this is a silly question, but why aren't you doing the:

                  class_dev->pinctrl_state_default = NULL;

                  if (ret == -ENODEV)
                          return 0;

thing the same way you are doing it for the pinctrl and the
pinctrl_state_wakeup?

> +               return dev_err_probe(dev, ret, "Failed to lookup pinctrl default state\n");
> +       }
> +
> +       return 0;
> +}
> +
>  struct m_can_classdev *m_can_class_allocate_dev(struct device *dev,
>                                                 int sizeof_priv)
>  {
> @@ -2380,7 +2440,15 @@ struct m_can_classdev *m_can_class_allocate_dev(struct device *dev,
>
>         m_can_of_parse_mram(class_dev, mram_config_vals);
>
> +       ret = m_can_class_setup_optional_pinctrl(class_dev);
> +       if (ret)
> +               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);

Hi Vincent,

On Wed, Oct 16, 2024 at 11:26:22PM GMT, Vincent MAILHOL wrote:
> Hi Markus,
> 
> This is a nice improvement from the v3.
> 
> On Wed. 16 Oct. 2024 at 04: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 | 68 +++++++++++++++++++++++++++++++++++++++++++
> >  drivers/net/can/m_can/m_can.h |  4 +++
> >  2 files changed, 72 insertions(+)
> >
> > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > index 5a4e0ad07e9ecc82de5f1f606707f3380d3679fc..c539375005f71c88fd1f7d1a885ce890ce0e9327 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 = {
> > @@ -2340,6 +2362,44 @@ int m_can_class_get_clocks(struct m_can_classdev *cdev)
> >  }
> >  EXPORT_SYMBOL_GPL(m_can_class_get_clocks);
> >
> > +static int m_can_class_setup_optional_pinctrl(struct m_can_classdev *class_dev)
> > +{
> > +       struct device *dev = class_dev->dev;
> > +       int ret;
> > +
> > +       class_dev->pinctrl = devm_pinctrl_get(dev);
> > +       if (IS_ERR(class_dev->pinctrl)) {
> > +               ret = PTR_ERR(class_dev->pinctrl);
> > +               class_dev->pinctrl = NULL;
> > +
> > +               if (ret == -ENODEV)
> > +                       return 0;
> > +
> > +               return dev_err_probe(dev, ret, "Failed to get pinctrl\n");
> > +       }
> > +
> > +       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);
> > +               class_dev->pinctrl_state_wakeup = NULL;
> > +
> > +               if (ret == -ENODEV)
> > +                       return 0;
> > +
> > +               return dev_err_probe(dev, ret, "Failed to lookup pinctrl wakeup state\n");
> > +       }
> > +
> > +       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);
> 
> Sorry if this is a silly question, but why aren't you doing the:
> 
>                   class_dev->pinctrl_state_default = NULL;
> 
>                   if (ret == -ENODEV)
>                           return 0;
> 
> thing the same way you are doing it for the pinctrl and the
> pinctrl_state_wakeup?

There are no silly questions.
The idea is that if the wakeup pinctrl state was already found, then the
default pinctrl state is required and not optional, so no check for
-ENODEV. Otherwise it doesn't make sense with the current binding or the
implementation of the driver.

Best
Markus

> 
> > +               return dev_err_probe(dev, ret, "Failed to lookup pinctrl default state\n");
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  struct m_can_classdev *m_can_class_allocate_dev(struct device *dev,
> >                                                 int sizeof_priv)
> >  {
> > @@ -2380,7 +2440,15 @@ struct m_can_classdev *m_can_class_allocate_dev(struct device *dev,
> >
> >         m_can_of_parse_mram(class_dev, mram_config_vals);
> >
> > +       ret = m_can_class_setup_optional_pinctrl(class_dev);
> > +       if (ret)
> > +               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);

On Thu. 17 Oct. 2024 at 00:42, Markus Schneider-Pargmann
<msp@baylibre.com> wrote:
> Hi Vincent,
>
> On Wed, Oct 16, 2024 at 11:26:22PM GMT, Vincent MAILHOL wrote:
> > Hi Markus,
> >
> > This is a nice improvement from the v3.
> >
> > On Wed. 16 Oct. 2024 at 04: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>

(...)

> > > +       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);
> >
> > Sorry if this is a silly question, but why aren't you doing the:
> >
> >                   class_dev->pinctrl_state_default = NULL;
> >
> >                   if (ret == -ENODEV)
> >                           return 0;
> >
> > thing the same way you are doing it for the pinctrl and the
> > pinctrl_state_wakeup?
>
> There are no silly questions.
> The idea is that if the wakeup pinctrl state was already found, then the
> default pinctrl state is required and not optional, so no check for
> -ENODEV. Otherwise it doesn't make sense with the current binding or the
> implementation of the driver.

ACK. With this:

Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

This concludes my review of your series. I am skipping the dt-bindings
and the dts parts because I am not knowledgeable in those domains.


Yours sincerely,
Vincent Mailhol