| Message ID | 20230411064743.273388-4-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:39PM -0700, Changhuang Liang wrote: > Modify ioremap to regmap, easy to simplify code. This doesn't simplify anything, adding regmap to the mix actually makes it less obvious what is going on here & it's not even fewer LoC: 1 file changed, 23 insertions(+), 20 deletions(-) Please write a commit message that explains the real motivation for this change. Thanks, Conor. > > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com> > --- > drivers/soc/starfive/jh71xx_pmu.c | 43 +++++++++++++++++-------------- > 1 file changed, 23 insertions(+), 20 deletions(-) > > diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c > index 7d5f50d71c0d..306218c83691 100644 > --- a/drivers/soc/starfive/jh71xx_pmu.c > +++ b/drivers/soc/starfive/jh71xx_pmu.c > @@ -6,13 +6,13 @@ > */ > > #include <linux/interrupt.h> > -#include <linux/io.h> > -#include <linux/iopoll.h> > +#include <linux/mfd/syscon.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/pm_domain.h> > +#include <linux/regmap.h> > #include <dt-bindings/power/starfive,jh7110-pmu.h> > > /* register offset */ > @@ -59,7 +59,7 @@ struct jh71xx_pmu_match_data { > struct jh71xx_pmu { > struct device *dev; > const struct jh71xx_pmu_match_data *match_data; > - void __iomem *base; > + struct regmap *base; > struct generic_pm_domain **genpd; > struct genpd_onecell_data genpd_data; > int irq; > @@ -75,11 +75,14 @@ struct jh71xx_pmu_dev { > static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on) > { > struct jh71xx_pmu *pmu = pmd->pmu; > + unsigned int val; > > if (!mask) > return -EINVAL; > > - *is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask; > + regmap_read(pmu->base, JH71XX_PMU_CURR_POWER_MODE, &val); > + > + *is_on = val & mask; > > return 0; > } > @@ -130,7 +133,7 @@ static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on) > encourage_hi = JH71XX_PMU_SW_ENCOURAGE_DIS_HI; > } > > - writel(mask, pmu->base + mode); > + regmap_write(pmu->base, mode, mask); > > /* > * 2.Write SW encourage command sequence to the Software Encourage Reg (offset 0x44) > @@ -140,21 +143,21 @@ static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on) > * Then write the lower bits of the command sequence, followed by the upper > * bits. The sequence differs between powering on & off a domain. > */ > - writel(JH71XX_PMU_SW_ENCOURAGE_ON, pmu->base + JH71XX_PMU_SW_ENCOURAGE); > - writel(encourage_lo, pmu->base + JH71XX_PMU_SW_ENCOURAGE); > - writel(encourage_hi, pmu->base + JH71XX_PMU_SW_ENCOURAGE); > + regmap_write(pmu->base, JH71XX_PMU_SW_ENCOURAGE, JH71XX_PMU_SW_ENCOURAGE_ON); > + regmap_write(pmu->base, JH71XX_PMU_SW_ENCOURAGE, encourage_lo); > + regmap_write(pmu->base, JH71XX_PMU_SW_ENCOURAGE, encourage_hi); > > spin_unlock_irqrestore(&pmu->lock, flags); > > /* Wait for the power domain bit to be enabled / disabled */ > if (on) { > - ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE, > - val, val & mask, > - 1, JH71XX_PMU_TIMEOUT_US); > + ret = regmap_read_poll_timeout_atomic(pmu->base, JH71XX_PMU_CURR_POWER_MODE, > + val, val & mask, > + 1, JH71XX_PMU_TIMEOUT_US); > } else { > - ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE, > - val, !(val & mask), > - 1, JH71XX_PMU_TIMEOUT_US); > + ret = regmap_read_poll_timeout_atomic(pmu->base, JH71XX_PMU_CURR_POWER_MODE, > + val, !(val & mask), > + 1, JH71XX_PMU_TIMEOUT_US); > } > > if (ret) { > @@ -190,14 +193,14 @@ static void jh71xx_pmu_int_enable(struct jh71xx_pmu *pmu, u32 mask, bool enable) > unsigned long flags; > > spin_lock_irqsave(&pmu->lock, flags); > - val = readl(pmu->base + JH71XX_PMU_TIMER_INT_MASK); > + regmap_read(pmu->base, JH71XX_PMU_TIMER_INT_MASK, &val); > > if (enable) > val &= ~mask; > else > val |= mask; > > - writel(val, pmu->base + JH71XX_PMU_TIMER_INT_MASK); > + regmap_write(pmu->base, JH71XX_PMU_TIMER_INT_MASK, val); > spin_unlock_irqrestore(&pmu->lock, flags); > } > > @@ -206,7 +209,7 @@ static irqreturn_t jh71xx_pmu_interrupt(int irq, void *data) > struct jh71xx_pmu *pmu = data; > u32 val; > > - val = readl(pmu->base + JH71XX_PMU_INT_STATUS); > + regmap_read(pmu->base, JH71XX_PMU_INT_STATUS, &val); > > if (val & JH71XX_PMU_INT_SEQ_DONE) > dev_dbg(pmu->dev, "sequence done.\n"); > @@ -220,8 +223,8 @@ static irqreturn_t jh71xx_pmu_interrupt(int irq, void *data) > dev_err(pmu->dev, "p-channel fail event.\n"); > > /* clear interrupts */ > - writel(val, pmu->base + JH71XX_PMU_INT_STATUS); > - writel(val, pmu->base + JH71XX_PMU_EVENT_STATUS); > + regmap_write(pmu->base, JH71XX_PMU_INT_STATUS, val); > + regmap_write(pmu->base, JH71XX_PMU_EVENT_STATUS, val); > > return IRQ_HANDLED; > } > @@ -271,7 +274,7 @@ static int jh71xx_pmu_probe(struct platform_device *pdev) > if (!pmu) > return -ENOMEM; > > - pmu->base = devm_platform_ioremap_resource(pdev, 0); > + pmu->base = device_node_to_regmap(np); > if (IS_ERR(pmu->base)) > return PTR_ERR(pmu->base); > > -- > 2.25.1 >
On 2023/4/12 4:26, Conor Dooley wrote: > On Mon, Apr 10, 2023 at 11:47:39PM -0700, Changhuang Liang wrote: >> Modify ioremap to regmap, easy to simplify code. > > This doesn't simplify anything, adding regmap to the mix actually makes > it less obvious what is going on here & it's not even fewer LoC: > 1 file changed, 23 insertions(+), 20 deletions(-) > > Please write a commit message that explains the real motivation for > this change. > > Thanks, > Conor. > When a new pmu is introduced later, they can use the same member "struct regmap *base" in "struct jh71xx_pmu". I will add more commit message later. >> >> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com> >> --- >> drivers/soc/starfive/jh71xx_pmu.c | 43 +++++++++++++++++-------------- >> 1 file changed, 23 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c >> index 7d5f50d71c0d..306218c83691 100644 >> --- a/drivers/soc/starfive/jh71xx_pmu.c >> +++ b/drivers/soc/starfive/jh71xx_pmu.c >> @@ -6,13 +6,13 @@ >> */ >> >> #include <linux/interrupt.h> >> -#include <linux/io.h> >> -#include <linux/iopoll.h> >> +#include <linux/mfd/syscon.h> >> #include <linux/module.h> >> #include <linux/of.h> >> #include <linux/of_device.h> >> #include <linux/platform_device.h> >> #include <linux/pm_domain.h> >> +#include <linux/regmap.h> >> #include <dt-bindings/power/starfive,jh7110-pmu.h> >> >> /* register offset */ >> @@ -59,7 +59,7 @@ struct jh71xx_pmu_match_data { >> struct jh71xx_pmu { >> struct device *dev; >> const struct jh71xx_pmu_match_data *match_data; >> - void __iomem *base; >> + struct regmap *base; >> struct generic_pm_domain **genpd; >> struct genpd_onecell_data genpd_data; >> int irq; >> @@ -75,11 +75,14 @@ struct jh71xx_pmu_dev { >> static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on) >> { >> struct jh71xx_pmu *pmu = pmd->pmu; >> + unsigned int val; >> >> if (!mask) >> return -EINVAL; >> >> - *is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask; >> + regmap_read(pmu->base, JH71XX_PMU_CURR_POWER_MODE, &val); >> + >> + *is_on = val & mask; >> >> return 0; >> } >> @@ -130,7 +133,7 @@ static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on) >> encourage_hi = JH71XX_PMU_SW_ENCOURAGE_DIS_HI; >> } >> >> - writel(mask, pmu->base + mode); >> + regmap_write(pmu->base, mode, mask); >> >> /* >> * 2.Write SW encourage command sequence to the Software Encourage Reg (offset 0x44) >> @@ -140,21 +143,21 @@ static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on) >> * Then write the lower bits of the command sequence, followed by the upper >> * bits. The sequence differs between powering on & off a domain. >> */ >> - writel(JH71XX_PMU_SW_ENCOURAGE_ON, pmu->base + JH71XX_PMU_SW_ENCOURAGE); >> - writel(encourage_lo, pmu->base + JH71XX_PMU_SW_ENCOURAGE); >> - writel(encourage_hi, pmu->base + JH71XX_PMU_SW_ENCOURAGE); >> + regmap_write(pmu->base, JH71XX_PMU_SW_ENCOURAGE, JH71XX_PMU_SW_ENCOURAGE_ON); >> + regmap_write(pmu->base, JH71XX_PMU_SW_ENCOURAGE, encourage_lo); >> + regmap_write(pmu->base, JH71XX_PMU_SW_ENCOURAGE, encourage_hi); >> >> spin_unlock_irqrestore(&pmu->lock, flags); >> >> /* Wait for the power domain bit to be enabled / disabled */ >> if (on) { >> - ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE, >> - val, val & mask, >> - 1, JH71XX_PMU_TIMEOUT_US); >> + ret = regmap_read_poll_timeout_atomic(pmu->base, JH71XX_PMU_CURR_POWER_MODE, >> + val, val & mask, >> + 1, JH71XX_PMU_TIMEOUT_US); >> } else { >> - ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE, >> - val, !(val & mask), >> - 1, JH71XX_PMU_TIMEOUT_US); >> + ret = regmap_read_poll_timeout_atomic(pmu->base, JH71XX_PMU_CURR_POWER_MODE, >> + val, !(val & mask), >> + 1, JH71XX_PMU_TIMEOUT_US); >> } >> >> if (ret) { >> @@ -190,14 +193,14 @@ static void jh71xx_pmu_int_enable(struct jh71xx_pmu *pmu, u32 mask, bool enable) >> unsigned long flags; >> >> spin_lock_irqsave(&pmu->lock, flags); >> - val = readl(pmu->base + JH71XX_PMU_TIMER_INT_MASK); >> + regmap_read(pmu->base, JH71XX_PMU_TIMER_INT_MASK, &val); >> >> if (enable) >> val &= ~mask; >> else >> val |= mask; >> >> - writel(val, pmu->base + JH71XX_PMU_TIMER_INT_MASK); >> + regmap_write(pmu->base, JH71XX_PMU_TIMER_INT_MASK, val); >> spin_unlock_irqrestore(&pmu->lock, flags); >> } >> >> @@ -206,7 +209,7 @@ static irqreturn_t jh71xx_pmu_interrupt(int irq, void *data) >> struct jh71xx_pmu *pmu = data; >> u32 val; >> >> - val = readl(pmu->base + JH71XX_PMU_INT_STATUS); >> + regmap_read(pmu->base, JH71XX_PMU_INT_STATUS, &val); >> >> if (val & JH71XX_PMU_INT_SEQ_DONE) >> dev_dbg(pmu->dev, "sequence done.\n"); >> @@ -220,8 +223,8 @@ static irqreturn_t jh71xx_pmu_interrupt(int irq, void *data) >> dev_err(pmu->dev, "p-channel fail event.\n"); >> >> /* clear interrupts */ >> - writel(val, pmu->base + JH71XX_PMU_INT_STATUS); >> - writel(val, pmu->base + JH71XX_PMU_EVENT_STATUS); >> + regmap_write(pmu->base, JH71XX_PMU_INT_STATUS, val); >> + regmap_write(pmu->base, JH71XX_PMU_EVENT_STATUS, val); >> >> return IRQ_HANDLED; >> } >> @@ -271,7 +274,7 @@ static int jh71xx_pmu_probe(struct platform_device *pdev) >> if (!pmu) >> return -ENOMEM; >> >> - pmu->base = devm_platform_ioremap_resource(pdev, 0); >> + pmu->base = device_node_to_regmap(np); >> if (IS_ERR(pmu->base)) >> return PTR_ERR(pmu->base); >> >> -- >> 2.25.1 >>
diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c index 7d5f50d71c0d..306218c83691 100644 --- a/drivers/soc/starfive/jh71xx_pmu.c +++ b/drivers/soc/starfive/jh71xx_pmu.c @@ -6,13 +6,13 @@ */ #include <linux/interrupt.h> -#include <linux/io.h> -#include <linux/iopoll.h> +#include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/of.h> #include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/pm_domain.h> +#include <linux/regmap.h> #include <dt-bindings/power/starfive,jh7110-pmu.h> /* register offset */ @@ -59,7 +59,7 @@ struct jh71xx_pmu_match_data { struct jh71xx_pmu { struct device *dev; const struct jh71xx_pmu_match_data *match_data; - void __iomem *base; + struct regmap *base; struct generic_pm_domain **genpd; struct genpd_onecell_data genpd_data; int irq; @@ -75,11 +75,14 @@ struct jh71xx_pmu_dev { static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on) { struct jh71xx_pmu *pmu = pmd->pmu; + unsigned int val; if (!mask) return -EINVAL; - *is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask; + regmap_read(pmu->base, JH71XX_PMU_CURR_POWER_MODE, &val); + + *is_on = val & mask; return 0; } @@ -130,7 +133,7 @@ static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on) encourage_hi = JH71XX_PMU_SW_ENCOURAGE_DIS_HI; } - writel(mask, pmu->base + mode); + regmap_write(pmu->base, mode, mask); /* * 2.Write SW encourage command sequence to the Software Encourage Reg (offset 0x44) @@ -140,21 +143,21 @@ static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on) * Then write the lower bits of the command sequence, followed by the upper * bits. The sequence differs between powering on & off a domain. */ - writel(JH71XX_PMU_SW_ENCOURAGE_ON, pmu->base + JH71XX_PMU_SW_ENCOURAGE); - writel(encourage_lo, pmu->base + JH71XX_PMU_SW_ENCOURAGE); - writel(encourage_hi, pmu->base + JH71XX_PMU_SW_ENCOURAGE); + regmap_write(pmu->base, JH71XX_PMU_SW_ENCOURAGE, JH71XX_PMU_SW_ENCOURAGE_ON); + regmap_write(pmu->base, JH71XX_PMU_SW_ENCOURAGE, encourage_lo); + regmap_write(pmu->base, JH71XX_PMU_SW_ENCOURAGE, encourage_hi); spin_unlock_irqrestore(&pmu->lock, flags); /* Wait for the power domain bit to be enabled / disabled */ if (on) { - ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE, - val, val & mask, - 1, JH71XX_PMU_TIMEOUT_US); + ret = regmap_read_poll_timeout_atomic(pmu->base, JH71XX_PMU_CURR_POWER_MODE, + val, val & mask, + 1, JH71XX_PMU_TIMEOUT_US); } else { - ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE, - val, !(val & mask), - 1, JH71XX_PMU_TIMEOUT_US); + ret = regmap_read_poll_timeout_atomic(pmu->base, JH71XX_PMU_CURR_POWER_MODE, + val, !(val & mask), + 1, JH71XX_PMU_TIMEOUT_US); } if (ret) { @@ -190,14 +193,14 @@ static void jh71xx_pmu_int_enable(struct jh71xx_pmu *pmu, u32 mask, bool enable) unsigned long flags; spin_lock_irqsave(&pmu->lock, flags); - val = readl(pmu->base + JH71XX_PMU_TIMER_INT_MASK); + regmap_read(pmu->base, JH71XX_PMU_TIMER_INT_MASK, &val); if (enable) val &= ~mask; else val |= mask; - writel(val, pmu->base + JH71XX_PMU_TIMER_INT_MASK); + regmap_write(pmu->base, JH71XX_PMU_TIMER_INT_MASK, val); spin_unlock_irqrestore(&pmu->lock, flags); } @@ -206,7 +209,7 @@ static irqreturn_t jh71xx_pmu_interrupt(int irq, void *data) struct jh71xx_pmu *pmu = data; u32 val; - val = readl(pmu->base + JH71XX_PMU_INT_STATUS); + regmap_read(pmu->base, JH71XX_PMU_INT_STATUS, &val); if (val & JH71XX_PMU_INT_SEQ_DONE) dev_dbg(pmu->dev, "sequence done.\n"); @@ -220,8 +223,8 @@ static irqreturn_t jh71xx_pmu_interrupt(int irq, void *data) dev_err(pmu->dev, "p-channel fail event.\n"); /* clear interrupts */ - writel(val, pmu->base + JH71XX_PMU_INT_STATUS); - writel(val, pmu->base + JH71XX_PMU_EVENT_STATUS); + regmap_write(pmu->base, JH71XX_PMU_INT_STATUS, val); + regmap_write(pmu->base, JH71XX_PMU_EVENT_STATUS, val); return IRQ_HANDLED; } @@ -271,7 +274,7 @@ static int jh71xx_pmu_probe(struct platform_device *pdev) if (!pmu) return -ENOMEM; - pmu->base = devm_platform_ioremap_resource(pdev, 0); + pmu->base = device_node_to_regmap(np); if (IS_ERR(pmu->base)) return PTR_ERR(pmu->base);
Modify ioremap to regmap, easy to simplify code. Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com> --- drivers/soc/starfive/jh71xx_pmu.c | 43 +++++++++++++++++-------------- 1 file changed, 23 insertions(+), 20 deletions(-)