Hi David, On Sat, 30 Oct 2010 19:34:23 -0400, David Woodhouse wrote: > 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. True enough. I've even been thinking of sending a similar patch long ago. The problem is that it's hard to justify the additional cost of dynamic memory allocation if there's no technical need for it. And until Sandy Bridge, there wasn't. > > > (...) > > > @@ -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, Normal according to who? I don't see anything wrong with them as they were before, and it's used in other places of the driver, so it's not an accident. > while I was modifying the lines before and after. > That's entirely normal practice, and not particularly gratuitous, > surely? When you send a patch to the wrong person at the wrong time, if you want it accepted still, you should make it change as little things as possible. > > > @@ -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. You got me wrong apparently. I have no objection to the single structure allocation, this is indeed fairly common and a good thing to avoid memory fragmentation. I have a strong objection on random casts from one structure to another on the undocumented assumption that one contains the other as its first member. All you have to do to make me happy is: i2c_set_adapdata(adap, priv); in the probe function and priv = i2c_get_adapdata(adap); in this function (and i801_func). -- Jean Delvare -- 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
