Re: [PATCH v2 3/5] drm/sysfb: simpledrm: Add support for interconnect paths

[Javier Martinez Canillas <javierm@redhat.com>]

"Luca Weiss" <luca.weiss@fairphone.com> writes:

Hello Luca,

> Hi Javier,
>
> On Fri Jun 27, 2025 at 9:51 AM CEST, Javier Martinez Canillas wrote:

[...]

>>> +static int simpledrm_device_attach_icc(struct simpledrm_device *sdev)
>>> +{
>>> +	struct device *dev = sdev->sysfb.dev.dev;
>>> +	int ret, count, i;
>>> +
>>> +	count = of_count_phandle_with_args(dev->of_node, "interconnects",
>>> +							 "#interconnect-cells");
>>> +	if (count < 0)
>>> +		return 0;
>>> +

You are already checking here the number of interconnects phandlers. IIUC
this should return -ENOENT if there's no "interconects" property and your
logic returns success in that case.

[...]

>>
>> You could use dev_err_probe() instead that already handles the -EPROBE_DEFER
>> case and also will get this message in the /sys/kernel/debug/devices_deferred
>> debugfs entry, as the reason why the probe deferral happened.
>
> Not quite sure how to implement dev_err_probe, but I think this should
> be quite okay?
>

And of_icc_get_by_index() should only return NULL if CONFIG_INTERCONNECT
is disabled but you have ifdef guards already for this so it should not
happen.

> 		if (IS_ERR_OR_NULL(sdev->icc_paths[i])) {

Then here you could just do a IS_ERR() check and not care about being NULL.

> 			ret = dev_err_probe(dev, PTR_ERR(sdev->icc_paths[i]),
> 				      "failed to get interconnect path %u\n", i);
> 			if (ret == -EPROBE_DEFER)
> 				goto err;

Why you only want to put the icc_paths get for the probe deferral case? I
think that you want to do it for any error?

> 			continue;

I'm not sure why you need this?

> 		}
>
> That would still keep the current behavior for defer vs permanent error
> while printing when necessary and having it for devices_deferred for the
> defer case.
>

As mentioned I still don't understand why you want the error path to only
be called for probe deferral. I would had thought that any failure to get
an interconnect would led to an error and cleanup.

> Not sure what the difference between drm_err and dev_err are, but I
> trust you on that.
>

The drm_err() adds DRM specific info but IMO the dev_err_probe() is better
to avoid printing errors in case of probe deferral and also to have it in
the devices_deferred debugfs entry.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat