Hi Ben, On Tue, 19 Aug 2008 16:02:29 +0100, Ben Hutchings wrote: > The lm87 driver normally assumes that firmware configured the chip > correctly. Since this is not always the case, alllow platform code to > set the channel register value via platform_data. All other > configuration registers can be changed after driver initialisation. Sounds almost sane to me, except... > > Signed-off-by: Ben Hutchings <bhutchings at solarflare.com> > --- > drivers/hwmon/lm87.c | 33 ++++++++++++++++++++++++--------- > 1 files changed, 24 insertions(+), 9 deletions(-) > > diff --git a/drivers/hwmon/lm87.c b/drivers/hwmon/lm87.c > index ddad6fc..1f5cac5 100644 > --- a/drivers/hwmon/lm87.c > +++ b/drivers/hwmon/lm87.c > @@ -21,11 +21,12 @@ > * http://www.national.com/pf/LM/LM87.html > * > * Some functions share pins, so not all functions are available at the same > - * time. Which are depends on the hardware setup. This driver assumes that > - * the BIOS configured the chip correctly. In that respect, it differs from > - * the original driver (from lm_sensors for Linux 2.4), which would force the > - * LM87 to an arbitrary, compile-time chosen mode, regardless of the actual > - * chipset wiring. > + * time. Which are depends on the hardware setup. This driver normally > + * assumes that firmware configured the chip correctly. Where this is not > + * the case, platform code must set the I2C client's platform_data to point > + * to a u8 value to be written to the channel register. In that case the > + * driver will assume that it is the only user of the chip and will reset > + * it in the probe and remove functions. I fail to see how setting the channel mode is related to resetting the chip. Not that I really get the point of resetting the chip in the first place, but if you want to do that, it should be independent of setting the channel mode. > * For reference, here is the list of exclusive functions: > * - in0+in5 (default) or temp3 > * - fan1 (default) or in6 > @@ -731,7 +732,7 @@ static int lm87_probe(struct i2c_client *new_client, > > /* Register sysfs hooks */ > if ((err = sysfs_create_group(&new_client->dev.kobj, &lm87_group))) > - goto exit_free; > + goto exit_reset; > > if (data->channel & CHAN_NO_FAN(0)) { > if ((err = device_create_file(&new_client->dev, > @@ -831,7 +832,10 @@ static int lm87_probe(struct i2c_client *new_client, > exit_remove: > sysfs_remove_group(&new_client->dev.kobj, &lm87_group); > sysfs_remove_group(&new_client->dev.kobj, &lm87_group_opt); > -exit_free: > +exit_reset: > + if (new_client->dev.platform_data) > + lm87_write_value(new_client, LM87_REG_CONFIG, 0x80); > + > kfree(data); > exit: > return err; > @@ -842,9 +846,17 @@ static void lm87_init_client(struct i2c_client *client) > struct lm87_data *data = i2c_get_clientdata(client); > u8 config; > > - data->channel = lm87_read_value(client, LM87_REG_CHANNEL_MODE); > + if (client->dev.platform_data) { > + config = 0x80; > + lm87_write_value(client, LM87_REG_CONFIG, config); > + data->channel = *(u8 *)client->dev.platform_data; I'm not fond of that kind of cast. Which leads me to the following porposal: define a proper structure controlling the LM87 device initialization. struct lm87_platform_data { u8 channel_mode; unsigned int reset:1; } So that the caller has control over the channel mode and the resetting of the chip, in a clear and independent way. This is much better than (ab)using the platform_data pointer for everything. > + lm87_write_value(client, > + LM87_REG_CHANNEL_MODE, data->channel); > + } else { > + data->channel = lm87_read_value(client, LM87_REG_CHANNEL_MODE); > + config = lm87_read_value(client, LM87_REG_CONFIG); > + } > > - config = lm87_read_value(client, LM87_REG_CONFIG); > if (!(config & 0x01)) { > int i; > > @@ -882,6 +894,9 @@ static int lm87_remove(struct i2c_client *client) > sysfs_remove_group(&client->dev.kobj, &lm87_group); > sysfs_remove_group(&client->dev.kobj, &lm87_group_opt); > > + if (client->dev.platform_data) > + lm87_write_value(client, LM87_REG_CONFIG, 0x80); > + > kfree(data); > return 0; > } -- Jean Delvare
