Hi Walker,
Am Montag, 16. Januar 2023, 08:42:58 CET schrieb Walker Chen:
> +static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on)
> +{
> + struct jh71xx_pmu *pmu = pmd->pmu;
> +
> + if (!mask) {
> + *is_on = false;
nit: While it's not necessarily bad, I don't think callers of
jh71xx_pmu_get_state() should expect this to be set in error case.
> + return -EINVAL;
> + }
> +
> + *is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask;
> +
> + return 0;
> +}
[...]
> +static int jh71xx_pmu_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + const struct jh71xx_pmu_match_data *match_data;
> + struct jh71xx_pmu *pmu;
> + unsigned int i;
> + int ret;
> +
> + pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
> + if (!pmu)
> + return -ENOMEM;
> +
> + pmu->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(pmu->base))
> + return PTR_ERR(pmu->base);
> +
> + pmu->irq = platform_get_irq(pdev, 0);
> + if (pmu->irq < 0)
> + return pmu->irq;
> +
> + ret = devm_request_irq(dev, pmu->irq, jh71xx_pmu_interrupt,
> + 0, pdev->name, pmu);
> + if (ret)
> + dev_err(dev, "failed to request irq\n");
> +
> + match_data = of_device_get_match_data(dev);
> + if (!match_data)
> + return -EINVAL;
> +
> + pmu->genpd = devm_kcalloc(dev, match_data->num_domains,
> + sizeof(struct generic_pm_domain *),
> + GFP_KERNEL);
> + if (!pmu->genpd)
> + return -ENOMEM;
> +
> + pmu->dev = dev;
> + pmu->match_data = match_data;
> + pmu->genpd_data.domains = pmu->genpd;
> + pmu->genpd_data.num_domains = match_data->num_domains;
> +
> + for (i = 0; i < match_data->num_domains; i++) {
> + ret = jh71xx_pmu_init_domain(pmu, i);
> + if (ret) {
> + dev_err(dev, "failed to initialize power domain\n");
> + return ret;
> + }
> + }
> +
> + spin_lock_init(&pmu->lock);
> + jh71xx_pmu_int_enable(pmu, JH71XX_PMU_INT_ALL_MASK & ~JH71XX_PMU_INT_PCH_FAIL, true);
> +
> + ret = of_genpd_add_provider_onecell(np, &pmu->genpd_data);
> + if (ret) {
> + dev_err(dev, "failed to register genpd driver: %d\n", ret);
> + return ret;
> + }
> +
> + dev_info(dev, "registered %u power domains\n", i);
nit: I guess that could be a dev_dbg to not spam the kernel log too much?
> + return 0;
> +}
> +
> +static const struct jh71xx_domain_info jh7110_power_domains[] = {
> + [JH7110_PD_SYSTOP] = {
> + .name = "SYSTOP",
> + .bit = 0,
> + .flags = GENPD_FLAG_ALWAYS_ON,
> + },
> + [JH7110_PD_CPU] = {
> + .name = "CPU",
> + .bit = 1,
> + .flags = GENPD_FLAG_ALWAYS_ON,
> + },
> + [JH7110_PD_GPUA] = {
> + .name = "GPUA",
> + .bit = 2,
> + },
> + [JH7110_PD_VDEC] = {
> + .name = "VDEC",
> + .bit = 3,
> + },
> + [JH7110_PD_VOUT] = {
> + .name = "VOUT",
> + .bit = 4,
> + },
> + [JH7110_PD_ISP] = {
> + .name = "ISP",
> + .bit = 5,
> + },
> + [JH7110_PD_VENC] = {
> + .name = "VENC",
> + .bit = 6,
> + },
> +};
> +
> +static const struct jh71xx_pmu_match_data jh7110_pmu = {
> + .num_domains = ARRAY_SIZE(jh7110_power_domains),
> + .domain_info = jh7110_power_domains,
> +};
> +
> +static const struct of_device_id jh71xx_pmu_of_match[] = {
> + {
> + .compatible = "starfive,jh7110-pmu",
> + .data = (void *)&jh7110_pmu,
> + }, {
> + /* sentinel */
> + }
> +};
> +
> +static struct platform_driver jh71xx_pmu_driver = {
> + .driver = {
> + .name = "jh71xx-pmu",
> + .of_match_table = jh71xx_pmu_of_match,
In the rockchip pm-domains driver we have
/*
* We can't forcibly eject devices from the power
* domain, so we can't really remove power domains
* once they were added.
*/
.suppress_bind_attrs = true,
here, which might be valid for your pmu driver as well.
Other than that
Reviewed-by: Heiko Stuebner <heiko@xxxxxxxxx>
