netdev - Re: [PATCH v2] can: rcar_canfd: Add transceiver support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAMZ6RqKWYWLUOgAHcz74dcc0NuGu2uxKNGFtNyQqoxdTo9uh7A@mail.gmail.com>
Date:   Fri, 10 Mar 2023 01:02:23 +0900
From:   Vincent Mailhol <vincent.mailhol@...il.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Wolfgang Grandegger <wg@...ndegger.com>,
        Marc Kleine-Budde <mkl@...gutronix.de>,
        linux-can@...r.kernel.org, netdev@...r.kernel.org,
        linux-renesas-soc@...r.kernel.org,
        Simon Horman <simon.horman@...igine.com>
Subject: Re: [PATCH v2] can: rcar_canfd: Add transceiver support

On Tue. 9 Mar 2023 at 17:12, Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
> Hi Vincent,
>
> On Wed, Mar 8, 2023 at 4:55 PM Vincent Mailhol
> <vincent.mailhol@...il.com> wrote:
> > On Wed. 8 Mar. 2023 at 22:20, Geert Uytterhoeven
> > <geert+renesas@...der.be> wrote:
> > > Add support for CAN transceivers described as PHYs.
> > >
> > > While simple CAN transceivers can do without, this is needed for CAN
> > > transceivers like NXP TJR1443 that need a configuration step (like
> > > pulling standby or enable lines), and/or impose a bitrate limit.
> > >
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@...der.be>
> > > Reviewed-by: Simon Horman <simon.horman@...igine.com>
> >
> > I have one nitpick (see below). Aside from that:
> > Reviewed-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>
>
> Thanks!
>
> >
> > > ---
> > > v2:
> > >   - Add Reviewed-by.
> > > ---
> > >  drivers/net/can/rcar/rcar_canfd.c | 30 +++++++++++++++++++++++++-----
> > >  1 file changed, 25 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> > > index ef4e1b9a9e1ee280..6df9a259e5e4f92c 100644
> > > --- a/drivers/net/can/rcar/rcar_canfd.c
> > > +++ b/drivers/net/can/rcar/rcar_canfd.c
> > > @@ -35,6 +35,7 @@
> > >  #include <linux/netdevice.h>
> > >  #include <linux/of.h>
> > >  #include <linux/of_device.h>
> > > +#include <linux/phy/phy.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/reset.h>
> > >  #include <linux/types.h>
> > > @@ -530,6 +531,7 @@ struct rcar_canfd_channel {
> > >         struct net_device *ndev;
> > >         struct rcar_canfd_global *gpriv;        /* Controller reference */
> > >         void __iomem *base;                     /* Register base address */
> > > +       struct phy *transceiver;                /* Optional transceiver */
> > >         struct napi_struct napi;
> > >         u32 tx_head;                            /* Incremented on xmit */
> > >         u32 tx_tail;                            /* Incremented on xmit done */
> > > @@ -1413,11 +1415,17 @@ static int rcar_canfd_open(struct net_device *ndev)
> > >         struct rcar_canfd_global *gpriv = priv->gpriv;
> > >         int err;
> > >
> > > +       err = phy_power_on(priv->transceiver);
> > > +       if (err) {
> > > +               netdev_err(ndev, "failed to power on PHY, error %d\n", err);

Actually, I wanted to comment on this line…

> > > +               return err;
> > > +       }
> > > +
> > >         /* Peripheral clock is already enabled in probe */
> > >         err = clk_prepare_enable(gpriv->can_clk);
> > >         if (err) {
> > >                 netdev_err(ndev, "failed to enable CAN clock, error %d\n", err);
> >                                                                       ^^
> >
> > Nitpick: can you print the mnemotechnic instead of the error value?

…but instead pointed to that line when writing my comment.

> >
> >                 netdev_err(ndev, "failed to enable CAN clock, error
> > %pe\n", ERR_PTR(err));
>
> Thanks for the suggestion!
>
> As you're pointing to pre-existing code, and there are several cases
> like that, I sent a follow-up patch to fix all of them at once:
> https://lore.kernel.org/r/8a39f99fc28967134826dff141b51a5df824b034.1678349267.git.geert+renesas@glider.be

Thanks for fixing the pre-existing code as well! My intent was to
point to the newly introduced code but inadvertently commented on the
old code.