| Message ID | 20230411064743.273388-6-changhuang.liang@starfivetech.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add JH7110 DPHY PMU support | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/tree_selection | fail | Failed to apply to next/pending-fixes or riscv/for-next |
On Mon, Apr 10, 2023 at 11:47:41PM -0700, Changhuang Liang wrote: > Different compatible parse device tree resources work in different ways. Right now there is only one compatible, so this commit message needs to be expanded on to provide more information on your motivation. > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com> > static int jh71xx_pmu_init_domain(struct jh71xx_pmu *pmu, int index) > { > struct jh71xx_pmu_dev *pmd; > @@ -296,23 +325,20 @@ static int jh71xx_pmu_probe(struct platform_device *pdev) > if (!pmu) > return -ENOMEM; > > - pmu->base = device_node_to_regmap(np); > - 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"); > + spin_lock_init(&pmu->lock); > > match_data = of_device_get_match_data(dev); > if (!match_data) > return -EINVAL; > > + if (match_data->pmu_parse_dt) { How can this be false? Cheers, Conor.
On 2023/4/11 14:47, Changhuang Liang wrote: > Different compatible parse device tree resources work in different ways. > > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com> I don't think it's necessary to submit multiple patches separately for the same .c file unless it is very necessary. Because the disadvantage of separating multiple patches is that some information lacks completeness and coherence. > --- > drivers/soc/starfive/jh71xx_pmu.c | 54 ++++++++++++++++++++++--------- > 1 file changed, 39 insertions(+), 15 deletions(-) > > diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c > index 98f6849d61de..990db6735c48 100644 > --- a/drivers/soc/starfive/jh71xx_pmu.c > +++ b/drivers/soc/starfive/jh71xx_pmu.c > @@ -57,10 +57,14 @@ struct jh71xx_domain_info { > u8 bit; > }; > > +struct jh71xx_pmu; > + > struct jh71xx_pmu_match_data { > const struct jh71xx_domain_info *domain_info; > int num_domains; > u8 pmu_type; > + int (*pmu_parse_dt)(struct platform_device *pdev, > + struct jh71xx_pmu *pmu); > }; > > struct jh71xx_pmu { > @@ -251,6 +255,31 @@ static irqreturn_t jh71xx_pmu_interrupt(int irq, void *data) > return IRQ_HANDLED; > } > > +static int jh7110_pmu_general_parse_dt(struct platform_device *pdev, > + struct jh71xx_pmu *pmu) > +{
On Wed, Apr 12, 2023 at 02:07:52PM +0800, Walker Chen wrote: > > > On 2023/4/11 14:47, Changhuang Liang wrote: > > Different compatible parse device tree resources work in different ways. > > > > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com> > > I don't think it's necessary to submit multiple patches separately for the same .c file > unless it is very necessary. Because the disadvantage of separating multiple patches > is that some information lacks completeness and coherence. Other than patches 4 & 6, which could be squashed, the breakdown here is fine IMO. Doing one thing per patch makes it obvious to the reader *what* is happening. There's just some missing boilerplate in the commit messages across the series that the DPHY PMU does not have a reg nor interrupts, and this work is being done to support that. Cheers, Conor.
On 2023/4/12 5:06, Conor Dooley wrote: > On Mon, Apr 10, 2023 at 11:47:41PM -0700, Changhuang Liang wrote: >> Different compatible parse device tree resources work in different ways. > > Right now there is only one compatible, so this commit message needs to > be expanded on to provide more information on your motivation. > OK, will add more commit message. >> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com> > >> static int jh71xx_pmu_init_domain(struct jh71xx_pmu *pmu, int index) >> { [...] >> >> match_data = of_device_get_match_data(dev); >> if (!match_data) >> return -EINVAL; >> >> + if (match_data->pmu_parse_dt) { > > How can this be false? > > Cheers, > Conor. Yes, it will not be false, I will delete this if condition
diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c index 98f6849d61de..990db6735c48 100644 --- a/drivers/soc/starfive/jh71xx_pmu.c +++ b/drivers/soc/starfive/jh71xx_pmu.c @@ -57,10 +57,14 @@ struct jh71xx_domain_info { u8 bit; }; +struct jh71xx_pmu; + struct jh71xx_pmu_match_data { const struct jh71xx_domain_info *domain_info; int num_domains; u8 pmu_type; + int (*pmu_parse_dt)(struct platform_device *pdev, + struct jh71xx_pmu *pmu); }; struct jh71xx_pmu { @@ -251,6 +255,31 @@ static irqreturn_t jh71xx_pmu_interrupt(int irq, void *data) return IRQ_HANDLED; } +static int jh7110_pmu_general_parse_dt(struct platform_device *pdev, + struct jh71xx_pmu *pmu) +{ + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + int ret; + + pmu->base = device_node_to_regmap(np); + 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"); + + jh71xx_pmu_int_enable(pmu, JH71XX_PMU_INT_ALL_MASK & ~JH71XX_PMU_INT_PCH_FAIL, true); + + return 0; +} + static int jh71xx_pmu_init_domain(struct jh71xx_pmu *pmu, int index) { struct jh71xx_pmu_dev *pmd; @@ -296,23 +325,20 @@ static int jh71xx_pmu_probe(struct platform_device *pdev) if (!pmu) return -ENOMEM; - pmu->base = device_node_to_regmap(np); - 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"); + spin_lock_init(&pmu->lock); match_data = of_device_get_match_data(dev); if (!match_data) return -EINVAL; + if (match_data->pmu_parse_dt) { + ret = match_data->pmu_parse_dt(pdev, pmu); + if (ret) { + dev_err(dev, "failed to parse dt\n"); + return ret; + } + } + pmu->genpd = devm_kcalloc(dev, match_data->num_domains, sizeof(struct generic_pm_domain *), GFP_KERNEL); @@ -332,9 +358,6 @@ static int jh71xx_pmu_probe(struct platform_device *pdev) } } - 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); @@ -383,6 +406,7 @@ static const struct jh71xx_pmu_match_data jh7110_pmu = { .num_domains = ARRAY_SIZE(jh7110_power_domains), .domain_info = jh7110_power_domains, .pmu_type = JH71XX_PMU_GENERAL, + .pmu_parse_dt = jh7110_pmu_general_parse_dt, }; static const struct of_device_id jh71xx_pmu_of_match[] = {
Different compatible parse device tree resources work in different ways. Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com> --- drivers/soc/starfive/jh71xx_pmu.c | 54 ++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 15 deletions(-)