Re: [PATCH v3 2/3] drm/tidss: Power up attached PM domains on probe

[Swamil Jain <s-jain1@ti.com>]

Hi Tomi,

On 1/9/26 19:48, Tomi Valkeinen wrote:
> Hi,
> 
> On 07/01/2026 19:45, Swamil Jain wrote:
>> From: Devarsh Thakkar <devarsht@ti.com>
>>
>> Some SoC's such as AM62P have dedicated power domains
>> for OLDI which need to be powered on separately along
>> with display controller.
>>
>> So during driver probe, power up all attached PM domains
>> enumerated in devicetree node for DSS.
>>
>> This also prepares base to add display support for AM62P.
>>
>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>> [j-choudhary@ti.com: fix PM call sequence causing kernel crash in OLDI]
>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>> Signed-off-by: Swamil Jain <s-jain1@ti.com>
>> ---
>>   drivers/gpu/drm/tidss/tidss_drv.c | 83 +++++++++++++++++++++++++++++--
>>   drivers/gpu/drm/tidss/tidss_drv.h |  4 ++
>>   2 files changed, 83 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
>> index 1c8cc18bc53c..33a10fba4325 100644
>> --- a/drivers/gpu/drm/tidss/tidss_drv.c
>> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
>> @@ -8,6 +8,7 @@
>>   #include <linux/of.h>
>>   #include <linux/module.h>
>>   #include <linux/pm_runtime.h>
>> +#include <linux/pm_domain.h>
>>   #include <linux/aperture.h>
>>   
>>   #include <drm/clients/drm_client_setup.h>
>> @@ -107,6 +108,68 @@ static const struct drm_driver tidss_driver = {
>>   	.minor			= 0,
>>   };
>>   
>> +static int tidss_detach_pm_domains(struct tidss_device *tidss)
>> +{
>> +	int i;
>> +
>> +	if (tidss->num_domains <= 1)
>> +		return 0;
>> +
>> +	for (i = 0; i < tidss->num_domains; i++) {
>> +		if (!IS_ERR_OR_NULL(tidss->pd_link[i]))
>> +			device_link_del(tidss->pd_link[i]);
>> +		if (!IS_ERR_OR_NULL(tidss->pd_dev[i]))
>> +			dev_pm_domain_detach(tidss->pd_dev[i], true);
>> +		tidss->pd_dev[i] = NULL;
>> +		tidss->pd_link[i] = NULL;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> This should be void, especially as you don't even check the return value
> below.

Sure, will make it void in v4.

> 
>> +
>> +static int tidss_attach_pm_domains(struct tidss_device *tidss)
>> +{
>> +	struct device *dev = tidss->dev;
>> +	int i;
>> +	int ret;
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct device_node *np = pdev->dev.of_node;
> 
> You should try to order these lines from longest to the shortest (not
> possible for every line).

Acked. But, here we are using pdev, so, can't re-order here.

> 
>> +
>> +	tidss->num_domains = of_count_phandle_with_args(np, "power-domains",
>> +							"#power-domain-cells");
>> +
>> +	tidss->pd_dev = devm_kmalloc_array(dev, tidss->num_domains,
>> +					   sizeof(*tidss->pd_dev), GFP_KERNEL);
>> +	if (!tidss->pd_dev)
>> +		return -ENOMEM;
>> +
>> +	tidss->pd_link = devm_kmalloc_array(dev, tidss->num_domains,
>> +					    sizeof(*tidss->pd_link), GFP_KERNEL);
>> +	if (!tidss->pd_link)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < tidss->num_domains; i++) {
>> +		tidss->pd_dev[i] = dev_pm_domain_attach_by_id(dev, i);
>> +		if (IS_ERR(tidss->pd_dev[i])) {
>> +			ret = PTR_ERR(tidss->pd_dev[i]);
>> +			goto fail;
>> +		}
>> +
>> +		tidss->pd_link[i] = device_link_add(dev, tidss->pd_dev[i],
>> +						    DL_FLAG_STATELESS |
>> +						    DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
>> +		if (!tidss->pd_link[i]) {
>> +			ret = -EINVAL;
>> +			goto fail;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +fail:
>> +	tidss_detach_pm_domains(tidss);
>> +	return ret;
>> +}
>> +
>>   static int tidss_probe(struct platform_device *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>> @@ -129,15 +192,21 @@ static int tidss_probe(struct platform_device *pdev)
>>   
>>   	spin_lock_init(&tidss->irq_lock);
>>   
>> +	ret = tidss_attach_pm_domains(tidss);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "failed to attach power domains\n");
>> +
>>   	ret = dispc_init(tidss);
>>   	if (ret) {
>> -		dev_err(dev, "failed to initialize dispc: %d\n", ret);
>> -		return ret;
>> +		dev_err_probe(dev, ret, "failed to initialize dispc\n");
>> +		goto err_detach_pm_domains;
>>   	}
>>   
>>   	ret = tidss_oldi_init(tidss);
>> -	if (ret)
>> -		return dev_err_probe(dev, ret, "failed to init OLDI\n");
>> +	if (ret) {
>> +		dev_err_probe(dev, ret, "failed to init OLDI\n");
>> +		goto err_oldi_deinit;
> 
> This doesn't look right.
> 

Will use err_detach_pm_domains in v4 and will take oldi_deinit() issue 
separately as tidss_oldi_init() is not doing own cleanup if it fails.

Regards,
Swamil.

>   Tomi
> 
> 
>> +	}
>>   
>>   	pm_runtime_enable(dev);
>>   
>> @@ -203,8 +272,12 @@ static int tidss_probe(struct platform_device *pdev)
>>   	pm_runtime_dont_use_autosuspend(dev);
>>   	pm_runtime_disable(dev);
>>   
>> +err_oldi_deinit:
>>   	tidss_oldi_deinit(tidss);
>>   
>> +err_detach_pm_domains:
>> +	tidss_detach_pm_domains(tidss);
>> +
>>   	return ret;
>>   }
>>   
>> @@ -232,6 +305,8 @@ static void tidss_remove(struct platform_device *pdev)
>>   	/* devm allocated dispc goes away with the dev so mark it NULL */
>>   	dispc_remove(tidss);
>>   
>> +	tidss_detach_pm_domains(tidss);
>> +
>>   	dev_dbg(dev, "%s done\n", __func__);
>>   }
>>   
>> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
>> index e1c1f41d8b4b..6625b989b815 100644
>> --- a/drivers/gpu/drm/tidss/tidss_drv.h
>> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
>> @@ -41,6 +41,10 @@ struct tidss_device {
>>   	/* protects the irq masks field and irqenable/irqstatus registers */
>>   	spinlock_t irq_lock;
>>   	dispc_irq_t irq_mask;	/* enabled irqs */
>> +
>> +	int num_domains; /* Count of PM domains to be handled */
>> +	struct device **pd_dev;
>> +	struct device_link **pd_link;
>>   };
>>   
>>   #define to_tidss(__dev) container_of(__dev, struct tidss_device, ddev)
>