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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Vincent,

On Wed, Mar 8, 2023 at 4:55 PM Vincent Mailhol
<vincent.mailhol@xxxxxxxxx> wrote:
> On Wed. 8 Mar. 2023 at 22:20, Geert Uytterhoeven
> <geert+renesas@xxxxxxxxx> 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@xxxxxxxxx>
> > Reviewed-by: Simon Horman <simon.horman@xxxxxxxxxxxx>
>
> I have one nitpick (see below). Aside from that:
> Reviewed-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>

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);
> > +               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?
>
>                 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@xxxxxxxxx

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux