[lm-sensors] [PATCH] Add driver for SMSC EMC2103 temperature monitor and fan controller

Jean Delvare khali at linux-fr.org
Sun Jun 27 18:25:58 CEST 2010


Hi Steve,

On Sun, 27 Jun 2010 16:40:52 +0100, Steve.Glendinning at smsc.com wrote:
> Hi Jean,
> 
> Thanks for the thorough review, lots of good catches there :)
> 
> I've posted a new version of the driver addressing pretty much all of 
> this.  A few notes on your review comments:
> 
> > > +
> > > +static int apd = 1;
> > > +module_param(apd, bool, 0);
> > > +MODULE_PARM_DESC(init, "Set to zero to disable anti-parallel diode mode");
> > > +
> > > +struct temperature {
> > > +   s8   degrees;
> > > +   u8   fraction;   /* 0-7 multiples of 0.125 */
> > > +};
> > 
> > This doesn't make much sense. Just use a left-aligned s16. Your code
> > will be much nicer.
> 
> I haven't done this, because I couldn't decide on the "nicest" way to do 
> it!

I'm not sure what you think is difficult about this... Many drivers do
exactly this, if you want to take a look: ad7414, ad7418, adm9240,
dme1737... Just take a look.

> > Did you check if the chip supports i2c_smbus_read_word_data()? This
> > would be faster. Might even improve reliability if the device doesn't
> > do register latching (I confess I didn't read the datasheet.)
> 
> No, unfortunately the part only supports byte accesses.  I tried it :)
> 
> > > +}
> > > +
> > > +static void read_fan_from_i2c(struct i2c_client *client, u16 *output,
> > > +               u8 hi_addr, u8 lo_addr)
> > 
> > Always called with hi_addr == lo_addr + 1, so it could be simplified
> > (would be consistent with what you do for temperatures above.)
> 
> No, it is not! The hi and lo tach registers are the wrong way round.

Wow, tricky :(

> > > +static ssize_t set_fan_target(struct device *dev, struct device_attribute *da,
> > > +               const char *buf, size_t count)
> > > +{
> > > +   struct emc2103_data *data = emc2103_update_device(dev);
> > > +   struct i2c_client *client = to_i2c_client(dev);
> > > +   long rpm_target;
> > > +   int fan_div = 1 << data->fan_range;
> > > +
> > > +   int result = strict_strtol(buf, 10, &rpm_target);
> > > +   if (result < 0)
> > > +      return -EINVAL;
> > > +
> > > +   if ((rpm_target < 0) || (rpm_target > 16384))
> > 
> > Odd arbitrary limit. Rationale please? The chip can handle speeds
> > faster than this.
> 
> The datasheet states this as the maximum supported speed.

Oh well.

> > > +
> > > +   write_fan_target_to_i2c(client, data->fan_target);
> > > +
> > > +   if (!data->fan_rpm_control) {
> > > +      /* RPM control mode is not currently enabled */
> > > +      int status = i2c_smbus_read_byte_data(client, REG_FAN_CONF1);
> > > +      if (status < 0) {
> > > +         dev_dbg(&client->dev, "reg 0x%02x, err %d\n",
> > > +            REG_FAN_CONF1, status);
> > > +         mutex_unlock(&data->update_lock);
> > > +         return -EIO;
> > > +      }
> > > +      status |= 0x80;
> > > +      i2c_smbus_write_byte_data(client, REG_FAN_CONF1, status);
> > 
> > This isn't OK. Fan speed control should be enabled (and disabled! -
> > your driver currently doesn't even allow it) explicitly by the user.
> > This should be controlled by file pwm1_enable: 0 for no control and 3
> > for target speed mode.
> 
> I've done this, is the meaning of the number 3 documented anywhere?  I
> didn't see it in Documentation/hwmon/sysfs-interface, that just says
> 2+ means automatic fan speed control enabled.

No, unfortunately we never standardized on the various automatic modes.
But as the most frequent mode is thermal-controlled and drivers use 2
for this, I tend to recommend 3 for the target speed mode.

-- 
Jean Delvare




More information about the lm-sensors mailing list