| Message ID | 20250228165755.405138-3-heiko@sntech.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/rockchip: lvds: probe logging improvements | expand |
Hi Heiko, On 2/28/25 5:57 PM, Heiko Stuebner wrote: > From: Heiko Stuebner <heiko.stuebner@cherry.de> > > Commit 52d11c863ac9 ("drm/rockchip: lvds: do not print scary message when > probing defer") already started hiding scary messages that are not relevant > if the requested supply just returned EPROBE_DEFER, but there are more > possible sources - like the phy. > > So modernize the whole logging in the probe path by replacing the > remaining deprecated DRM_DEV_ERROR with appropriate dev_err(_probe) > and drm_err calls. > > The distinction here is that all messages talking about mishaps of the > lvds element use dev_err(_probe) while messages caused by interaction > with the main Rockchip drm-device use drm_err. > > Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de> > --- > drivers/gpu/drm/rockchip/rockchip_lvds.c | 59 +++++++++++------------- > 1 file changed, 26 insertions(+), 33 deletions(-) > > diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c > index c19b7b1f6cb5..2b5101a764c8 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c > +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c > @@ -453,10 +453,9 @@ static int rk3288_lvds_probe(struct platform_device *pdev, > return PTR_ERR(lvds->regs); > > lvds->pclk = devm_clk_get_prepared(lvds->dev, "pclk_lvds"); > - if (IS_ERR(lvds->pclk)) { > - DRM_DEV_ERROR(lvds->dev, "could not get pclk_lvds\n"); > - return PTR_ERR(lvds->pclk); > - } > + if (IS_ERR(lvds->pclk)) > + return dev_err_probe(lvds->dev, PTR_ERR(lvds->pclk), > + "could not get pclk_lvds\n"); > > lvds->pins = devm_kzalloc(lvds->dev, sizeof(*lvds->pins), > GFP_KERNEL); > @@ -465,14 +464,14 @@ static int rk3288_lvds_probe(struct platform_device *pdev, > > lvds->pins->p = devm_pinctrl_get(lvds->dev); > if (IS_ERR(lvds->pins->p)) { > - DRM_DEV_ERROR(lvds->dev, "no pinctrl handle\n"); > + dev_err(lvds->dev, "no pinctrl handle\n"); > devm_kfree(lvds->dev, lvds->pins); > lvds->pins = NULL; > } else { > lvds->pins->default_state = > pinctrl_lookup_state(lvds->pins->p, "lcdc"); > if (IS_ERR(lvds->pins->default_state)) { > - DRM_DEV_ERROR(lvds->dev, "no default pinctrl state\n"); > + dev_err(lvds->dev, "no default pinctrl state\n"); > devm_kfree(lvds->dev, lvds->pins); > lvds->pins = NULL; > } > @@ -547,11 +546,10 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master, > > lvds->drm_dev = drm_dev; > port = of_graph_get_port_by_id(dev->of_node, 1); > - if (!port) { > - DRM_DEV_ERROR(dev, > - "can't found port point, please init lvds panel port!\n"); > - return -EINVAL; > - } > + if (!port) > + return dev_err_probe(dev, -EINVAL, > + "can't found port point, please init lvds panel port!\n"); > + > for_each_child_of_node(port, endpoint) { > child_count++; > of_property_read_u32(endpoint, "reg", &endpoint_id); > @@ -563,7 +561,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master, > } > } > if (!child_count) { > - DRM_DEV_ERROR(dev, "lvds port does not have any children\n"); > + dev_err(dev, "lvds port does not have any children\n"); > ret = -EINVAL; What about ret = dev_err_probe(dev, -EINVAL, "lvds port does not have any children\n") ? > goto err_put_port; > } else if (ret) { > @@ -581,7 +579,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master, > lvds->output = rockchip_lvds_name_to_output(name); > > if (lvds->output < 0) { > - DRM_DEV_ERROR(dev, "invalid output type [%s]\n", name); > + dev_err(dev, "invalid output type [%s]\n", name); > ret = lvds->output; ret = dev_err_probe(dev, lvds->output, "invalid output type [%s]\n", name); maybe? > goto err_put_remote; > } > @@ -593,7 +591,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master, > lvds->format = rockchip_lvds_name_to_format(name); > > if (lvds->format < 0) { > - DRM_DEV_ERROR(dev, "invalid data-mapping format [%s]\n", name); > + dev_err(dev, "invalid data-mapping format [%s]\n", name); > ret = lvds->format; ret = dev_err_probe(dev, lvds->format, "invalid data-mapping format [%s]\n", name); maybe? > goto err_put_remote; > } > @@ -604,8 +602,8 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master, > > ret = drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_LVDS); > if (ret < 0) { > - DRM_DEV_ERROR(drm_dev->dev, > - "failed to initialize encoder: %d\n", ret); > + drm_err(drm_dev, > + "failed to initialize encoder: %d\n", ret); All the above are using dev_err, but starting here, it's drm_err, is that on purpose? > goto err_put_remote; > } > > @@ -618,8 +616,8 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master, > &rockchip_lvds_connector_funcs, > DRM_MODE_CONNECTOR_LVDS); > if (ret < 0) { > - DRM_DEV_ERROR(drm_dev->dev, > - "failed to initialize connector: %d\n", ret); > + drm_err(drm_dev, > + "failed to initialize connector: %d\n", ret); > goto err_free_encoder; > } > > @@ -633,9 +631,9 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master, > > connector = drm_bridge_connector_init(lvds->drm_dev, encoder); > if (IS_ERR(connector)) { > - DRM_DEV_ERROR(drm_dev->dev, > - "failed to initialize bridge connector: %pe\n", > - connector); > + drm_err(drm_dev, > + "failed to initialize bridge connector: %pe\n", > + connector); > ret = PTR_ERR(connector); What about using dev_err_probe() here as well? Looks ok to me otherwise! Cheers, Quentin
Am Freitag, 28. Februar 2025, 18:42:32 MEZ schrieb Quentin Schulz: > Hi Heiko, > > On 2/28/25 5:57 PM, Heiko Stuebner wrote: > > From: Heiko Stuebner <heiko.stuebner@cherry.de> > > > > Commit 52d11c863ac9 ("drm/rockchip: lvds: do not print scary message when > > probing defer") already started hiding scary messages that are not relevant > > if the requested supply just returned EPROBE_DEFER, but there are more > > possible sources - like the phy. > > > > So modernize the whole logging in the probe path by replacing the > > remaining deprecated DRM_DEV_ERROR with appropriate dev_err(_probe) > > and drm_err calls. > > > > The distinction here is that all messages talking about mishaps of the > > lvds element use dev_err(_probe) while messages caused by interaction > > with the main Rockchip drm-device use drm_err. > > > > Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de> > > @@ -604,8 +602,8 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master, > > > > ret = drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_LVDS); > > if (ret < 0) { > > - DRM_DEV_ERROR(drm_dev->dev, > > - "failed to initialize encoder: %d\n", ret); > > + drm_err(drm_dev, > > + "failed to initialize encoder: %d\n", ret); > > All the above are using dev_err, but starting here, it's drm_err, is > that on purpose? The last paragraph of the commit message was supposed to explain that (which it seemingly did poorly :-) ) : > > The distinction here is that all messages talking about mishaps of the > > lvds element use dev_err(_probe) while messages caused by interaction > > with the main Rockchip drm-device use drm_err.
Hi Heiko, On 2/28/25 6:45 PM, Heiko Stübner wrote: > Am Freitag, 28. Februar 2025, 18:42:32 MEZ schrieb Quentin Schulz: >> Hi Heiko, >> >> On 2/28/25 5:57 PM, Heiko Stuebner wrote: >>> From: Heiko Stuebner <heiko.stuebner@cherry.de> >>> >>> Commit 52d11c863ac9 ("drm/rockchip: lvds: do not print scary message when >>> probing defer") already started hiding scary messages that are not relevant >>> if the requested supply just returned EPROBE_DEFER, but there are more >>> possible sources - like the phy. >>> >>> So modernize the whole logging in the probe path by replacing the >>> remaining deprecated DRM_DEV_ERROR with appropriate dev_err(_probe) >>> and drm_err calls. >>> >>> The distinction here is that all messages talking about mishaps of the >>> lvds element use dev_err(_probe) while messages caused by interaction >>> with the main Rockchip drm-device use drm_err. >>> >>> Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de> > >>> @@ -604,8 +602,8 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master, >>> >>> ret = drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_LVDS); >>> if (ret < 0) { >>> - DRM_DEV_ERROR(drm_dev->dev, >>> - "failed to initialize encoder: %d\n", ret); >>> + drm_err(drm_dev, >>> + "failed to initialize encoder: %d\n", ret); >> >> All the above are using dev_err, but starting here, it's drm_err, is >> that on purpose? > > The last paragraph of the commit message was supposed to explain that > (which it seemingly did poorly :-) ) : > Mmmm someone didn't read that commit log entirely. That someone apologizes for the noise. Cheers, Quentin
diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c index c19b7b1f6cb5..2b5101a764c8 100644 --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c @@ -453,10 +453,9 @@ static int rk3288_lvds_probe(struct platform_device *pdev, return PTR_ERR(lvds->regs); lvds->pclk = devm_clk_get_prepared(lvds->dev, "pclk_lvds"); - if (IS_ERR(lvds->pclk)) { - DRM_DEV_ERROR(lvds->dev, "could not get pclk_lvds\n"); - return PTR_ERR(lvds->pclk); - } + if (IS_ERR(lvds->pclk)) + return dev_err_probe(lvds->dev, PTR_ERR(lvds->pclk), + "could not get pclk_lvds\n"); lvds->pins = devm_kzalloc(lvds->dev, sizeof(*lvds->pins), GFP_KERNEL); @@ -465,14 +464,14 @@ static int rk3288_lvds_probe(struct platform_device *pdev, lvds->pins->p = devm_pinctrl_get(lvds->dev); if (IS_ERR(lvds->pins->p)) { - DRM_DEV_ERROR(lvds->dev, "no pinctrl handle\n"); + dev_err(lvds->dev, "no pinctrl handle\n"); devm_kfree(lvds->dev, lvds->pins); lvds->pins = NULL; } else { lvds->pins->default_state = pinctrl_lookup_state(lvds->pins->p, "lcdc"); if (IS_ERR(lvds->pins->default_state)) { - DRM_DEV_ERROR(lvds->dev, "no default pinctrl state\n"); + dev_err(lvds->dev, "no default pinctrl state\n"); devm_kfree(lvds->dev, lvds->pins); lvds->pins = NULL; } @@ -547,11 +546,10 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master, lvds->drm_dev = drm_dev; port = of_graph_get_port_by_id(dev->of_node, 1); - if (!port) { - DRM_DEV_ERROR(dev, - "can't found port point, please init lvds panel port!\n"); - return -EINVAL; - } + if (!port) + return dev_err_probe(dev, -EINVAL, + "can't found port point, please init lvds panel port!\n"); + for_each_child_of_node(port, endpoint) { child_count++; of_property_read_u32(endpoint, "reg", &endpoint_id); @@ -563,7 +561,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master, } } if (!child_count) { - DRM_DEV_ERROR(dev, "lvds port does not have any children\n"); + dev_err(dev, "lvds port does not have any children\n"); ret = -EINVAL; goto err_put_port; } else if (ret) { @@ -581,7 +579,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master, lvds->output = rockchip_lvds_name_to_output(name); if (lvds->output < 0) { - DRM_DEV_ERROR(dev, "invalid output type [%s]\n", name); + dev_err(dev, "invalid output type [%s]\n", name); ret = lvds->output; goto err_put_remote; } @@ -593,7 +591,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master, lvds->format = rockchip_lvds_name_to_format(name); if (lvds->format < 0) { - DRM_DEV_ERROR(dev, "invalid data-mapping format [%s]\n", name); + dev_err(dev, "invalid data-mapping format [%s]\n", name); ret = lvds->format; goto err_put_remote; } @@ -604,8 +602,8 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master, ret = drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_LVDS); if (ret < 0) { - DRM_DEV_ERROR(drm_dev->dev, - "failed to initialize encoder: %d\n", ret); + drm_err(drm_dev, + "failed to initialize encoder: %d\n", ret); goto err_put_remote; } @@ -618,8 +616,8 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master, &rockchip_lvds_connector_funcs, DRM_MODE_CONNECTOR_LVDS); if (ret < 0) { - DRM_DEV_ERROR(drm_dev->dev, - "failed to initialize connector: %d\n", ret); + drm_err(drm_dev, + "failed to initialize connector: %d\n", ret); goto err_free_encoder; } @@ -633,9 +631,9 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master, connector = drm_bridge_connector_init(lvds->drm_dev, encoder); if (IS_ERR(connector)) { - DRM_DEV_ERROR(drm_dev->dev, - "failed to initialize bridge connector: %pe\n", - connector); + drm_err(drm_dev, + "failed to initialize bridge connector: %pe\n", + connector); ret = PTR_ERR(connector); goto err_free_encoder; } @@ -643,8 +641,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master, ret = drm_connector_attach_encoder(connector, encoder); if (ret < 0) { - DRM_DEV_ERROR(drm_dev->dev, - "failed to attach encoder: %d\n", ret); + drm_err(drm_dev, "failed to attach encoder: %d\n", ret); goto err_free_connector; } @@ -706,24 +703,20 @@ static int rockchip_lvds_probe(struct platform_device *pdev) lvds->grf = syscon_regmap_lookup_by_phandle(dev->of_node, "rockchip,grf"); - if (IS_ERR(lvds->grf)) { - DRM_DEV_ERROR(dev, "missing rockchip,grf property\n"); - return PTR_ERR(lvds->grf); - } + if (IS_ERR(lvds->grf)) + return dev_err_probe(dev, PTR_ERR(lvds->grf), "missing rockchip,grf property\n"); ret = lvds->soc_data->probe(pdev, lvds); - if (ret) { - DRM_DEV_ERROR(dev, "Platform initialization failed\n"); - return ret; - } + if (ret) + return dev_err_probe(dev, ret, "Platform initialization failed\n"); dev_set_drvdata(dev, lvds); ret = component_add(&pdev->dev, &rockchip_lvds_component_ops); if (ret < 0) - DRM_DEV_ERROR(dev, "failed to add component\n"); + return dev_err_probe(dev, ret, "failed to add component\n"); - return ret; + return 0; } static void rockchip_lvds_remove(struct platform_device *pdev)