| Message ID | 20241011-topic-mcan-wakeup-source-v6-12-v3-5-9752c714ad12@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | can: m_can: Add am62 wakeup support | expand |
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 > >
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 > > > >
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);
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(+)