Re: [PATCH v3 5/9] can: m_can: Support pinctrl wakeup state

[Vincent MAILHOL]

On Fri. 11 Oct. 2024 at 22:19, Markus Schneider-Pargmann
<msp@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
>  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
>
>