* Re: [lm-sensors] [PATCH] hwmon: LTC4261 Hardware monitoring driver
2010-09-18 4:32 [lm-sensors] [PATCH] hwmon: LTC4261 Hardware monitoring driver Guenter Roeck
@ 2010-09-18 15:04 ` Guenter Roeck
2010-10-03 16:29 ` Jean Delvare
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2010-09-18 15:04 UTC (permalink / raw)
To: lm-sensors
On Sat, Sep 18, 2010 at 12:32:41AM -0400, Guenter Roeck wrote:
> This driver adds support for Linear Technology LTC4261 I2C Negative
> Voltage Hot Swap Controller.
>
> Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
>
[...]
> +
> +/* chip registers */
> +#define LTC4261_STATUS 0x00 /* readonly */
> +#define LTC4261_FAULT 0x01
> +#define LTC4261_ALERT 0x02
> +#define LTC4261_CONTROL 0x03
> +#define LTC4261_SENSE_H 0x04
> +#define LTC4261_SENSE_L 0x05
> +#define LTC4261_ADIN2_H 0x06
> +#define LTC4261_ADIN2_L 0x07
> +#define LTC4261_ADIN_H 0x08
> +#define LTC4261_ADIN_L 0x09
> +
Wretched tab after define. Always hits me. I'll fix that
with the next version of the patch. No need to comment.
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [lm-sensors] [PATCH] hwmon: LTC4261 Hardware monitoring driver
2010-09-18 4:32 [lm-sensors] [PATCH] hwmon: LTC4261 Hardware monitoring driver Guenter Roeck
2010-09-18 15:04 ` Guenter Roeck
@ 2010-10-03 16:29 ` Jean Delvare
2010-10-03 18:10 ` Guenter Roeck
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Jean Delvare @ 2010-10-03 16:29 UTC (permalink / raw)
To: lm-sensors
On Fri, 17 Sep 2010 21:32:41 -0700, Guenter Roeck wrote:
> This driver adds support for Linear Technology LTC4261 I2C Negative
> Voltage Hot Swap Controller.
>
> Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
Ira, you are familiar with Linear Technology hardware monitoring
devices, would you mind reviewing this driver? Thanks.
> ---
> v2 changes:
> - Report voltage alarms on both voltage sensor channels
> - Merged patchset into a single patch
> - Updated MAINTAINERS
> - Replaced curr1_max_alarm with curr1_alarm
> - Fixed code to clear faults
>
> Documentation/hwmon/ltc4261 | 63 +++++++++
> MAINTAINERS | 7 +
> drivers/hwmon/Kconfig | 11 ++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/ltc4261.c | 315 +++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 397 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/hwmon/ltc4261
> create mode 100644 drivers/hwmon/ltc4261.c
>
> diff --git a/Documentation/hwmon/ltc4261 b/Documentation/hwmon/ltc4261
> new file mode 100644
> index 0000000..01d85b5
> --- /dev/null
> +++ b/Documentation/hwmon/ltc4261
> @@ -0,0 +1,63 @@
> +Kernel driver ltc4261
> +==========> +
> +Supported chips:
> + * Linear Technology LTC4261
> + Prefix: 'ltc4261'
> + Addresses scanned: -
> + Datasheet:
> + http://cds.linear.com/docs/Datasheet/42612fb.pdf
> +
> +Author: Guenter Roeck <guenter.roeck@ericsson.com>
> +
> +
> +Description
> +-----------
> +
> +The LTC4261/LTC4261-2 negative voltage Hot Swap controllers allow a board
> +to be safely inserted and removed from a live backplane.
> +
> +
> +Usage Notes
> +-----------
> +
> +This driver does not probe for LTC4261 devices, since there is no register
> +which can be safely used to identify the chip. You will have to instantiate
> +the devices explicitly.
> +
> +Example: the following will load the driver for an LTC4261 at address 0x10
> +on I2C bus #1:
> +$ modprobe ltc4261
> +$ echo ltc4261 0x10 > /sys/bus/i2c/devices/i2c-1/new_device
> +
> +
> +Sysfs entries
> +-------------
> +
> +Voltage readings provided by this driver are reported as obtained from the ADC
> +registers. If a set of voltage divider resistors is installed, calculate the
> +real voltage by multiplying the reported value with (R1+R2)/R2, where R1 is the
> +value of the divider resistor against the measured voltage and R2 is the value
> +of the divider resistor against Ground.
> +
> +Current reading provided by this driver is reported as obtained from the ADC
> +Current Sense register. The reported value assumes that a 1 mOhm sense resistor
> +is installed. If a different sense resistor is installed, calculate the real
> +current by dividing the reported value by the sense resistor value in mOhm.
> +
> +The chip has two voltage sensors, but only one set of voltage alarm status bits.
> +In many many designs, those alarms are associated with the ADIN2 sensor, due to
> +the proximity of the ADIN2 pin to the OV pin. ADIN2 is, however, not available
> +on all chip variants. To ensure that the alarm condition is reported to the user,
> +report it with both voltage sensors.
> +
> +in1_input ADIN2 voltage (mV)
> +in1_min_alarm ADIN/ADIN2 Undervoltage alarm
> +in1_max_alarm ADIN/ADIN2 Overvoltage alarm
> +
> +in2_input ADIN voltage (mV)
> +in2_min_alarm ADIN/ADIN2 Undervoltage alarm
> +in2_max_alarm ADIN/ADIN2 Overvoltage alarm
> +
> +curr1_input SENSE current (mA)
> +curr1_alarm SENSE overcurrent alarm
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e7c528f..f585a98 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3666,6 +3666,13 @@ L: linux-scsi@vger.kernel.org
> S: Maintained
> F: drivers/scsi/sym53c8xx_2/
>
> +LTC4261 HARDWARE MONITOR DRIVER
> +M: Guenter Roeck <linux@roeck-us.net>
> +L: lm-sensors@lm-sensors.org
> +S: Maintained
> +F: Documentation/hwmon/ltc4261
> +F: drivers/hwmon/ltc4261.c
> +
> LTP (Linux Test Project)
> M: Rishikesh K Rajak <risrajak@linux.vnet.ibm.com>
> M: Garrett Cooper <yanegomi@gmail.com>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 4d4d09b..71434bd 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -654,6 +654,17 @@ config SENSORS_LTC4245
> This driver can also be built as a module. If so, the module will
> be called ltc4245.
>
> +config SENSORS_LTC4261
> + tristate "Linear Technology LTC4261"
> + depends on I2C && EXPERIMENTAL
> + default n
> + help
> + If you say yes here you get support for Linear Technology LTC4261
> + Negative Voltage Hot Swap Controller I2C interface.
> +
> + This driver can also be built as a module. If so, the module will
> + be called ltc4261.
> +
> config SENSORS_LM95241
> tristate "National Semiconductor LM95241 sensor chip"
> depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index e3c2484..2d445ad 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -80,6 +80,7 @@ obj-$(CONFIG_SENSORS_LM93) += lm93.o
> obj-$(CONFIG_SENSORS_LM95241) += lm95241.o
> obj-$(CONFIG_SENSORS_LTC4215) += ltc4215.o
> obj-$(CONFIG_SENSORS_LTC4245) += ltc4245.o
> +obj-$(CONFIG_SENSORS_LTC4261) += ltc4261.o
> obj-$(CONFIG_SENSORS_MAX1111) += max1111.o
> obj-$(CONFIG_SENSORS_MAX1619) += max1619.o
> obj-$(CONFIG_SENSORS_MAX6650) += max6650.o
> diff --git a/drivers/hwmon/ltc4261.c b/drivers/hwmon/ltc4261.c
> new file mode 100644
> index 0000000..1a8bbf0
> --- /dev/null
> +++ b/drivers/hwmon/ltc4261.c
> @@ -0,0 +1,315 @@
> +/*
> + * Driver for Linear Technology LTC4261 I2C Negative Voltage Hot Swap Controller
> + *
> + * Copyright (C) 2010 Ericsson AB.
> + *
> + * Derived from:
> + *
> + * Driver for Linear Technology LTC4245 I2C Multiple Supply Hot Swap Controller
> + * Copyright (C) 2008 Ira W. Snyder <iws@ovro.caltech.edu>
> + *
> + * Datasheet: http://cds.linear.com/docs/Datasheet/42612fb.pdf
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +
> +/* chip registers */
> +#define LTC4261_STATUS 0x00 /* readonly */
> +#define LTC4261_FAULT 0x01
> +#define LTC4261_ALERT 0x02
> +#define LTC4261_CONTROL 0x03
> +#define LTC4261_SENSE_H 0x04
> +#define LTC4261_SENSE_L 0x05
> +#define LTC4261_ADIN2_H 0x06
> +#define LTC4261_ADIN2_L 0x07
> +#define LTC4261_ADIN_H 0x08
> +#define LTC4261_ADIN_L 0x09
> +
> +/*
> + * Fault register bits
> + */
> +#define FAULT_OV (1<<0)
> +#define FAULT_UV (1<<1)
> +#define FAULT_OC (1<<2)
> +
> +struct ltc4261_data {
> + struct device *hwmon_dev;
> +
> + struct mutex update_lock;
> + bool valid;
> + unsigned long last_updated; /* in jiffies */
> +
> + /* Registers */
> + u8 regs[10];
> +};
> +
> +static struct ltc4261_data *ltc4261_update_device(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct ltc4261_data *data = i2c_get_clientdata(client);
> + struct ltc4261_data *ret = data;
> +
> + mutex_lock(&data->update_lock);
> +
> + if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
> + int i;
> +
> + /* Read registers -- 0x00 to 0x09 */
> + for (i = 0; i < ARRAY_SIZE(data->regs); i++) {
> + int val;
> +
> + val = i2c_smbus_read_byte_data(client, i);
> + if (unlikely(val < 0)) {
> + dev_dbg(dev,
> + "Failed to read ADC value: error %d",
> + val);
> + ret = ERR_PTR(val);
> + goto abort;
> + }
> + data->regs[i] = val;
> + }
> + data->last_updated = jiffies;
> + data->valid = 1;
> + }
> +abort:
> + mutex_unlock(&data->update_lock);
> + return ret;
> +}
> +
> +/* Return the voltage from the given register in mV or mA */
> +static int ltc4261_get_value(struct ltc4261_data *data, u8 reg)
> +{
> + u32 val;
> +
> + val = (data->regs[reg] << 2) + (data->regs[reg + 1] >> 6);
> +
> + switch (reg) {
> + case LTC4261_ADIN_H:
> + case LTC4261_ADIN2_H:
> + /* 2.5mV resolution. Convert to mV. */
> + val = val * 25 / 10;
> + break;
> + case LTC4261_SENSE_H:
> + /*
> + * 62.5uV resolution. Convert to current as measured with
> + * an 1 mOhm sense resistor, in mA. If a different sense
> + * resistor is installed, calculate the actual current by
> + * dividing the reported current by the sense resistor value
> + * in mOhm.
> + */
> + val = val * 625 / 10;
> + break;
> + default:
> + /* If we get here, the developer messed up */
> + WARN_ON_ONCE(1);
> + val = 0;
> + break;
> + }
> +
> + return val;
> +}
> +
> +static ssize_t ltc4261_show_value(struct device *dev,
> + struct device_attribute *da, char *buf)
> +{
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> + struct ltc4261_data *data = ltc4261_update_device(dev);
> + int value;
> +
> + if (IS_ERR(data))
> + return PTR_ERR(data);
> +
> + value = ltc4261_get_value(data, attr->index);
> + return snprintf(buf, PAGE_SIZE, "%d\n", value);
> +}
> +
> +static ssize_t ltc4261_show_bool(struct device *dev,
> + struct device_attribute *da, char *buf)
> +{
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> + struct i2c_client *client = to_i2c_client(dev);
> + struct ltc4261_data *data = ltc4261_update_device(dev);
> + u8 fault;
> +
> + if (IS_ERR(data))
> + return PTR_ERR(data);
> +
> + fault = data->regs[LTC4261_FAULT] & attr->index;
> + if (fault) /* Clear reported faults in chip register */
> + i2c_smbus_write_byte_data(client, LTC4261_FAULT, ~fault);
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n", fault ? 1 : 0);
> +}
> +
> +/*
> + * These macros are used below in constructing device attribute objects
> + * for use with sysfs_create_group() to make a sysfs device file
> + * for each register.
> + */
> +
> +#define LTC4261_VALUE(name, ltc4261_cmd_idx) \
> + static SENSOR_DEVICE_ATTR(name, S_IRUGO, \
> + ltc4261_show_value, NULL, ltc4261_cmd_idx)
> +
> +#define LTC4261_BOOL(name, mask) \
> + static SENSOR_DEVICE_ATTR(name, S_IRUGO, \
> + ltc4261_show_bool, NULL, (mask))
> +
> +/*
> + * Input voltages.
> + */
> +LTC4261_VALUE(in1_input, LTC4261_ADIN_H);
> +LTC4261_VALUE(in2_input, LTC4261_ADIN2_H);
> +
> +/*
> + * Voltage alarms. The chip has only one set of voltage alarm status bits,
> + * triggered by input voltage alarms. In many designs, those alarms are
> + * associated with the ADIN2 sensor, due to the proximity of the ADIN2 pin
> + * to the OV pin. ADIN2 is, however, not available on all chip variants.
> + * To ensure that the alarm condition is reported to the user, report it
> + * with both voltage sensors.
> + */
> +LTC4261_BOOL(in1_min_alarm, FAULT_UV);
> +LTC4261_BOOL(in1_max_alarm, FAULT_OV);
> +LTC4261_BOOL(in2_min_alarm, FAULT_UV);
> +LTC4261_BOOL(in2_max_alarm, FAULT_OV);
> +
> +/* Currents (via sense resistor) */
> +LTC4261_VALUE(curr1_input, LTC4261_SENSE_H);
> +
> +/* Overcurrent alarm */
> +LTC4261_BOOL(curr1_max_alarm, FAULT_OC);
> +
> +static struct attribute *ltc4261_attributes[] = {
> + &sensor_dev_attr_in1_input.dev_attr.attr,
> + &sensor_dev_attr_in1_min_alarm.dev_attr.attr,
> + &sensor_dev_attr_in1_max_alarm.dev_attr.attr,
> + &sensor_dev_attr_in2_input.dev_attr.attr,
> + &sensor_dev_attr_in2_min_alarm.dev_attr.attr,
> + &sensor_dev_attr_in2_max_alarm.dev_attr.attr,
> +
> + &sensor_dev_attr_curr1_input.dev_attr.attr,
> + &sensor_dev_attr_curr1_max_alarm.dev_attr.attr,
> +
> + NULL,
> +};
> +
> +static const struct attribute_group ltc4261_group = {
> + .attrs = ltc4261_attributes,
> +};
> +
> +static int ltc4261_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct i2c_adapter *adapter = client->adapter;
> + struct ltc4261_data *data;
> + int ret;
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> + return -ENODEV;
> +
> + if (i2c_smbus_read_byte_data(client, LTC4261_STATUS) < 0) {
> + dev_err(&client->dev, "Failed to read register %d:%02x:%02x\n",
> + adapter->id, client->addr, LTC4261_STATUS);
> + return -ENODEV;
> + }
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data) {
> + ret = -ENOMEM;
> + goto out_kzalloc;
> + }
> +
> + i2c_set_clientdata(client, data);
> + mutex_init(&data->update_lock);
> +
> + /* Clear faults */
> + i2c_smbus_write_byte_data(client, LTC4261_FAULT, 0x00);
> +
> + /* Register sysfs hooks */
> + ret = sysfs_create_group(&client->dev.kobj, <c4261_group);
> + if (ret)
> + goto out_sysfs_create_group;
> +
> + data->hwmon_dev = hwmon_device_register(&client->dev);
> + if (IS_ERR(data->hwmon_dev)) {
> + ret = PTR_ERR(data->hwmon_dev);
> + goto out_hwmon_device_register;
> + }
> +
> + return 0;
> +
> +out_hwmon_device_register:
> + sysfs_remove_group(&client->dev.kobj, <c4261_group);
> +out_sysfs_create_group:
> + kfree(data);
> +out_kzalloc:
> + return ret;
> +}
> +
> +static int ltc4261_remove(struct i2c_client *client)
> +{
> + struct ltc4261_data *data = i2c_get_clientdata(client);
> +
> + hwmon_device_unregister(data->hwmon_dev);
> + sysfs_remove_group(&client->dev.kobj, <c4261_group);
> +
> + kfree(data);
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id ltc4261_id[] = {
> + {"ltc4261", 0},
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, ltc4261_id);
> +
> +/* This is the driver that will be inserted */
> +static struct i2c_driver ltc4261_driver = {
> + .driver = {
> + .name = "ltc4261",
> + },
> + .probe = ltc4261_probe,
> + .remove = ltc4261_remove,
> + .id_table = ltc4261_id,
> +};
> +
> +static int __init ltc4261_init(void)
> +{
> + return i2c_add_driver(<c4261_driver);
> +}
> +
> +static void __exit ltc4261_exit(void)
> +{
> + i2c_del_driver(<c4261_driver);
> +}
> +
> +MODULE_AUTHOR("Guenter Roeck <guenter.roeck@ericsson.com>");
> +MODULE_DESCRIPTION("LTC4261 driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(ltc4261_init);
> +module_exit(ltc4261_exit);
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [lm-sensors] [PATCH] hwmon: LTC4261 Hardware monitoring driver
2010-09-18 4:32 [lm-sensors] [PATCH] hwmon: LTC4261 Hardware monitoring driver Guenter Roeck
2010-09-18 15:04 ` Guenter Roeck
2010-10-03 16:29 ` Jean Delvare
@ 2010-10-03 18:10 ` Guenter Roeck
2010-10-03 21:17 ` Ira W. Snyder
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2010-10-03 18:10 UTC (permalink / raw)
To: lm-sensors
On Sun, Oct 03, 2010 at 12:29:34PM -0400, Jean Delvare wrote:
> On Fri, 17 Sep 2010 21:32:41 -0700, Guenter Roeck wrote:
> > This driver adds support for Linear Technology LTC4261 I2C Negative
> > Voltage Hot Swap Controller.
> >
> > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
>
> Ira, you are familiar with Linear Technology hardware monitoring
> devices, would you mind reviewing this driver? Thanks.
>
Yes, that would be great. I had it reviewed internally, but that just isn't the same.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [lm-sensors] [PATCH] hwmon: LTC4261 Hardware monitoring driver
2010-09-18 4:32 [lm-sensors] [PATCH] hwmon: LTC4261 Hardware monitoring driver Guenter Roeck
` (2 preceding siblings ...)
2010-10-03 18:10 ` Guenter Roeck
@ 2010-10-03 21:17 ` Ira W. Snyder
2010-10-04 1:18 ` Guenter Roeck
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Ira W. Snyder @ 2010-10-03 21:17 UTC (permalink / raw)
To: lm-sensors
On Sun, Oct 03, 2010 at 06:29:34PM +0200, Jean Delvare wrote:
> On Fri, 17 Sep 2010 21:32:41 -0700, Guenter Roeck wrote:
> > This driver adds support for Linear Technology LTC4261 I2C Negative
> > Voltage Hot Swap Controller.
> >
> > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
>
> Ira, you are familiar with Linear Technology hardware monitoring
> devices, would you mind reviewing this driver? Thanks.
>
Sure, not a problem.
Guenter, there are only a few minor nitpicks below. Please fix them and
then add this after your Signed-off-by line:
Reviewed-by: Ira W. Snyder <iws@ovro.caltech.edu>
Overall, the driver looks ready for inclusion.
Ira
> > ---
> > v2 changes:
> > - Report voltage alarms on both voltage sensor channels
> > - Merged patchset into a single patch
> > - Updated MAINTAINERS
> > - Replaced curr1_max_alarm with curr1_alarm
> > - Fixed code to clear faults
> >
> > Documentation/hwmon/ltc4261 | 63 +++++++++
> > MAINTAINERS | 7 +
> > drivers/hwmon/Kconfig | 11 ++
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/ltc4261.c | 315 +++++++++++++++++++++++++++++++++++++++++++
> > 5 files changed, 397 insertions(+), 0 deletions(-)
> > create mode 100644 Documentation/hwmon/ltc4261
> > create mode 100644 drivers/hwmon/ltc4261.c
> >
> > diff --git a/Documentation/hwmon/ltc4261 b/Documentation/hwmon/ltc4261
> > new file mode 100644
> > index 0000000..01d85b5
> > --- /dev/null
> > +++ b/Documentation/hwmon/ltc4261
> > @@ -0,0 +1,63 @@
> > +Kernel driver ltc4261
> > +==========> > +
> > +Supported chips:
> > + * Linear Technology LTC4261
> > + Prefix: 'ltc4261'
> > + Addresses scanned: -
> > + Datasheet:
> > + http://cds.linear.com/docs/Datasheet/42612fb.pdf
> > +
> > +Author: Guenter Roeck <guenter.roeck@ericsson.com>
> > +
> > +
> > +Description
> > +-----------
> > +
> > +The LTC4261/LTC4261-2 negative voltage Hot Swap controllers allow a board
> > +to be safely inserted and removed from a live backplane.
> > +
> > +
> > +Usage Notes
> > +-----------
> > +
> > +This driver does not probe for LTC4261 devices, since there is no register
> > +which can be safely used to identify the chip. You will have to instantiate
> > +the devices explicitly.
> > +
> > +Example: the following will load the driver for an LTC4261 at address 0x10
> > +on I2C bus #1:
> > +$ modprobe ltc4261
> > +$ echo ltc4261 0x10 > /sys/bus/i2c/devices/i2c-1/new_device
> > +
> > +
> > +Sysfs entries
> > +-------------
> > +
> > +Voltage readings provided by this driver are reported as obtained from the ADC
> > +registers. If a set of voltage divider resistors is installed, calculate the
> > +real voltage by multiplying the reported value with (R1+R2)/R2, where R1 is the
> > +value of the divider resistor against the measured voltage and R2 is the value
> > +of the divider resistor against Ground.
> > +
> > +Current reading provided by this driver is reported as obtained from the ADC
> > +Current Sense register. The reported value assumes that a 1 mOhm sense resistor
> > +is installed. If a different sense resistor is installed, calculate the real
> > +current by dividing the reported value by the sense resistor value in mOhm.
> > +
> > +The chip has two voltage sensors, but only one set of voltage alarm status bits.
> > +In many many designs, those alarms are associated with the ADIN2 sensor, due to
> > +the proximity of the ADIN2 pin to the OV pin. ADIN2 is, however, not available
> > +on all chip variants. To ensure that the alarm condition is reported to the user,
> > +report it with both voltage sensors.
> > +
> > +in1_input ADIN2 voltage (mV)
> > +in1_min_alarm ADIN/ADIN2 Undervoltage alarm
> > +in1_max_alarm ADIN/ADIN2 Overvoltage alarm
> > +
> > +in2_input ADIN voltage (mV)
> > +in2_min_alarm ADIN/ADIN2 Undervoltage alarm
> > +in2_max_alarm ADIN/ADIN2 Overvoltage alarm
> > +
> > +curr1_input SENSE current (mA)
> > +curr1_alarm SENSE overcurrent alarm
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index e7c528f..f585a98 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3666,6 +3666,13 @@ L: linux-scsi@vger.kernel.org
> > S: Maintained
> > F: drivers/scsi/sym53c8xx_2/
> >
> > +LTC4261 HARDWARE MONITOR DRIVER
> > +M: Guenter Roeck <linux@roeck-us.net>
> > +L: lm-sensors@lm-sensors.org
> > +S: Maintained
> > +F: Documentation/hwmon/ltc4261
> > +F: drivers/hwmon/ltc4261.c
> > +
> > LTP (Linux Test Project)
> > M: Rishikesh K Rajak <risrajak@linux.vnet.ibm.com>
> > M: Garrett Cooper <yanegomi@gmail.com>
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 4d4d09b..71434bd 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -654,6 +654,17 @@ config SENSORS_LTC4245
> > This driver can also be built as a module. If so, the module will
> > be called ltc4245.
> >
> > +config SENSORS_LTC4261
> > + tristate "Linear Technology LTC4261"
> > + depends on I2C && EXPERIMENTAL
> > + default n
> > + help
> > + If you say yes here you get support for Linear Technology LTC4261
> > + Negative Voltage Hot Swap Controller I2C interface.
> > +
> > + This driver can also be built as a module. If so, the module will
> > + be called ltc4261.
> > +
> > config SENSORS_LM95241
> > tristate "National Semiconductor LM95241 sensor chip"
> > depends on I2C
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index e3c2484..2d445ad 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -80,6 +80,7 @@ obj-$(CONFIG_SENSORS_LM93) += lm93.o
> > obj-$(CONFIG_SENSORS_LM95241) += lm95241.o
> > obj-$(CONFIG_SENSORS_LTC4215) += ltc4215.o
> > obj-$(CONFIG_SENSORS_LTC4245) += ltc4245.o
> > +obj-$(CONFIG_SENSORS_LTC4261) += ltc4261.o
> > obj-$(CONFIG_SENSORS_MAX1111) += max1111.o
> > obj-$(CONFIG_SENSORS_MAX1619) += max1619.o
> > obj-$(CONFIG_SENSORS_MAX6650) += max6650.o
> > diff --git a/drivers/hwmon/ltc4261.c b/drivers/hwmon/ltc4261.c
> > new file mode 100644
> > index 0000000..1a8bbf0
> > --- /dev/null
> > +++ b/drivers/hwmon/ltc4261.c
> > @@ -0,0 +1,315 @@
> > +/*
> > + * Driver for Linear Technology LTC4261 I2C Negative Voltage Hot Swap Controller
> > + *
> > + * Copyright (C) 2010 Ericsson AB.
> > + *
> > + * Derived from:
> > + *
> > + * Driver for Linear Technology LTC4245 I2C Multiple Supply Hot Swap Controller
> > + * Copyright (C) 2008 Ira W. Snyder <iws@ovro.caltech.edu>
> > + *
> > + * Datasheet: http://cds.linear.com/docs/Datasheet/42612fb.pdf
> > + *
> > + * 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.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/err.h>
> > +#include <linux/slab.h>
> > +#include <linux/i2c.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
> > +
> > +/* chip registers */
> > +#define LTC4261_STATUS 0x00 /* readonly */
> > +#define LTC4261_FAULT 0x01
> > +#define LTC4261_ALERT 0x02
> > +#define LTC4261_CONTROL 0x03
> > +#define LTC4261_SENSE_H 0x04
> > +#define LTC4261_SENSE_L 0x05
> > +#define LTC4261_ADIN2_H 0x06
> > +#define LTC4261_ADIN2_L 0x07
> > +#define LTC4261_ADIN_H 0x08
> > +#define LTC4261_ADIN_L 0x09
> > +
Tabs after #define here should be spaces. Keep the tabs just before the
numbers to keep things lined up. You got it right on the fault register
bits below.
> > +/*
> > + * Fault register bits
> > + */
> > +#define FAULT_OV (1<<0)
> > +#define FAULT_UV (1<<1)
> > +#define FAULT_OC (1<<2)
> > +
> > +struct ltc4261_data {
> > + struct device *hwmon_dev;
> > +
> > + struct mutex update_lock;
> > + bool valid;
> > + unsigned long last_updated; /* in jiffies */
> > +
> > + /* Registers */
> > + u8 regs[10];
> > +};
> > +
> > +static struct ltc4261_data *ltc4261_update_device(struct device *dev)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct ltc4261_data *data = i2c_get_clientdata(client);
> > + struct ltc4261_data *ret = data;
> > +
> > + mutex_lock(&data->update_lock);
> > +
> > + if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
The datasheet claims that registers E-J (SENSE and ADIN/OV registers)
are updated at 7.3 Hz. See page 21: Data Converter. A 1 Hz update rate
is awfully slow for an application that wants to do fast monitoring. I
think (HZ / 4) or (HZ / 6) would be better.
> > + int i;
> > +
> > + /* Read registers -- 0x00 to 0x09 */
> > + for (i = 0; i < ARRAY_SIZE(data->regs); i++) {
> > + int val;
> > +
I'd move the "int i" and "int val" variable declarations to the top of
the function. I don't have a strong preference, though.
> > + val = i2c_smbus_read_byte_data(client, i);
> > + if (unlikely(val < 0)) {
> > + dev_dbg(dev,
> > + "Failed to read ADC value: error %d",
> > + val);
> > + ret = ERR_PTR(val);
> > + goto abort;
> > + }
> > + data->regs[i] = val;
> > + }
> > + data->last_updated = jiffies;
> > + data->valid = 1;
> > + }
> > +abort:
> > + mutex_unlock(&data->update_lock);
> > + return ret;
> > +}
> > +
> > +/* Return the voltage from the given register in mV or mA */
> > +static int ltc4261_get_value(struct ltc4261_data *data, u8 reg)
> > +{
> > + u32 val;
> > +
> > + val = (data->regs[reg] << 2) + (data->regs[reg + 1] >> 6);
> > +
> > + switch (reg) {
> > + case LTC4261_ADIN_H:
> > + case LTC4261_ADIN2_H:
> > + /* 2.5mV resolution. Convert to mV. */
> > + val = val * 25 / 10;
> > + break;
> > + case LTC4261_SENSE_H:
> > + /*
> > + * 62.5uV resolution. Convert to current as measured with
> > + * an 1 mOhm sense resistor, in mA. If a different sense
> > + * resistor is installed, calculate the actual current by
> > + * dividing the reported current by the sense resistor value
> > + * in mOhm.
> > + */
> > + val = val * 625 / 10;
> > + break;
> > + default:
> > + /* If we get here, the developer messed up */
> > + WARN_ON_ONCE(1);
> > + val = 0;
> > + break;
> > + }
> > +
> > + return val;
> > +}
> > +
> > +static ssize_t ltc4261_show_value(struct device *dev,
> > + struct device_attribute *da, char *buf)
> > +{
> > + struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> > + struct ltc4261_data *data = ltc4261_update_device(dev);
> > + int value;
> > +
> > + if (IS_ERR(data))
> > + return PTR_ERR(data);
> > +
> > + value = ltc4261_get_value(data, attr->index);
> > + return snprintf(buf, PAGE_SIZE, "%d\n", value);
> > +}
> > +
> > +static ssize_t ltc4261_show_bool(struct device *dev,
> > + struct device_attribute *da, char *buf)
> > +{
> > + struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct ltc4261_data *data = ltc4261_update_device(dev);
> > + u8 fault;
> > +
> > + if (IS_ERR(data))
> > + return PTR_ERR(data);
> > +
> > + fault = data->regs[LTC4261_FAULT] & attr->index;
> > + if (fault) /* Clear reported faults in chip register */
> > + i2c_smbus_write_byte_data(client, LTC4261_FAULT, ~fault);
> > +
> > + return snprintf(buf, PAGE_SIZE, "%d\n", fault ? 1 : 0);
> > +}
> > +
> > +/*
> > + * These macros are used below in constructing device attribute objects
> > + * for use with sysfs_create_group() to make a sysfs device file
> > + * for each register.
> > + */
> > +
> > +#define LTC4261_VALUE(name, ltc4261_cmd_idx) \
> > + static SENSOR_DEVICE_ATTR(name, S_IRUGO, \
> > + ltc4261_show_value, NULL, ltc4261_cmd_idx)
> > +
> > +#define LTC4261_BOOL(name, mask) \
> > + static SENSOR_DEVICE_ATTR(name, S_IRUGO, \
> > + ltc4261_show_bool, NULL, (mask))
> > +
> > +/*
> > + * Input voltages.
> > + */
> > +LTC4261_VALUE(in1_input, LTC4261_ADIN_H);
> > +LTC4261_VALUE(in2_input, LTC4261_ADIN2_H);
> > +
> > +/*
> > + * Voltage alarms. The chip has only one set of voltage alarm status bits,
> > + * triggered by input voltage alarms. In many designs, those alarms are
> > + * associated with the ADIN2 sensor, due to the proximity of the ADIN2 pin
> > + * to the OV pin. ADIN2 is, however, not available on all chip variants.
> > + * To ensure that the alarm condition is reported to the user, report it
> > + * with both voltage sensors.
> > + */
> > +LTC4261_BOOL(in1_min_alarm, FAULT_UV);
> > +LTC4261_BOOL(in1_max_alarm, FAULT_OV);
> > +LTC4261_BOOL(in2_min_alarm, FAULT_UV);
> > +LTC4261_BOOL(in2_max_alarm, FAULT_OV);
> > +
> > +/* Currents (via sense resistor) */
> > +LTC4261_VALUE(curr1_input, LTC4261_SENSE_H);
> > +
> > +/* Overcurrent alarm */
> > +LTC4261_BOOL(curr1_max_alarm, FAULT_OC);
> > +
> > +static struct attribute *ltc4261_attributes[] = {
> > + &sensor_dev_attr_in1_input.dev_attr.attr,
> > + &sensor_dev_attr_in1_min_alarm.dev_attr.attr,
> > + &sensor_dev_attr_in1_max_alarm.dev_attr.attr,
> > + &sensor_dev_attr_in2_input.dev_attr.attr,
> > + &sensor_dev_attr_in2_min_alarm.dev_attr.attr,
> > + &sensor_dev_attr_in2_max_alarm.dev_attr.attr,
> > +
> > + &sensor_dev_attr_curr1_input.dev_attr.attr,
> > + &sensor_dev_attr_curr1_max_alarm.dev_attr.attr,
> > +
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group ltc4261_group = {
> > + .attrs = ltc4261_attributes,
> > +};
> > +
> > +static int ltc4261_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct i2c_adapter *adapter = client->adapter;
> > + struct ltc4261_data *data;
> > + int ret;
> > +
> > + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> > + return -ENODEV;
> > +
> > + if (i2c_smbus_read_byte_data(client, LTC4261_STATUS) < 0) {
> > + dev_err(&client->dev, "Failed to read register %d:%02x:%02x\n",
> > + adapter->id, client->addr, LTC4261_STATUS);
> > + return -ENODEV;
> > + }
> > +
> > + data = kzalloc(sizeof(*data), GFP_KERNEL);
> > + if (!data) {
> > + ret = -ENOMEM;
> > + goto out_kzalloc;
> > + }
> > +
> > + i2c_set_clientdata(client, data);
> > + mutex_init(&data->update_lock);
> > +
> > + /* Clear faults */
> > + i2c_smbus_write_byte_data(client, LTC4261_FAULT, 0x00);
> > +
> > + /* Register sysfs hooks */
> > + ret = sysfs_create_group(&client->dev.kobj, <c4261_group);
> > + if (ret)
> > + goto out_sysfs_create_group;
> > +
> > + data->hwmon_dev = hwmon_device_register(&client->dev);
> > + if (IS_ERR(data->hwmon_dev)) {
> > + ret = PTR_ERR(data->hwmon_dev);
> > + goto out_hwmon_device_register;
> > + }
> > +
> > + return 0;
> > +
> > +out_hwmon_device_register:
> > + sysfs_remove_group(&client->dev.kobj, <c4261_group);
> > +out_sysfs_create_group:
> > + kfree(data);
> > +out_kzalloc:
> > + return ret;
> > +}
> > +
> > +static int ltc4261_remove(struct i2c_client *client)
> > +{
> > + struct ltc4261_data *data = i2c_get_clientdata(client);
> > +
> > + hwmon_device_unregister(data->hwmon_dev);
> > + sysfs_remove_group(&client->dev.kobj, <c4261_group);
> > +
> > + kfree(data);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct i2c_device_id ltc4261_id[] = {
> > + {"ltc4261", 0},
> > + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, ltc4261_id);
> > +
> > +/* This is the driver that will be inserted */
> > +static struct i2c_driver ltc4261_driver = {
> > + .driver = {
> > + .name = "ltc4261",
> > + },
> > + .probe = ltc4261_probe,
> > + .remove = ltc4261_remove,
> > + .id_table = ltc4261_id,
> > +};
> > +
I'd align the equal signs, like the ltc4245 driver does. No strong
preference, though. Like this:
static struct i2c_driver ltc4261_driver = {
.driver = {
.name = "ltc4261",
},
.probe = ltc4261_probe,
.remove = ltc4261_remove,
.id_table = ltc4261_id,
};
> > +static int __init ltc4261_init(void)
> > +{
> > + return i2c_add_driver(<c4261_driver);
> > +}
> > +
> > +static void __exit ltc4261_exit(void)
> > +{
> > + i2c_del_driver(<c4261_driver);
> > +}
> > +
> > +MODULE_AUTHOR("Guenter Roeck <guenter.roeck@ericsson.com>");
> > +MODULE_DESCRIPTION("LTC4261 driver");
> > +MODULE_LICENSE("GPL");
> > +
> > +module_init(ltc4261_init);
> > +module_exit(ltc4261_exit);
>
>
> --
> Jean Delvare
>
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [lm-sensors] [PATCH] hwmon: LTC4261 Hardware monitoring driver
2010-09-18 4:32 [lm-sensors] [PATCH] hwmon: LTC4261 Hardware monitoring driver Guenter Roeck
` (3 preceding siblings ...)
2010-10-03 21:17 ` Ira W. Snyder
@ 2010-10-04 1:18 ` Guenter Roeck
2010-10-04 6:59 ` Jean Delvare
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2010-10-04 1:18 UTC (permalink / raw)
To: lm-sensors
Hi Ira,
On Sun, Oct 03, 2010 at 05:17:42PM -0400, Ira W. Snyder wrote:
> On Sun, Oct 03, 2010 at 06:29:34PM +0200, Jean Delvare wrote:
> > On Fri, 17 Sep 2010 21:32:41 -0700, Guenter Roeck wrote:
> > > This driver adds support for Linear Technology LTC4261 I2C Negative
> > > Voltage Hot Swap Controller.
> > >
> > > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> >
> > Ira, you are familiar with Linear Technology hardware monitoring
> > devices, would you mind reviewing this driver? Thanks.
> >
>
> Sure, not a problem.
>
> Guenter, there are only a few minor nitpicks below. Please fix them and
> then add this after your Signed-off-by line:
> Reviewed-by: Ira W. Snyder <iws@ovro.caltech.edu>
>
> Overall, the driver looks ready for inclusion.
>
Thanks a lot - feedback inline
> Ira
[...]
>
> > > +
> > > +/* chip registers */
> > > +#define LTC4261_STATUS 0x00 /* readonly */
> > > +#define LTC4261_FAULT 0x01
> > > +#define LTC4261_ALERT 0x02
> > > +#define LTC4261_CONTROL 0x03
> > > +#define LTC4261_SENSE_H 0x04
> > > +#define LTC4261_SENSE_L 0x05
> > > +#define LTC4261_ADIN2_H 0x06
> > > +#define LTC4261_ADIN2_L 0x07
> > > +#define LTC4261_ADIN_H 0x08
> > > +#define LTC4261_ADIN_L 0x09
> > > +
>
> Tabs after #define here should be spaces. Keep the tabs just before the
> numbers to keep things lined up. You got it right on the fault register
> bits below.
>
Yes, I know. Always happens to me. Fixed that already right after I submitted the patch.
> > > +/*
> > > + * Fault register bits
> > > + */
> > > +#define FAULT_OV (1<<0)
> > > +#define FAULT_UV (1<<1)
> > > +#define FAULT_OC (1<<2)
> > > +
> > > +struct ltc4261_data {
> > > + struct device *hwmon_dev;
> > > +
> > > + struct mutex update_lock;
> > > + bool valid;
> > > + unsigned long last_updated; /* in jiffies */
> > > +
> > > + /* Registers */
> > > + u8 regs[10];
> > > +};
> > > +
> > > +static struct ltc4261_data *ltc4261_update_device(struct device *dev)
> > > +{
> > > + struct i2c_client *client = to_i2c_client(dev);
> > > + struct ltc4261_data *data = i2c_get_clientdata(client);
> > > + struct ltc4261_data *ret = data;
> > > +
> > > + mutex_lock(&data->update_lock);
> > > +
> > > + if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
>
> The datasheet claims that registers E-J (SENSE and ADIN/OV registers)
> are updated at 7.3 Hz. See page 21: Data Converter. A 1 Hz update rate
> is awfully slow for an application that wants to do fast monitoring. I
> think (HZ / 4) or (HZ / 6) would be better.
>
Makes sense. I'll use HZ/4.
> > > + int i;
> > > +
> > > + /* Read registers -- 0x00 to 0x09 */
> > > + for (i = 0; i < ARRAY_SIZE(data->regs); i++) {
> > > + int val;
> > > +
>
> I'd move the "int i" and "int val" variable declarations to the top of
> the function. I don't have a strong preference, though.
>
Common tendency is that a variable should be defined in the innermost block
of its use. I know I am not always strict with that rule, but try to follow
the idea.
[...]
> > > +MODULE_DEVICE_TABLE(i2c, ltc4261_id);
> > > +
> > > +/* This is the driver that will be inserted */
> > > +static struct i2c_driver ltc4261_driver = {
> > > + .driver = {
> > > + .name = "ltc4261",
> > > + },
> > > + .probe = ltc4261_probe,
> > > + .remove = ltc4261_remove,
> > > + .id_table = ltc4261_id,
> > > +};
> > > +
>
> I'd align the equal signs, like the ltc4245 driver does. No strong
> preference, though. Like this:
>
> static struct i2c_driver ltc4261_driver = {
> .driver = {
> .name = "ltc4261",
> },
> .probe = ltc4261_probe,
> .remove = ltc4261_remove,
> .id_table = ltc4261_id,
> };
>
The formatting is the result of Lindent, which doesn't seem to like
the extra blanks. In questions like this - where much of it is
personal preference - I prefer to go with the Lindent results.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [lm-sensors] [PATCH] hwmon: LTC4261 Hardware monitoring driver
2010-09-18 4:32 [lm-sensors] [PATCH] hwmon: LTC4261 Hardware monitoring driver Guenter Roeck
` (4 preceding siblings ...)
2010-10-04 1:18 ` Guenter Roeck
@ 2010-10-04 6:59 ` Jean Delvare
2010-10-04 15:21 ` Ira W. Snyder
2010-10-04 15:42 ` Guenter Roeck
7 siblings, 0 replies; 9+ messages in thread
From: Jean Delvare @ 2010-10-04 6:59 UTC (permalink / raw)
To: lm-sensors
On Sun, 3 Oct 2010 18:18:07 -0700, Guenter Roeck wrote:
> Hi Ira,
>
> On Sun, Oct 03, 2010 at 05:17:42PM -0400, Ira W. Snyder wrote:
> > > +MODULE_DEVICE_TABLE(i2c, ltc4261_id);
> > > +
> > > +/* This is the driver that will be inserted */
> > > +static struct i2c_driver ltc4261_driver = {
> > > + .driver = {
> > > + .name = "ltc4261",
> > > + },
> > > + .probe = ltc4261_probe,
> > > + .remove = ltc4261_remove,
> > > + .id_table = ltc4261_id,
> > > +};
> > > +
> >
> > I'd align the equal signs, like the ltc4245 driver does. No strong
> > preference, though. Like this:
> >
> > static struct i2c_driver ltc4261_driver = {
> > .driver = {
> > .name = "ltc4261",
> > },
> > .probe = ltc4261_probe,
> > .remove = ltc4261_remove,
> > .id_table = ltc4261_id,
> > };
> >
> The formatting is the result of Lindent, which doesn't seem to like
> the extra blanks. In questions like this - where much of it is
> personal preference - I prefer to go with the Lindent results.
FWIW, I've never used Lindent. I only consider it as a tool for
converting really ugly-formatted code to acceptably-formatted code, not
as the perfect formatting script.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [lm-sensors] [PATCH] hwmon: LTC4261 Hardware monitoring driver
2010-09-18 4:32 [lm-sensors] [PATCH] hwmon: LTC4261 Hardware monitoring driver Guenter Roeck
` (5 preceding siblings ...)
2010-10-04 6:59 ` Jean Delvare
@ 2010-10-04 15:21 ` Ira W. Snyder
2010-10-04 15:42 ` Guenter Roeck
7 siblings, 0 replies; 9+ messages in thread
From: Ira W. Snyder @ 2010-10-04 15:21 UTC (permalink / raw)
To: lm-sensors
On Sun, Oct 03, 2010 at 06:18:07PM -0700, Guenter Roeck wrote:
> Hi Ira,
>
> On Sun, Oct 03, 2010 at 05:17:42PM -0400, Ira W. Snyder wrote:
> > On Sun, Oct 03, 2010 at 06:29:34PM +0200, Jean Delvare wrote:
> > > On Fri, 17 Sep 2010 21:32:41 -0700, Guenter Roeck wrote:
> > > > This driver adds support for Linear Technology LTC4261 I2C Negative
> > > > Voltage Hot Swap Controller.
> > > >
> > > > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > >
> > > Ira, you are familiar with Linear Technology hardware monitoring
> > > devices, would you mind reviewing this driver? Thanks.
> > >
> >
> > Sure, not a problem.
> >
> > Guenter, there are only a few minor nitpicks below. Please fix them and
> > then add this after your Signed-off-by line:
> > Reviewed-by: Ira W. Snyder <iws@ovro.caltech.edu>
> >
> > Overall, the driver looks ready for inclusion.
> >
> Thanks a lot - feedback inline
>
> > Ira
>
> [...]
> >
> > > > +
> > > > +/* chip registers */
> > > > +#define LTC4261_STATUS 0x00 /* readonly */
> > > > +#define LTC4261_FAULT 0x01
> > > > +#define LTC4261_ALERT 0x02
> > > > +#define LTC4261_CONTROL 0x03
> > > > +#define LTC4261_SENSE_H 0x04
> > > > +#define LTC4261_SENSE_L 0x05
> > > > +#define LTC4261_ADIN2_H 0x06
> > > > +#define LTC4261_ADIN2_L 0x07
> > > > +#define LTC4261_ADIN_H 0x08
> > > > +#define LTC4261_ADIN_L 0x09
> > > > +
> >
> > Tabs after #define here should be spaces. Keep the tabs just before the
> > numbers to keep things lined up. You got it right on the fault register
> > bits below.
> >
> Yes, I know. Always happens to me. Fixed that already right after I submitted the patch.
>
> > > > +/*
> > > > + * Fault register bits
> > > > + */
> > > > +#define FAULT_OV (1<<0)
> > > > +#define FAULT_UV (1<<1)
> > > > +#define FAULT_OC (1<<2)
> > > > +
> > > > +struct ltc4261_data {
> > > > + struct device *hwmon_dev;
> > > > +
> > > > + struct mutex update_lock;
> > > > + bool valid;
> > > > + unsigned long last_updated; /* in jiffies */
> > > > +
> > > > + /* Registers */
> > > > + u8 regs[10];
> > > > +};
> > > > +
> > > > +static struct ltc4261_data *ltc4261_update_device(struct device *dev)
> > > > +{
> > > > + struct i2c_client *client = to_i2c_client(dev);
> > > > + struct ltc4261_data *data = i2c_get_clientdata(client);
> > > > + struct ltc4261_data *ret = data;
> > > > +
> > > > + mutex_lock(&data->update_lock);
> > > > +
> > > > + if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
> >
> > The datasheet claims that registers E-J (SENSE and ADIN/OV registers)
> > are updated at 7.3 Hz. See page 21: Data Converter. A 1 Hz update rate
> > is awfully slow for an application that wants to do fast monitoring. I
> > think (HZ / 4) or (HZ / 6) would be better.
> >
> Makes sense. I'll use HZ/4.
>
> > > > + int i;
> > > > +
> > > > + /* Read registers -- 0x00 to 0x09 */
> > > > + for (i = 0; i < ARRAY_SIZE(data->regs); i++) {
> > > > + int val;
> > > > +
> >
> > I'd move the "int i" and "int val" variable declarations to the top of
> > the function. I don't have a strong preference, though.
> >
> Common tendency is that a variable should be defined in the innermost block
> of its use. I know I am not always strict with that rule, but try to follow
> the idea.
>
> [...]
>
> > > > +MODULE_DEVICE_TABLE(i2c, ltc4261_id);
> > > > +
> > > > +/* This is the driver that will be inserted */
> > > > +static struct i2c_driver ltc4261_driver = {
> > > > + .driver = {
> > > > + .name = "ltc4261",
> > > > + },
> > > > + .probe = ltc4261_probe,
> > > > + .remove = ltc4261_remove,
> > > > + .id_table = ltc4261_id,
> > > > +};
> > > > +
> >
> > I'd align the equal signs, like the ltc4245 driver does. No strong
> > preference, though. Like this:
> >
> > static struct i2c_driver ltc4261_driver = {
> > .driver = {
> > .name = "ltc4261",
> > },
> > .probe = ltc4261_probe,
> > .remove = ltc4261_remove,
> > .id_table = ltc4261_id,
> > };
> >
> The formatting is the result of Lindent, which doesn't seem to like
> the extra blanks. In questions like this - where much of it is
> personal preference - I prefer to go with the Lindent results.
>
For both this and the variable declarations above, I don't have a strong
preference. They're just things I would personally do differently. Like
Jean commented below, I don't use Lindent either. Please still add my
Reviewed-by on the next version. You've fixed the stuff I was concerned
about.
Thanks,
Ira
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [lm-sensors] [PATCH] hwmon: LTC4261 Hardware monitoring driver
2010-09-18 4:32 [lm-sensors] [PATCH] hwmon: LTC4261 Hardware monitoring driver Guenter Roeck
` (6 preceding siblings ...)
2010-10-04 15:21 ` Ira W. Snyder
@ 2010-10-04 15:42 ` Guenter Roeck
7 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2010-10-04 15:42 UTC (permalink / raw)
To: lm-sensors
On Mon, 2010-10-04 at 11:21 -0400, Ira W. Snyder wrote:
> On Sun, Oct 03, 2010 at 06:18:07PM -0700, Guenter Roeck wrote:
> > Hi Ira,
> >
> > On Sun, Oct 03, 2010 at 05:17:42PM -0400, Ira W. Snyder wrote:
> > > On Sun, Oct 03, 2010 at 06:29:34PM +0200, Jean Delvare wrote:
> > > > On Fri, 17 Sep 2010 21:32:41 -0700, Guenter Roeck wrote:
> > > > > This driver adds support for Linear Technology LTC4261 I2C Negative
> > > > > Voltage Hot Swap Controller.
> > > > >
> > > > > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > > >
> > > > Ira, you are familiar with Linear Technology hardware monitoring
> > > > devices, would you mind reviewing this driver? Thanks.
> > > >
> > >
> > > Sure, not a problem.
> > >
> > > Guenter, there are only a few minor nitpicks below. Please fix them and
> > > then add this after your Signed-off-by line:
> > > Reviewed-by: Ira W. Snyder <iws@ovro.caltech.edu>
> > >
> > > Overall, the driver looks ready for inclusion.
> > >
> > Thanks a lot - feedback inline
> >
> > > Ira
> >
> > [...]
> > >
> > > > > +
> > > > > +/* chip registers */
> > > > > +#define LTC4261_STATUS 0x00 /* readonly */
> > > > > +#define LTC4261_FAULT 0x01
> > > > > +#define LTC4261_ALERT 0x02
> > > > > +#define LTC4261_CONTROL 0x03
> > > > > +#define LTC4261_SENSE_H 0x04
> > > > > +#define LTC4261_SENSE_L 0x05
> > > > > +#define LTC4261_ADIN2_H 0x06
> > > > > +#define LTC4261_ADIN2_L 0x07
> > > > > +#define LTC4261_ADIN_H 0x08
> > > > > +#define LTC4261_ADIN_L 0x09
> > > > > +
> > >
> > > Tabs after #define here should be spaces. Keep the tabs just before the
> > > numbers to keep things lined up. You got it right on the fault register
> > > bits below.
> > >
> > Yes, I know. Always happens to me. Fixed that already right after I submitted the patch.
> >
> > > > > +/*
> > > > > + * Fault register bits
> > > > > + */
> > > > > +#define FAULT_OV (1<<0)
> > > > > +#define FAULT_UV (1<<1)
> > > > > +#define FAULT_OC (1<<2)
> > > > > +
> > > > > +struct ltc4261_data {
> > > > > + struct device *hwmon_dev;
> > > > > +
> > > > > + struct mutex update_lock;
> > > > > + bool valid;
> > > > > + unsigned long last_updated; /* in jiffies */
> > > > > +
> > > > > + /* Registers */
> > > > > + u8 regs[10];
> > > > > +};
> > > > > +
> > > > > +static struct ltc4261_data *ltc4261_update_device(struct device *dev)
> > > > > +{
> > > > > + struct i2c_client *client = to_i2c_client(dev);
> > > > > + struct ltc4261_data *data = i2c_get_clientdata(client);
> > > > > + struct ltc4261_data *ret = data;
> > > > > +
> > > > > + mutex_lock(&data->update_lock);
> > > > > +
> > > > > + if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
> > >
> > > The datasheet claims that registers E-J (SENSE and ADIN/OV registers)
> > > are updated at 7.3 Hz. See page 21: Data Converter. A 1 Hz update rate
> > > is awfully slow for an application that wants to do fast monitoring. I
> > > think (HZ / 4) or (HZ / 6) would be better.
> > >
> > Makes sense. I'll use HZ/4.
> >
> > > > > + int i;
> > > > > +
> > > > > + /* Read registers -- 0x00 to 0x09 */
> > > > > + for (i = 0; i < ARRAY_SIZE(data->regs); i++) {
> > > > > + int val;
> > > > > +
> > >
> > > I'd move the "int i" and "int val" variable declarations to the top of
> > > the function. I don't have a strong preference, though.
> > >
> > Common tendency is that a variable should be defined in the innermost block
> > of its use. I know I am not always strict with that rule, but try to follow
> > the idea.
> >
> > [...]
> >
> > > > > +MODULE_DEVICE_TABLE(i2c, ltc4261_id);
> > > > > +
> > > > > +/* This is the driver that will be inserted */
> > > > > +static struct i2c_driver ltc4261_driver = {
> > > > > + .driver = {
> > > > > + .name = "ltc4261",
> > > > > + },
> > > > > + .probe = ltc4261_probe,
> > > > > + .remove = ltc4261_remove,
> > > > > + .id_table = ltc4261_id,
> > > > > +};
> > > > > +
> > >
> > > I'd align the equal signs, like the ltc4245 driver does. No strong
> > > preference, though. Like this:
> > >
> > > static struct i2c_driver ltc4261_driver = {
> > > .driver = {
> > > .name = "ltc4261",
> > > },
> > > .probe = ltc4261_probe,
> > > .remove = ltc4261_remove,
> > > .id_table = ltc4261_id,
> > > };
> > >
> > The formatting is the result of Lindent, which doesn't seem to like
> > the extra blanks. In questions like this - where much of it is
> > personal preference - I prefer to go with the Lindent results.
> >
>
> For both this and the variable declarations above, I don't have a strong
> preference. They're just things I would personally do differently. Like
> Jean commented below, I don't use Lindent either. Please still add my
> Reviewed-by on the next version. You've fixed the stuff I was concerned
> about.
>
Thanks!
As for Lindent ... working in an environment where everyone insists in a
personal coding style, I find it quite useful in helping to enforce a
common coding style without much argument about it. At the very least,
it establishes a common baseline. Of course, that only works if one
gives a good example.
Also, I don't take the output of Lindent as written in stone (sometimes
its output is a bit odd), but tend to rely on it as common baseline if
there is a disagreement.
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 9+ messages in thread