Re: [PATCH 1/2] i2c-i801: Handle multiple instances instead of keeping global state

[David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>]

On Sat, 2010-10-30 at 18:24 +0200, Jean Delvare wrote:
> An explanation why this change is needed would be nice.

Um, does it really need explaining? It's really poor form to keep driver
state in global variables rather than per-instance, even if you *don't*
actually have more than one device.

> Note that the patch got corrupted on the way: all leading spaces have
> been doubled. I had to fix it.

Sorry about that. Was sending from my phone, as I said before.

> > +    Copyright © 2010         Intel Corporation
> > +                             David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
> 
> I'd rather stick to (C). Using non-ASCII characters is asking for
> trouble, and this doesn't add value.

It really isn't trouble. We're well into the 21st century now — even
akpm can cope with UTF-8 :)
 
> > @@ -429,22 +439,23 @@ static int i801_block_transaction(union i2c_smbus_data *data, char read_write,
> >   	/* Experience has shown that the block buffer can only be used for
> >   	   SMBus (not I2C) block transactions, even though the datasheet
> >   	   doesn't mention this limitation. */
> > -	if ((i801_features & FEATURE_BLOCK_BUFFER)
> > -	 && command != I2C_SMBUS_I2C_BLOCK_DATA
> > -	 && i801_set_block_buffer_mode() == 0)
> > -		result = i801_block_transaction_by_block(data, read_write,
> > -							 hwpec);
> > +	if ((priv->features & FEATURE_BLOCK_BUFFER)
> > +	    && command != I2C_SMBUS_I2C_BLOCK_DATA
> > +	    && i801_set_block_buffer_mode(priv) == 0)
> 
> Gratuitous reindentation of code.

The two continuation lines? I just fixed them to conform to the normal
kernel indentation, while I was modifying the lines before and after.
That's entirely normal practice, and not particularly gratuitous,
surely?

> > @@ -457,81 +468,83 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
> >   	int hwpec;
> >   	int block = 0;
> >   	int ret, xact = 0;
> > +	struct i801_priv *priv = (void *)adap;
> 
> This is horrible and only works by accident (or misdesign if you
> prefer). Please don't do this. I'm glad I insisted to review this
> patch...
> 
> We have i2c_set/get_adapdata() for this. If you really care about the
> extra cost, please use the proper container_of() construct. But I don't
> think the cost is problematic.

It wasn't the cost I was thinking of; it was the simplicity. One
allocation, one failure case, one error path. That's why this method is
fairly common within the kernel.

However, I have no particular objection to doing separate allocations,
even though it's not what I'd normally do. I'll send another version.

> >   	switch (xact & 0x7f) {
> >   	case I801_BYTE:	/* Result put in SMBHSTDAT0 */
> >   	case I801_BYTE_DATA:
> > -		data->byte = inb_p(SMBHSTDAT0);
> > +		data->byte = inb_p(SMBHSTDAT0(priv));
> >   		break;
> >   	case I801_WORD_DATA:
> > -		data->word = inb_p(SMBHSTDAT0) + (inb_p(SMBHSTDAT1) << 8);
> > +		data->word = inb_p(SMBHSTDAT0(priv)) +
> > +			(inb_p(SMBHSTDAT1(priv)) << 8);
> 
> Please align.

Yeah, I remember wondering why emacs put it there, but figured I'd trust
it :)

> > +	priv->features = 0;
> 
> You just kzalloc'd the structure, so features are already 0.

True.

> > @@ -801,27 +821,32 @@ static int __devinit i801_probe(struct pci_dev *dev,
> >   		memset(&info, 0, sizeof(struct i2c_board_info));
> >   		info.addr = apanel_addr;
> >   		strlcpy(info.type, "fujitsu_apanel", I2C_NAME_SIZE);
> > -		i2c_new_device(&i801_adapter, &info);
> > +		i2c_new_device(&priv->adapter, &info);
> >   	}
> >   #endif
> >   #if defined CONFIG_SENSORS_FSCHMD || defined CONFIG_SENSORS_FSCHMD_MODULE
> >   	if (dmi_name_in_vendors("FUJITSU"))
> > -		dmi_walk(dmi_check_onboard_devices, &i801_adapter);
> > +		dmi_walk(dmi_check_onboard_devices, &priv->adapter);
> >   #endif
> > -
> 
> Please leave this blank line in place.

Um, OK.

> Patch tested OK on ICH10.

Cool, thanks. I'll send an updated patch; hopefully later this evening.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org                              Intel Corporation