Re: [PATCH v2 2/2] drm/rockchip: lvds: Hide scary error messages on probe deferral

["Heiko Stübner" <heiko@sntech.de>]

Am Dienstag, 4. März 2025, 12:46:59 MEZ schrieb Quentin Schulz:
> > @@ -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;
> 
> Should those be dev_err if they are not breaking anything? After all, 
> the device will actually probe? Maybe dev_warn would be more appropriate?
> 
> Maybe a separate patch since we had DRM_DEV_ERROR already, so switching 
> to dev_err in one and then lowering the log level in a second would make 
> "more" sense?

I did debate a bit whether to ignore here and go directly to dev_warn,
but opted for DRM_DEV_ERROR -> dev_err -> dev_warn, to keep the commit
description intact. Otherwise people looking at this patch alone might ask
if this part was forgotten, when the commit message indicates "all".

So expect an additional patch in v3 :-) .


> > @@ -593,7 +589,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;
> 
> nipitck:
> 
> ret = dev_err_probe(dev, lvds->format, "invalid data-mapping format 
> [%s]\n", name);

you're right of course

...

> >   	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");
> >   
> 
> Should this rather be drm_error? I believe this is related to the 
> Rockchip DRM main device?

I would group this more to general device error.
Component_add happens way before the component master binds anything.

drm_err is supposed to also point to the main drm_device as its parameter,
which does not even exist at this point.


Heiko



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip