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 <[email protected]> > > 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 [email protected] Intel Corporation -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
