On Sun, 9 Feb 2020 at 13:50, Nicolas Boichat drinkcat@chromium.org wrote:
On Fri, Feb 7, 2020 at 10:26 PM Ulf Hansson ulf.hansson@linaro.org wrote:
On Fri, 7 Feb 2020 at 06:27, Nicolas Boichat drinkcat@chromium.org wrote:
When there is a single power domain per device, the core will ensure the power domain is switched on (so it is technically equivalent to having not power domain specified at all).
However, when there are multiple domains, as in MT8183 Bifrost GPU, we need to handle them in driver code.
Signed-off-by: Nicolas Boichat drinkcat@chromium.org
Besides a minor nitpick, feel free to add:
Reviewed-by: Ulf Hansson ulf.hansson@linaro.org
Kind regards Uffe
[snip]
+static int panfrost_pm_domain_init(struct panfrost_device *pfdev) +{
int err;int i, num_domains;num_domains = of_count_phandle_with_args(pfdev->dev->of_node,"power-domains","#power-domain-cells");/** Single domain is handled by the core, and, if only a single power* the power domain is requested, the property is optional.*/if (num_domains < 2 && pfdev->comp->num_pm_domains < 2)return 0;if (num_domains != pfdev->comp->num_pm_domains) {dev_err(pfdev->dev,"Incorrect number of power domains: %d provided, %d needed\n",num_domains, pfdev->comp->num_pm_domains);return -EINVAL;}if (WARN(num_domains > ARRAY_SIZE(pfdev->pm_domain_devs),"Too many supplies in compatible structure.\n"))Nitpick: Not sure this deserves a WARN. Perhaps a regular dev_err() is sufficient.
Ah well I had a BUG_ON before so presumably this is already a little better ,-)
You can only reach there if you set pfdev->comp->num_pm_domains > MAX_PM_DOMAINS in the currently matched struct panfrost_compatible (pfdev->comp->num_pm_domains == num_domains, and see below too), so the kernel code would actually be actually broken (not the device tree, nor anything that could be probed). So I'm wondering if the loudness of a WARN is better in this case? Arguable ,-)
I see. It's not a big a deal, so feel free to keep as is.
[...]
Kind regards Uffe