Re: [PATCH v2 4/5] i2c-piix4: Add support for multiplexed main adapter in SB800

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Sun, 2015-11-01 at 17:32 +0100, Christian Fetzer wrote:
> The SB800 chipset supports a multiplexed main SMBus controller with
> four ports. The multiplexed ports share the same SMBus address and
> register set. The port is selected by bits 2:1 of the smb_en register
> (0x2C).
> 
> Only one port can be active at any point in time therefore a mutex is
> needed in order to synchronize access.
> 
> Tested on HP ProLiant MicroServer G7 N54L (where this patch adds
> support to access sensor data from the w83795adg).


> 
> +static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned
> short smba)
> +{
> +	unsigned short port;
> +	int retval;
> +	struct i2c_piix4_adapdata *adapdata;
> +
> +	for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
> +		retval = piix4_add_adapter(dev, smba,
> +					   &piix4_main_adapters[port
> ]);
> +		if (retval < 0)
> +			goto error;
> +
> +		piix4_main_adapters[port]->algo =
> &piix4_smbus_algorithm_sb800;
> +
> +		adapdata =
> i2c_get_adapdata(piix4_main_adapters[port]);
> +		adapdata->port = port;
> +	}
> +
> +	return retval;
> +
> +error:
> +	dev_err(&dev->dev, "Error setting up SB800 adapters. "
> +		"Unregistering all adapters!\n");

This one Mika told already about.
You might use as well

dev_err(&dev->dev,
 "…\n")


> +	for (port--; port >= 0; port--) {

Isn't look complicated? Like I comment in one of previous patches:

while (--port) {

> +		adapdata =
> i2c_get_adapdata(piix4_main_adapters[port]);
> +		if (adapdata->smba) {
> +			i2c_del_adapter(piix4_main_adapters[port]);
> +			kfree(adapdata);
> +			kfree(piix4_main_adapters[port]);
> +			piix4_main_adapters[port] = NULL;
> +		}
> +	}
> +
> +	return retval;
> +}
> +
>  static int piix4_probe(struct pci_dev *dev, const struct
> pci_device_id *id)
>  {
>  	unsigned short smba_idx = 0xcd6;
> @@ -629,19 +714,26 @@ static int piix4_probe(struct pci_dev *dev,
> const struct pci_device_id *id)
>  
>  		/* base address location etc changed in SB800 */
>  		retval = piix4_setup_sb800(dev, id, 0);
> +		if (retval < 0)
> +			return retval;
> +
> +		/* Try to register multiplexed main SMBus adapter,
> +		 * give up if we can't */

Block comment
/*
 * text1
 * text2
 */

> +		retval = piix4_add_adapters_sb800(dev, retval);
>  	} else {
>  		retval = piix4_setup(dev, id);
> +		if (retval < 0)
> +			return retval;
> +
> +		/* Try to register main SMBus adapter, give up if we
> can't */
> +		retval = piix4_add_adapter(dev, retval,
> +					   &piix4_main_adapters[0]);
>  	}
>  
>  	/* If no main SMBus found, give up */
>  	if (retval < 0)
>  		return retval;
>  
> -	/* Try to register main SMBus adapter, give up if we can't
> */
> -	retval = piix4_add_adapter(dev, retval,
> &piix4_main_adapters[0]);
> -	if (retval < 0)
> -		return retval;
> -
>  	/* Check for auxiliary SMBus on some AMD chipsets */
>  	retval = -ENODEV;
>  

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