On 06/10/2021 16.28, Krzysztof Kozlowski wrote:
+static int apple_pmgr_ps_set(struct generic_pm_domain *genpd, u32 pstate) +{ + int ret; + struct apple_pmgr_ps *ps = genpd_to_apple_pmgr_ps(genpd); + u32 reg; + + regmap_read(ps->regmap, ps->offset, ®);MMIO accesses should not fail, but regmap API could fail if for example clk_enable() fails. In such case you will write below value based on random stack init. Please check the return value here.
Ack, will fix for v2 (as well as the related ones below).
+static int apple_pmgr_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id) +{ + struct apple_pmgr_ps *ps = rcdev_to_apple_pmgr_ps(rcdev); + + mutex_lock(&ps->genpd.mlock);This looks wrong: it can be a spin-lock, not mutex, so you should use genpd_lock.
genpd_lock() is not part of the public API, which is why I did it like this. This gets decided by whether the GENPD_FLAG_IRQ_SAFE flag is set, so it should be a mutex in this case, as that is not set.
However now I wonder if there could be a case when a reset-controller consumer calls it from it's GENPD_NOTIFY_ON notifier? In such case you would have this lock taken.
Hm, yeah, I wonder if we'll hit that use case. Probably not, though. I mostly expect our drivers to only reset devices on initial probe or in some kind of panic recovery scenario, not while doing PM stuff.
+static const struct of_device_id apple_pmgr_ps_of_match[] = { + { .compatible = "apple,t8103-pmgr-pwrstate" }, + { .compatible = "apple,pmgr-pwrstate" },You call the device/driver "pwrstate", which it seems is "power state". These are not power states. These are power controllers or power domains. Power state is rather a state of power domain - e.g. on or gated. How about renaming it to power domain or pd?
It's a bit confusing. Apple calls these registers "ps" registers, which presumably stands for "power state". They can both clockgate and powergate devices (where supported), as well as enable auto-PM and also handle reset. So they're a bit more complex and higher level than a simple power domain, which is why I called the driver "pwrstate", since it controls the power state of a specific SoC domain or block. In fact, the device PM is controlled via a 4-bit power state index, though right now only 0, 4, 15 are used (power gated, clock gated, active). Many devices will not support individual power gating and would just clockgate at 0, and right now the driver never uses 4, but might in the future. If that needs to be exposed to consumers, then it'd have to be via genpd idle states.
-- Hector Martin (marcan@xxxxxxxxx) Public Key: https://mrcn.st/pub
