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

From: Markus Schneider-Pargmann
Date: Tue Oct 15 2024 - 05:29:18 EST

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@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...

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: signature.asc
Description: PGP signature