Re: [PATCH v5 6/6] qcom/soc/drivers: Add DTPM description for sdm845

[Daniel Lezcano <daniel.lezcano@linaro.org>]

Hi Bjorn,

On 07/01/2022 20:27, Bjorn Andersson wrote:

[ ... ]

>> +#include <linux/dtpm.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +
>> +static struct dtpm_node __initdata sdm845_hierarchy[] = {
>> +	[0]{ .name = "sdm845" },
> 
> Why is the index signifiant here?
> Doesn't this imply risk that we forget one element, which will be
> thereby implicitly be left initialized as {} and hence denote
> termination of the list?

Yes, that is possible. The other annotation is also possible. The index
helps to refer from the .parent field.

That said nothing forces to use the index, so it is a matter of taste.

>> +	[1]{ .name = "package",
>> +	     .parent = &sdm845_hierarchy[0] },
>> +	[2]{ .name = "/cpus/cpu@0",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[3]{ .name = "/cpus/cpu@100",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[4]{ .name = "/cpus/cpu@200",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[5]{ .name = "/cpus/cpu@300",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[6]{ .name = "/cpus/cpu@400",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[7]{ .name = "/cpus/cpu@500",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[8]{ .name = "/cpus/cpu@600",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[9]{ .name = "/cpus/cpu@700",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[10]{ .name = "/soc@0/gpu@5000000",
> 
> It worries me that we encode the textual structure of the dts in the
> kernel. E.g. for quite a while this was "/soc/gpu@5000000", so if this
> landed a year ago this driver would have prevented us from correcting
> the dts.

Why ? The change should be reflected in the driver also, no ?

> Another concern is that not all busses in the system are capable of
> 36-bit wide addresses, so it's plausible that we might one day have to
> create a more accurate representation of the address space. Maybe not on
> SDM845, but this would force us to be inconsistent.

Sorry, I'm missing the point :/

If a change is done in the DT, the code using the description must be
changed accordingly, no?


> Regards,
> Bjorn
> 
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[11]{ },
>> +};
>> +
>> +static struct of_device_id __initdata sdm845_dtpm_match_table[] = {
>> +        { .compatible = "qcom,sdm845", .data = sdm845_hierarchy },
>> +        {},
>> +};
>> +
>> +static int __init sdm845_dtpm_init(void)
>> +{
>> +	return dtpm_create_hierarchy(sdm845_dtpm_match_table);
>> +}
>> +late_initcall(sdm845_dtpm_init);
>> +
>> +MODULE_DESCRIPTION("Qualcomm DTPM driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:dtpm");
>> +MODULE_AUTHOR("Daniel Lezcano <daniel.lezcano@kernel.org");
>> +
>> -- 
>> 2.25.1
>>


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog