* Re: [lm-sensors] [PATCH] hwmon: Add MCP3021 hardware monitoring
2011-12-27 9:51 [lm-sensors] [PATCH] hwmon: Add MCP3021 hardware monitoring driver Xie Xiaobo
@ 2011-12-31 7:36 ` Guenter Roeck
0 siblings, 0 replies; 2+ messages in thread
From: Guenter Roeck @ 2011-12-31 7:36 UTC (permalink / raw)
To: lm-sensors
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@freescale.com>
> Signed-off-by: Xie Xiaobo <X.Xie@freescale.com>
> ---
> 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@freescale.com>
> + *
> + * 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@freescale.com>");
> +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@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 2+ messages in thread