Hi Ben,
> [PATCH] I2C: simplify max6875 driver
>
> This is an update to the max6875 driver.
> It no longer does any detection, so the address must be forced on
> module load. It only makes available the user EEPROM (read-only).
Sorry for the delay. I see that Greg picked your patch, so it's about
time that I take a look and comment on it.
> +static void max6875_update_slice(struct i2c_client *client, int slice)
> {
> struct max6875_data *data = i2c_get_clientdata(client);
> - int i, j, addr, count;
> - u8 rdbuf[SLICE_SIZE];
> + int i, j, addr;
> + u8 *buf;
> int retval = 0;
retval is set in various place in this function, but not returned?
> +static inline struct i2c_client *kobj_to_i2c_client(struct kobject *kobj)
I don't much like seeing a function with such a generic name in a
specific driver. This could easily collide with something similar
declared in a public header (i2c.h comes to mind). Shouldn't this inline
function be moved to i2c.h right now?
> if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA
> | I2C_FUNC_SMBUS_READ_BYTE))
You don't seem to be calling i2c_smbus_read_byte_data() anywhere, so you
could check for only I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
I2C_FUNC_SMBUS_READ_BYTE. Not that it should change anything practically
though.
> + strlcpy(fake_client->name, "max6875-dummy", I2C_NAME_SIZE);
The typical name for "fake clients" is "<client name> subclient".
On the documentation front:
> modprobe max6875 force=0,0x50
This made me realize that forcing eeprom drivers (eeprom and max6875) is
nor properly handled WRT 24RF08 corruption prevention. I will submit a
separate patch fixing this for both drivers.
> Reads and write are performed differently depending on the address range.
I'd guess write_s_?
As Greg accepted your last updates already, any fix you would submit now
would be as an incremental patch.
Thanks,
--
Jean Delvare
