"Luca Weiss" <luca.we...@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
