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

Hi Javier,

On Fri Jul 11, 2025 at 11:21 AM CEST, Javier Martinez Canillas wrote:

"Luca Weiss" [off-list ref] writes:

Hello Luca,

quoted

Hi Javier,

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

[...]

quoted

quoted

quoted

+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?

[...]

quoted

quoted

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.

quoted

		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.

quoted

			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

quoted

			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.

quoted

		}

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

quoted

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.