Re: [PATCH] hwmon: Add support for max6695 and max6696 to lm90driver
From: Guenter Roeck
Date: Mon Sep 06 2010 - 22:02:27 EST
Hi Jean,
On Mon, Sep 06, 2010 at 12:12:29PM -0400, Jean Delvare wrote:
> Hi Guenter,
>
> On Sat, 4 Sep 2010 15:34:35 -0700, Guenter Roeck wrote:
> > Signed-off-by: Guenter Roeck <guenter.roeck@xxxxxxxxxxxx>
> > ---
> > To apply this patch, the previously submitted lm90 cleanup patch has to be
> > applied first.
> >
> > My main concern with this patch is the chip detection code, specifically if it
> > is able to safely distinguish between MAX6680/81 and MAX6695/96.
> > Would be great to get some test coverage from a system with one of those chips.
>
> Unfortunately I don't have any of these Maxim chips at hand. I have an
> ADM1032 but it won't offer much coverage obviously. And I have dumps of
> Maxim chips, but the real chips behave differently, so it's of little
> help.
>
> Can you please add detection support to sensors-detect as well (and
> then update wiki/Devices)?
>
Sure. I planned to do that after the review is complete, but it makes sense
to add it to sensors-detect now.
> Review below:
>
> >
> > Sample sensors output:
> >
> > max6695-i2c-0-19
> > Adapter: SMBus I801 adapter at 5080
> > temp1: +24.5 C (low = -55.0 C, high = +70.0 C)
> > (crit = +70.0 C, hyst = +60.0 C)
> > temp2: +26.5 C (low = -55.0 C, high = +70.0 C)
> > (crit = +90.0 C, hyst = +80.0 C)
> > temp3: +24.1 C (low = -54.1 C, high = +70.2 C)
> > (crit = +90.0 C, hyst = +80.0 C)
> >
> > drivers/hwmon/lm90.c | 280 +++++++++++++++++++++++++++++++++++++++++++++-----
> > 1 files changed, 252 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > index aafed28..52ed792 100644
> > --- a/drivers/hwmon/lm90.c
> > +++ b/drivers/hwmon/lm90.c
> > @@ -42,6 +42,11 @@
> > * chips. The MAX6680 and MAX6681 only differ in the pinout so they can
> > * be treated identically.
> > *
> > + * This driver also supports the MAX6695 and MAX6696, two other sensor
> > + * chips made by Maxim. These are also quite similar to other Maxim
> > + * chips, but support three temperature sensors instead of two. MAX6695
> > + * and MAX6696 only differ in the pinout so they can be treated identically.
> > + *
>
> Please also update Documentation/hwmon/lm90 and drivers/hwmon/Kconfig.
>
Ok.
> You could also mention the additional emergency temperature limits, as
> this is a feature unique to these chips.
>
Ok.
> > * This driver also supports the ADT7461 chip from Analog Devices.
> > * It's supported in both compatibility and extended mode. It is mostly
> > * compatible with LM90 except for a data format difference for the
> > @@ -94,7 +99,7 @@ static const unsigned short normal_i2c[] = {
> > 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x4c, 0x4d, 0x4e, I2C_CLIENT_END };
> >
> > enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
> > - w83l771 };
> > + max6695, w83l771 };
> >
> > /*
> > * The LM90 registers
> > @@ -109,6 +114,7 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
> > #define LM90_REG_R_CONVRATE 0x04
> > #define LM90_REG_W_CONVRATE 0x0A
> > #define LM90_REG_R_STATUS 0x02
> > +#define LM90_REG_R_STATUS2 0x12
> > #define LM90_REG_R_LOCAL_TEMP 0x00
> > #define LM90_REG_R_LOCAL_HIGH 0x05
> > #define LM90_REG_W_LOCAL_HIGH 0x0B
> > @@ -130,12 +136,16 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
> > #define LM90_REG_W_REMOTE_LOWH 0x0E
> > #define LM90_REG_R_REMOTE_LOWL 0x14
> > #define LM90_REG_W_REMOTE_LOWL 0x14
> > +#define LM90_REG_R_REMOTE_EMERG 0x16
> > +#define LM90_REG_W_REMOTE_EMERG 0x16
> > +#define LM90_REG_R_LOCAL_EMERG 0x17
> > +#define LM90_REG_W_LOCAL_EMERG 0x17
> > #define LM90_REG_R_REMOTE_CRIT 0x19
> > #define LM90_REG_W_REMOTE_CRIT 0x19
> > #define LM90_REG_R_TCRIT_HYST 0x21
> > #define LM90_REG_W_TCRIT_HYST 0x21
> >
> > -/* MAX6646/6647/6649/6657/6658/6659 registers */
> > +/* MAX6646/6647/6649/6657/6658/6659/6695/6696 registers */
> >
> > #define MAX6657_REG_R_LOCAL_TEMPL 0x11
> >
> > @@ -148,6 +158,7 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
> > * Functions declaration
> > */
> >
> > +static int lm90_read_reg(struct i2c_client *client, u8 reg, u8 *value);
> > static int lm90_detect(struct i2c_client *client, struct i2c_board_info *info);
> > static int lm90_probe(struct i2c_client *client,
> > const struct i2c_device_id *id);
> > @@ -157,6 +168,23 @@ static int lm90_remove(struct i2c_client *client);
> > static struct lm90_data *lm90_update_device(struct device *dev);
> >
> > /*
> > + * Some useful macros
> > + */
> > +#define lm90_have_offset(data) \
> > + (data->kind != max6657 && data->kind != max6646 \
> > + && data->kind != max6695)
> > +
> > +#define lm90_have_local_temp_ext(data) \
> > + (data->kind == max6657 || data->kind == max6646 \
> > + || data->kind == max6695)
> > +
> > +#define lm90_have_remote_limit_ext(data) \
> > + (data->kind != max6657 && data->kind != max6646 \
> > + && data->kind != max6680 && data->kind != max6695)
> > +
> > +#define lm90_have_emergency(data) (data->kind == max6695)
>
> Makes the code more readable, I agree, but OTOH it hides complexity.
> Such tests are OK during probe or remove, as they happen only once, but
> seeing them in runtime code and in particular in the update function,
> seems wrong (even though I can't disagree that the overhead is quite
> small compared to the cost of SMBus transactions.)
>
Ultimately, hiding complexity was the reason to introduce the macros.
I figured it was better than having the comparisons spread through the code.
> I am wondering if it wouldn't be better to use data->flag to carry such
> feature information, which would be computed at probe time, once and
> for all. What do you think?
>
> Also, these macros could have been introduced in a separate patch, to
> make this one smaller, as they are good to have even without the
> max6695/96 support.
>
Makes sense. I'll do that (using flags) and resubmit as two separate patches.
> > +
> > +/*
> > * Driver data (common to all clients)
> > */
> >
> > @@ -175,6 +203,8 @@ static const struct i2c_device_id lm90_id[] = {
> > { "max6659", max6657 },
> > { "max6680", max6680 },
> > { "max6681", max6680 },
> > + { "max6695", max6695 },
> > + { "max6696", max6695 },
> > { "w83l771", w83l771 },
> > { }
> > };
> > @@ -206,20 +236,29 @@ struct lm90_data {
> > int flags;
> >
> > u8 config_orig; /* Original configuration register value */
> > - u8 alert_alarms; /* Which alarm bits trigger ALERT# */
> > + u16 alert_alarms; /* Which alarm bits trigger ALERT# */
> > + /* Upper 8 bits from max6695 STATUS2 register */
>
> The comment isn't quite correct. The contents of the STATUS2 register
> go to struct member alarms below, not alert_alarms. alert_alarms is set
> by the driver at initialization time.
>
ok
> >
> > /* registers values */
> > - s8 temp8[4]; /* 0: local low limit
> > + s8 temp8[8]; /* 0: local low limit
> > 1: local high limit
> > 2: local critical limit
> > - 3: remote critical limit */
> > - s16 temp11[5]; /* 0: remote input
> > + 3: remote critical limit
> > + 4: local emergency limit (max6695/96 only)
> > + 5: remote emergency limit (max6695/96 only)
> > + 6: remote 2 critical limit (max6695/96 only)
> > + 7: remote 2 emergency limit (max6695/96 only) */
> > + s16 temp11[8]; /* 0: remote input
> > 1: remote low limit
> > 2: remote high limit
> > - 3: remote offset (except max6646 and max6657)
> > - 4: local input */
> > + 3: remote offset (except max6646, max6657/59,
>
> And 58 too, no?
>
Correct.
> > + and max6695/96)
> > + 4: local input
> > + 5: remote 2 input (max6695/96 only)
> > + 6: remote 2 low limit (max6695/96 only)
> > + 7: remote 2 high limit (ma6695/96 only) */
> > u8 temp_hyst;
> > - u8 alarms; /* bitvector */
> > + u16 alarms; /* bitvector (upper 8 bits for max6695/96) */
> > };
> >
> > /*
> > @@ -377,11 +416,15 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
> > static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> > const char *buf, size_t count)
> > {
> > - static const u8 reg[4] = {
> > + static const u8 reg[8] = {
> > LM90_REG_W_LOCAL_LOW,
> > LM90_REG_W_LOCAL_HIGH,
> > LM90_REG_W_LOCAL_CRIT,
> > LM90_REG_W_REMOTE_CRIT,
> > + LM90_REG_W_LOCAL_EMERG,
> > + LM90_REG_W_REMOTE_EMERG,
> > + LM90_REG_W_REMOTE_CRIT,
> > + LM90_REG_W_REMOTE_EMERG,
> > };
> >
> > struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > @@ -390,6 +433,7 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> > int nr = attr->index;
> > long val;
> > int err;
> > + u8 config;
> >
> > err = strict_strtol(buf, 10, &val);
> > if (err < 0)
> > @@ -406,7 +450,18 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> > data->temp8[nr] = temp_to_u8(val);
> > else
> > data->temp8[nr] = temp_to_s8(val);
> > +
> > + if (data->kind == max6695 && nr >= 6) {
> > + lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
> > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> > + config | 0x08);
> > + }
> > +
> > i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);
> > +
> > + if (data->kind == max6695 && nr >= 6)
> > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
> > +
> > mutex_unlock(&data->update_lock);
> > return count;
> > }
> > @@ -450,6 +505,8 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> > int nr = attr->index;
> > long val;
> > int err;
> > + int offset = 1;
> > + u8 config;
> >
> > err = strict_strtol(buf, 10, &val);
> > if (err < 0)
> > @@ -462,19 +519,31 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> > mutex_lock(&data->update_lock);
> > if (data->kind == adt7461)
> > data->temp11[nr] = temp_to_u16_adt7461(data, val);
> > - else if (data->kind == max6657 || data->kind == max6680)
> > - data->temp11[nr] = temp_to_s8(val) << 8;
> > else if (data->kind == max6646)
> > data->temp11[nr] = temp_to_u8(val) << 8;
> > + else if (!lm90_have_remote_limit_ext(data))
> > + data->temp11[nr] = temp_to_s8(val) << 8;
> > else
> > data->temp11[nr] = temp_to_s16(val);
> >
> > - i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2],
> > + if (data->kind == max6695 && nr >= 6) {
> > + /* select output channel 2 */
> > + lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
> > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> > + config | 0x08);
> > + offset = 6;
> > + }
> > +
> > + i2c_smbus_write_byte_data(client, reg[(nr - offset) * 2],
> > data->temp11[nr] >> 8);
>
> This all gets a little tricky... Maybe it is time to rethink the whole
> thing.
>
You mean using the offset variable ? I would agree. No idea right now
how to improve it, though. I'll think about it.
> > - if (data->kind != max6657 && data->kind != max6680
> > - && data->kind != max6646)
> > - i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2 + 1],
> > + if (lm90_have_remote_limit_ext(data))
> > + i2c_smbus_write_byte_data(client, reg[(nr - offset) * 2 + 1],
> > data->temp11[nr] & 0xff);
> > +
> > + if (data->kind == max6695 && nr >= 6)
> > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> > + config);
> > +
> > mutex_unlock(&data->update_lock);
> > return count;
> > }
> > @@ -604,6 +673,62 @@ static const struct attribute_group lm90_group = {
> > .attrs = lm90_attributes,
> > };
> >
> > +/*
> > + * Additional attributes for devices with emergency sensors
> > + */
> > +static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8,
> > + set_temp8, 4);
> > +static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8,
> > + set_temp8, 5);
> > +
> > +/*
> > + * Additional attributes for devices with 3 temperature sensors
> > + */
> > +static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, show_temp11, NULL, 5);
> > +static SENSOR_DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO, show_temp11,
> > + set_temp11, 6);
> > +static SENSOR_DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO, show_temp11,
> > + set_temp11, 7);
> > +static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO, show_temp8,
> > + set_temp8, 6);
> > +static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, 4);
> > +static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8,
> > + set_temp8, 7);
> > +
> > +static SENSOR_DEVICE_ATTR(temp3_crit_alarm, S_IRUGO, show_alarm, NULL, 9);
> > +static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_alarm, NULL, 10);
> > +static SENSOR_DEVICE_ATTR(temp3_min_alarm, S_IRUGO, show_alarm, NULL, 11);
> > +static SENSOR_DEVICE_ATTR(temp3_max_alarm, S_IRUGO, show_alarm, NULL, 12);
>
> No alarms files for emergency limits?
>
Actually, there should be. Status register bits are there. No idea why I missed those.
Oh well, another ABI change. Is tempX_emergency_alarm ok ?
> > +
> > +static struct attribute *lm90_emergency_attributes[] = {
> > + &sensor_dev_attr_temp1_emergency.dev_attr.attr,
> > + &sensor_dev_attr_temp2_emergency.dev_attr.attr,
> > + NULL
> > +};
> > +
> > +static const struct attribute_group lm90_emergency_group = {
> > + .attrs = lm90_emergency_attributes,
> > +};
> > +
> > +static struct attribute *lm90_temp3_attributes[] = {
> > + &sensor_dev_attr_temp3_input.dev_attr.attr,
> > + &sensor_dev_attr_temp3_min.dev_attr.attr,
> > + &sensor_dev_attr_temp3_max.dev_attr.attr,
> > + &sensor_dev_attr_temp3_crit.dev_attr.attr,
> > + &sensor_dev_attr_temp3_crit_hyst.dev_attr.attr,
> > + &sensor_dev_attr_temp3_emergency.dev_attr.attr,
> > +
> > + &sensor_dev_attr_temp3_crit_alarm.dev_attr.attr,
> > + &sensor_dev_attr_temp3_fault.dev_attr.attr,
> > + &sensor_dev_attr_temp3_min_alarm.dev_attr.attr,
> > + &sensor_dev_attr_temp3_max_alarm.dev_attr.attr,
> > + NULL
> > +};
> > +
> > +static const struct attribute_group lm90_temp3_group = {
> > + .attrs = lm90_temp3_attributes,
> > +};
> > +
> > /* pec used for ADM1032 only */
> > static ssize_t show_pec(struct device *dev, struct device_attribute *dummy,
> > char *buf)
> > @@ -688,7 +813,7 @@ static int lm90_detect(struct i2c_client *new_client,
> > struct i2c_adapter *adapter = new_client->adapter;
> > int address = new_client->addr;
> > const char *name = NULL;
> > - int man_id, chip_id, reg_config1, reg_convrate;
> > + int man_id, chip_id, reg_config1, reg_convrate, reg_emerg;
> >
> > if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> > return -ENODEV;
> > @@ -704,6 +829,11 @@ static int lm90_detect(struct i2c_client *new_client,
> > LM90_REG_R_CONVRATE)) < 0)
> > return -ENODEV;
> >
> > + reg_emerg = i2c_smbus_read_byte_data(new_client,
> > + LM90_REG_R_REMOTE_EMERG);
> > + if (reg_emerg < 0)
> > + return -ENODEV;
> > +
>
> Seems like a rude action, considering that not all supported devices
> even have this register. In fact, even reading this register at that
> point of the detection is undesirable. At the very least, it will slow
> down driver probing for other devices. You should read the register
> only on Maxim chips.
>
Ok, makes sense. Basic idea was that we read chip_id all the time even if
the register doesn't exist, and react to it the same way. But you are right,
it should only be read for maxim chips.
> > if ((address == 0x4C || address == 0x4D)
> > && man_id == 0x01) { /* National Semiconductor */
> > int reg_config2;
> > @@ -770,6 +900,22 @@ static int lm90_detect(struct i2c_client *new_client,
> > name = "max6657";
> > } else
> > /*
> > + * Even though MAX6695 and MAX6696 do not have a chip ID
> > + * register, reading it returns 0x01.
>
> Regardless of the last read register value?
>
Unfortunately, yes. I thought the read would return man_id, but it doesn't.
> Bad Maxim, they really should learn from their past mistakes. Having a
> device ID register really isn't that hard :(
>
> > Bit 4 of the config1
> > + * register is unused and should return zero when read.
> > + *
> > + * MAX6695 and MAX6696 have an additional set of temperature
> > + * limit registers. We can detect those chips by checking if
> > + * one of those registers exists (and thus returns a value
> > + * different to the previous reading).
> > + */
> > + if (chip_id == 0x01
> > + && (reg_config1 & 0x10) == 0x00
> > + && reg_emerg != reg_convrate
>
> Note that there is a remote chance that both values are equal even
> though the registers are different. Of course this would mean a very
> low emergency limit (below 10°C), is this the reason why you're
> ignoring this case?
>
Yes. I'll think about it some more - maybe I find something better.
> I'm not even sure what you are trying to protect yourself against.
> Given the code flow, the MAX6657/58/59 have already been handled. Are
> you aware of other Maxim chips, not handled by the lm90 driver,
> behaving the same way?
>
There is still max6680, tested afterwards. I wanted to make sure as good as I can
that I don't detect max6680 as max6695.
Of course, who knows what max6680 returns when reading registers 0x16 / 0x17.
> > + && reg_convrate <= 0x07) {
> > + name = "max6695";
> > + } else
> > + /*
>
> As detection is weak, you may also want to check that bit 0 of status2
> register is 0. Will slow things down a bit but... that's what you get
> for poorly identifiable chips.
>
Ok. Maybe I can read the additional registers only after max6657 was detected.
> > * The chip_id register of the MAX6680 and MAX6681 holds the
> > * revision of the chip. The lowest bit of the config1 register
> > * is unused and should return zero when read, so should the
> > @@ -842,6 +988,9 @@ static int lm90_probe(struct i2c_client *new_client,
> > case lm86:
> > data->alert_alarms = 0x7b;
> > break;
> > + case max6695:
> > + data->alert_alarms = (0x18 << 8) | 0x7c;
>
> I think 0x187c would be just as readable, wouldn't it?
>
Yes.
> > + break;
> > default:
> > data->alert_alarms = 0x7c;
> > break;
> > @@ -854,12 +1003,27 @@ static int lm90_probe(struct i2c_client *new_client,
> > err = sysfs_create_group(&new_client->dev.kobj, &lm90_group);
> > if (err)
> > goto exit_free;
> > +
> > + if (lm90_have_emergency(data)) {
> > + err = sysfs_create_group(&new_client->dev.kobj,
> > + &lm90_emergency_group);
> > + if (err)
> > + goto exit_remove_base;
> > + }
> > +
> > + if (data->kind == max6695) {
>
> Don't we want lm90_have_temp3(data) or similar for this?
>
Ok.
> > + err = sysfs_create_group(&new_client->dev.kobj,
> > + &lm90_temp3_group);
>
> Please align on opening parenthesis as the rest of the code does.
>
Sure.
> > + if (err)
> > + goto exit_remove_emergency;
> > + }
> > +
> > if (new_client->flags & I2C_CLIENT_PEC) {
> > err = device_create_file(&new_client->dev, &dev_attr_pec);
> > if (err)
> > goto exit_remove_files;
> > }
> > - if (data->kind != max6657 && data->kind != max6646) {
> > + if (lm90_have_offset(data)) {
> > err = device_create_file(&new_client->dev,
> > &sensor_dev_attr_temp2_offset.dev_attr);
> > if (err)
> > @@ -875,6 +1039,13 @@ static int lm90_probe(struct i2c_client *new_client,
> > return 0;
> >
> > exit_remove_files:
> > + if (data->kind == max6695)
> > + sysfs_remove_group(&new_client->dev.kobj, &lm90_temp3_group);
> > +exit_remove_emergency:
> > + if (lm90_have_emergency(data))
> > + sysfs_remove_group(&new_client->dev.kobj,
> > + &lm90_emergency_group);
> > +exit_remove_base:
>
> You know, it's always OK to remove files you didn't create, so you
> don't have to add these labels. Every error path can basically point to
> exit_remove_files. As a matter of fact, dev_attr_pec is created last
> and removed last too.
>
Ok.
> > sysfs_remove_group(&new_client->dev.kobj, &lm90_group);
> > device_remove_file(&new_client->dev, &dev_attr_pec);
> > exit_free:
> > @@ -913,6 +1084,12 @@ static void lm90_init_client(struct i2c_client *client)
> > if (data->kind == max6680)
> > config |= 0x18;
> >
> > + /*
> > + * Select external channel 1 for max6695
> > + */
> > + if (data->kind == max6695)
> > + config &= ~0x08;
> > +
> > config &= 0xBF; /* run */
> > if (config != data->config_orig) /* Only write if changed */
> > i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
> > @@ -923,9 +1100,13 @@ static int lm90_remove(struct i2c_client *client)
> > struct lm90_data *data = i2c_get_clientdata(client);
> >
> > hwmon_device_unregister(data->hwmon_dev);
> > + if (lm90_have_emergency(data))
> > + sysfs_remove_group(&client->dev.kobj, &lm90_emergency_group);
> > + if (data->kind == max6695)
> > + sysfs_remove_group(&client->dev.kobj, &lm90_temp3_group);
> > sysfs_remove_group(&client->dev.kobj, &lm90_group);
> > device_remove_file(&client->dev, &dev_attr_pec);
> > - if (data->kind != max6657 && data->kind != max6646)
> > + if (lm90_have_offset(data))
> > device_remove_file(&client->dev,
> > &sensor_dev_attr_temp2_offset.dev_attr);
>
> BTW, we (you) may want to move all file removal code to a separate
> function so that the code can be shared between lm90_probe and
> lm90_remove.
>
Makes sense. I'll check if it also makes sense to put that into a separate patch.
> >
> > @@ -940,10 +1121,14 @@ static int lm90_remove(struct i2c_client *client)
> > static void lm90_alert(struct i2c_client *client, unsigned int flag)
> > {
> > struct lm90_data *data = i2c_get_clientdata(client);
> > - u8 config, alarms;
> > + u8 config, alarms, alarms2 = 0;
> >
> > lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
> > - if ((alarms & 0x7f) == 0) {
> > +
> > + if (data->kind == max6695)
> > + lm90_read_reg(client, LM90_REG_R_STATUS2, &alarms2);
> > +
> > + if ((alarms & 0x7f) == 0 && (alarms2 & 0xfe) == 0) {
> > dev_info(&client->dev, "Everything OK\n");
> > } else {
> > if (alarms & 0x61)
> > @@ -956,6 +1141,10 @@ static void lm90_alert(struct i2c_client *client, unsigned int flag)
> > dev_warn(&client->dev,
> > "temp%d diode open, please check!\n", 2);
> >
> > + if (alarms2 & 0x18)
> > + dev_warn(&client->dev,
> > + "temp%d out of range, please check!\n", 3);
> > +
> > /* Disable ALERT# output, because these chips don't implement
> > SMBus alert correctly; they should only hold the alert line
> > low briefly. */
> > @@ -1011,6 +1200,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
> > if (time_after(jiffies, data->last_updated + HZ / 2 + HZ / 10)
> > || !data->valid) {
> > u8 h, l;
> > + u8 alarms, alarms2 = 0;
>
> You don't really need alarms, only alarms2. alarms only adds a copy for
> all chips, which could be avoided.
>
You are right. I'll move alarms2 into the max6695 path and keep it local there.
> >
> > dev_dbg(&client->dev, "Updating lm90 data.\n");
> > lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, &data->temp8[0]);
> > @@ -1019,7 +1209,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
> > lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]);
> > lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst);
> >
> > - if (data->kind == max6657 || data->kind == max6646) {
> > + if (lm90_have_local_temp_ext(data)) {
> > lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
> > MAX6657_REG_R_LOCAL_TEMPL,
> > &data->temp11[4]);
> > @@ -1033,29 +1223,63 @@ static struct lm90_data *lm90_update_device(struct device *dev)
> >
> > if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) {
> > data->temp11[1] = h << 8;
> > - if (data->kind != max6657 && data->kind != max6680
> > - && data->kind != max6646
> > + if (lm90_have_remote_limit_ext(data)
> > && lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL,
> > &l) == 0)
> > data->temp11[1] |= l;
> > }
> > if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) {
> > data->temp11[2] = h << 8;
> > - if (data->kind != max6657 && data->kind != max6680
> > - && data->kind != max6646
> > + if (lm90_have_remote_limit_ext(data)
> > && lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL,
> > &l) == 0)
> > data->temp11[2] |= l;
> > }
> >
> > - if (data->kind != max6657 && data->kind != max6646) {
> > + if (lm90_have_offset(data)) {
> > if (lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSH,
> > &h) == 0
> > && lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL,
> > &l) == 0)
> > data->temp11[3] = (h << 8) | l;
> > }
> > - lm90_read_reg(client, LM90_REG_R_STATUS, &data->alarms);
> > +
> > + if (data->kind == max6695) {
> > + u8 config;
> > +
> > + lm90_read_reg(client, LM90_REG_R_LOCAL_EMERG,
> > + &data->temp8[4]);
> > + lm90_read_reg(client, LM90_REG_R_REMOTE_EMERG,
> > + &data->temp8[5]);
>
> These two should be read if (lm90_have_emergency()), as this is the
> condition under which the corresponding attributes have been created.
>
Ok.
> > +
> > + lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
> > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> > + config | 0x08);
> > +
> > + lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
> > + &data->temp8[6]);
> > + lm90_read_reg(client, LM90_REG_R_REMOTE_EMERG,
> > + &data->temp8[7]);
> > + lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
> > + LM90_REG_R_REMOTE_TEMPL, &data->temp11[5]);
> > + if (!lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h)
> > + && !lm90_read_reg(client,
> > + LM90_REG_R_REMOTE_LOWL, &l))
> > + data->temp11[6] = (h << 8) | l;
> > + if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h)
> > + && !lm90_read_reg(client,
> > + LM90_REG_R_REMOTE_HIGHL, &l))
> > + data->temp11[7] = (h << 8) | l;
>
> Alignment of && is slightly different from what is done in the rest of
> the driver.
>
I'll make sure it matches.
> > +
> > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> > + config);
> > +
> > + lm90_read_reg(client, LM90_REG_R_STATUS2,
> > + &alarms2);
> > + }
> > +
> > + lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
> > + data->alarms = (alarms2 << 8) | alarms;
> >
> > /* Re-enable ALERT# output if it was originally enabled and
> > * relevant alarms are all clear */
>
> Overall it looks pretty good. Too bad these changes are heavily
> underlining the design limitations of my driver. It has grown way
> beyond what I imagined when writing it, and supports many more devices
> with different features than it originally did.
>
Unfortunately that is true. I was struggling for a while if I should write
a separate driver.
> If you are motivated to improve the driver's code to be more robust and
> readable, feel free, I have no objection!
>
I'll see if I can do some more cleanup.
Thanks a lot for the review!
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/