Hi, On Tue, Dec 27, 2011 at 04:51:02AM -0500, Xie Xiaobo wrote: > Add I2C driver for MCP3021 that is an ADC chip from Microchip. > On the MPC8536DS board, there are two MCP3021 chips to monitor the > core and platform current individually. The current is converted > to voltage through a current shunt monitor(INA196) and the voltage > then connected to MCP3021's analog input through a voltage-divider > network. On the board, the relationship between the core/platform > current and the MCP3021's input voltage is as follows: > Icore = 6.0 * Vin > Iplat = 5.5 * Vin > 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. > Please refrain from adding board details to the driver commit log. > Signed-off-by: Mingkai Hu <Mingkai.hu@xxxxxxxxxxxxx> > Signed-off-by: Xie Xiaobo <X.Xie@xxxxxxxxxxxxx> > --- > Documentation/hwmon/mcp3021 | 20 ++++ > drivers/hwmon/Kconfig | 12 +++ > drivers/hwmon/Makefile | 1 + > drivers/hwmon/mcp3021.c | 223 +++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 256 insertions(+), 0 deletions(-) > create mode 100644 Documentation/hwmon/mcp3021 > create mode 100644 drivers/hwmon/mcp3021.c > > diff --git a/Documentation/hwmon/mcp3021 b/Documentation/hwmon/mcp3021 > new file mode 100644 > index 0000000..e5d67d7 > --- /dev/null > +++ b/Documentation/hwmon/mcp3021 > @@ -0,0 +1,20 @@ > +Kernel driver MCP3021 > +====================== > + > +Supported chips: > + * Microchip Technology MCP3021 > + Prefix: 'mcp3021' > + Addresses scanned: I2C 0x4d There is no _detect function, nor has the chip any means to detect it. So there are no addresses to scan. Besides, the chip supports other addresses as well. > + Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/21805a.pdf > + > +Author: Mingkai Hu > + > +Description > +----------- > + > +This driver implements support for the Microchip Technology MCP3021 chip. > + > +The Microchip Technology Inc. MCP3021 is a successive > +approximation A/D converter (ADC) with 10-bit resolution. > +This device provides one single-ended input with very low > +power consumption. > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 91be41f..5af42db 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1360,6 +1360,18 @@ config SENSORS_MC13783_ADC > help > Support for the A/D converter on MC13783 PMIC. > > +config SENSORS_MCP3021 > + tristate "Microchip MCP3021" > + depends on I2C Should also depend on EXPERIMENTAL. > + select HWMON_VID Why do you select HWMON_VID ? I do not see a reason for doing it, as the chip does not support any VID lines. > + default n > + help > + If you say yes here you get support for the hardware > + monitoring functionality of the MCP3021 chip. > + > + This driver can also be built as a module. If so, the module > + will be called mcp3021. > + > if ACPI > > comment "ACPI drivers" > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index 8251ce8..3912557 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -125,6 +125,7 @@ obj-$(CONFIG_SENSORS_W83L785TS) += w83l785ts.o > obj-$(CONFIG_SENSORS_W83L786NG) += w83l786ng.o > obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o > obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o > +obj-$(CONFIG_SENSORS_MCP3021) += mcp3021.o > Please retain alphabetical order. > obj-$(CONFIG_PMBUS) += pmbus/ > > diff --git a/drivers/hwmon/mcp3021.c b/drivers/hwmon/mcp3021.c > new file mode 100644 > index 0000000..e2ebb8c > --- /dev/null > +++ b/drivers/hwmon/mcp3021.c > @@ -0,0 +1,223 @@ > +/* > + * mcp3021.c - driver for the Microchip MCP3021 chip > + * > + * Copyright (C) 2008-2009, 2011 Freescale Semiconductor, Inc. > + * Author: Mingkai Hu <Mingkai.hu@xxxxxxxxxxxxx> > + * > + * This driver export the value of analog input voltage to sysfs, the > + * voltage unit is mV. Through the sysfs interface, lm-sensors tool > + * can also display the input voltage. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/jiffies.h> > +#include <linux/hwmon.h> > +#include <linux/hwmon-sysfs.h> > +#include <linux/slab.h> > +#include <linux/i2c.h> > +#include <linux/err.h> > +#include <linux/mutex.h> > +#include <linux/device.h> > + > +/* > + * MCP3021 macros > + */ > +/* Vdd info */ > +#define MCP3021_VDD_MAX 5500 > +#define MCP3021_VDD_MIN 2700 The above defines are not used and thus unnecessary (unless used for VDD validation - see below). > +#define MCP3021_VDD_REF 3300 > + > +/* output format */ > +#define MCP3021_SAR_SHIFT 2 > +#define MCP3021_SAR_MASK 0x3ff > + > +#define MCP3021_OUTPUT_RES 10 /* 10-bit resolution */ > +#define MCP3021_OUTPUT_SCALE 4 > +#define MCP3021_OUTPUT_MAX_VAL ((1 << MCP3021_OUTPUT_RES) - 1) > + > +/* > + * Client data (each client gets its own) > + */ > +struct mcp3021_data { > + struct device *hwmon_dev; > + struct mutex update_lock; /* protect register access */ > + > + /* Register values */ > + u16 in_input; update_lock and in_input are unnecessary. Please see below. Consider adding vdd, to be passed as platform data. > +}; > + > +/* > + * Driver data (common to all clients) > + */ > +static int mcp3021_probe(struct i2c_client *client, > + const struct i2c_device_id *id); > +static int mcp3021_remove(struct i2c_client *client); > + Please no forward declarations. Rearrange the code to avoid it. > +enum chips { mcp3021 }; The driver only supports a single chip. While the define is used to initialize mcp3021_id, the value is never used, and the enum thus unnecessary. > + > +static int mcp3021_read16(struct i2c_client *client, u16 *data) > +{ There is no need to return the value in a u16 * and an error code as return value. Please merge as done in other drivers to simplify the code. > + int ret; > + int reg; > + uint8_t buf[2]; > + > + ret = i2c_master_recv(client, buf, 2); > + if (ret != 2) > + return ret; > + With this also have positive error return values, which makes the code much more difficult to understand, and doesn't even cover the case where i2c_master_recv() for unexplicable reasons returns 0. If you don't trust i2c_master_recv() to return 2 or an error, please at least make it clean. ret = i2c_master_recv(client, buf, 2); if (ret < 0) return ret; if (ret != 2) return -ENXIO; ... return reg; > + /* The output code of the MCP3021 is transmitted with MSB first. */ > + reg = (buf[0] << 8) | buf[1]; > + > + /* The ten-bit output code is composed of the lower 4-bit of the > + * first byte and the upper 6-bit of the second byte. > + */ > + reg = (reg >> MCP3021_SAR_SHIFT) & MCP3021_SAR_MASK; > + *data = reg; > + > + return 0; > +} > + > +/* > + * The basic transfer function is volts = vdd * val / 1024, except > + * that at val's limits (0 and 1023), val should have 0.25 added to it. > + * We scale val by a factor of four so we can add one instead of 0.25. Please provide a pointer to the datasheet mentioning this requirement. I don't find it, and it does not make much sense to me since it results in never being able to measure 0. Per datasheet, a measured value of 0 means that the measured voltage is below 0.5 * LSB, which should return 0, not arbitrarily 0.25 * LSB. The same applies for the maximum measurement of 1023; all you know is that the measured voltage is above VDD - 1.5*LSB, and you should not arbitrarily add 0.25 * LSB to the result. Besides, this makes the code more complex for no or very little gain. > + */ > +static inline u16 volts_from_reg(u16 vdd, u16 val) > +{ > + val = val * MCP3021_OUTPUT_SCALE; > + > + if ((val == 0) || > + (val == MCP3021_OUTPUT_MAX_VAL * MCP3021_OUTPUT_SCALE)) > + val++; > + Unnecessary extra ( ). > + return val * vdd / ((1 << MCP3021_OUTPUT_RES) Unnecessary extra ' '. > + * MCP3021_OUTPUT_SCALE); > +} > + > +static struct mcp3021_data *mcp3021_update_device(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct mcp3021_data *data = i2c_get_clientdata(client); > + u16 vdd = MCP3021_VDD_REF; > + vdd is only used to pass a constant to volts_from_reg(). This is unnecessary. On the other side, VDD is platform dependent. Consider adding platform data to provide it, and store it in data->vdd or similar. You can still initialize data->vdd with MCP3021_VDD_REF if there is no platform data (and actually use of MCP3021_VDD_MAX and MCP3021_VDD_MIN to validate the platform data range). > + mutex_lock(&data->update_lock); > + mcp3021_read16(client, &data->in_input); > + data->in_input = volts_from_reg(vdd, data->in_input); > + mutex_unlock(&data->update_lock); > + > + return data; > +} This function, and storing the value in in_input, is unnecessary. It would only make sense if the value was cached, which it isn't. data->update_lock and data->in_input are thus unnecessary. Just merge the code into show_in_input(). Also, one variable, one object, please. As currently written, you managed to introduce a race condition on data->in_input, making the lock quite useless. > + > +static ssize_t show_in_input(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct mcp3021_data *data = mcp3021_update_device(dev); > + return sprintf(buf, "%d\n", data->in_input); > +} > + > +static DEVICE_ATTR(in0_input, S_IRUGO, show_in_input, NULL); > + > +static struct attribute *mcp3021_attributes[] = { > + &dev_attr_in0_input.attr, > + NULL > +}; > + > +static const struct attribute_group mcp3021_group = { > + .attrs = mcp3021_attributes, > +}; > + > +static int mcp3021_check_status(struct i2c_client *client) > +{ > + u16 reg; > + return mcp3021_read16(client, ®); Unnecessary extra ' '. > +} > + > +static int mcp3021_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + int err; > + struct mcp3021_data *data = NULL; > + > + if (mcp3021_check_status(client)) > + return -ENXIO; -ENODEV, please. > + if (!i2c_check_functionality(client->adapter, > + I2C_FUNC_SMBUS_WORD_DATA)) > + return -EIO; > + Why -EIO instead of -ENODEV, and why do you check for support of I2C_FUNC_SMBUS_WORD_DATA anyway ? You don't call any SMBUS functions. > + data = kzalloc(sizeof(struct mcp3021_data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + i2c_set_clientdata(client, data); > + > + mutex_init(&data->update_lock); > + > + err = sysfs_create_group(&client->dev.kobj, &mcp3021_group); > + if (err) > + goto exit_free; > + > + data->hwmon_dev = hwmon_device_register(&client->dev); > + if (IS_ERR(data->hwmon_dev)) { > + err = PTR_ERR(data->hwmon_dev); > + goto exit_remove; > + } > + > + return 0; > + > +exit_remove: > + sysfs_remove_group(&client->dev.kobj, &mcp3021_group); > +exit_free: > + kfree(data); > + i2c_set_clientdata(client, NULL); Unnecessary. > + return err; > +} > + > +static int mcp3021_remove(struct i2c_client *client) > +{ > + struct mcp3021_data *data = i2c_get_clientdata(client); > + > + hwmon_device_unregister(data->hwmon_dev); > + sysfs_remove_group(&client->dev.kobj, &mcp3021_group); > + kfree(data); > + i2c_set_clientdata(client, NULL); > + Unnecessary. > + return 0; > +} > + > +static const struct i2c_device_id mcp3021_id[] = { > + { "mcp3021", mcp3021 }, > + { } > +}; > + > +static struct i2c_driver mcp3021_driver = { > + .driver = { > + .name = "mcp3021", > + }, > + .probe = mcp3021_probe, > + .remove = mcp3021_remove, > + .id_table = mcp3021_id, > +}; > + > +static int __init mcp3021_init(void) > +{ > + return i2c_add_driver(&mcp3021_driver); > +} > + > +static void __exit mcp3021_exit(void) > +{ > + i2c_del_driver(&mcp3021_driver); > +} > + > +MODULE_AUTHOR("Mingkai Hu <Mingkai.hu@xxxxxxxxxxxxx>"); > +MODULE_DESCRIPTION("Microchip MCP3021 driver"); > +MODULE_LICENSE("GPL"); > + > +module_init(mcp3021_init); > +module_exit(mcp3021_exit); > -- > 1.7.5.1 > > _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
