[PATCH] mfd: lpc_ich: Separate device cells for clarity

[PATCH] mfd: lpc_ich: Separate device cells for clarity

[Aaron Sierra]

The lpc_ich_cells array gives the wrong impression about the
relationship between the watchdog and GPIO devices. They are
completely distinct devices, so this patch separates the
array into distinct mfd_cell structs per device.

A side effect of removing the array, is that the lpc_cells enum
is no longer needed.

Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++------------------------
 1 file changed, 18 insertions(+), 24 deletions(-)

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index 8de3439..7a01d430 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -131,24 +131,18 @@ static struct resource gpio_ich_res[] = {
 	},
 };
 
-enum lpc_cells {
-	LPC_WDT = 0,
-	LPC_GPIO,
+static struct mfd_cell lpc_ich_wdt_cell = {
+	.name = "iTCO_wdt",
+	.num_resources = ARRAY_SIZE(wdt_ich_res),
+	.resources = wdt_ich_res,
+	.ignore_resource_conflicts = true,
 };
 
-static struct mfd_cell lpc_ich_cells[] = {
-	[LPC_WDT] = {
-		.name = "iTCO_wdt",
-		.num_resources = ARRAY_SIZE(wdt_ich_res),
-		.resources = wdt_ich_res,
-		.ignore_resource_conflicts = true,
-	},
-	[LPC_GPIO] = {
-		.name = "gpio_ich",
-		.num_resources = ARRAY_SIZE(gpio_ich_res),
-		.resources = gpio_ich_res,
-		.ignore_resource_conflicts = true,
-	},
+static struct mfd_cell lpc_ich_gpio_cell = {
+	.name = "gpio_ich",
+	.num_resources = ARRAY_SIZE(gpio_ich_res),
+	.resources = gpio_ich_res,
+	.ignore_resource_conflicts = true,
 };
 
 /* chipset related info */
@@ -881,7 +875,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
 	base_addr = base_addr_cfg & 0x0000ff80;
 	if (!base_addr) {
 		dev_notice(&dev->dev, "I/O space for ACPI uninitialized\n");
-		lpc_ich_cells[LPC_GPIO].num_resources--;
+		lpc_ich_gpio_cell.num_resources--;
 		goto gpe0_done;
 	}
 
@@ -895,7 +889,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
 		 * the platform_device subsystem doesn't see this resource
 		 * or it will register an invalid region.
 		 */
-		lpc_ich_cells[LPC_GPIO].num_resources--;
+		lpc_ich_gpio_cell.num_resources--;
 		acpi_conflict = true;
 	} else {
 		lpc_ich_enable_acpi_space(dev);
@@ -933,14 +927,14 @@ gpe0_done:
 	lpc_chipset_info[priv->chipset].use_gpio = ret;
 	lpc_ich_enable_gpio_space(dev);
 
-	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
+	lpc_ich_finalize_cell(dev, &lpc_ich_gpio_cell);
 	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
-			      &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
+			      &lpc_ich_gpio_cell, 1, NULL, 0, NULL);
 
 gpio_done:
 	if (acpi_conflict)
 		pr_warn("Resource conflict(s) found affecting %s\n",
-				lpc_ich_cells[LPC_GPIO].name);
+				lpc_ich_gpio_cell.name);
 	return ret;
 }
 
@@ -984,7 +978,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
 	 */
 	if (lpc_chipset_info[priv->chipset].iTCO_version == 1) {
 		/* Don't register iomem for TCO ver 1 */
-		lpc_ich_cells[LPC_WDT].num_resources--;
+		lpc_ich_wdt_cell.num_resources--;
 	} else if (lpc_chipset_info[priv->chipset].iTCO_version == 2) {
 		pci_read_config_dword(dev, RCBABASE, &base_addr_cfg);
 		base_addr = base_addr_cfg & 0xffffc000;
@@ -1007,9 +1001,9 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
 		res->end = base_addr + ACPIBASE_PMC_END;
 	}
 
-	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
+	lpc_ich_finalize_cell(dev, &lpc_ich_wdt_cell);
 	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
-			      &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
+			      &lpc_ich_wdt_cell, 1, NULL, 0, NULL);
 
 wdt_done:
 	return ret;
-- 
1.9.1

Re: [PATCH] mfd: lpc_ich: Separate device cells for clarity

[Andy Shevchenko]

On Thu, 2015-09-03 at 14:14 -0500, Aaron Sierra wrote:
> The lpc_ich_cells array gives the wrong impression about the
> relationship between the watchdog and GPIO devices. They are
> completely distinct devices, so this patch separates the
> array into distinct mfd_cell structs per device.
> 
> A side effect of removing the array, is that the lpc_cells enum
> is no longer needed.
> 

Looks good for me.

> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> ---
>  drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++-----------------------
> -
>  1 file changed, 18 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index 8de3439..7a01d430 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -131,24 +131,18 @@ static struct resource gpio_ich_res[] = {
>  	},
>  };
>  
> -enum lpc_cells {
> -	LPC_WDT = 0,
> -	LPC_GPIO,
> +static struct mfd_cell lpc_ich_wdt_cell = {
> +	.name = "iTCO_wdt",
> +	.num_resources = ARRAY_SIZE(wdt_ich_res),
> +	.resources = wdt_ich_res,
> +	.ignore_resource_conflicts = true,
>  };
>  
> -static struct mfd_cell lpc_ich_cells[] = {
> -	[LPC_WDT] = {
> -		.name = "iTCO_wdt",
> -		.num_resources = ARRAY_SIZE(wdt_ich_res),
> -		.resources = wdt_ich_res,
> -		.ignore_resource_conflicts = true,
> -	},
> -	[LPC_GPIO] = {
> -		.name = "gpio_ich",
> -		.num_resources = ARRAY_SIZE(gpio_ich_res),
> -		.resources = gpio_ich_res,
> -		.ignore_resource_conflicts = true,
> -	},
> +static struct mfd_cell lpc_ich_gpio_cell = {
> +	.name = "gpio_ich",
> +	.num_resources = ARRAY_SIZE(gpio_ich_res),
> +	.resources = gpio_ich_res,
> +	.ignore_resource_conflicts = true,
>  };
>  
>  /* chipset related info */
> @@ -881,7 +875,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
>  	base_addr = base_addr_cfg & 0x0000ff80;
>  	if (!base_addr) {
>  		dev_notice(&dev->dev, "I/O space for ACPI 
> uninitialized\n");
> -		lpc_ich_cells[LPC_GPIO].num_resources--;
> +		lpc_ich_gpio_cell.num_resources--;
>  		goto gpe0_done;
>  	}
>  
> @@ -895,7 +889,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
>  		 * the platform_device subsystem doesn't see this 
> resource
>  		 * or it will register an invalid region.
>  		 */
> -		lpc_ich_cells[LPC_GPIO].num_resources--;
> +		lpc_ich_gpio_cell.num_resources--;
>  		acpi_conflict = true;
>  	} else {
>  		lpc_ich_enable_acpi_space(dev);
> @@ -933,14 +927,14 @@ gpe0_done:
>  	lpc_chipset_info[priv->chipset].use_gpio = ret;
>  	lpc_ich_enable_gpio_space(dev);
>  
> -	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> +	lpc_ich_finalize_cell(dev, &lpc_ich_gpio_cell);
>  	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> -			      &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, 
> NULL);
> +			      &lpc_ich_gpio_cell, 1, NULL, 0, NULL);
>  
>  gpio_done:
>  	if (acpi_conflict)
>  		pr_warn("Resource conflict(s) found affecting %s\n",
> -				lpc_ich_cells[LPC_GPIO].name);
> +				lpc_ich_gpio_cell.name);
>  	return ret;
>  }
>  
> @@ -984,7 +978,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
>  	 */
>  	if (lpc_chipset_info[priv->chipset].iTCO_version == 1) {
>  		/* Don't register iomem for TCO ver 1 */
> -		lpc_ich_cells[LPC_WDT].num_resources--;
> +		lpc_ich_wdt_cell.num_resources--;
>  	} else if (lpc_chipset_info[priv->chipset].iTCO_version == 
> 2) {
>  		pci_read_config_dword(dev, RCBABASE, 
> &base_addr_cfg);
>  		base_addr = base_addr_cfg & 0xffffc000;
> @@ -1007,9 +1001,9 @@ static int lpc_ich_init_wdt(struct pci_dev 
> *dev)
>  		res->end = base_addr + ACPIBASE_PMC_END;
>  	}
>  
> -	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> +	lpc_ich_finalize_cell(dev, &lpc_ich_wdt_cell);
>  	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> -			      &lpc_ich_cells[LPC_WDT], 1, NULL, 0, 
> NULL);
> +			      &lpc_ich_wdt_cell, 1, NULL, 0, NULL);
>  
>  wdt_done:
>  	return ret;

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] mfd: lpc_ich: Separate device cells for clarity

[Lee Jones]

On Thu, 17 Sep 2015, Andy Shevchenko wrote:

> On Thu, 2015-09-03 at 14:14 -0500, Aaron Sierra wrote:
> > The lpc_ich_cells array gives the wrong impression about the
> > relationship between the watchdog and GPIO devices. They are
> > completely distinct devices, so this patch separates the
> > array into distinct mfd_cell structs per device.
> > 
> > A side effect of removing the array, is that the lpc_cells enum
> > is no longer needed.
> > 
> 
> Looks good for me.

Is that an Ack?  If so, can you formally provide one please?

> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> > ---
> >  drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++-----------------------
> > -
> >  1 file changed, 18 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> > index 8de3439..7a01d430 100644
> > --- a/drivers/mfd/lpc_ich.c
> > +++ b/drivers/mfd/lpc_ich.c
> > @@ -131,24 +131,18 @@ static struct resource gpio_ich_res[] = {
> >  	},
> >  };
> >  
> > -enum lpc_cells {
> > -	LPC_WDT = 0,
> > -	LPC_GPIO,
> > +static struct mfd_cell lpc_ich_wdt_cell = {
> > +	.name = "iTCO_wdt",
> > +	.num_resources = ARRAY_SIZE(wdt_ich_res),
> > +	.resources = wdt_ich_res,
> > +	.ignore_resource_conflicts = true,
> >  };
> >  
> > -static struct mfd_cell lpc_ich_cells[] = {
> > -	[LPC_WDT] = {
> > -		.name = "iTCO_wdt",
> > -		.num_resources = ARRAY_SIZE(wdt_ich_res),
> > -		.resources = wdt_ich_res,
> > -		.ignore_resource_conflicts = true,
> > -	},
> > -	[LPC_GPIO] = {
> > -		.name = "gpio_ich",
> > -		.num_resources = ARRAY_SIZE(gpio_ich_res),
> > -		.resources = gpio_ich_res,
> > -		.ignore_resource_conflicts = true,
> > -	},
> > +static struct mfd_cell lpc_ich_gpio_cell = {
> > +	.name = "gpio_ich",
> > +	.num_resources = ARRAY_SIZE(gpio_ich_res),
> > +	.resources = gpio_ich_res,
> > +	.ignore_resource_conflicts = true,
> >  };
> >  
> >  /* chipset related info */
> > @@ -881,7 +875,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
> >  	base_addr = base_addr_cfg & 0x0000ff80;
> >  	if (!base_addr) {
> >  		dev_notice(&dev->dev, "I/O space for ACPI 
> > uninitialized\n");
> > -		lpc_ich_cells[LPC_GPIO].num_resources--;
> > +		lpc_ich_gpio_cell.num_resources--;
> >  		goto gpe0_done;
> >  	}
> >  
> > @@ -895,7 +889,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
> >  		 * the platform_device subsystem doesn't see this 
> > resource
> >  		 * or it will register an invalid region.
> >  		 */
> > -		lpc_ich_cells[LPC_GPIO].num_resources--;
> > +		lpc_ich_gpio_cell.num_resources--;
> >  		acpi_conflict = true;
> >  	} else {
> >  		lpc_ich_enable_acpi_space(dev);
> > @@ -933,14 +927,14 @@ gpe0_done:
> >  	lpc_chipset_info[priv->chipset].use_gpio = ret;
> >  	lpc_ich_enable_gpio_space(dev);
> >  
> > -	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> > +	lpc_ich_finalize_cell(dev, &lpc_ich_gpio_cell);
> >  	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > -			      &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, 
> > NULL);
> > +			      &lpc_ich_gpio_cell, 1, NULL, 0, NULL);
> >  
> >  gpio_done:
> >  	if (acpi_conflict)
> >  		pr_warn("Resource conflict(s) found affecting %s\n",
> > -				lpc_ich_cells[LPC_GPIO].name);
> > +				lpc_ich_gpio_cell.name);
> >  	return ret;
> >  }
> >  
> > @@ -984,7 +978,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> >  	 */
> >  	if (lpc_chipset_info[priv->chipset].iTCO_version == 1) {
> >  		/* Don't register iomem for TCO ver 1 */
> > -		lpc_ich_cells[LPC_WDT].num_resources--;
> > +		lpc_ich_wdt_cell.num_resources--;
> >  	} else if (lpc_chipset_info[priv->chipset].iTCO_version == 
> > 2) {
> >  		pci_read_config_dword(dev, RCBABASE, 
> > &base_addr_cfg);
> >  		base_addr = base_addr_cfg & 0xffffc000;
> > @@ -1007,9 +1001,9 @@ static int lpc_ich_init_wdt(struct pci_dev 
> > *dev)
> >  		res->end = base_addr + ACPIBASE_PMC_END;
> >  	}
> >  
> > -	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> > +	lpc_ich_finalize_cell(dev, &lpc_ich_wdt_cell);
> >  	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > -			      &lpc_ich_cells[LPC_WDT], 1, NULL, 0, 
> > NULL);
> > +			      &lpc_ich_wdt_cell, 1, NULL, 0, NULL);
> >  
> >  wdt_done:
> >  	return ret;
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] mfd: lpc_ich: Separate device cells for clarity

[Lee Jones]

On Thu, 03 Sep 2015, Aaron Sierra wrote:

> The lpc_ich_cells array gives the wrong impression about the
> relationship between the watchdog and GPIO devices. They are
> completely distinct devices, so this patch separates the
> array into distinct mfd_cell structs per device.

What does this mean?  In what way are they distinct?

> A side effect of removing the array, is that the lpc_cells enum
> is no longer needed.
> 
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> ---
>  drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++------------------------
>  1 file changed, 18 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index 8de3439..7a01d430 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -131,24 +131,18 @@ static struct resource gpio_ich_res[] = {
>  	},
>  };
>  
> -enum lpc_cells {
> -	LPC_WDT = 0,
> -	LPC_GPIO,
> +static struct mfd_cell lpc_ich_wdt_cell = {
> +	.name = "iTCO_wdt",
> +	.num_resources = ARRAY_SIZE(wdt_ich_res),
> +	.resources = wdt_ich_res,
> +	.ignore_resource_conflicts = true,
>  };
>  
> -static struct mfd_cell lpc_ich_cells[] = {
> -	[LPC_WDT] = {
> -		.name = "iTCO_wdt",
> -		.num_resources = ARRAY_SIZE(wdt_ich_res),
> -		.resources = wdt_ich_res,
> -		.ignore_resource_conflicts = true,
> -	},
> -	[LPC_GPIO] = {
> -		.name = "gpio_ich",
> -		.num_resources = ARRAY_SIZE(gpio_ich_res),
> -		.resources = gpio_ich_res,
> -		.ignore_resource_conflicts = true,
> -	},
> +static struct mfd_cell lpc_ich_gpio_cell = {
> +	.name = "gpio_ich",
> +	.num_resources = ARRAY_SIZE(gpio_ich_res),
> +	.resources = gpio_ich_res,
> +	.ignore_resource_conflicts = true,
>  };
>  
>  /* chipset related info */
> @@ -881,7 +875,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
>  	base_addr = base_addr_cfg & 0x0000ff80;
>  	if (!base_addr) {
>  		dev_notice(&dev->dev, "I/O space for ACPI uninitialized\n");
> -		lpc_ich_cells[LPC_GPIO].num_resources--;
> +		lpc_ich_gpio_cell.num_resources--;
>  		goto gpe0_done;
>  	}
>  
> @@ -895,7 +889,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
>  		 * the platform_device subsystem doesn't see this resource
>  		 * or it will register an invalid region.
>  		 */
> -		lpc_ich_cells[LPC_GPIO].num_resources--;
> +		lpc_ich_gpio_cell.num_resources--;
>  		acpi_conflict = true;
>  	} else {
>  		lpc_ich_enable_acpi_space(dev);
> @@ -933,14 +927,14 @@ gpe0_done:
>  	lpc_chipset_info[priv->chipset].use_gpio = ret;
>  	lpc_ich_enable_gpio_space(dev);
>  
> -	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> +	lpc_ich_finalize_cell(dev, &lpc_ich_gpio_cell);
>  	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> -			      &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
> +			      &lpc_ich_gpio_cell, 1, NULL, 0, NULL);
>  
>  gpio_done:
>  	if (acpi_conflict)
>  		pr_warn("Resource conflict(s) found affecting %s\n",
> -				lpc_ich_cells[LPC_GPIO].name);
> +				lpc_ich_gpio_cell.name);
>  	return ret;
>  }
>  
> @@ -984,7 +978,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
>  	 */
>  	if (lpc_chipset_info[priv->chipset].iTCO_version == 1) {
>  		/* Don't register iomem for TCO ver 1 */
> -		lpc_ich_cells[LPC_WDT].num_resources--;
> +		lpc_ich_wdt_cell.num_resources--;
>  	} else if (lpc_chipset_info[priv->chipset].iTCO_version == 2) {
>  		pci_read_config_dword(dev, RCBABASE, &base_addr_cfg);
>  		base_addr = base_addr_cfg & 0xffffc000;
> @@ -1007,9 +1001,9 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
>  		res->end = base_addr + ACPIBASE_PMC_END;
>  	}
>  
> -	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> +	lpc_ich_finalize_cell(dev, &lpc_ich_wdt_cell);
>  	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> -			      &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
> +			      &lpc_ich_wdt_cell, 1, NULL, 0, NULL);
>  
>  wdt_done:
>  	return ret;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] mfd: lpc_ich: Separate device cells for clarity

[Aaron Sierra]

----- Original Message -----
> From: "Lee Jones" <lee.jones@linaro.org>
> Sent: Saturday, September 19, 2015 5:16:06 AM
> 
> On Thu, 03 Sep 2015, Aaron Sierra wrote:
> 
> > The lpc_ich_cells array gives the wrong impression about the
> > relationship between the watchdog and GPIO devices. They are
> > completely distinct devices, so this patch separates the
> > array into distinct mfd_cell structs per device.
> 
> What does this mean?  In what way are they distinct?

Lee,
This issue was discussed in late July regarding this patch Matt Fleming submitted:

    [PATCH 1/5] iTCO_wdt: Expose watchdog properties using platform data

You asked why there were multiple calls to mfd_add_devices() in the driver.
The reason that I provided was that the WDT cells can be registered even
if the GPIO cells cannot. And vice versa.

Having their cells stored in the same array added to the illusion that a
failure preparing/registering the cells of one should cause all cells to
be unregistered.

-Aaron

> > A side effect of removing the array, is that the lpc_cells enum
> > is no longer needed.
> > 
> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> > ---
> >  drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++------------------------
> >  1 file changed, 18 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> > index 8de3439..7a01d430 100644
> > --- a/drivers/mfd/lpc_ich.c
> > +++ b/drivers/mfd/lpc_ich.c
> > @@ -131,24 +131,18 @@ static struct resource gpio_ich_res[] = {
> >  	},
> >  };
> >  
> > -enum lpc_cells {
> > -	LPC_WDT = 0,
> > -	LPC_GPIO,
> > +static struct mfd_cell lpc_ich_wdt_cell = {
> > +	.name = "iTCO_wdt",
> > +	.num_resources = ARRAY_SIZE(wdt_ich_res),
> > +	.resources = wdt_ich_res,
> > +	.ignore_resource_conflicts = true,
> >  };
> >  
> > -static struct mfd_cell lpc_ich_cells[] = {
> > -	[LPC_WDT] = {
> > -		.name = "iTCO_wdt",
> > -		.num_resources = ARRAY_SIZE(wdt_ich_res),
> > -		.resources = wdt_ich_res,
> > -		.ignore_resource_conflicts = true,
> > -	},
> > -	[LPC_GPIO] = {
> > -		.name = "gpio_ich",
> > -		.num_resources = ARRAY_SIZE(gpio_ich_res),
> > -		.resources = gpio_ich_res,
> > -		.ignore_resource_conflicts = true,
> > -	},
> > +static struct mfd_cell lpc_ich_gpio_cell = {
> > +	.name = "gpio_ich",
> > +	.num_resources = ARRAY_SIZE(gpio_ich_res),
> > +	.resources = gpio_ich_res,
> > +	.ignore_resource_conflicts = true,
> >  };
> >  
> >  /* chipset related info */
> > @@ -881,7 +875,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
> >  	base_addr = base_addr_cfg & 0x0000ff80;
> >  	if (!base_addr) {
> >  		dev_notice(&dev->dev, "I/O space for ACPI uninitialized\n");
> > -		lpc_ich_cells[LPC_GPIO].num_resources--;
> > +		lpc_ich_gpio_cell.num_resources--;
> >  		goto gpe0_done;
> >  	}
> >  
> > @@ -895,7 +889,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
> >  		 * the platform_device subsystem doesn't see this resource
> >  		 * or it will register an invalid region.
> >  		 */
> > -		lpc_ich_cells[LPC_GPIO].num_resources--;
> > +		lpc_ich_gpio_cell.num_resources--;
> >  		acpi_conflict = true;
> >  	} else {
> >  		lpc_ich_enable_acpi_space(dev);
> > @@ -933,14 +927,14 @@ gpe0_done:
> >  	lpc_chipset_info[priv->chipset].use_gpio = ret;
> >  	lpc_ich_enable_gpio_space(dev);
> >  
> > -	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> > +	lpc_ich_finalize_cell(dev, &lpc_ich_gpio_cell);
> >  	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > -			      &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
> > +			      &lpc_ich_gpio_cell, 1, NULL, 0, NULL);
> >  
> >  gpio_done:
> >  	if (acpi_conflict)
> >  		pr_warn("Resource conflict(s) found affecting %s\n",
> > -				lpc_ich_cells[LPC_GPIO].name);
> > +				lpc_ich_gpio_cell.name);
> >  	return ret;
> >  }
> >  
> > @@ -984,7 +978,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> >  	 */
> >  	if (lpc_chipset_info[priv->chipset].iTCO_version == 1) {
> >  		/* Don't register iomem for TCO ver 1 */
> > -		lpc_ich_cells[LPC_WDT].num_resources--;
> > +		lpc_ich_wdt_cell.num_resources--;
> >  	} else if (lpc_chipset_info[priv->chipset].iTCO_version == 2) {
> >  		pci_read_config_dword(dev, RCBABASE, &base_addr_cfg);
> >  		base_addr = base_addr_cfg & 0xffffc000;
> > @@ -1007,9 +1001,9 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
> >  		res->end = base_addr + ACPIBASE_PMC_END;
> >  	}
> >  
> > -	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> > +	lpc_ich_finalize_cell(dev, &lpc_ich_wdt_cell);
> >  	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > -			      &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
> > +			      &lpc_ich_wdt_cell, 1, NULL, 0, NULL);
> >  
> >  wdt_done:
> >  	return ret;

Re: [PATCH] mfd: lpc_ich: Separate device cells for clarity

[Lee Jones]

On Thu, 03 Sep 2015, Aaron Sierra wrote:

> The lpc_ich_cells array gives the wrong impression about the
> relationship between the watchdog and GPIO devices. They are
> completely distinct devices, so this patch separates the
> array into distinct mfd_cell structs per device.
> 
> A side effect of removing the array, is that the lpc_cells enum
> is no longer needed.
> 
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> ---
>  drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++------------------------
>  1 file changed, 18 insertions(+), 24 deletions(-)

Applied, thanks.

> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index 8de3439..7a01d430 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -131,24 +131,18 @@ static struct resource gpio_ich_res[] = {
>  	},
>  };
>  
> -enum lpc_cells {
> -	LPC_WDT = 0,
> -	LPC_GPIO,
> +static struct mfd_cell lpc_ich_wdt_cell = {
> +	.name = "iTCO_wdt",
> +	.num_resources = ARRAY_SIZE(wdt_ich_res),
> +	.resources = wdt_ich_res,
> +	.ignore_resource_conflicts = true,
>  };
>  
> -static struct mfd_cell lpc_ich_cells[] = {
> -	[LPC_WDT] = {
> -		.name = "iTCO_wdt",
> -		.num_resources = ARRAY_SIZE(wdt_ich_res),
> -		.resources = wdt_ich_res,
> -		.ignore_resource_conflicts = true,
> -	},
> -	[LPC_GPIO] = {
> -		.name = "gpio_ich",
> -		.num_resources = ARRAY_SIZE(gpio_ich_res),
> -		.resources = gpio_ich_res,
> -		.ignore_resource_conflicts = true,
> -	},
> +static struct mfd_cell lpc_ich_gpio_cell = {
> +	.name = "gpio_ich",
> +	.num_resources = ARRAY_SIZE(gpio_ich_res),
> +	.resources = gpio_ich_res,
> +	.ignore_resource_conflicts = true,
>  };
>  
>  /* chipset related info */
> @@ -881,7 +875,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
>  	base_addr = base_addr_cfg & 0x0000ff80;
>  	if (!base_addr) {
>  		dev_notice(&dev->dev, "I/O space for ACPI uninitialized\n");
> -		lpc_ich_cells[LPC_GPIO].num_resources--;
> +		lpc_ich_gpio_cell.num_resources--;
>  		goto gpe0_done;
>  	}
>  
> @@ -895,7 +889,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
>  		 * the platform_device subsystem doesn't see this resource
>  		 * or it will register an invalid region.
>  		 */
> -		lpc_ich_cells[LPC_GPIO].num_resources--;
> +		lpc_ich_gpio_cell.num_resources--;
>  		acpi_conflict = true;
>  	} else {
>  		lpc_ich_enable_acpi_space(dev);
> @@ -933,14 +927,14 @@ gpe0_done:
>  	lpc_chipset_info[priv->chipset].use_gpio = ret;
>  	lpc_ich_enable_gpio_space(dev);
>  
> -	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> +	lpc_ich_finalize_cell(dev, &lpc_ich_gpio_cell);
>  	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> -			      &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
> +			      &lpc_ich_gpio_cell, 1, NULL, 0, NULL);
>  
>  gpio_done:
>  	if (acpi_conflict)
>  		pr_warn("Resource conflict(s) found affecting %s\n",
> -				lpc_ich_cells[LPC_GPIO].name);
> +				lpc_ich_gpio_cell.name);
>  	return ret;
>  }
>  
> @@ -984,7 +978,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
>  	 */
>  	if (lpc_chipset_info[priv->chipset].iTCO_version == 1) {
>  		/* Don't register iomem for TCO ver 1 */
> -		lpc_ich_cells[LPC_WDT].num_resources--;
> +		lpc_ich_wdt_cell.num_resources--;
>  	} else if (lpc_chipset_info[priv->chipset].iTCO_version == 2) {
>  		pci_read_config_dword(dev, RCBABASE, &base_addr_cfg);
>  		base_addr = base_addr_cfg & 0xffffc000;
> @@ -1007,9 +1001,9 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
>  		res->end = base_addr + ACPIBASE_PMC_END;
>  	}
>  
> -	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> +	lpc_ich_finalize_cell(dev, &lpc_ich_wdt_cell);
>  	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> -			      &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
> +			      &lpc_ich_wdt_cell, 1, NULL, 0, NULL);
>  
>  wdt_done:
>  	return ret;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] mfd: lpc_ich: Separate device cells for clarity

[Andy Shevchenko]

On Sat, 2015-09-19 at 11:13 +0100, Lee Jones wrote:
> On Thu, 17 Sep 2015, Andy Shevchenko wrote:
> 
> > On Thu, 2015-09-03 at 14:14 -0500, Aaron Sierra wrote:
> > > The lpc_ich_cells array gives the wrong impression about the
> > > relationship between the watchdog and GPIO devices. They are
> > > completely distinct devices, so this patch separates the
> > > array into distinct mfd_cell structs per device.
> > > 
> > > A side effect of removing the array, is that the lpc_cells enum
> > > is no longer needed.
> > > 
> > 
> > Looks good for me.
> 
> Is that an Ack?  If so, can you formally provide one please?

Sorry, might be late. Was too busy by some hw quirks yesterday.

Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> > > Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> > > ---
> > >  drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++-------------------
> > > ----
> > > -
> > >  1 file changed, 18 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> > > index 8de3439..7a01d430 100644
> > > --- a/drivers/mfd/lpc_ich.c
> > > +++ b/drivers/mfd/lpc_ich.c
> > > @@ -131,24 +131,18 @@ static struct resource gpio_ich_res[] = {
> > >  	},
> > >  };
> > >  
> > > -enum lpc_cells {
> > > -	LPC_WDT = 0,
> > > -	LPC_GPIO,
> > > +static struct mfd_cell lpc_ich_wdt_cell = {
> > > +	.name = "iTCO_wdt",
> > > +	.num_resources = ARRAY_SIZE(wdt_ich_res),
> > > +	.resources = wdt_ich_res,
> > > +	.ignore_resource_conflicts = true,
> > >  };
> > >  
> > > -static struct mfd_cell lpc_ich_cells[] = {
> > > -	[LPC_WDT] = {
> > > -		.name = "iTCO_wdt",
> > > -		.num_resources = ARRAY_SIZE(wdt_ich_res),
> > > -		.resources = wdt_ich_res,
> > > -		.ignore_resource_conflicts = true,
> > > -	},
> > > -	[LPC_GPIO] = {
> > > -		.name = "gpio_ich",
> > > -		.num_resources = ARRAY_SIZE(gpio_ich_res),
> > > -		.resources = gpio_ich_res,
> > > -		.ignore_resource_conflicts = true,
> > > -	},
> > > +static struct mfd_cell lpc_ich_gpio_cell = {
> > > +	.name = "gpio_ich",
> > > +	.num_resources = ARRAY_SIZE(gpio_ich_res),
> > > +	.resources = gpio_ich_res,
> > > +	.ignore_resource_conflicts = true,
> > >  };
> > >  
> > >  /* chipset related info */
> > > @@ -881,7 +875,7 @@ static int lpc_ich_init_gpio(struct pci_dev 
> > > *dev)
> > >  	base_addr = base_addr_cfg & 0x0000ff80;
> > >  	if (!base_addr) {
> > >  		dev_notice(&dev->dev, "I/O space for ACPI 
> > > uninitialized\n");
> > > -		lpc_ich_cells[LPC_GPIO].num_resources--;
> > > +		lpc_ich_gpio_cell.num_resources--;
> > >  		goto gpe0_done;
> > >  	}
> > >  
> > > @@ -895,7 +889,7 @@ static int lpc_ich_init_gpio(struct pci_dev 
> > > *dev)
> > >  		 * the platform_device subsystem doesn't see 
> > > this 
> > > resource
> > >  		 * or it will register an invalid region.
> > >  		 */
> > > -		lpc_ich_cells[LPC_GPIO].num_resources--;
> > > +		lpc_ich_gpio_cell.num_resources--;
> > >  		acpi_conflict = true;
> > >  	} else {
> > >  		lpc_ich_enable_acpi_space(dev);
> > > @@ -933,14 +927,14 @@ gpe0_done:
> > >  	lpc_chipset_info[priv->chipset].use_gpio = ret;
> > >  	lpc_ich_enable_gpio_space(dev);
> > >  
> > > -	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> > > +	lpc_ich_finalize_cell(dev, &lpc_ich_gpio_cell);
> > >  	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > > -			      &lpc_ich_cells[LPC_GPIO], 1, NULL, 
> > > 0, 
> > > NULL);
> > > +			      &lpc_ich_gpio_cell, 1, NULL, 0, 
> > > NULL);
> > >  
> > >  gpio_done:
> > >  	if (acpi_conflict)
> > >  		pr_warn("Resource conflict(s) found affecting 
> > > %s\n",
> > > -				lpc_ich_cells[LPC_GPIO].name);
> > > +				lpc_ich_gpio_cell.name);
> > >  	return ret;
> > >  }
> > >  
> > > @@ -984,7 +978,7 @@ static int lpc_ich_init_wdt(struct pci_dev 
> > > *dev)
> > >  	 */
> > >  	if (lpc_chipset_info[priv->chipset].iTCO_version == 1) {
> > >  		/* Don't register iomem for TCO ver 1 */
> > > -		lpc_ich_cells[LPC_WDT].num_resources--;
> > > +		lpc_ich_wdt_cell.num_resources--;
> > >  	} else if (lpc_chipset_info[priv->chipset].iTCO_version 
> > > == 
> > > 2) {
> > >  		pci_read_config_dword(dev, RCBABASE, 
> > > &base_addr_cfg);
> > >  		base_addr = base_addr_cfg & 0xffffc000;
> > > @@ -1007,9 +1001,9 @@ static int lpc_ich_init_wdt(struct pci_dev 
> > > *dev)
> > >  		res->end = base_addr + ACPIBASE_PMC_END;
> > >  	}
> > >  
> > > -	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> > > +	lpc_ich_finalize_cell(dev, &lpc_ich_wdt_cell);
> > >  	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> > > -			      &lpc_ich_cells[LPC_WDT], 1, NULL, 
> > > 0, 
> > > NULL);
> > > +			      &lpc_ich_wdt_cell, 1, NULL, 0, 
> > > NULL);
> > >  
> > >  wdt_done:
> > >  	return ret;
> > 
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] mfd: lpc_ich: Separate device cells for clarity

[Lee Jones]

On Thu, 03 Sep 2015, Aaron Sierra wrote:

> The lpc_ich_cells array gives the wrong impression about the
> relationship between the watchdog and GPIO devices. They are
> completely distinct devices, so this patch separates the
> array into distinct mfd_cell structs per device.
> 
> A side effect of removing the array, is that the lpc_cells enum
> is no longer needed.
> 
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> ---
>  drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++------------------------
>  1 file changed, 18 insertions(+), 24 deletions(-)

You need to rebase this.  It doesn't apply.

> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index 8de3439..7a01d430 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -131,24 +131,18 @@ static struct resource gpio_ich_res[] = {
>  	},
>  };
>  
> -enum lpc_cells {
> -	LPC_WDT = 0,
> -	LPC_GPIO,
> +static struct mfd_cell lpc_ich_wdt_cell = {
> +	.name = "iTCO_wdt",
> +	.num_resources = ARRAY_SIZE(wdt_ich_res),
> +	.resources = wdt_ich_res,
> +	.ignore_resource_conflicts = true,
>  };
>  
> -static struct mfd_cell lpc_ich_cells[] = {
> -	[LPC_WDT] = {
> -		.name = "iTCO_wdt",
> -		.num_resources = ARRAY_SIZE(wdt_ich_res),
> -		.resources = wdt_ich_res,
> -		.ignore_resource_conflicts = true,
> -	},
> -	[LPC_GPIO] = {
> -		.name = "gpio_ich",
> -		.num_resources = ARRAY_SIZE(gpio_ich_res),
> -		.resources = gpio_ich_res,
> -		.ignore_resource_conflicts = true,
> -	},
> +static struct mfd_cell lpc_ich_gpio_cell = {
> +	.name = "gpio_ich",
> +	.num_resources = ARRAY_SIZE(gpio_ich_res),
> +	.resources = gpio_ich_res,
> +	.ignore_resource_conflicts = true,
>  };
>  
>  /* chipset related info */
> @@ -881,7 +875,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
>  	base_addr = base_addr_cfg & 0x0000ff80;
>  	if (!base_addr) {
>  		dev_notice(&dev->dev, "I/O space for ACPI uninitialized\n");
> -		lpc_ich_cells[LPC_GPIO].num_resources--;
> +		lpc_ich_gpio_cell.num_resources--;
>  		goto gpe0_done;
>  	}
>  
> @@ -895,7 +889,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
>  		 * the platform_device subsystem doesn't see this resource
>  		 * or it will register an invalid region.
>  		 */
> -		lpc_ich_cells[LPC_GPIO].num_resources--;
> +		lpc_ich_gpio_cell.num_resources--;
>  		acpi_conflict = true;
>  	} else {
>  		lpc_ich_enable_acpi_space(dev);
> @@ -933,14 +927,14 @@ gpe0_done:
>  	lpc_chipset_info[priv->chipset].use_gpio = ret;
>  	lpc_ich_enable_gpio_space(dev);
>  
> -	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
> +	lpc_ich_finalize_cell(dev, &lpc_ich_gpio_cell);
>  	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> -			      &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
> +			      &lpc_ich_gpio_cell, 1, NULL, 0, NULL);
>  
>  gpio_done:
>  	if (acpi_conflict)
>  		pr_warn("Resource conflict(s) found affecting %s\n",
> -				lpc_ich_cells[LPC_GPIO].name);
> +				lpc_ich_gpio_cell.name);
>  	return ret;
>  }
>  
> @@ -984,7 +978,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
>  	 */
>  	if (lpc_chipset_info[priv->chipset].iTCO_version == 1) {
>  		/* Don't register iomem for TCO ver 1 */
> -		lpc_ich_cells[LPC_WDT].num_resources--;
> +		lpc_ich_wdt_cell.num_resources--;
>  	} else if (lpc_chipset_info[priv->chipset].iTCO_version == 2) {
>  		pci_read_config_dword(dev, RCBABASE, &base_addr_cfg);
>  		base_addr = base_addr_cfg & 0xffffc000;
> @@ -1007,9 +1001,9 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
>  		res->end = base_addr + ACPIBASE_PMC_END;
>  	}
>  
> -	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
> +	lpc_ich_finalize_cell(dev, &lpc_ich_wdt_cell);
>  	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> -			      &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
> +			      &lpc_ich_wdt_cell, 1, NULL, 0, NULL);
>  
>  wdt_done:
>  	return ret;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2] mfd: lpc_ich: Separate device cells for clarity

[Aaron Sierra]

The lpc_ich_cells array gives the wrong impression about the
relationship between the watchdog and GPIO devices. They are
completely distinct devices, so this patch separates the
array into distinct mfd_cell structs per device.

A side effect of removing the array, is that the lpc_cells enum
is no longer needed.

Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 v2 - rebase onto 4.3-rc2

 drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++------------------------
 1 file changed, 18 insertions(+), 24 deletions(-)

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index c5a9a08..b514f3c 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -132,24 +132,18 @@ static struct resource gpio_ich_res[] = {
 	},
 };
 
-enum lpc_cells {
-	LPC_WDT = 0,
-	LPC_GPIO,
+static struct mfd_cell lpc_ich_wdt_cell = {
+	.name = "iTCO_wdt",
+	.num_resources = ARRAY_SIZE(wdt_ich_res),
+	.resources = wdt_ich_res,
+	.ignore_resource_conflicts = true,
 };
 
-static struct mfd_cell lpc_ich_cells[] = {
-	[LPC_WDT] = {
-		.name = "iTCO_wdt",
-		.num_resources = ARRAY_SIZE(wdt_ich_res),
-		.resources = wdt_ich_res,
-		.ignore_resource_conflicts = true,
-	},
-	[LPC_GPIO] = {
-		.name = "gpio_ich",
-		.num_resources = ARRAY_SIZE(gpio_ich_res),
-		.resources = gpio_ich_res,
-		.ignore_resource_conflicts = true,
-	},
+static struct mfd_cell lpc_ich_gpio_cell = {
+	.name = "gpio_ich",
+	.num_resources = ARRAY_SIZE(gpio_ich_res),
+	.resources = gpio_ich_res,
+	.ignore_resource_conflicts = true,
 };
 
 /* chipset related info */
@@ -841,7 +835,7 @@ static int lpc_ich_finalize_wdt_cell(struct pci_dev *dev)
 	struct itco_wdt_platform_data *pdata;
 	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
 	struct lpc_ich_info *info;
-	struct mfd_cell *cell = &lpc_ich_cells[LPC_WDT];
+	struct mfd_cell *cell = &lpc_ich_wdt_cell;
 
 	pdata = devm_kzalloc(&dev->dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
@@ -860,7 +854,7 @@ static int lpc_ich_finalize_wdt_cell(struct pci_dev *dev)
 static void lpc_ich_finalize_gpio_cell(struct pci_dev *dev)
 {
 	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
-	struct mfd_cell *cell = &lpc_ich_cells[LPC_GPIO];
+	struct mfd_cell *cell = &lpc_ich_gpio_cell;
 
 	cell->platform_data = &lpc_chipset_info[priv->chipset];
 	cell->pdata_size = sizeof(struct lpc_ich_info);
@@ -904,7 +898,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
 	base_addr = base_addr_cfg & 0x0000ff80;
 	if (!base_addr) {
 		dev_notice(&dev->dev, "I/O space for ACPI uninitialized\n");
-		lpc_ich_cells[LPC_GPIO].num_resources--;
+		lpc_ich_gpio_cell.num_resources--;
 		goto gpe0_done;
 	}
 
@@ -918,7 +912,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
 		 * the platform_device subsystem doesn't see this resource
 		 * or it will register an invalid region.
 		 */
-		lpc_ich_cells[LPC_GPIO].num_resources--;
+		lpc_ich_gpio_cell.num_resources--;
 		acpi_conflict = true;
 	} else {
 		lpc_ich_enable_acpi_space(dev);
@@ -958,12 +952,12 @@ gpe0_done:
 
 	lpc_ich_finalize_gpio_cell(dev);
 	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
-			      &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
+			      &lpc_ich_gpio_cell, 1, NULL, 0, NULL);
 
 gpio_done:
 	if (acpi_conflict)
 		pr_warn("Resource conflict(s) found affecting %s\n",
-				lpc_ich_cells[LPC_GPIO].name);
+				lpc_ich_gpio_cell.name);
 	return ret;
 }
 
@@ -1007,7 +1001,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
 	 */
 	if (lpc_chipset_info[priv->chipset].iTCO_version == 1) {
 		/* Don't register iomem for TCO ver 1 */
-		lpc_ich_cells[LPC_WDT].num_resources--;
+		lpc_ich_wdt_cell.num_resources--;
 	} else if (lpc_chipset_info[priv->chipset].iTCO_version == 2) {
 		pci_read_config_dword(dev, RCBABASE, &base_addr_cfg);
 		base_addr = base_addr_cfg & 0xffffc000;
@@ -1035,7 +1029,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
 		goto wdt_done;
 
 	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
-			      &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
+			      &lpc_ich_wdt_cell, 1, NULL, 0, NULL);
 
 wdt_done:
 	return ret;
-- 
1.9.1

Re: [PATCH v2] mfd: lpc_ich: Separate device cells for clarity

[Lee Jones]

On Tue, 22 Sep 2015, Aaron Sierra wrote:

> The lpc_ich_cells array gives the wrong impression about the
> relationship between the watchdog and GPIO devices. They are
> completely distinct devices, so this patch separates the
> array into distinct mfd_cell structs per device.
> 
> A side effect of removing the array, is that the lpc_cells enum
> is no longer needed.
> 
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> ---
>  v2 - rebase onto 4.3-rc2
> 
>  drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++------------------------
>  1 file changed, 18 insertions(+), 24 deletions(-)

Looks like you didn't apply Andy's Ack!

Applied with Andy's Ack.

> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index c5a9a08..b514f3c 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -132,24 +132,18 @@ static struct resource gpio_ich_res[] = {
>  	},
>  };
>  
> -enum lpc_cells {
> -	LPC_WDT = 0,
> -	LPC_GPIO,
> +static struct mfd_cell lpc_ich_wdt_cell = {
> +	.name = "iTCO_wdt",
> +	.num_resources = ARRAY_SIZE(wdt_ich_res),
> +	.resources = wdt_ich_res,
> +	.ignore_resource_conflicts = true,
>  };
>  
> -static struct mfd_cell lpc_ich_cells[] = {
> -	[LPC_WDT] = {
> -		.name = "iTCO_wdt",
> -		.num_resources = ARRAY_SIZE(wdt_ich_res),
> -		.resources = wdt_ich_res,
> -		.ignore_resource_conflicts = true,
> -	},
> -	[LPC_GPIO] = {
> -		.name = "gpio_ich",
> -		.num_resources = ARRAY_SIZE(gpio_ich_res),
> -		.resources = gpio_ich_res,
> -		.ignore_resource_conflicts = true,
> -	},
> +static struct mfd_cell lpc_ich_gpio_cell = {
> +	.name = "gpio_ich",
> +	.num_resources = ARRAY_SIZE(gpio_ich_res),
> +	.resources = gpio_ich_res,
> +	.ignore_resource_conflicts = true,
>  };
>  
>  /* chipset related info */
> @@ -841,7 +835,7 @@ static int lpc_ich_finalize_wdt_cell(struct pci_dev *dev)
>  	struct itco_wdt_platform_data *pdata;
>  	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
>  	struct lpc_ich_info *info;
> -	struct mfd_cell *cell = &lpc_ich_cells[LPC_WDT];
> +	struct mfd_cell *cell = &lpc_ich_wdt_cell;
>  
>  	pdata = devm_kzalloc(&dev->dev, sizeof(*pdata), GFP_KERNEL);
>  	if (!pdata)
> @@ -860,7 +854,7 @@ static int lpc_ich_finalize_wdt_cell(struct pci_dev *dev)
>  static void lpc_ich_finalize_gpio_cell(struct pci_dev *dev)
>  {
>  	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> -	struct mfd_cell *cell = &lpc_ich_cells[LPC_GPIO];
> +	struct mfd_cell *cell = &lpc_ich_gpio_cell;
>  
>  	cell->platform_data = &lpc_chipset_info[priv->chipset];
>  	cell->pdata_size = sizeof(struct lpc_ich_info);
> @@ -904,7 +898,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
>  	base_addr = base_addr_cfg & 0x0000ff80;
>  	if (!base_addr) {
>  		dev_notice(&dev->dev, "I/O space for ACPI uninitialized\n");
> -		lpc_ich_cells[LPC_GPIO].num_resources--;
> +		lpc_ich_gpio_cell.num_resources--;
>  		goto gpe0_done;
>  	}
>  
> @@ -918,7 +912,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
>  		 * the platform_device subsystem doesn't see this resource
>  		 * or it will register an invalid region.
>  		 */
> -		lpc_ich_cells[LPC_GPIO].num_resources--;
> +		lpc_ich_gpio_cell.num_resources--;
>  		acpi_conflict = true;
>  	} else {
>  		lpc_ich_enable_acpi_space(dev);
> @@ -958,12 +952,12 @@ gpe0_done:
>  
>  	lpc_ich_finalize_gpio_cell(dev);
>  	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> -			      &lpc_ich_cells[LPC_GPIO], 1, NULL, 0, NULL);
> +			      &lpc_ich_gpio_cell, 1, NULL, 0, NULL);
>  
>  gpio_done:
>  	if (acpi_conflict)
>  		pr_warn("Resource conflict(s) found affecting %s\n",
> -				lpc_ich_cells[LPC_GPIO].name);
> +				lpc_ich_gpio_cell.name);
>  	return ret;
>  }
>  
> @@ -1007,7 +1001,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
>  	 */
>  	if (lpc_chipset_info[priv->chipset].iTCO_version == 1) {
>  		/* Don't register iomem for TCO ver 1 */
> -		lpc_ich_cells[LPC_WDT].num_resources--;
> +		lpc_ich_wdt_cell.num_resources--;
>  	} else if (lpc_chipset_info[priv->chipset].iTCO_version == 2) {
>  		pci_read_config_dword(dev, RCBABASE, &base_addr_cfg);
>  		base_addr = base_addr_cfg & 0xffffc000;
> @@ -1035,7 +1029,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
>  		goto wdt_done;
>  
>  	ret = mfd_add_devices(&dev->dev, PLATFORM_DEVID_AUTO,
> -			      &lpc_ich_cells[LPC_WDT], 1, NULL, 0, NULL);
> +			      &lpc_ich_wdt_cell, 1, NULL, 0, NULL);
>  
>  wdt_done:
>  	return ret;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2] mfd: lpc_ich: Separate device cells for clarity

[Aaron Sierra]

----- Original Message -----
> From: "Lee Jones" <lee.jones@linaro.org>
> Sent: Tuesday, September 22, 2015 7:31:19 PM
> 
> On Tue, 22 Sep 2015, Aaron Sierra wrote:
> 
> > The lpc_ich_cells array gives the wrong impression about the
> > relationship between the watchdog and GPIO devices. They are
> > completely distinct devices, so this patch separates the
> > array into distinct mfd_cell structs per device.
> > 
> > A side effect of removing the array, is that the lpc_cells enum
> > is no longer needed.
> > 
> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> > ---
> >  v2 - rebase onto 4.3-rc2
> > 
> >  drivers/mfd/lpc_ich.c | 42 ++++++++++++++++++------------------------
> >  1 file changed, 18 insertions(+), 24 deletions(-)
> 
> Looks like you didn't apply Andy's Ack!

Sorry, Andy! I erred on the side of caution. I wasn't sure if
the Ack applied to the concept or the iteration of the patch.

> Applied with Andy's Ack.

Thanks

-Aaron