[lm-sensors] [PATCH] hwmon: add driver for ADT7411

[Wolfram Sang]

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/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20100118/95d69f8c/attachment.sig>