Guten Morgen Guenter,
On Mon, 6 Sep 2010 19:00:53 -0700, Guenter Roeck wrote:
> On Mon, Sep 06, 2010 at 12:12:29PM -0400, Jean Delvare wrote:
> > On Sat, 4 Sep 2010 15:34:35 -0700, Guenter Roeck wrote:
> > > @@ -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.
One option would be to switch to SENSOR_ATTR_2 and use the second
parameter as an index into the reg array, which in turn could become a
2-D array (or 1-D array of pair structs). I wasn't a big fan of
SENSOR_ATTR_2 originally because its attribute are small (u8) and
easily confused (index vs. nr) but it has its technical merits. Maybe
someday we'll unify SENSOR_ATTR and SENSOR_ATTR_2 into a single
structure with reasonably sized and named attributes. That will be a
lot of work though.
> > (...)
> > 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 ?
Of course it is.
> > (...)
> > Regardless of the last read register value?
>
> Unfortunately, yes. I thought the read would return man_id, but it doesn't.
It's not that bad. A constant, non-00, non-ff device ID, even
non-documented, has some value.
> > (...)
> > 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.
Oh right. Same device ID, of course :(
> Of course, who knows what max6680 returns when reading registers 0x16 / 0x17.
I don't. But I can try obtaining the information for you, stay tuned.
> > (...)
> > 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.
Yes, delaying register reads until they are really needed is good
practice.
> > (...)
> > 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.
Yes, a separate patch would be good.
> (...)
> Thanks a lot for the review!
Welcome :)
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
