diff mbox series

[RESEND,v2,4/6] soc: starfive: Extract JH7110 pmu private operations

Message ID 20230419035646.43702-5-changhuang.liang@starfivetech.com
State New
Headers
Series Add JH7110 AON PMU support |

Commit Message

Changhuang Liang April 19, 2023, 3:56 a.m. UTC 
  Move JH7110 private operation into private data of compatible.
Convenient to expand different compatible.

Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
---
 drivers/soc/starfive/jh71xx_pmu.c | 98 +++++++++++++++++++++----------
 1 file changed, 67 insertions(+), 31 deletions(-)

Comments

Conor Dooley April 19, 2023, 5:47 p.m. UTC | #1 
On Tue, Apr 18, 2023 at 08:56:44PM -0700, Changhuang Liang wrote:
> Move JH7110 private operation into private data of compatible.
> Convenient to expand different compatible.

I prefer how the code looks in v2, thanks.
However, just as in the prior patch, "Convenient to expand different
compatible" isn't really a justification - specifically, supporting the
power domain controller serving the dphy is your motivation here. The
important difference being that it uses a regmap from a syscon and has
no interrupts nor the encourage features.

Although, given the only real similarity the code driving each of the
PMUs is the variable names, I guess you could argue that this driver
should be left alone and the "aon dphy" should be a different driver
altogether.

I don't have a strong opinion though & if it's fine with Walker and
noone else objects, it's fine with me...
Changhuang Liang April 21, 2023, 3:27 a.m. UTC | #2 
On 2023/4/20 1:47, Conor Dooley wrote:
> On Tue, Apr 18, 2023 at 08:56:44PM -0700, Changhuang Liang wrote:
>> Move JH7110 private operation into private data of compatible.
>> Convenient to expand different compatible.
> 
> I prefer how the code looks in v2, thanks.
> However, just as in the prior patch, "Convenient to expand different
> compatible" isn't really a justification - specifically, supporting the
> power domain controller serving the dphy is your motivation here. The
> important difference being that it uses a regmap from a syscon and has
> no interrupts nor the encourage features.
> 

So should I expand the commit message which called "in order to add the 
aon power domain" although the patch is applied behind current patch.

> Although, given the only real similarity the code driving each of the
> PMUs is the variable names, I guess you could argue that this driver
> should be left alone and the "aon dphy" should be a different driver
> altogether.
> 

I have tried independent this aon pmu, but it code is very similar to the
original pmu, so I think they can put together, reduce linux kernel bloat.

> I don't have a strong opinion though & if it's fine with Walker and
> noone else objects, it's fine with me...
Conor Dooley April 21, 2023, 6:57 a.m. UTC | #3 
On Fri, Apr 21, 2023 at 11:27:52AM +0800, Changhuang Liang wrote:
> On 2023/4/20 1:47, Conor Dooley wrote:
> > On Tue, Apr 18, 2023 at 08:56:44PM -0700, Changhuang Liang wrote:
> >> Move JH7110 private operation into private data of compatible.
> >> Convenient to expand different compatible.
> > 
> > I prefer how the code looks in v2, thanks.
> > However, just as in the prior patch, "Convenient to expand different
> > compatible" isn't really a justification - specifically, supporting the
> > power domain controller serving the dphy is your motivation here. The
> > important difference being that it uses a regmap from a syscon and has
> > no interrupts nor the encourage features.
> > 
> 
> So should I expand the commit message which called "in order to add the 
> aon power domain" although the patch is applied behind current patch.

Only if you have to resend for some other reason. If there is no other
reason to resend then I will do this when I apply the patch.

Thanks,
Conor.
Changhuang Liang April 21, 2023, 7:47 a.m. UTC | #4 
On 2023/4/21 14:57, Conor Dooley wrote:
> On Fri, Apr 21, 2023 at 11:27:52AM +0800, Changhuang Liang wrote:
>> On 2023/4/20 1:47, Conor Dooley wrote:
>>> On Tue, Apr 18, 2023 at 08:56:44PM -0700, Changhuang Liang wrote:
>>>> Move JH7110 private operation into private data of compatible.
>>>> Convenient to expand different compatible.
>>>
>>> I prefer how the code looks in v2, thanks.
>>> However, just as in the prior patch, "Convenient to expand different
>>> compatible" isn't really a justification - specifically, supporting the
>>> power domain controller serving the dphy is your motivation here. The
>>> important difference being that it uses a regmap from a syscon and has
>>> no interrupts nor the encourage features.
>>>
>>
>> So should I expand the commit message which called "in order to add the 
>> aon power domain" although the patch is applied behind current patch.
> 
> Only if you have to resend for some other reason. If there is no other
> reason to resend then I will do this when I apply the patch.
> 
> Thanks,
> Conor.

OK, thanks for your support.
diff mbox series

Patch

diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c
index 306218c83691..bb44cc93e822 100644
--- a/drivers/soc/starfive/jh71xx_pmu.c
+++ b/drivers/soc/starfive/jh71xx_pmu.c
@@ -51,9 +51,17 @@  struct jh71xx_domain_info {
 	u8 bit;
 };
 
+struct jh71xx_pmu;
+struct jh71xx_pmu_dev;
+
 struct jh71xx_pmu_match_data {
 	const struct jh71xx_domain_info *domain_info;
 	int num_domains;
+	unsigned int pmu_status;
+	int (*pmu_parse_dt)(struct platform_device *pdev,
+			    struct jh71xx_pmu *pmu);
+	int (*pmu_set_state)(struct jh71xx_pmu_dev *pmd,
+			     u32 mask, bool on);
 };
 
 struct jh71xx_pmu {
@@ -80,14 +88,14 @@  static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_o
 	if (!mask)
 		return -EINVAL;
 
-	regmap_read(pmu->base, JH71XX_PMU_CURR_POWER_MODE, &val);
+	regmap_read(pmu->base, pmu->match_data->pmu_status, &val);
 
 	*is_on = val & mask;
 
 	return 0;
 }
 
-static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
+static int jh7110_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
 {
 	struct jh71xx_pmu *pmu = pmd->pmu;
 	unsigned long flags;
@@ -95,22 +103,8 @@  static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
 	u32 mode;
 	u32 encourage_lo;
 	u32 encourage_hi;
-	bool is_on;
 	int ret;
 
-	ret = jh71xx_pmu_get_state(pmd, mask, &is_on);
-	if (ret) {
-		dev_dbg(pmu->dev, "unable to get current state for %s\n",
-			pmd->genpd.name);
-		return ret;
-	}
-
-	if (is_on == on) {
-		dev_dbg(pmu->dev, "pm domain [%s] is already %sable status.\n",
-			pmd->genpd.name, on ? "en" : "dis");
-		return 0;
-	}
-
 	spin_lock_irqsave(&pmu->lock, flags);
 
 	/*
@@ -169,6 +163,29 @@  static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
 	return 0;
 }
 
+static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
+{
+	struct jh71xx_pmu *pmu = pmd->pmu;
+	const struct jh71xx_pmu_match_data *match_data = pmu->match_data;
+	bool is_on;
+	int ret;
+
+	ret = jh71xx_pmu_get_state(pmd, mask, &is_on);
+	if (ret) {
+		dev_dbg(pmu->dev, "unable to get current state for %s\n",
+			pmd->genpd.name);
+		return ret;
+	}
+
+	if (is_on == on) {
+		dev_dbg(pmu->dev, "pm domain [%s] is already %sable status.\n",
+			pmd->genpd.name, on ? "en" : "dis");
+		return 0;
+	}
+
+	return match_data->pmu_set_state(pmd, mask, on);
+}
+
 static int jh71xx_pmu_on(struct generic_pm_domain *genpd)
 {
 	struct jh71xx_pmu_dev *pmd = container_of(genpd,
@@ -229,6 +246,30 @@  static irqreturn_t jh71xx_pmu_interrupt(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static int jh7110_pmu_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;
@@ -274,23 +315,18 @@  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;
 
+	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);
@@ -310,9 +346,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);
@@ -360,6 +393,9 @@  static const struct jh71xx_domain_info jh7110_power_domains[] = {
 static const struct jh71xx_pmu_match_data jh7110_pmu = {
 	.num_domains = ARRAY_SIZE(jh7110_power_domains),
 	.domain_info = jh7110_power_domains,
+	.pmu_status = JH71XX_PMU_CURR_POWER_MODE,
+	.pmu_parse_dt = jh7110_pmu_parse_dt,
+	.pmu_set_state = jh7110_pmu_set_state,
 };
 
 static const struct of_device_id jh71xx_pmu_of_match[] = {