Hi Jean, Thanks for your comments. See the reply inline. > -----Original Message----- > From: Jean Delvare [mailto:khali@xxxxxxxxxxxx] > Sent: 2012年2月21日 0:39 > To: Xie Xiaobo-R63061 > Cc: lm-sensors@xxxxxxxxxxxxxx; guenter.roeck@xxxxxxxxxxxx; Hu > Mingkai-B21284 > Subject: Re: [PATCH V4] hwmon: Add MCP3021 ADC driver > > Hi Xie, > > On Mon, 20 Feb 2012 12:13:59 +0800, Xie Xiaobo wrote: > > Add I2C driver for MCP3021 that is an ADC chip from Microchip. > > The MCP3021 is a successive approximation A/D converter (ADC) with > > 10-bit resolution. > > The driver export the value of Vin to sysfs, the voltage unit is mV. > > Through the sysfs interface, lm-sensors tool can also display Vin > > voltage. > > > > Signed-off-by: Mingkai Hu <Mingkai.hu@xxxxxxxxxxxxx> > > Signed-off-by: Xie Xiaobo <X.Xie@xxxxxxxxxxxxx> > > --- > > When submitting a new version of a previous patch, please include a changelog > here so that reviewers know what changed. I'm using quilt's wonderful snapshot > feature to get an idea but it would take me less time if I knew what to expect. > Yes, it's my experience Insufficiency, and I'm sorry for that. I submitted patch V4, because I missed one memory freeing in patch V3. > Still a few issues remaining in this version, although most problems were properly > fixed (thanks!) > > > +static inline u16 volts_from_reg(u16 vdd, u16 val) { > > + if (val == 0) > > + return 0; > > + > > + val = val * MCP3021_OUTPUT_SCALE - MCP3021_OUTPUT_SCALE / 2; > > + > > + return val * DIV_ROUND_CLOSEST(vdd, > > + (1 << MCP3021_OUTPUT_RES) * MCP3021_OUTPUT_SCALE); } > > I did not notice before, but can't this overflow? Say val == 0xfff0, 0xfff0 * 4 - 4 / 2 > == 0x3ffbe, that doesn't fit in u16. I think you should introduce a local int variable > and return its value (as an int, not u16.) > I used the u16, because the val is from mcp3021_read16(), So the val always <= 0x3ff (I already defined MCP3021_SAR_MASK). > > + > > +static ssize_t show_in_input(struct device *dev, struct device_attribute *attr, > > + char *buf) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct mcp3021_data *data = i2c_get_clientdata(client); > > + int reg, in_input; > > + > > + reg = mcp3021_read16(client); > > + if (reg >= 0) > > + in_input = volts_from_reg(data->vdd, reg); > > + else > > + in_input = reg; > > No, this would be interpreted by userspace as a negative voltage, not an error. > You want to return reg directly, not print it. > > > + return sprintf(buf, "%d\n", in_input); } > > + > > +static DEVICE_ATTR(in0_input, S_IRUGO, show_in_input, NULL); > > + > > +static struct attribute *mcp3021_attributes[] = { > > + &dev_attr_in0_input.attr, > > + NULL > > +}; > > You hardly need an array for one attribute, just access the attribute directly. > > All the rest looks good. > -- > Jean Delvare _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
