[PATCH v1 0/2] mfd: lpc_ich: Simplify GPIO resource handling

[PATCH v1 0/2] mfd: lpc_ich: Simplify GPIO resource handling

[Andy Shevchenko]

Introduce a helper to iopoll.h which allows to simplify GPIO resource handling
in lpc_ich driver for the starter. The helper can be used in many other cases.

Andy Shevchenko (2):
  resource: Introduce resource_rebase() helper
  mfd: lpc_ich: Convert to use resource_rebase()

 drivers/mfd/lpc_ich.c  | 38 ++++++++------------------------------
 include/linux/ioport.h |  6 ++++++
 2 files changed, 14 insertions(+), 30 deletions(-)

-- 
2.50.1

[PATCH v1 1/2] resource: Introduce resource_rebase() helper

[Andy Shevchenko]

Introduce a helper to add an offset to the resource. This is helpful
in the cases when, for example) the resource has statically defined
the start and end fields, but the base of it is yet to be defined,
usually dynamically at run-time.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/ioport.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index e8b2d6aa4013..159e74284d0b 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -290,6 +290,12 @@ static inline resource_size_t resource_size(const struct resource *res)
 {
 	return res->end - res->start + 1;
 }
+
+static inline void resource_rebase(struct resource *res, resource_size_t start)
+{
+	resource_set_range(res, start + res->start, resource_size(res));
+}
+
 static inline unsigned long resource_type(const struct resource *res)
 {
 	return res->flags & IORESOURCE_TYPE_BITS;
-- 
2.50.1

[PATCH v1 2/2] mfd: lpc_ich: Convert to use resource_rebase()

[Andy Shevchenko]

Simplify the resource handling by converting to use resource_rebase().
No functional change intended.

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

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index 4b7d0cb9340f..6942a744e87b 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -132,15 +132,12 @@ static struct mfd_cell lpc_ich_gpio_cell = {
 	.ignore_resource_conflicts = true,
 };
 
-#define INTEL_GPIO_RESOURCE_SIZE	0x1000
-
 struct lpc_ich_gpio_info {
 	const char *hid;
 	const struct mfd_cell *devices;
 	size_t nr_devices;
 	struct resource **resources;
 	size_t nr_resources;
-	const resource_size_t *offsets;
 };
 
 #define APL_GPIO_NORTH		0
@@ -151,31 +148,23 @@ struct lpc_ich_gpio_info {
 #define APL_GPIO_NR_DEVICES	4
 #define APL_GPIO_NR_RESOURCES	4
 
-/* Offset data for Apollo Lake GPIO controllers */
-static const resource_size_t apl_gpio_offsets[APL_GPIO_NR_RESOURCES] = {
-	[APL_GPIO_NORTH]	= 0xc50000,
-	[APL_GPIO_NORTHWEST]	= 0xc40000,
-	[APL_GPIO_WEST]		= 0xc70000,
-	[APL_GPIO_SOUTHWEST]	= 0xc00000,
-};
-
 #define APL_GPIO_IRQ			14
 
 static struct resource apl_gpio_resources[APL_GPIO_NR_DEVICES][2] = {
 	[APL_GPIO_NORTH] = {
-		DEFINE_RES_MEM(0, 0),
+		DEFINE_RES_MEM(0xc50000, 0x1000),
 		DEFINE_RES_IRQ(APL_GPIO_IRQ),
 	},
 	[APL_GPIO_NORTHWEST] = {
-		DEFINE_RES_MEM(0, 0),
+		DEFINE_RES_MEM(0xc40000, 0x1000),
 		DEFINE_RES_IRQ(APL_GPIO_IRQ),
 	},
 	[APL_GPIO_WEST] = {
-		DEFINE_RES_MEM(0, 0),
+		DEFINE_RES_MEM(0xc70000, 0x1000),
 		DEFINE_RES_IRQ(APL_GPIO_IRQ),
 	},
 	[APL_GPIO_SOUTHWEST] = {
-		DEFINE_RES_MEM(0, 0),
+		DEFINE_RES_MEM(0xc00000, 0x1000),
 		DEFINE_RES_IRQ(APL_GPIO_IRQ),
 	},
 };
@@ -224,7 +213,6 @@ static const struct lpc_ich_gpio_info apl_gpio_info = {
 	.nr_devices = ARRAY_SIZE(apl_gpio_devices),
 	.resources = apl_gpio_mem_resources,
 	.nr_resources = ARRAY_SIZE(apl_gpio_mem_resources),
-	.offsets = apl_gpio_offsets,
 };
 
 #define DNV_GPIO_NORTH		0
@@ -233,17 +221,11 @@ static const struct lpc_ich_gpio_info apl_gpio_info = {
 #define DNV_GPIO_NR_DEVICES	1
 #define DNV_GPIO_NR_RESOURCES	2
 
-/* Offset data for Denverton GPIO controllers */
-static const resource_size_t dnv_gpio_offsets[DNV_GPIO_NR_RESOURCES] = {
-	[DNV_GPIO_NORTH]	= 0xc20000,
-	[DNV_GPIO_SOUTH]	= 0xc50000,
-};
-
 #define DNV_GPIO_IRQ			14
 
 static struct resource dnv_gpio_resources[DNV_GPIO_NR_RESOURCES + 1] = {
-	[DNV_GPIO_NORTH] = DEFINE_RES_MEM(0, 0),
-	[DNV_GPIO_SOUTH] = DEFINE_RES_MEM(0, 0),
+	[DNV_GPIO_NORTH] = DEFINE_RES_MEM(0xc20000, 0x1000),
+	[DNV_GPIO_SOUTH] = DEFINE_RES_MEM(0xc50000, 0x1000),
 	DEFINE_RES_IRQ(DNV_GPIO_IRQ),
 };
 
@@ -267,7 +249,6 @@ static const struct lpc_ich_gpio_info dnv_gpio_info = {
 	.nr_devices = ARRAY_SIZE(dnv_gpio_devices),
 	.resources = dnv_gpio_mem_resources,
 	.nr_resources = ARRAY_SIZE(dnv_gpio_mem_resources),
-	.offsets = dnv_gpio_offsets,
 };
 
 static struct mfd_cell lpc_ich_spi_cell = {
@@ -1251,12 +1232,9 @@ static int lpc_ich_init_pinctrl(struct pci_dev *dev)
 
 	for (i = 0; i < info->nr_resources; i++) {
 		struct resource *mem = info->resources[i];
-		resource_size_t offset = info->offsets[i];
 
-		/* Fill MEM resource */
-		mem->start = base.start + offset;
-		mem->end = base.start + offset + INTEL_GPIO_RESOURCE_SIZE - 1;
-		mem->flags = base.flags;
+		/* Rebase MEM resource */
+		resource_rebase(mem, base.start);
 	}
 
 	return mfd_add_devices(&dev->dev, 0, info->devices, info->nr_devices,
-- 
2.50.1

Re: [PATCH v1 2/2] mfd: lpc_ich: Convert to use resource_rebase()

[Andy Shevchenko]

On Wed, Sep 03, 2025 at 10:12:29AM +0200, Andy Shevchenko wrote:
> Simplify the resource handling by converting to use resource_rebase().
> No functional change intended.

...

>  	for (i = 0; i < info->nr_resources; i++) {
>  		struct resource *mem = info->resources[i];
> -		resource_size_t offset = info->offsets[i];
>  
> -		/* Fill MEM resource */
> -		mem->start = base.start + offset;
> -		mem->end = base.start + offset + INTEL_GPIO_RESOURCE_SIZE - 1;
> -		mem->flags = base.flags;
> +		/* Rebase MEM resource */
> +		resource_rebase(mem, base.start);
>  	}

We may gain 3 LoC more by dropping temporary variable.

But I will wait for the reviews in general before mocking up v2 of this.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] mfd: lpc_ich: Convert to use resource_rebase()

[Ilpo Järvinen]

On Wed, 3 Sep 2025, Andy Shevchenko wrote:

> On Wed, Sep 03, 2025 at 10:12:29AM +0200, Andy Shevchenko wrote:
> > Simplify the resource handling by converting to use resource_rebase().
> > No functional change intended.
> 
> ...
> 
> >  	for (i = 0; i < info->nr_resources; i++) {
> >  		struct resource *mem = info->resources[i];
> > -		resource_size_t offset = info->offsets[i];
> >  
> > -		/* Fill MEM resource */
> > -		mem->start = base.start + offset;
> > -		mem->end = base.start + offset + INTEL_GPIO_RESOURCE_SIZE - 1;
> > -		mem->flags = base.flags;
> > +		/* Rebase MEM resource */
> > +		resource_rebase(mem, base.start);
> >  	}
> 
> We may gain 3 LoC more by dropping temporary variable.

Yeah, drop that super-obvious comment as well. :-)

> But I will wait for the reviews in general before mocking up v2 of this.
> 
> 

-- 
 i.

Re: [PATCH v1 1/2] resource: Introduce resource_rebase() helper

[Ilpo Järvinen]

On Wed, 3 Sep 2025, Andy Shevchenko wrote:

> Introduce a helper to add an offset to the resource. This is helpful
> in the cases when, for example) the resource has statically defined
> the start and end fields, but the base of it is yet to be defined,
> usually dynamically at run-time.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/ioport.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index e8b2d6aa4013..159e74284d0b 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -290,6 +290,12 @@ static inline resource_size_t resource_size(const struct resource *res)
>  {
>  	return res->end - res->start + 1;
>  }
> +
> +static inline void resource_rebase(struct resource *res, resource_size_t start)
> +{
> +	resource_set_range(res, start + res->start, resource_size(res));
> +}

Hi Andy,

This seems fine, it's nice to get rid of complex ->end calculations. But I 
wanted to mention another common case which is resetting the base to zero.
Are we expected to use resource_rebase() for those cases too? I've been 
thinking of adding something like resource_reset().

resource_rebase(res, 0) would work for those cases but it doesn't then  
carry the intent of "removing" the base in its name. Opinions?

-- 
 i.

Re: [PATCH v1 1/2] resource: Introduce resource_rebase() helper

[Andy Shevchenko]

On Wed, Sep 03, 2025 at 03:29:00PM +0300, Ilpo Järvinen wrote:
> On Wed, 3 Sep 2025, Andy Shevchenko wrote:

...

> > +static inline void resource_rebase(struct resource *res, resource_size_t start)
> > +{
> > +	resource_set_range(res, start + res->start, resource_size(res));
> > +}
> 
> This seems fine, it's nice to get rid of complex ->end calculations. But I 
> wanted to mention another common case which is resetting the base to zero.
> Are we expected to use resource_rebase() for those cases too? I've been 
> thinking of adding something like resource_reset().
> 
> resource_rebase(res, 0) would work for those cases but it doesn't then  
> carry the intent of "removing" the base in its name. Opinions?

Another case I have just realised is repeated "rebase" over the statically
global resource (when driver is in but device is bind-unbind-and-repeat).

Perhaps rebase has to be idempotent. That will do exactly, if I'm not mistaken,
what you are telling as a _reset() by default.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/2] mfd: lpc_ich: Simplify GPIO resource handling

[Andy Shevchenko]

On Wed, Sep 03, 2025 at 10:12:27AM +0200, Andy Shevchenko wrote:
> Introduce a helper to iopoll.h which allows to simplify GPIO resource handling
> in lpc_ich driver for the starter. The helper can be used in many other cases.

After thinking a bit, this series is no go as it will regress on module unbind-bind
and I have no clear view right now how to avoid that. So self-NAK.

-- 
With Best Regards,
Andy Shevchenko