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

[Aaron Sierra <asierra@xes-inc.com>]

----- 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;