Re: [PATCH] lpc_sch: Accomodate partial population of the MFD devices

[Darren Hart <dvhart@linux.intel.com>]

On 02/08/2013 03:20 PM, Darren Hart wrote:
> The current probe aborts if any of the 3 base address registers are
> disabled. On a TunnelCreek system I am working on, this resulted in the
> SMBIOS and GPIO devices being removed when it couldn't read the base
> address for the watchdog timer.
> 
> This patch accommodates partial population of the lpc_sch_cells array and
> only aborts if all the base address registers are disabled. A max size
> array is allocated and the individual device cells are added to it after
> their base addresses are successfully determined. This simplifies the
> code a bit by removing the need for the separate tunnelcreek cells array
> and combining some of the add/remove logic.

I later found that someone else attempted to address this problem in a
slightly different way, but it was never merged. One difference was that
it allowed for the device to be disabled, but not for it to be enabled
with a 0x0 base address. In that case their change would still abort,
remove any added devices, and return ENODEV.

I can see the value in either approach, and don't feel strongly about
either. So long as it doesn't abort everything when something is
disabled, I'm happy. Would anyone care to argue for one approach or the
other.

Previous attempt:
http://article.gmane.org/gmane.linux.kernel/1295891/match=lpc_sch+tolerate

Thanks,

Darren

> 
> Signed-off-by: Darren Hart <dvhart@linux.intel.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>,
> Cc: Denis Turischev <denis@compulab.co.il>,
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Signed-off-by: Darren Hart <dvhart@linux.intel.com>
> ---
>  drivers/mfd/lpc_sch.c | 148 ++++++++++++++++++++++++--------------------------
>  1 file changed, 70 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> index 5624fcb..9c255f2 100644
> --- a/drivers/mfd/lpc_sch.c
> +++ b/drivers/mfd/lpc_sch.c
> @@ -45,34 +45,32 @@ static struct resource smbus_sch_resource = {
>  		.flags = IORESOURCE_IO,
>  };
>  
> -
>  static struct resource gpio_sch_resource = {
>  		.flags = IORESOURCE_IO,
>  };
>  
> -static struct mfd_cell lpc_sch_cells[] = {
> -	{
> -		.name = "isch_smbus",
> -		.num_resources = 1,
> -		.resources = &smbus_sch_resource,
> -	},
> -	{
> -		.name = "sch_gpio",
> -		.num_resources = 1,
> -		.resources = &gpio_sch_resource,
> -	},
> -};
> -
>  static struct resource wdt_sch_resource = {
>  		.flags = IORESOURCE_IO,
>  };
>  
> -static struct mfd_cell tunnelcreek_cells[] = {
> -	{
> -		.name = "ie6xx_wdt",
> -		.num_resources = 1,
> -		.resources = &wdt_sch_resource,
> -	},
> +static struct mfd_cell lpc_sch_cells[3];
> +
> +static struct mfd_cell isch_smbus_cell = {
> +	.name = "isch_smbus",
> +	.num_resources = 1,
> +	.resources = &smbus_sch_resource,
> +};
> +
> +static struct mfd_cell sch_gpio_cell = {
> +	.name = "sch_gpio",
> +	.num_resources = 1,
> +	.resources = &gpio_sch_resource,
> +};
> +
> +static struct mfd_cell wdt_sch_cell = {
> +	.name = "ie6xx_wdt",
> +	.num_resources = 1,
> +	.resources = &wdt_sch_resource,
>  };
>  
>  static DEFINE_PCI_DEVICE_TABLE(lpc_sch_ids) = {
> @@ -88,79 +86,73 @@ static int lpc_sch_probe(struct pci_dev *dev,
>  {
>  	unsigned int base_addr_cfg;
>  	unsigned short base_addr;
> -	int i;
> +	int i, cells = 0;
>  	int ret;
>  
>  	pci_read_config_dword(dev, SMBASE, &base_addr_cfg);
> -	if (!(base_addr_cfg & (1 << 31))) {
> -		dev_err(&dev->dev, "Decode of the SMBus I/O range disabled\n");
> -		return -ENODEV;
> -	}
> -	base_addr = (unsigned short)base_addr_cfg;
> -	if (base_addr == 0) {
> -		dev_err(&dev->dev, "I/O space for SMBus uninitialized\n");
> -		return -ENODEV;
> +	base_addr = 0;
> +	if (!(base_addr_cfg & (1 << 31)))
> +		dev_warn(&dev->dev, "Decode of the SMBus I/O range disabled\n");
> +	else
> +		base_addr = (unsigned short)base_addr_cfg;
> +	if (base_addr == 0)
> +		dev_warn(&dev->dev, "I/O space for SMBus uninitialized\n");
> +	else {
> +		lpc_sch_cells[cells++] = isch_smbus_cell;
> +		smbus_sch_resource.start = base_addr;
> +		smbus_sch_resource.end = base_addr + SMBUS_IO_SIZE - 1;
>  	}
>  
> -	smbus_sch_resource.start = base_addr;
> -	smbus_sch_resource.end = base_addr + SMBUS_IO_SIZE - 1;
> -
>  	pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg);
> -	if (!(base_addr_cfg & (1 << 31))) {
> -		dev_err(&dev->dev, "Decode of the GPIO I/O range disabled\n");
> -		return -ENODEV;
> -	}
> -	base_addr = (unsigned short)base_addr_cfg;
> -	if (base_addr == 0) {
> -		dev_err(&dev->dev, "I/O space for GPIO uninitialized\n");
> -		return -ENODEV;
> -	}
> -
> -	gpio_sch_resource.start = base_addr;
> -
> -	if (id->device == PCI_DEVICE_ID_INTEL_CENTERTON_ILB)
> -		gpio_sch_resource.end = base_addr + GPIO_IO_SIZE_CENTERTON - 1;
> +	base_addr = 0;
> +	if (!(base_addr_cfg & (1 << 31)))
> +		dev_warn(&dev->dev, "Decode of the GPIO I/O range disabled\n");
>  	else
> -		gpio_sch_resource.end = base_addr + GPIO_IO_SIZE - 1;
> -
> -	for (i=0; i < ARRAY_SIZE(lpc_sch_cells); i++)
> -		lpc_sch_cells[i].id = id->device;
> -
> -	ret = mfd_add_devices(&dev->dev, 0,
> -			      lpc_sch_cells, ARRAY_SIZE(lpc_sch_cells), NULL,
> -			      0, NULL);
> -	if (ret)
> -		goto out_dev;
> +		base_addr = (unsigned short)base_addr_cfg;
> +	if (base_addr == 0)
> +		dev_warn(&dev->dev, "I/O space for GPIO uninitialized\n");
> +	else {
> +		lpc_sch_cells[cells++] = sch_gpio_cell;
> +		gpio_sch_resource.start = base_addr;
> +		if (id->device == PCI_DEVICE_ID_INTEL_CENTERTON_ILB)
> +			gpio_sch_resource.end = base_addr + GPIO_IO_SIZE_CENTERTON - 1;
> +		else
> +			gpio_sch_resource.end = base_addr + GPIO_IO_SIZE - 1;
> +	}
>  
>  	if (id->device == PCI_DEVICE_ID_INTEL_ITC_LPC
>  	 || id->device == PCI_DEVICE_ID_INTEL_CENTERTON_ILB) {
>  		pci_read_config_dword(dev, WDTBASE, &base_addr_cfg);
> -		if (!(base_addr_cfg & (1 << 31))) {
> -			dev_err(&dev->dev, "Decode of the WDT I/O range disabled\n");
> -			ret = -ENODEV;
> -			goto out_dev;
> -		}
> -		base_addr = (unsigned short)base_addr_cfg;
> -		if (base_addr == 0) {
> -			dev_err(&dev->dev, "I/O space for WDT uninitialized\n");
> -			ret = -ENODEV;
> -			goto out_dev;
> +		base_addr = 0;
> +		if (!(base_addr_cfg & (1 << 31)))
> +			dev_warn(&dev->dev, "Decode of the WDT I/O range disabled\n");
> +		else
> +			base_addr = (unsigned short)base_addr_cfg;
> +		if (base_addr == 0)
> +			dev_warn(&dev->dev, "I/O space for WDT uninitialized\n");
> +		else {
> +			lpc_sch_cells[cells++] = wdt_sch_cell;
> +			wdt_sch_resource.start = base_addr;
> +			wdt_sch_resource.end = base_addr + WDT_IO_SIZE - 1;
>  		}
> +	}
>  
> -		wdt_sch_resource.start = base_addr;
> -		wdt_sch_resource.end = base_addr + WDT_IO_SIZE - 1;
> -
> -		for (i = 0; i < ARRAY_SIZE(tunnelcreek_cells); i++)
> -			tunnelcreek_cells[i].id = id->device;
> -
> -		ret = mfd_add_devices(&dev->dev, 0, tunnelcreek_cells,
> -				      ARRAY_SIZE(tunnelcreek_cells), NULL,
> -				      0, NULL);
> +	if (WARN_ON(cells > ARRAY_SIZE(lpc_sch_cells))) {
> +		dev_err(&dev->dev, "Cell count exceeds array size");
> +		return -ENODEV;
> +	}
> +	if (cells == 0) {
> +		dev_err(&dev->dev, "All decode registers disabled.\n");
> +		return -ENODEV;
>  	}
>  
> -	return ret;
> -out_dev:
> -	mfd_remove_devices(&dev->dev);
> +	for (i = 0; i < cells; i++)
> +		lpc_sch_cells[i].id = id->device;
> +	ret = mfd_add_devices(&dev->dev, 0, lpc_sch_cells, cells, NULL, 0, NULL);
> +	
> +	if (ret)
> +		mfd_remove_devices(&dev->dev);
> +
>  	return ret;
>  }
>  
> 

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel