Hi Javier,
On Fri Jul 11, 2025 at 11:21 AM CEST, Javier Martinez Canillas wrote:
> "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.
We shouldn't error out in case there's no interconnects defined for this
simple-framebuffer though? That'd break all other usages of it?
>
> [...]
>
>>>
>>> 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.
But checking also for NULL shouldn't hurt either, in case the compile
guards get removed in the future or something?
Quote:
> * Return: icc_path pointer on success or ERR_PTR() on error. NULL is returned
> * when the API is disabled or the "interconnects" DT property is missing.
>
>> 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?
This is the same logic as e.g. for the regulator code in simpledrm. The
idea seems to be that in case some regulator (or here interconnect)
doesn't probe correctly, we still try anyways. Just for EPROBE_DEFER we
defer and wait until the supplier is available.
So defer -> defer simpledrm probe
So error -> ignore error and continue probe
>
>> continue;
>
> I'm not sure why you need this?
For the above behavior.
I guess there were some original design decisions behind handling it
this way, so I don't see a reason to handle it differently for
interconnects.
>
>> }
>>
>> 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.
See above.
Regards
Luca
>
>> 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.
