* [PATCHv1 0/2] Fix RK3588 error prints @ 2023-03-17 17:42 Sebastian Reichel 2023-03-17 17:42 ` [PATCHv1 1/2] net: ethernet: stmmac: dwmac-rk: fix optional clock handling Sebastian Reichel 2023-03-17 17:42 ` [PATCHv1 2/2] net: ethernet: stmmac: dwmac-rk: fix optional phy regulator handling Sebastian Reichel 0 siblings, 2 replies; 8+ messages in thread From: Sebastian Reichel @ 2023-03-17 17:42 UTC (permalink / raw) To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin, netdev, linux-kernel, Sebastian Reichel, kernel Hi, On 6.3-rc1 RK3588 EVB1 board currently prints 5 error level kernel messages. Four of them are from the stmmac driver and taken care of by this series. -- Sebastian Sebastian Reichel (2): net: ethernet: stmmac: dwmac-rk: fix optional clock handling net: ethernet: stmmac: dwmac-rk: fix optional phy regulator handling .../net/ethernet/stmicro/stmmac/dwmac-rk.c | 58 +++++++++---------- 1 file changed, 28 insertions(+), 30 deletions(-) -- 2.39.2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCHv1 1/2] net: ethernet: stmmac: dwmac-rk: fix optional clock handling 2023-03-17 17:42 [PATCHv1 0/2] Fix RK3588 error prints Sebastian Reichel @ 2023-03-17 17:42 ` Sebastian Reichel 2023-03-17 18:24 ` Piotr Raczynski 2023-03-22 3:10 ` Jakub Kicinski 2023-03-17 17:42 ` [PATCHv1 2/2] net: ethernet: stmmac: dwmac-rk: fix optional phy regulator handling Sebastian Reichel 1 sibling, 2 replies; 8+ messages in thread From: Sebastian Reichel @ 2023-03-17 17:42 UTC (permalink / raw) To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin, netdev, linux-kernel, Sebastian Reichel, kernel Right now any clock errors are printed and otherwise ignored. This has multiple disadvantages: 1. it prints errors for clocks that do not exist (e.g. rk3588 reports errors for "mac_clk_rx", "mac_clk_tx" and "clk_mac_speed") 2. it does not handle errors like -EPROBE_DEFER correctly This series fixes it by switching to devm_clk_get_optional(), so that missing clocks are not considered an error and then passing on any other errors using dev_err_probe(). Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> --- .../net/ethernet/stmicro/stmmac/dwmac-rk.c | 47 ++++++++++--------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c index 4b8fd11563e4..126812cd17e6 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c @@ -1479,49 +1479,50 @@ static int rk_gmac_clk_init(struct plat_stmmacenet_data *plat) bsp_priv->clk_enabled = false; - bsp_priv->mac_clk_rx = devm_clk_get(dev, "mac_clk_rx"); + bsp_priv->mac_clk_rx = devm_clk_get_optional(dev, "mac_clk_rx"); if (IS_ERR(bsp_priv->mac_clk_rx)) - dev_err(dev, "cannot get clock %s\n", - "mac_clk_rx"); + return dev_err_probe(dev, PTR_ERR(bsp_priv->mac_clk_rx), + "cannot get clock %s\n", "mac_clk_rx"); - bsp_priv->mac_clk_tx = devm_clk_get(dev, "mac_clk_tx"); + bsp_priv->mac_clk_tx = devm_clk_get_optional(dev, "mac_clk_tx"); if (IS_ERR(bsp_priv->mac_clk_tx)) - dev_err(dev, "cannot get clock %s\n", - "mac_clk_tx"); + return dev_err_probe(dev, PTR_ERR(bsp_priv->mac_clk_tx), + "cannot get clock %s\n", "mac_clk_tx"); - bsp_priv->aclk_mac = devm_clk_get(dev, "aclk_mac"); + bsp_priv->aclk_mac = devm_clk_get_optional(dev, "aclk_mac"); if (IS_ERR(bsp_priv->aclk_mac)) - dev_err(dev, "cannot get clock %s\n", - "aclk_mac"); + return dev_err_probe(dev, PTR_ERR(bsp_priv->aclk_mac), + "cannot get clock %s\n", "aclk_mac"); - bsp_priv->pclk_mac = devm_clk_get(dev, "pclk_mac"); + bsp_priv->pclk_mac = devm_clk_get_optional(dev, "pclk_mac"); if (IS_ERR(bsp_priv->pclk_mac)) - dev_err(dev, "cannot get clock %s\n", - "pclk_mac"); + return dev_err_probe(dev, PTR_ERR(bsp_priv->pclk_mac), + "cannot get clock %s\n", "pclk_mac"); - bsp_priv->clk_mac = devm_clk_get(dev, "stmmaceth"); + bsp_priv->clk_mac = devm_clk_get_optional(dev, "stmmaceth"); if (IS_ERR(bsp_priv->clk_mac)) - dev_err(dev, "cannot get clock %s\n", - "stmmaceth"); + return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac), + "cannot get clock %s\n", "stmmaceth"); if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII) { - bsp_priv->clk_mac_ref = devm_clk_get(dev, "clk_mac_ref"); + bsp_priv->clk_mac_ref = devm_clk_get_optional(dev, "clk_mac_ref"); if (IS_ERR(bsp_priv->clk_mac_ref)) - dev_err(dev, "cannot get clock %s\n", - "clk_mac_ref"); + return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac_ref), + "cannot get clock %s\n", "clk_mac_ref"); if (!bsp_priv->clock_input) { bsp_priv->clk_mac_refout = - devm_clk_get(dev, "clk_mac_refout"); + devm_clk_get_optional(dev, "clk_mac_refout"); if (IS_ERR(bsp_priv->clk_mac_refout)) - dev_err(dev, "cannot get clock %s\n", - "clk_mac_refout"); + return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac_refout), + "cannot get clock %s\n", "clk_mac_refout"); } } - bsp_priv->clk_mac_speed = devm_clk_get(dev, "clk_mac_speed"); + bsp_priv->clk_mac_speed = devm_clk_get_optional(dev, "clk_mac_speed"); if (IS_ERR(bsp_priv->clk_mac_speed)) - dev_err(dev, "cannot get clock %s\n", "clk_mac_speed"); + return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac_speed), + "cannot get clock %s\n", "clk_mac_speed"); if (bsp_priv->clock_input) { dev_info(dev, "clock input from PHY\n"); -- 2.39.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCHv1 1/2] net: ethernet: stmmac: dwmac-rk: fix optional clock handling 2023-03-17 17:42 ` [PATCHv1 1/2] net: ethernet: stmmac: dwmac-rk: fix optional clock handling Sebastian Reichel @ 2023-03-17 18:24 ` Piotr Raczynski 2023-03-17 20:04 ` Sebastian Reichel 2023-03-22 3:10 ` Jakub Kicinski 1 sibling, 1 reply; 8+ messages in thread From: Piotr Raczynski @ 2023-03-17 18:24 UTC (permalink / raw) To: Sebastian Reichel Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin, netdev, linux-kernel, kernel On Fri, Mar 17, 2023 at 06:42:42PM +0100, Sebastian Reichel wrote: > Right now any clock errors are printed and otherwise ignored. > This has multiple disadvantages: > > 1. it prints errors for clocks that do not exist (e.g. rk3588 > reports errors for "mac_clk_rx", "mac_clk_tx" and "clk_mac_speed") > > 2. it does not handle errors like -EPROBE_DEFER correctly > > This series fixes it by switching to devm_clk_get_optional(), > so that missing clocks are not considered an error and then > passing on any other errors using dev_err_probe(). > Fixes tag would help here. Piotr > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > --- > .../net/ethernet/stmicro/stmmac/dwmac-rk.c | 47 ++++++++++--------- > 1 file changed, 24 insertions(+), 23 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > index 4b8fd11563e4..126812cd17e6 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > @@ -1479,49 +1479,50 @@ static int rk_gmac_clk_init(struct plat_stmmacenet_data *plat) > > bsp_priv->clk_enabled = false; > > - bsp_priv->mac_clk_rx = devm_clk_get(dev, "mac_clk_rx"); > + bsp_priv->mac_clk_rx = devm_clk_get_optional(dev, "mac_clk_rx"); > if (IS_ERR(bsp_priv->mac_clk_rx)) > - dev_err(dev, "cannot get clock %s\n", > - "mac_clk_rx"); > + return dev_err_probe(dev, PTR_ERR(bsp_priv->mac_clk_rx), > + "cannot get clock %s\n", "mac_clk_rx"); > > - bsp_priv->mac_clk_tx = devm_clk_get(dev, "mac_clk_tx"); > + bsp_priv->mac_clk_tx = devm_clk_get_optional(dev, "mac_clk_tx"); > if (IS_ERR(bsp_priv->mac_clk_tx)) > - dev_err(dev, "cannot get clock %s\n", > - "mac_clk_tx"); > + return dev_err_probe(dev, PTR_ERR(bsp_priv->mac_clk_tx), > + "cannot get clock %s\n", "mac_clk_tx"); > > - bsp_priv->aclk_mac = devm_clk_get(dev, "aclk_mac"); > + bsp_priv->aclk_mac = devm_clk_get_optional(dev, "aclk_mac"); > if (IS_ERR(bsp_priv->aclk_mac)) > - dev_err(dev, "cannot get clock %s\n", > - "aclk_mac"); > + return dev_err_probe(dev, PTR_ERR(bsp_priv->aclk_mac), > + "cannot get clock %s\n", "aclk_mac"); > > - bsp_priv->pclk_mac = devm_clk_get(dev, "pclk_mac"); > + bsp_priv->pclk_mac = devm_clk_get_optional(dev, "pclk_mac"); > if (IS_ERR(bsp_priv->pclk_mac)) > - dev_err(dev, "cannot get clock %s\n", > - "pclk_mac"); > + return dev_err_probe(dev, PTR_ERR(bsp_priv->pclk_mac), > + "cannot get clock %s\n", "pclk_mac"); > > - bsp_priv->clk_mac = devm_clk_get(dev, "stmmaceth"); > + bsp_priv->clk_mac = devm_clk_get_optional(dev, "stmmaceth"); > if (IS_ERR(bsp_priv->clk_mac)) > - dev_err(dev, "cannot get clock %s\n", > - "stmmaceth"); > + return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac), > + "cannot get clock %s\n", "stmmaceth"); > > if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII) { > - bsp_priv->clk_mac_ref = devm_clk_get(dev, "clk_mac_ref"); > + bsp_priv->clk_mac_ref = devm_clk_get_optional(dev, "clk_mac_ref"); > if (IS_ERR(bsp_priv->clk_mac_ref)) > - dev_err(dev, "cannot get clock %s\n", > - "clk_mac_ref"); > + return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac_ref), > + "cannot get clock %s\n", "clk_mac_ref"); > > if (!bsp_priv->clock_input) { > bsp_priv->clk_mac_refout = > - devm_clk_get(dev, "clk_mac_refout"); > + devm_clk_get_optional(dev, "clk_mac_refout"); > if (IS_ERR(bsp_priv->clk_mac_refout)) > - dev_err(dev, "cannot get clock %s\n", > - "clk_mac_refout"); > + return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac_refout), > + "cannot get clock %s\n", "clk_mac_refout"); > } > } > > - bsp_priv->clk_mac_speed = devm_clk_get(dev, "clk_mac_speed"); > + bsp_priv->clk_mac_speed = devm_clk_get_optional(dev, "clk_mac_speed"); > if (IS_ERR(bsp_priv->clk_mac_speed)) > - dev_err(dev, "cannot get clock %s\n", "clk_mac_speed"); > + return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac_speed), > + "cannot get clock %s\n", "clk_mac_speed"); > > if (bsp_priv->clock_input) { > dev_info(dev, "clock input from PHY\n"); > -- > 2.39.2 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv1 1/2] net: ethernet: stmmac: dwmac-rk: fix optional clock handling 2023-03-17 18:24 ` Piotr Raczynski @ 2023-03-17 20:04 ` Sebastian Reichel 0 siblings, 0 replies; 8+ messages in thread From: Sebastian Reichel @ 2023-03-17 20:04 UTC (permalink / raw) To: Piotr Raczynski Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin, netdev, linux-kernel, kernel [-- Attachment #1: Type: text/plain, Size: 4919 bytes --] Hi, On Fri, Mar 17, 2023 at 07:24:31PM +0100, Piotr Raczynski wrote: > On Fri, Mar 17, 2023 at 06:42:42PM +0100, Sebastian Reichel wrote: > > Right now any clock errors are printed and otherwise ignored. > > This has multiple disadvantages: > > > > 1. it prints errors for clocks that do not exist (e.g. rk3588 > > reports errors for "mac_clk_rx", "mac_clk_tx" and "clk_mac_speed") > > > > 2. it does not handle errors like -EPROBE_DEFER correctly > > > > This series fixes it by switching to devm_clk_get_optional(), > > so that missing clocks are not considered an error and then > > passing on any other errors using dev_err_probe(). > > > > Fixes tag would help here. Fixes: 7ad269ea1a2b7 ("GMAC: add driver for Rockchip RK3288 SoCs integrated GMAC") That commit is from 2014-12-29. I only noticed the dev_err() messages on RK3588, so going via -next without backporting should be fine. Please tell me if I should resend with the Fixes tag. -- Sebastian > Piotr > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > > --- > > .../net/ethernet/stmicro/stmmac/dwmac-rk.c | 47 ++++++++++--------- > > 1 file changed, 24 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > > index 4b8fd11563e4..126812cd17e6 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > > @@ -1479,49 +1479,50 @@ static int rk_gmac_clk_init(struct plat_stmmacenet_data *plat) > > > > bsp_priv->clk_enabled = false; > > > > - bsp_priv->mac_clk_rx = devm_clk_get(dev, "mac_clk_rx"); > > + bsp_priv->mac_clk_rx = devm_clk_get_optional(dev, "mac_clk_rx"); > > if (IS_ERR(bsp_priv->mac_clk_rx)) > > - dev_err(dev, "cannot get clock %s\n", > > - "mac_clk_rx"); > > + return dev_err_probe(dev, PTR_ERR(bsp_priv->mac_clk_rx), > > + "cannot get clock %s\n", "mac_clk_rx"); > > > > - bsp_priv->mac_clk_tx = devm_clk_get(dev, "mac_clk_tx"); > > + bsp_priv->mac_clk_tx = devm_clk_get_optional(dev, "mac_clk_tx"); > > if (IS_ERR(bsp_priv->mac_clk_tx)) > > - dev_err(dev, "cannot get clock %s\n", > > - "mac_clk_tx"); > > + return dev_err_probe(dev, PTR_ERR(bsp_priv->mac_clk_tx), > > + "cannot get clock %s\n", "mac_clk_tx"); > > > > - bsp_priv->aclk_mac = devm_clk_get(dev, "aclk_mac"); > > + bsp_priv->aclk_mac = devm_clk_get_optional(dev, "aclk_mac"); > > if (IS_ERR(bsp_priv->aclk_mac)) > > - dev_err(dev, "cannot get clock %s\n", > > - "aclk_mac"); > > + return dev_err_probe(dev, PTR_ERR(bsp_priv->aclk_mac), > > + "cannot get clock %s\n", "aclk_mac"); > > > > - bsp_priv->pclk_mac = devm_clk_get(dev, "pclk_mac"); > > + bsp_priv->pclk_mac = devm_clk_get_optional(dev, "pclk_mac"); > > if (IS_ERR(bsp_priv->pclk_mac)) > > - dev_err(dev, "cannot get clock %s\n", > > - "pclk_mac"); > > + return dev_err_probe(dev, PTR_ERR(bsp_priv->pclk_mac), > > + "cannot get clock %s\n", "pclk_mac"); > > > > - bsp_priv->clk_mac = devm_clk_get(dev, "stmmaceth"); > > + bsp_priv->clk_mac = devm_clk_get_optional(dev, "stmmaceth"); > > if (IS_ERR(bsp_priv->clk_mac)) > > - dev_err(dev, "cannot get clock %s\n", > > - "stmmaceth"); > > + return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac), > > + "cannot get clock %s\n", "stmmaceth"); > > > > if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII) { > > - bsp_priv->clk_mac_ref = devm_clk_get(dev, "clk_mac_ref"); > > + bsp_priv->clk_mac_ref = devm_clk_get_optional(dev, "clk_mac_ref"); > > if (IS_ERR(bsp_priv->clk_mac_ref)) > > - dev_err(dev, "cannot get clock %s\n", > > - "clk_mac_ref"); > > + return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac_ref), > > + "cannot get clock %s\n", "clk_mac_ref"); > > > > if (!bsp_priv->clock_input) { > > bsp_priv->clk_mac_refout = > > - devm_clk_get(dev, "clk_mac_refout"); > > + devm_clk_get_optional(dev, "clk_mac_refout"); > > if (IS_ERR(bsp_priv->clk_mac_refout)) > > - dev_err(dev, "cannot get clock %s\n", > > - "clk_mac_refout"); > > + return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac_refout), > > + "cannot get clock %s\n", "clk_mac_refout"); > > } > > } > > > > - bsp_priv->clk_mac_speed = devm_clk_get(dev, "clk_mac_speed"); > > + bsp_priv->clk_mac_speed = devm_clk_get_optional(dev, "clk_mac_speed"); > > if (IS_ERR(bsp_priv->clk_mac_speed)) > > - dev_err(dev, "cannot get clock %s\n", "clk_mac_speed"); > > + return dev_err_probe(dev, PTR_ERR(bsp_priv->clk_mac_speed), > > + "cannot get clock %s\n", "clk_mac_speed"); > > > > if (bsp_priv->clock_input) { > > dev_info(dev, "clock input from PHY\n"); > > -- > > 2.39.2 > > > > -- > To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv1 1/2] net: ethernet: stmmac: dwmac-rk: fix optional clock handling 2023-03-17 17:42 ` [PATCHv1 1/2] net: ethernet: stmmac: dwmac-rk: fix optional clock handling Sebastian Reichel 2023-03-17 18:24 ` Piotr Raczynski @ 2023-03-22 3:10 ` Jakub Kicinski 1 sibling, 0 replies; 8+ messages in thread From: Jakub Kicinski @ 2023-03-22 3:10 UTC (permalink / raw) To: Sebastian Reichel Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, David S. Miller, Eric Dumazet, Paolo Abeni, Maxime Coquelin, netdev, linux-kernel, kernel On Fri, 17 Mar 2023 18:42:42 +0100 Sebastian Reichel wrote: > - bsp_priv->mac_clk_rx = devm_clk_get(dev, "mac_clk_rx"); > + bsp_priv->mac_clk_rx = devm_clk_get_optional(dev, "mac_clk_rx"); > if (IS_ERR(bsp_priv->mac_clk_rx)) > - dev_err(dev, "cannot get clock %s\n", > - "mac_clk_rx"); > + return dev_err_probe(dev, PTR_ERR(bsp_priv->mac_clk_rx), > + "cannot get clock %s\n", "mac_clk_rx"); > > - bsp_priv->mac_clk_tx = devm_clk_get(dev, "mac_clk_tx"); > + bsp_priv->mac_clk_tx = devm_clk_get_optional(dev, "mac_clk_tx"); > if (IS_ERR(bsp_priv->mac_clk_tx)) > - dev_err(dev, "cannot get clock %s\n", > - "mac_clk_tx"); > + return dev_err_probe(dev, PTR_ERR(bsp_priv->mac_clk_tx), > + "cannot get clock %s\n", "mac_clk_tx"); > > - bsp_priv->aclk_mac = devm_clk_get(dev, "aclk_mac"); > + bsp_priv->aclk_mac = devm_clk_get_optional(dev, "aclk_mac"); > if (IS_ERR(bsp_priv->aclk_mac)) > - dev_err(dev, "cannot get clock %s\n", > - "aclk_mac"); > + return dev_err_probe(dev, PTR_ERR(bsp_priv->aclk_mac), > + "cannot get clock %s\n", "aclk_mac"); Can we turn this into a loop struct { struct whatever **ptr; const char *name; } clocks[] = { { &bsp_priv->mac_clk_rx, "mac_clk_rx" }, { &bsp_priv->mac_clk_tx, "mac_clk_tx" }, ... } for (i=0; i < ARRAY_SIZE...) { *clocks[i]->ptr = devm_clk_get_optional(dev, clocks[i]->name); if (IS_ERR(*clocks[i]->ptr)) return dev_err_probe(dev, PTR_ERR(*clocks[i]->ptr), "cannot get clock %s\n", *clocks[i]->name); } ? Or alternatively inline the name of the clock into the error message? Right now the %s format printing looks neither here nor there, and also the continuation line is misaligned (should start right under "dev"). FWIW seems like it should be fine for net-next without the fixes tag. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCHv1 2/2] net: ethernet: stmmac: dwmac-rk: fix optional phy regulator handling 2023-03-17 17:42 [PATCHv1 0/2] Fix RK3588 error prints Sebastian Reichel 2023-03-17 17:42 ` [PATCHv1 1/2] net: ethernet: stmmac: dwmac-rk: fix optional clock handling Sebastian Reichel @ 2023-03-17 17:42 ` Sebastian Reichel 2023-03-17 18:16 ` Piotr Raczynski 1 sibling, 1 reply; 8+ messages in thread From: Sebastian Reichel @ 2023-03-17 17:42 UTC (permalink / raw) To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin, netdev, linux-kernel, Sebastian Reichel, kernel The usual devm_regulator_get() call already handles "optional" regulators by returning a valid dummy and printing a warning that the dummy regulator should be described properly. This code open coded the same behaviour, but masked any errors that are not -EPROBE_DEFER and is quite noisy. This change effectively unmasks and propagates regulators errors not involving -ENODEV, downgrades the error print to warning level if no regulator is specified and captures the probe defer message for /sys/kernel/debug/devices_deferred. Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> --- drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c index 126812cd17e6..01de0174fa18 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c @@ -1680,14 +1680,11 @@ static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev, } } - bsp_priv->regulator = devm_regulator_get_optional(dev, "phy"); + bsp_priv->regulator = devm_regulator_get(dev, "phy"); if (IS_ERR(bsp_priv->regulator)) { - if (PTR_ERR(bsp_priv->regulator) == -EPROBE_DEFER) { - dev_err(dev, "phy regulator is not available yet, deferred probing\n"); - return ERR_PTR(-EPROBE_DEFER); - } - dev_err(dev, "no regulator found\n"); - bsp_priv->regulator = NULL; + ret = PTR_ERR(bsp_priv->regulator); + dev_err_probe(dev, ret, "failed to get phy regulator\n"); + return ERR_PTR(ret); } ret = of_property_read_string(dev->of_node, "clock_in_out", &strings); -- 2.39.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCHv1 2/2] net: ethernet: stmmac: dwmac-rk: fix optional phy regulator handling 2023-03-17 17:42 ` [PATCHv1 2/2] net: ethernet: stmmac: dwmac-rk: fix optional phy regulator handling Sebastian Reichel @ 2023-03-17 18:16 ` Piotr Raczynski 2023-03-17 20:12 ` Sebastian Reichel 0 siblings, 1 reply; 8+ messages in thread From: Piotr Raczynski @ 2023-03-17 18:16 UTC (permalink / raw) To: Sebastian Reichel Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin, netdev, linux-kernel, kernel On Fri, Mar 17, 2023 at 06:42:43PM +0100, Sebastian Reichel wrote: > The usual devm_regulator_get() call already handles "optional" > regulators by returning a valid dummy and printing a warning > that the dummy regulator should be described properly. This > code open coded the same behaviour, but masked any errors that > are not -EPROBE_DEFER and is quite noisy. > > This change effectively unmasks and propagates regulators errors > not involving -ENODEV, downgrades the error print to warning level > if no regulator is specified and captures the probe defer message > for /sys/kernel/debug/devices_deferred. Code looks fine, however this seems like a fix, then Fixes tag would be nice. Also target tree (net?) should be specified. > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > --- > drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > index 126812cd17e6..01de0174fa18 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > @@ -1680,14 +1680,11 @@ static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev, > } > } > > - bsp_priv->regulator = devm_regulator_get_optional(dev, "phy"); > + bsp_priv->regulator = devm_regulator_get(dev, "phy"); > if (IS_ERR(bsp_priv->regulator)) { > - if (PTR_ERR(bsp_priv->regulator) == -EPROBE_DEFER) { > - dev_err(dev, "phy regulator is not available yet, deferred probing\n"); > - return ERR_PTR(-EPROBE_DEFER); > - } > - dev_err(dev, "no regulator found\n"); > - bsp_priv->regulator = NULL; > + ret = PTR_ERR(bsp_priv->regulator); > + dev_err_probe(dev, ret, "failed to get phy regulator\n"); > + return ERR_PTR(ret); > } > > ret = of_property_read_string(dev->of_node, "clock_in_out", &strings); > -- > 2.39.2 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv1 2/2] net: ethernet: stmmac: dwmac-rk: fix optional phy regulator handling 2023-03-17 18:16 ` Piotr Raczynski @ 2023-03-17 20:12 ` Sebastian Reichel 0 siblings, 0 replies; 8+ messages in thread From: Sebastian Reichel @ 2023-03-17 20:12 UTC (permalink / raw) To: Piotr Raczynski Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin, netdev, linux-kernel, kernel [-- Attachment #1: Type: text/plain, Size: 2424 bytes --] Hi, On Fri, Mar 17, 2023 at 07:16:50PM +0100, Piotr Raczynski wrote: > On Fri, Mar 17, 2023 at 06:42:43PM +0100, Sebastian Reichel wrote: > > The usual devm_regulator_get() call already handles "optional" > > regulators by returning a valid dummy and printing a warning > > that the dummy regulator should be described properly. This > > code open coded the same behaviour, but masked any errors that > > are not -EPROBE_DEFER and is quite noisy. > > > > This change effectively unmasks and propagates regulators errors > > not involving -ENODEV, downgrades the error print to warning level > > if no regulator is specified and captures the probe defer message > > for /sys/kernel/debug/devices_deferred. > > Code looks fine, however this seems like a fix, then Fixes tag would > be nice. Also target tree (net?) should be specified. Fixes: 2e12f536635f8 ("net: stmmac: dwmac-rk: Use standard devicetree property for phy regulator") net-next should be good enough. -- Sebastian > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > > --- > > drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 11 ++++------- > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > > index 126812cd17e6..01de0174fa18 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > > @@ -1680,14 +1680,11 @@ static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev, > > } > > } > > > > - bsp_priv->regulator = devm_regulator_get_optional(dev, "phy"); > > + bsp_priv->regulator = devm_regulator_get(dev, "phy"); > > if (IS_ERR(bsp_priv->regulator)) { > > - if (PTR_ERR(bsp_priv->regulator) == -EPROBE_DEFER) { > > - dev_err(dev, "phy regulator is not available yet, deferred probing\n"); > > - return ERR_PTR(-EPROBE_DEFER); > > - } > > - dev_err(dev, "no regulator found\n"); > > - bsp_priv->regulator = NULL; > > + ret = PTR_ERR(bsp_priv->regulator); > > + dev_err_probe(dev, ret, "failed to get phy regulator\n"); > > + return ERR_PTR(ret); > > } > > > > ret = of_property_read_string(dev->of_node, "clock_in_out", &strings); > > -- > > 2.39.2 > > > > -- > To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-03-22 3:10 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-03-17 17:42 [PATCHv1 0/2] Fix RK3588 error prints Sebastian Reichel 2023-03-17 17:42 ` [PATCHv1 1/2] net: ethernet: stmmac: dwmac-rk: fix optional clock handling Sebastian Reichel 2023-03-17 18:24 ` Piotr Raczynski 2023-03-17 20:04 ` Sebastian Reichel 2023-03-22 3:10 ` Jakub Kicinski 2023-03-17 17:42 ` [PATCHv1 2/2] net: ethernet: stmmac: dwmac-rk: fix optional phy regulator handling Sebastian Reichel 2023-03-17 18:16 ` Piotr Raczynski 2023-03-17 20:12 ` Sebastian Reichel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).