Hi Jean,
> > I polished up an older driver from us. Hope I didn't overlook some issues...
>
> Review:
Seems like it was a bad first try, after all :(
> It seems to me that you are only using a small subset of the registers
> defined above. I suggest not defining registers you don't use, as it
> only makes the code bigger with no benefit. The datasheet of this chip
> is publicly available, if anybody needs a reference.
The intention was to spare other people copying this data again, but I get your
point and agree.
> Logic seems overly complex. Do you have such an unreliable I2C bus that
> transient failures are frequent?
The customer had a very "hostile" environment, so this was needed. It could be
rated as a special extension and dropped, of course. I saw it in some other
drivers (lm93) and wasn't sure about the preferred way.
> > + u8 lsb_reg = ADT7411_REG_EXT_TEMP_AIN14_LSB + (nr >> 2);
> > + u8 lsb_shift = (nr & 0x03) << 1;
>
> "2 * (nr & 0x03)" would be easier to understand.
Really? Will do, but when it comes to registers and bit positions, I like
shifting better.
> > +static ssize_t adt7411_set_bit(struct device *dev, struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct sensor_device_attribute_2 *s_attr2 = to_sensor_dev_attr_2(attr);
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct adt7411_data *data = i2c_get_clientdata(client);
> > + int reg, bit, ret;
> > + unsigned long val;
> > +
> > + ret = strict_strtoul(buf, 0, &val);
> > +
> > + if (ret || val > 1)
> > + return -EINVAL;
> > +
> > + reg = s_attr2->index;
> > + bit = s_attr2->nr;
> > +
> > + if (val)
> > + data->config[reg] |= bit;
> > + else
> > + data->config[reg] &= ~bit;
> > +
> > + i2c_smbus_write_byte_data(client, ADT7411_REG_CFG1 + reg,
> > + data->config[reg]);
> > + return count;
> > +}
>
> This assumes that nobody else ever changes the configuration registers.
> That's an unsafe assumption... Not only external users of the chip
> could (think multi-master I2C setups) but even concurrent users of your
> own driver could! So you want to read the register value instead of
> relying on a cached value. And if you want to keep a cache (which
> doesn't seem terribly needed to me),you also want to protect the
> read-modify-write operation with a mutex.
Right, but the mutex is needed for the non-cached I2C-based RMW, too, no?
Rest is also accepted, of course. Thanks for the review!
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Attachment:
signature.asc
Description: Digital signature
_______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
