[PATCH v1 1/3] pinctrl: intel: Introduce intel_restore_padcfg() helper

[PATCH v1 1/3] pinctrl: intel: Introduce intel_restore_padcfg() helper

[Andy Shevchenko]

Deduplicate restoring PADCFGx registers by using a common helper.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 51 +++++++++++++--------------
 1 file changed, 24 insertions(+), 27 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index bc013599a9a3..18467e2efe99 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1588,6 +1588,27 @@ intel_gpio_update_pad_mode(void __iomem *hostown, u32 mask, u32 value)
 	return curr;
 }
 
+static void intel_restore_padcfg(struct intel_pinctrl *pctrl, unsigned int pin,
+				 unsigned int reg, u32 value)
+{
+	struct device *dev = pctrl->dev;
+	unsigned int n = reg / sizeof(u32);
+	void __iomem *padcfg;
+	u32 mask = (reg == PADCFG0) ? PADCFG0_GPIORXSTATE : 0;
+	u32 val;
+
+	padcfg = intel_get_padcfg(pctrl, pin, reg);
+	if (!padcfg)
+		return;
+
+	val = readl(padcfg) & ~mask;
+	if (val == value)
+		return;
+
+	writel(value, padcfg);
+	dev_dbg(dev, "restored pin %u padcfg%u %#08x\n", pin, n, readl(padcfg));
+}
+
 int intel_pinctrl_resume_noirq(struct device *dev)
 {
 	struct intel_pinctrl *pctrl = dev_get_drvdata(dev);
@@ -1601,37 +1622,13 @@ int intel_pinctrl_resume_noirq(struct device *dev)
 	pads = pctrl->context.pads;
 	for (i = 0; i < pctrl->soc->npins; i++) {
 		const struct pinctrl_pin_desc *desc = &pctrl->soc->pins[i];
-		void __iomem *padcfg;
-		u32 val;
 
 		if (!intel_pinctrl_should_save(pctrl, desc->number))
 			continue;
 
-		padcfg = intel_get_padcfg(pctrl, desc->number, PADCFG0);
-		val = readl(padcfg) & ~PADCFG0_GPIORXSTATE;
-		if (val != pads[i].padcfg0) {
-			writel(pads[i].padcfg0, padcfg);
-			dev_dbg(dev, "restored pin %u padcfg0 %#08x\n",
-				desc->number, readl(padcfg));
-		}
-
-		padcfg = intel_get_padcfg(pctrl, desc->number, PADCFG1);
-		val = readl(padcfg);
-		if (val != pads[i].padcfg1) {
-			writel(pads[i].padcfg1, padcfg);
-			dev_dbg(dev, "restored pin %u padcfg1 %#08x\n",
-				desc->number, readl(padcfg));
-		}
-
-		padcfg = intel_get_padcfg(pctrl, desc->number, PADCFG2);
-		if (padcfg) {
-			val = readl(padcfg);
-			if (val != pads[i].padcfg2) {
-				writel(pads[i].padcfg2, padcfg);
-				dev_dbg(dev, "restored pin %u padcfg2 %#08x\n",
-					desc->number, readl(padcfg));
-			}
-		}
+		intel_restore_padcfg(pctrl, desc->number, PADCFG0, pads[i].padcfg0);
+		intel_restore_padcfg(pctrl, desc->number, PADCFG1, pads[i].padcfg1);
+		intel_restore_padcfg(pctrl, desc->number, PADCFG2, pads[i].padcfg2);
 	}
 
 	communities = pctrl->context.communities;
-- 
2.23.0

[PATCH v1 2/3] pinctrl: intel: Introduce intel_restore_hostown() helper

[Andy Shevchenko]

Refactor restoring HOSTSW_OWN registers by using an introduced helper.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 38 +++++++++++++++------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 18467e2efe99..e59ac31921e7 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1588,6 +1588,25 @@ intel_gpio_update_pad_mode(void __iomem *hostown, u32 mask, u32 value)
 	return curr;
 }
 
+static void intel_restore_hostown(struct intel_pinctrl *pctrl, unsigned int c,
+				  void __iomem *base, unsigned int gpp, u32 saved)
+{
+	struct intel_community *community = &pctrl->communities[c];
+	const struct intel_padgroup *padgrp = &community->gpps[gpp];
+	struct device *dev = pctrl->dev;
+	u32 requested, value;
+
+	if (padgrp->gpio_base < 0)
+		return;
+
+	requested = intel_gpio_is_requested(&pctrl->chip, padgrp->gpio_base, padgrp->size);
+	value = intel_gpio_update_pad_mode(base + gpp * 4, requested, saved);
+	if (!((value ^ saved) & requested))
+		return;
+
+	dev_warn(dev, "restored hostown %u/%u %#8x->%#8x\n", c, gpp, value, saved);
+}
+
 static void intel_restore_padcfg(struct intel_pinctrl *pctrl, unsigned int pin,
 				 unsigned int reg, u32 value)
 {
@@ -1645,23 +1664,8 @@ int intel_pinctrl_resume_noirq(struct device *dev)
 		}
 
 		base = community->regs + community->hostown_offset;
-		for (gpp = 0; gpp < community->ngpps; gpp++) {
-			const struct intel_padgroup *padgrp = &community->gpps[gpp];
-			u32 requested = 0, value = 0;
-			u32 saved = communities[i].hostown[gpp];
-
-			if (padgrp->gpio_base < 0)
-				continue;
-
-			requested = intel_gpio_is_requested(&pctrl->chip,
-					padgrp->gpio_base, padgrp->size);
-			value = intel_gpio_update_pad_mode(base + gpp * 4,
-					requested, saved);
-			if ((value ^ saved) & requested) {
-				dev_warn(dev, "restore hostown %d/%u %#8x->%#8x\n",
-					i, gpp, value, saved);
-			}
-		}
+		for (gpp = 0; gpp < community->ngpps; gpp++)
+			intel_restore_hostown(pctrl, i, base, gpp, communities[i].hostown[gpp]);
 	}
 
 	return 0;
-- 
2.23.0

Re: [PATCH v1 2/3] pinctrl: intel: Introduce intel_restore_hostown() helper

[Mika Westerberg]

On Mon, Oct 14, 2019 at 11:43:47AM +0300, Andy Shevchenko wrote:
> Refactor restoring HOSTSW_OWN registers by using an introduced helper.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 38 +++++++++++++++------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 18467e2efe99..e59ac31921e7 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -1588,6 +1588,25 @@ intel_gpio_update_pad_mode(void __iomem *hostown, u32 mask, u32 value)
>  	return curr;
>  }
>  
> +static void intel_restore_hostown(struct intel_pinctrl *pctrl, unsigned int c,
> +				  void __iomem *base, unsigned int gpp, u32 saved)
> +{
> +	struct intel_community *community = &pctrl->communities[c];

const?

[PATCH v1 3/3] pinctrl: intel: Introduce intel_restore_intmask() helper

[Andy Shevchenko]

Refactor restoring GPI_IE registers by using an introduced helper.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index e59ac31921e7..b9df243e19cf 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1607,6 +1607,15 @@ static void intel_restore_hostown(struct intel_pinctrl *pctrl, unsigned int c,
 	dev_warn(dev, "restored hostown %u/%u %#8x->%#8x\n", c, gpp, value, saved);
 }
 
+static void intel_restore_intmask(struct intel_pinctrl *pctrl, unsigned int c,
+				  void __iomem *base, unsigned int gpp, u32 saved)
+{
+	struct device *dev = pctrl->dev;
+
+	writel(saved, base + gpp * 4);
+	dev_dbg(dev, "restored mask %u/%u %#08x\n", c, gpp, readl(base + gpp * 4));
+}
+
 static void intel_restore_padcfg(struct intel_pinctrl *pctrl, unsigned int pin,
 				 unsigned int reg, u32 value)
 {
@@ -1657,11 +1666,8 @@ int intel_pinctrl_resume_noirq(struct device *dev)
 		unsigned int gpp;
 
 		base = community->regs + community->ie_offset;
-		for (gpp = 0; gpp < community->ngpps; gpp++) {
-			writel(communities[i].intmask[gpp], base + gpp * 4);
-			dev_dbg(dev, "restored mask %d/%u %#08x\n", i, gpp,
-				readl(base + gpp * 4));
-		}
+		for (gpp = 0; gpp < community->ngpps; gpp++)
+			intel_restore_intmask(pctrl, i, base, gpp, communities[i].intmask[gpp]);
 
 		base = community->regs + community->hostown_offset;
 		for (gpp = 0; gpp < community->ngpps; gpp++)
-- 
2.23.0

Re: [PATCH v1 3/3] pinctrl: intel: Introduce intel_restore_intmask() helper

[Mika Westerberg]

On Mon, Oct 14, 2019 at 11:43:48AM +0300, Andy Shevchenko wrote:
> Refactor restoring GPI_IE registers by using an introduced helper.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index e59ac31921e7..b9df243e19cf 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -1607,6 +1607,15 @@ static void intel_restore_hostown(struct intel_pinctrl *pctrl, unsigned int c,
>  	dev_warn(dev, "restored hostown %u/%u %#8x->%#8x\n", c, gpp, value, saved);
>  }
>  
> +static void intel_restore_intmask(struct intel_pinctrl *pctrl, unsigned int c,
> +				  void __iomem *base, unsigned int gpp, u32 saved)
> +{
> +	struct device *dev = pctrl->dev;

const?

Also should we do the same here as we do with others and check first
whether we need to update the mask at all?

Re: [PATCH v1 3/3] pinctrl: intel: Introduce intel_restore_intmask() helper

[Andy Shevchenko]

On Mon, Oct 21, 2019 at 03:27:20PM +0300, Mika Westerberg wrote:
> On Mon, Oct 14, 2019 at 11:43:48AM +0300, Andy Shevchenko wrote:
> > Refactor restoring GPI_IE registers by using an introduced helper.

> > +	struct device *dev = pctrl->dev;
> 
> const?

It's only about dozen occurrences in 8 drivers altogether in the kernel
to have const with struct device.

So, I consider this pattern is unusual.

> Also should we do the same here as we do with others and check first
> whether we need to update the mask at all?

I will look at it.
I think we may do it as a separate change (this doesn't change any
functionality).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/3] pinctrl: intel: Introduce intel_restore_padcfg() helper

[Mika Westerberg]

On Mon, Oct 14, 2019 at 11:43:46AM +0300, Andy Shevchenko wrote:
> Deduplicate restoring PADCFGx registers by using a common helper.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 51 +++++++++++++--------------
>  1 file changed, 24 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index bc013599a9a3..18467e2efe99 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -1588,6 +1588,27 @@ intel_gpio_update_pad_mode(void __iomem *hostown, u32 mask, u32 value)
>  	return curr;
>  }
>  
> +static void intel_restore_padcfg(struct intel_pinctrl *pctrl, unsigned int pin,
> +				 unsigned int reg, u32 value)
> +{
> +	struct device *dev = pctrl->dev;
> +	unsigned int n = reg / sizeof(u32);
> +	void __iomem *padcfg;
> +	u32 mask = (reg == PADCFG0) ? PADCFG0_GPIORXSTATE : 0;
> +	u32 val;

Looks better if you order them like:

	u32 mask = (reg == PADCFG0) ? PADCFG0_GPIORXSTATE : 0;
	unsigned int n = reg / sizeof(u32);
	struct device *dev = pctrl->dev;
	void __iomem *padcfg;
	u32 val;