| Message ID | 8a39f99fc28967134826dff141b51a5df824b034.1678349267.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: rcar_canfd: Print mnemotechnic error codes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
On Thu, Mar 09, 2023 at 09:10:22AM +0100, Geert Uytterhoeven wrote: > Replace printed numerical error codes by mnemotechnic error codes, to > improve the user experience in case of errors. > > While at it, drop printing of an error message in case of out-of-memory, > as the core memory allocation code already takes care of this. > > Suggested-by: Vincent Mailhol <vincent.mailhol@gmail.com> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Reviewed-by: Simon Horman <simon.horman@corigine.com>
On Thu. 9 Mar. 2023 at 17:10, Geert Uytterhoeven <geert+renesas@glider.be> wrote: > Replace printed numerical error codes by mnemotechnic error codes, to > improve the user experience in case of errors. > > While at it, drop printing of an error message in case of out-of-memory, > as the core memory allocation code already takes care of this. > > Suggested-by: Vincent Mailhol <vincent.mailhol@gmail.com> ^^^^^^^^^^^^^^^^^^^^^^^^^ Can you use my other address? Suggested-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> Thanks for the patch. Same as before, I have one nitpick on the already existing code (c.f. below). You can add my review tag in next version: Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > This depends on "[PATCH v2] can: rcar_canfd: Add transceiver support" > https://lore.kernel.org/r/e825b50a843ffe40e33f34e4d858c07c1b2ff259.1678280913.git.geert+renesas@glider.be > --- > drivers/net/can/rcar/rcar_canfd.c | 43 +++++++++++++++---------------- > 1 file changed, 21 insertions(+), 22 deletions(-) > > diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c > index 6df9a259e5e4f92c..bc75da349676d867 100644 > --- a/drivers/net/can/rcar/rcar_canfd.c > +++ b/drivers/net/can/rcar/rcar_canfd.c > @@ -1417,20 +1417,20 @@ static int rcar_canfd_open(struct net_device *ndev) > > err = phy_power_on(priv->transceiver); > if (err) { > - netdev_err(ndev, "failed to power on PHY, error %d\n", err); > + netdev_err(ndev, "failed to power on PHY, %pe\n", ERR_PTR(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); > + netdev_err(ndev, "failed to enable CAN clock, %pe\n", ERR_PTR(err)); > goto out_phy; > } > > err = open_candev(ndev); > if (err) { > - netdev_err(ndev, "open_candev() failed, error %d\n", err); > + netdev_err(ndev, "open_candev() failed, %pe\n", ERR_PTR(err)); > goto out_can_clock; > } > > @@ -1731,10 +1731,9 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch, > int err = -ENODEV; > > ndev = alloc_candev(sizeof(*priv), RCANFD_FIFO_DEPTH); > - if (!ndev) { > - dev_err(dev, "alloc_candev() failed\n"); > + if (!ndev) > return -ENOMEM; > - } > + > priv = netdev_priv(ndev); > > ndev->netdev_ops = &rcar_canfd_netdev_ops; > @@ -1777,8 +1776,8 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch, > rcar_canfd_channel_err_interrupt, 0, > irq_name, priv); > if (err) { > - dev_err(dev, "devm_request_irq CH Err(%d) failed, error %d\n", > - err_irq, err); > + dev_err(dev, "devm_request_irq CH Err(%d) failed, %pe\n", ^^^^ From the Linux coding style: Printing numbers in parentheses (%d) adds no value and should be avoided. Ref: https://www.kernel.org/doc/html/v4.10/process/coding-style.html#printing-kernel-messages One more time, this is already existing code, so bonus points if you fix this as well, but I will not blame you if you feel lazy and keep the patch as is. > + err_irq, ERR_PTR(err)); > goto fail; > } > irq_name = devm_kasprintf(dev, GFP_KERNEL, "canfd.ch%d_trx", > @@ -1791,8 +1790,8 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch, > rcar_canfd_channel_tx_interrupt, 0, > irq_name, priv); > if (err) { > - dev_err(dev, "devm_request_irq Tx (%d) failed, error %d\n", > - tx_irq, err); > + dev_err(dev, "devm_request_irq Tx (%d) failed, %pe\n", > + tx_irq, ERR_PTR(err)); > goto fail; > } > } > @@ -1823,7 +1822,7 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch, > gpriv->ch[priv->channel] = priv; > err = register_candev(ndev); > if (err) { > - dev_err(dev, "register_candev() failed, error %d\n", err); > + dev_err(dev, "register_candev() failed, %pe\n", ERR_PTR(err)); > goto fail_candev; > } > dev_info(dev, "device registered (channel %u)\n", priv->channel); > @@ -1967,16 +1966,16 @@ static int rcar_canfd_probe(struct platform_device *pdev) > rcar_canfd_channel_interrupt, 0, > "canfd.ch_int", gpriv); > if (err) { > - dev_err(dev, "devm_request_irq(%d) failed, error %d\n", > - ch_irq, err); > + dev_err(dev, "devm_request_irq(%d) failed, %pe\n", Same as above. > + ch_irq, ERR_PTR(err)); > goto fail_dev; > } > > err = devm_request_irq(dev, g_irq, rcar_canfd_global_interrupt, > 0, "canfd.g_int", gpriv); > if (err) { > - dev_err(dev, "devm_request_irq(%d) failed, error %d\n", > - g_irq, err); > + dev_err(dev, "devm_request_irq(%d) failed, %pe\n", Same as above. > + g_irq, ERR_PTR(err)); > goto fail_dev; > } > } else { > @@ -1985,8 +1984,8 @@ static int rcar_canfd_probe(struct platform_device *pdev) > "canfd.g_recc", gpriv); > > if (err) { > - dev_err(dev, "devm_request_irq(%d) failed, error %d\n", > - g_recc_irq, err); > + dev_err(dev, "devm_request_irq(%d) failed, %pe\n", > + g_recc_irq, ERR_PTR(err)); > goto fail_dev; > } > > @@ -1994,8 +1993,8 @@ static int rcar_canfd_probe(struct platform_device *pdev) > rcar_canfd_global_err_interrupt, 0, > "canfd.g_err", gpriv); > if (err) { > - dev_err(dev, "devm_request_irq(%d) failed, error %d\n", > - g_err_irq, err); > + dev_err(dev, "devm_request_irq(%d) failed, %pe\n", Same as above. > + g_err_irq, ERR_PTR(err)); > goto fail_dev; > } > } > @@ -2012,14 +2011,14 @@ static int rcar_canfd_probe(struct platform_device *pdev) > /* Enable peripheral clock for register access */ > err = clk_prepare_enable(gpriv->clkp); > if (err) { > - dev_err(dev, "failed to enable peripheral clock, error %d\n", > - err); > + dev_err(dev, "failed to enable peripheral clock, %pe\n", > + ERR_PTR(err)); > goto fail_reset; > } > > err = rcar_canfd_reset_controller(gpriv); > if (err) { > - dev_err(dev, "reset controller failed\n"); > + dev_err(dev, "reset controller failed, %pe\n", ERR_PTR(err)); > goto fail_clk; > } > > -- > 2.34.1 >
diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c index 6df9a259e5e4f92c..bc75da349676d867 100644 --- a/drivers/net/can/rcar/rcar_canfd.c +++ b/drivers/net/can/rcar/rcar_canfd.c @@ -1417,20 +1417,20 @@ static int rcar_canfd_open(struct net_device *ndev) err = phy_power_on(priv->transceiver); if (err) { - netdev_err(ndev, "failed to power on PHY, error %d\n", err); + netdev_err(ndev, "failed to power on PHY, %pe\n", ERR_PTR(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); + netdev_err(ndev, "failed to enable CAN clock, %pe\n", ERR_PTR(err)); goto out_phy; } err = open_candev(ndev); if (err) { - netdev_err(ndev, "open_candev() failed, error %d\n", err); + netdev_err(ndev, "open_candev() failed, %pe\n", ERR_PTR(err)); goto out_can_clock; } @@ -1731,10 +1731,9 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch, int err = -ENODEV; ndev = alloc_candev(sizeof(*priv), RCANFD_FIFO_DEPTH); - if (!ndev) { - dev_err(dev, "alloc_candev() failed\n"); + if (!ndev) return -ENOMEM; - } + priv = netdev_priv(ndev); ndev->netdev_ops = &rcar_canfd_netdev_ops; @@ -1777,8 +1776,8 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch, rcar_canfd_channel_err_interrupt, 0, irq_name, priv); if (err) { - dev_err(dev, "devm_request_irq CH Err(%d) failed, error %d\n", - err_irq, err); + dev_err(dev, "devm_request_irq CH Err(%d) failed, %pe\n", + err_irq, ERR_PTR(err)); goto fail; } irq_name = devm_kasprintf(dev, GFP_KERNEL, "canfd.ch%d_trx", @@ -1791,8 +1790,8 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch, rcar_canfd_channel_tx_interrupt, 0, irq_name, priv); if (err) { - dev_err(dev, "devm_request_irq Tx (%d) failed, error %d\n", - tx_irq, err); + dev_err(dev, "devm_request_irq Tx (%d) failed, %pe\n", + tx_irq, ERR_PTR(err)); goto fail; } } @@ -1823,7 +1822,7 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch, gpriv->ch[priv->channel] = priv; err = register_candev(ndev); if (err) { - dev_err(dev, "register_candev() failed, error %d\n", err); + dev_err(dev, "register_candev() failed, %pe\n", ERR_PTR(err)); goto fail_candev; } dev_info(dev, "device registered (channel %u)\n", priv->channel); @@ -1967,16 +1966,16 @@ static int rcar_canfd_probe(struct platform_device *pdev) rcar_canfd_channel_interrupt, 0, "canfd.ch_int", gpriv); if (err) { - dev_err(dev, "devm_request_irq(%d) failed, error %d\n", - ch_irq, err); + dev_err(dev, "devm_request_irq(%d) failed, %pe\n", + ch_irq, ERR_PTR(err)); goto fail_dev; } err = devm_request_irq(dev, g_irq, rcar_canfd_global_interrupt, 0, "canfd.g_int", gpriv); if (err) { - dev_err(dev, "devm_request_irq(%d) failed, error %d\n", - g_irq, err); + dev_err(dev, "devm_request_irq(%d) failed, %pe\n", + g_irq, ERR_PTR(err)); goto fail_dev; } } else { @@ -1985,8 +1984,8 @@ static int rcar_canfd_probe(struct platform_device *pdev) "canfd.g_recc", gpriv); if (err) { - dev_err(dev, "devm_request_irq(%d) failed, error %d\n", - g_recc_irq, err); + dev_err(dev, "devm_request_irq(%d) failed, %pe\n", + g_recc_irq, ERR_PTR(err)); goto fail_dev; } @@ -1994,8 +1993,8 @@ static int rcar_canfd_probe(struct platform_device *pdev) rcar_canfd_global_err_interrupt, 0, "canfd.g_err", gpriv); if (err) { - dev_err(dev, "devm_request_irq(%d) failed, error %d\n", - g_err_irq, err); + dev_err(dev, "devm_request_irq(%d) failed, %pe\n", + g_err_irq, ERR_PTR(err)); goto fail_dev; } } @@ -2012,14 +2011,14 @@ static int rcar_canfd_probe(struct platform_device *pdev) /* Enable peripheral clock for register access */ err = clk_prepare_enable(gpriv->clkp); if (err) { - dev_err(dev, "failed to enable peripheral clock, error %d\n", - err); + dev_err(dev, "failed to enable peripheral clock, %pe\n", + ERR_PTR(err)); goto fail_reset; } err = rcar_canfd_reset_controller(gpriv); if (err) { - dev_err(dev, "reset controller failed\n"); + dev_err(dev, "reset controller failed, %pe\n", ERR_PTR(err)); goto fail_clk; }
Replace printed numerical error codes by mnemotechnic error codes, to improve the user experience in case of errors. While at it, drop printing of an error message in case of out-of-memory, as the core memory allocation code already takes care of this. Suggested-by: Vincent Mailhol <vincent.mailhol@gmail.com> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- This depends on "[PATCH v2] can: rcar_canfd: Add transceiver support" https://lore.kernel.org/r/e825b50a843ffe40e33f34e4d858c07c1b2ff259.1678280913.git.geert+renesas@glider.be --- drivers/net/can/rcar/rcar_canfd.c | 43 +++++++++++++++---------------- 1 file changed, 21 insertions(+), 22 deletions(-)