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

["Luca Weiss" <luca.weiss@fairphone.com>]

Hi Javier,

On Fri Jun 27, 2025 at 9:51 AM CEST, Javier Martinez Canillas wrote:
> Luca Weiss <luca.weiss@fairphone.com> writes:
>
>> Some devices might require keeping an interconnect path alive so that
>> the framebuffer continues working. Add support for that by setting the
>> bandwidth requirements appropriately for all provided interconnect
>> paths.
>>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>> ---
>>  drivers/gpu/drm/sysfb/simpledrm.c | 83 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 83 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
>> index 349219330314e3421a6bb26ad5cf39a679a5cb7a..47d213e20cab1dd1e19528674a95edea00f4bb30 100644
>> --- a/drivers/gpu/drm/sysfb/simpledrm.c
>> +++ b/drivers/gpu/drm/sysfb/simpledrm.c
>> @@ -2,6 +2,7 @@
>>  
>>  #include <linux/aperture.h>
>>  #include <linux/clk.h>
>> +#include <linux/interconnect.h>
>>  #include <linux/minmax.h>
>>  #include <linux/of_address.h>
>>  #include <linux/of_clk.h>
>> @@ -225,6 +226,10 @@ struct simpledrm_device {
>>  	struct device **pwr_dom_devs;
>>  	struct device_link **pwr_dom_links;
>>  #endif
>
> Can you add a /* interconnects */ comment here? Similarly how there is one
> for clocks, regulators, power domains, etc.

Sure!

>
>> +#if defined CONFIG_OF && defined CONFIG_INTERCONNECT
>> +	unsigned int icc_count;
>> +	struct icc_path **icc_paths;
>> +#endif
>>  
>
> ...
>
>> +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;
>> +
>> +	/* An interconnect path consists of two elements */
>> +	if (count % 2) {
>> +		drm_err(&sdev->sysfb.dev,
>> +			"invalid interconnects value\n");
>> +		return -EINVAL;
>> +	}
>> +	sdev->icc_count = count / 2;
>> +
>> +	sdev->icc_paths = devm_kcalloc(dev, sdev->icc_count,
>> +					       sizeof(*sdev->icc_paths),
>> +					       GFP_KERNEL);
>> +	if (!sdev->icc_paths)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < sdev->icc_count; i++) {
>> +		sdev->icc_paths[i] = of_icc_get_by_index(dev, i);
>> +		if (IS_ERR_OR_NULL(sdev->icc_paths[i])) {
>> +			ret = PTR_ERR(sdev->icc_paths[i]);
>> +			if (ret == -EPROBE_DEFER)
>> +				goto err;
>> +			drm_err(&sdev->sysfb.dev, "failed to get interconnect path %u: %d\n",
>> +				i, ret);
>
> 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?

		if (IS_ERR_OR_NULL(sdev->icc_paths[i])) {
			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;
			continue;
		}

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.

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

Let me know.

Regards
Luca

>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>