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