Le Wed, 1 Feb 2012 13:35:38 -0800, Guenter Roeck <guenter.roeck@xxxxxxxxxxxx> a écrit : > Hi Vivien, > > On Wed, 2012-02-01 at 16:05 -0500, Vivien Didelot wrote: > > Signed-off-by: Vivien Didelot <vivien.didelot@xxxxxxxxxxxxxxxxxxxx> > > Nicely done. Looks pretty good. Couple of minor comments below. > > > --- > > Documentation/hwmon/max197 | 54 ++++++ > > drivers/hwmon/Kconfig | 9 + > > drivers/hwmon/Makefile | 1 + > > drivers/hwmon/max197.c | 403 > > ++++++++++++++++++++++++++++++++++++++++++++ > > include/linux/max197.h | 22 +++ 5 files changed, 489 > > insertions(+), 0 deletions(-) create mode 100644 > > Documentation/hwmon/max197 create mode 100644 drivers/hwmon/max197.c > > create mode 100644 include/linux/max197.h > > > > diff --git a/Documentation/hwmon/max197 b/Documentation/hwmon/max197 > > new file mode 100644 > > index 0000000..aa0934f > > --- /dev/null > > +++ b/Documentation/hwmon/max197 > > @@ -0,0 +1,54 @@ > > +Maxim MAX197 driver > > +=================== > > + > > +Author: > > + * Vivien Didelot <vivien.didelot@xxxxxxxxxxxxxxxxxxxx> > > + > > +Supported chips: > > + * Maxim MAX197 > > + Prefix: 'max197' > > + Datasheet: http://datasheets.maxim-ic.com/en/ds/MAX197.pdf > > + > > + * Maxim MAX199 > > + Prefix: 'max199' > > + Datasheet: http://datasheets.maxim-ic.com/en/ds/MAX199.pdf > > + > > +Description > > +----------- > > + > > +The A/D converters MAX197 and MAX199 are both 8-Channel, > > Multi-Range, 5V, +12-Bit DAS with 8+4 Bus Interface and Fault > > Protection. + > > +The available ranges for the MAX197 are {0,-5V} to 5V, and > > {0,-10V} to 10V, +while they are {0,-2V} to 2V, and {0,-4V} to 4V > > on the MAX199. + > > +Platform data > > +------------- > > + > > +The MAX197 platform data (defined in linux/max197.h) should be > > filled with two +function pointers: > > + > > +* start: > > + The function which writes the control byte to start a new > > conversion. +* read: > > + The function used to read the raw value from the chip. > > + > > +Control-Byte Format: > > + > > +Bit Name Description > > +7,6 PD1,PD0 Clock and Power-Down modes > > +5 ACQMOD Internal or External Controlled Acquisition > > +4 RNG Full-scale voltage magnitude at the input > > +3 BIP Unipolar or Bipolar conversion mode > > +2,1,0 A2,A1,A0 Channel > > + > > +Sysfs interface > > +--------------- > > + > > +* in[0-7]_input: The conversion value for the corresponding > > channel. +* in[0-7]_min: The lower limit (in mV) for the > > corresponding channel. > > + It can be one value in -10000, -5000, -4000, > > -2000, 0, > > + depending on the chip. > > +* in[0-7]_max: The higher limit (in mV) for the corresponding > > channel. > > + It can be one value in 2000, 4000, 5000, 10000, > > + depending on the chip. > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > > index 91be41f..ccdf59b 100644 > > --- a/drivers/hwmon/Kconfig > > +++ b/drivers/hwmon/Kconfig > > @@ -1360,6 +1360,15 @@ config SENSORS_MC13783_ADC > > help > > Support for the A/D converter on MC13783 PMIC. > > > > +config SENSORS_MAX197 > > + tristate "Maxim MAX197 and compatibles." > > + help > > + If you say yes here you get support for the Maxim MAX197, > > + and MAX199 A/D converters. > > + > > + This driver can also be built as a module. If so, the > > module > > + will be called max197. > > + > > if ACPI > > > > comment "ACPI drivers" > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > > index 8251ce8..dfb73d9 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_MAX197) += max197.o > > > > obj-$(CONFIG_PMBUS) += pmbus/ > > > > diff --git a/drivers/hwmon/max197.c b/drivers/hwmon/max197.c > > new file mode 100644 > > index 0000000..985615c > > --- /dev/null > > +++ b/drivers/hwmon/max197.c > > @@ -0,0 +1,403 @@ > > +/* > > + * Maxim MAX197 A/D Converter Driver > > + * > > + * Copyright (c) 2012 Savoir-faire Linux Inc. > > + * Vivien Didelot <vivien.didelot@xxxxxxxxxxxxxxxxxxxx> > > + * > > + * This program is free software; you can redistribute it and/or > > modify > > + * it under the terms of the GNU General Public License version 2 > > as > > + * published by the Free Software Foundation. > > + * > > + * For further information, see the Documentation/hwmon/max197 > > file. > > + */ > > + > > +#include <linux/init.h> > > +#include <linux/module.h> > > +#include <linux/kernel.h> > > +#include <linux/device.h> > > +#include <linux/delay.h> > > +#include <linux/platform_device.h> > > +#include <linux/err.h> > > +#include <linux/sysfs.h> > > +#include <linux/hwmon.h> > > +#include <linux/hwmon-sysfs.h> > > +#include <linux/mutex.h> > > +#include <linux/slab.h> > > +#include <linux/io.h> > > +#include <linux/max197.h> > > + > > +#define MAX199_LIMIT 4000 /* 4V */ > > +#define MAX197_LIMIT 10000 /* 10V */ > > + > > +#define MAX197_NUM_CH 8 /* 8 Analog Input Channels */ > > + > > +/* Control byte format */ > > +#define MAX197_A0 0x01 /* Channel bit 0 */ > > +#define MAX197_A1 0x02 /* Channel bit 1 */ > > +#define MAX197_A2 0x04 /* Channel bit 2 */ > > +#define MAX197_BIP 0x08 /* Bipolarity */ > > +#define MAX197_RNG 0x10 /* Full range */ > > +#define MAX197_ACQMOD 0x20 /* Internal/External controlled > > acquisition */ +#define MAX197_PD0 0x40 /* Clock/Power-Down > > modes bit 1 */ +#define MAX197_PD1 0x80 /* Clock/Power-Down > > modes bit 2 */ + > > +#define MAX197_SCALE 12207 /* Scale coefficient for raw data */ > > + > > +/** > > + * struct max197_chip - device instance specific data > > + * @pdata: Platform data. > > + * @hwmon_dev: The hwmon device. > > + * @lock: Read/Write mutex. > > + * @limit: Max range value (10V for MAX197, 4V for > > MAX199). > > + * @scale: Need to scale. > > + * @ctrl_bytes: Channels control byte. > > + */ > > +struct max197_chip { > > + struct max197_platform_data *pdata; > > + struct device *hwmon_dev; > > + struct mutex lock; > > + int limit; > > + bool scale; > > + u8 ctrl_bytes[MAX197_NUM_CH]; > > +}; > > + > > +static inline void max197_set_unipolarity(struct max197_chip > > *chip, int channel) +{ > > + chip->ctrl_bytes[channel] &= ~MAX197_BIP; > > +} > > + > > +static inline void max197_set_bipolarity(struct max197_chip *chip, > > int channel) +{ > > + chip->ctrl_bytes[channel] |= MAX197_BIP; > > +} > > + > > +static inline void max197_set_half_range(struct max197_chip *chip, > > int channel) +{ > > + chip->ctrl_bytes[channel] &= ~MAX197_RNG; > > +} > > + > > +static inline void max197_set_full_range(struct max197_chip *chip, > > int channel) +{ > > + chip->ctrl_bytes[channel] |= MAX197_RNG; > > +} > > + > > +static inline bool max197_is_bipolar(struct max197_chip *chip, int > > channel) +{ > > + return !!(chip->ctrl_bytes[channel] & MAX197_BIP); > > +} > > + > > +static inline bool max197_is_full_range(struct max197_chip *chip, > > int channel) +{ > > + return !!(chip->ctrl_bytes[channel] & MAX197_RNG); > > +} > > + > Interestingly enough, you don't need the !!() above. The C language > defines that the result of an operation assigned to a bool is always > converted to either 0 or 1. Not that it matters, really - I personally > actually prefer you style. Thanks for the tip, I'll get rid of it. > > > +/** > > + * max197_show_range() - Display range on user output > > + * > > + * Function called on read access on in{0,1,2,3,4,5,6,7}_{min,max} > > + */ > > +static ssize_t max197_show_range(struct device *dev, > > + struct device_attribute *devattr, > > char *buf) +{ > > + struct max197_chip *chip = dev_get_drvdata(dev); > > + struct sensor_device_attribute_2 *attr = > > to_sensor_dev_attr_2(devattr); > > + int channel = attr->index; > > + bool is_min = attr->nr; > > + int range; > > + > > + if (mutex_lock_interruptible(&chip->lock)) > > + return -ERESTARTSYS; > > + > > + range = max197_is_full_range(chip, channel) ? > > + chip->limit : chip->limit / 2; > > + if (is_min) { > > + if (max197_is_bipolar(chip, channel)) > > + range = -range; > > + else > > + range = 0; > > + } > > + > > + mutex_unlock(&chip->lock); > > + > > + return sprintf(buf, "%d\n", range); > > +} > > + > > +/** > > + * max197_store_range() - Write range from user input > > + * > > + * Function called on write access on in{0,1,2,3,4,5,6,7}_{min,max} > > + */ > > +static ssize_t max197_store_range(struct device *dev, > > + struct device_attribute *devattr, > > + const char *buf, size_t count) > > +{ > > + struct max197_chip *chip = dev_get_drvdata(dev); > > + struct sensor_device_attribute_2 *attr = > > to_sensor_dev_attr_2(devattr); > > + int channel = attr->index; > > + bool is_min = attr->nr; > > + long value; > > + int half = chip->limit / 2; > > + int full = chip->limit; > > + > > + if (kstrtol(buf, 10, &value)) > > + return -EINVAL; > > + > > + if (is_min) { > > + if ((value != 0) && (value != -half) && (value != > > -full)) > > + return -EINVAL; > > + } else { > > + if ((value != half) && (value != full)) > > + return -EINVAL; > > + } > > + > Technically ok, except for the unnecessary ( ). Would be great if you > could remove those. > > Since the actual limits are chip dependent, and not easily to figure > out for the ordinary user, it would be nice to either accept any > number and adjust it to one of the accepted values, or to explicitly > spell out the accepted the per-chip accepted values in the > documentation (and not just say "depending on the chip"). I'll leave > it up to you to decide which way to go. > > Not too difficult - if you change the code, something like > > if (is_min) { > if (value <= -full) > value = -full; > else if (value < 0) > value = -half; > else value = 0; > } else { > if (value >= full) > value = full; > else > value = half; > } It sounds better to accept any value and adjust it depending on the chip. > > would be good enough. > > Thanks, > Guenter > > BTW, about the TS-5500 ADC part, is a platform ts5500_adc.c file the better solution, or should the device be declared in the ts5500.c platform code? Thanks! -- Vivien Didelot Savoir-faire Linux Inc. Tel: (514) 276-5468 #149 _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
