Re: [PATCH V3 02/20] OPP: Make dev_pm_opp_set_regulators() accept NULL terminated list
From: Steven Price <steven.price@arm.com>
Date: 2022-07-06 08:09:59
Also in:
dri-devel, linux-pm, linux-samsung-soc, linux-tegra, lkml
On 05/07/2022 05:34, Viresh Kumar wrote:
On 04-07-22, 15:35, Steven Price wrote:quoted
I have to say the 'new improved' list ending with NULL approach doesn't work out so well for Panfrost. We already have to have a separate 'num_supplies' variable for devm_regulator_bulk_get() / regulator_bulk_{en,dis}able(), so the keeping everything in sync argument is lost here. I would suggest added the NULL on the end of the lists in panfrost_drv.c but then it would break the use of ARRAY_SIZE() to automagically keep the length correct...Actually we can still make it work.quoted
For now the approach isn't too bad because Panfrost doesn't yet support enabling devfreq with more than one supply. But that array isn't going to work so nicely when that restriction is removed. The only sane way I can see of handling this in Panfrost would be replicating the loop to count the supplies in the Panfrost code which would allow dropping num_supplies from struct panfrost_compatible and then supply_names in the same struct could be NULL terminated ready for devm_pm_opp_set_regulators().Or doing this, which will simplify both the cases.
Yes the below works, it's just a bit ugly having the "- 1", and potentially easy to forgot when adding another. However I don't suppose it would get far in that case so I think it would be spotted quickly when adding a new compatible. It's probably the best solution at the moment. Thanks, Steve
quoted hunk ↗ jump to hunk
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index 7fcbc2a5b6cd..b3b55565b8ef 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c@@ -625,24 +625,29 @@ static int panfrost_remove(struct platform_device *pdev) return 0; } -static const char * const default_supplies[] = { "mali" }; +/* + * The OPP core wants the supply names to be NULL terminated, but we need the + * correct num_supplies value for regulator core. Hence, we NULL terminate here + * and then initialize num_supplies with ARRAY_SIZE - 1. + */ +static const char * const default_supplies[] = { "mali", NULL }; static const struct panfrost_compatible default_data = { - .num_supplies = ARRAY_SIZE(default_supplies), + .num_supplies = ARRAY_SIZE(default_supplies) - 1, .supply_names = default_supplies, .num_pm_domains = 1, /* optional */ .pm_domain_names = NULL, }; static const struct panfrost_compatible amlogic_data = { - .num_supplies = ARRAY_SIZE(default_supplies), + .num_supplies = ARRAY_SIZE(default_supplies) - 1, .supply_names = default_supplies, .vendor_quirk = panfrost_gpu_amlogic_quirk, }; -static const char * const mediatek_mt8183_supplies[] = { "mali", "sram" }; +static const char * const mediatek_mt8183_supplies[] = { "mali", "sram", NULL }; static const char * const mediatek_mt8183_pm_domains[] = { "core0", "core1", "core2" }; static const struct panfrost_compatible mediatek_mt8183_data = { - .num_supplies = ARRAY_SIZE(mediatek_mt8183_supplies), + .num_supplies = ARRAY_SIZE(mediatek_mt8183_supplies) - 1, .supply_names = mediatek_mt8183_supplies, .num_pm_domains = ARRAY_SIZE(mediatek_mt8183_pm_domains), .pm_domain_names = mediatek_mt8183_pm_domains,
_______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel