On 2023/1/17 18:09, Heiko Stübner wrote:
> 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.
Maybe setting 'is_on' in this case is redundant operation. Will be dropped.
>
>> + 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?
OK, will be used with dev_dbg().
>
>
>> + 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.
Okay, this attribute will be added in next version.
>
>
> Other than that
> Reviewed-by: Heiko Stuebner <heiko@xxxxxxxxx>
>
Thanks for your review.
Best regards,
Walker Chen
