On Wed, Jan 18, 2012 at 06:55:49PM -0800, Guenter Roeck wrote:
> On Wed, Jan 18, 2012 at 06:52:11PM -0500, Vivien Didelot wrote:
> > From: Jonas Fonseca <jonas.fonseca@xxxxxxxxxxxxxxxxxxxx>
> >
> > Signed-off-by: Jonas Fonseca <jonas.fonseca@xxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Vivien Didelot <vivien.didelot@xxxxxxxxxxxxxxxxxxxx>
> > ---
> > arch/x86/platform/ts5500/Kconfig | 7 +
> > arch/x86/platform/ts5500/Makefile | 1 +
> > arch/x86/platform/ts5500/ts5500_adc.c | 478 +++++++++++++++++++++++++++++++++
> > 3 files changed, 486 insertions(+), 0 deletions(-)
> > create mode 100644 arch/x86/platform/ts5500/ts5500_adc.c
> >
> I can not comment about the other drivers, but this is a hwmon driver, and thus should
> reside in drivers/hwmon and be reviewed and handled on the hwmon mailing list (copied).
>
Now I forgot that myself. Copying lm-sensors now.
> Couple of additional comments below; just some things I noticed, not a complete review.
>
> > diff --git a/arch/x86/platform/ts5500/Kconfig b/arch/x86/platform/ts5500/Kconfig
> > index 76f777f..f1a5f1d 100644
> > --- a/arch/x86/platform/ts5500/Kconfig
> > +++ b/arch/x86/platform/ts5500/Kconfig
> > @@ -20,3 +20,10 @@ config TS5500_LED
> > default y
> > help
> > This option enables support for the on-chip LED.
> > +
> > +config TS5500_ADC
> > + tristate "TS-5500 ADC"
> > + depends on TS5500 && HWMON=y
> > + default y
> > + help
> > + Support for the A/D converter on Technologic Systems TS-5500 SBCs.
> > diff --git a/arch/x86/platform/ts5500/Makefile b/arch/x86/platform/ts5500/Makefile
> > index 88eccc9..dcf46d8 100644
> > --- a/arch/x86/platform/ts5500/Makefile
> > +++ b/arch/x86/platform/ts5500/Makefile
> > @@ -1,3 +1,4 @@
> > obj-$(CONFIG_TS5500) += ts5500.o
> > obj-$(CONFIG_TS5500_GPIO) += ts5500_gpio.o
> > obj-$(CONFIG_TS5500_LED) += ts5500_led.o
> > +obj-$(CONFIG_TS5500_ADC) += ts5500_adc.o
> > diff --git a/arch/x86/platform/ts5500/ts5500_adc.c b/arch/x86/platform/ts5500/ts5500_adc.c
> > new file mode 100644
> > index 0000000..fc4560f
> > --- /dev/null
> > +++ b/arch/x86/platform/ts5500/ts5500_adc.c
> > @@ -0,0 +1,478 @@
> > +/*
> > + * Technologic Systems TS-5500 boards - Mapped MAX197 ADC driver
> > + *
> > + * Copyright (c) 2010-2012 Savoir-faire Linux Inc.
> > + * Jonas Fonseca <jonas.fonseca@xxxxxxxxxxxxxxxxxxxx>
> > + *
> > + * Portions Copyright (C) 2008 Marc Pignat <marc.pignat@xxxxxxx>
> > + *
> > + * The driver uses direct access for communication with the ADC.
> > + * Should work unchanged with the MAX199 chip.
> > + *
> > + * 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.
> > + *
> > + * The TS-5500 uses a CPLD to abstract the interface to a MAX197.
> > + */
> > +
> > +#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 "ts5500.h"
> > +
> > +#define TS5500_ADC_CTRL_REG 0x195 /* Conversion state register */
> > +#define TS5500_ADC_INIT_LSB_REG 0x196 /* Init conv. / LSB register */
> > +#define TS5500_ADC_MSB_REG 0x197 /* MSB register */
> > +/*
> > + * Control bits of A/D command
> > + * bits 0-2: selected channel (0 - 7)
> > + * bits 3: uni/bipolar (0 = unipolar (ie 0 to +5V))
> > + * (1 = bipolar (ie -5 to +5V))
> > + * bit 4: selected range (0 = 5v range, 1 = 10V range)
> > + * bit 5-7: padded zero (unused)
> > + */
> > +
> > +#define TS5500_ADC_CHANNELS_MAX 8 /* 0 to 7 channels on device */
> > +
> > +#define TS5500_ADC_BIPOLAR 0x08
> > +#define TS5500_ADC_UNIPOLAR 0x00
> > +#define TS5500_ADC_RANGE_5V 0x00 /* 0 to 5V range */
> > +#define TS5500_ADC_RANGE_10V 0x10 /* 0 to 10V range */
> > +
> > +#define TS5500_ADC_READ_DELAY 15 /* usec */
> > +#define TS5500_ADC_READ_BUSY_MASK 0x01
> > +#define TS5500_ADC_NAME "MAX197 (8 channels)"
> > +
> > +/**
> > + * struct ts5500_adc_platform_data
> > + * @name: Name of the device.
> > + * @ioaddr: I/O address containing:
> > + * .data: Data register for conversion reading.
> > + * .ctrl: A/D Control Register (bit 0 = 0 when
> > + * conversion completed).
> > + * @read: Information about conversion reading, with:
> > + * .delay: Delay before next conversion.
> > + * .busy_mask: Control register bit 0 equals 1 means
> > + * conversion is not completed yet.
> > + * @ctrl: Data tables addressable by [polarity][range].
> > + * @ranges: Ranges.
> > + * .min: Min value.
> > + * .max: Max value.
> > + * @scale: Polarity/Range coefficients to scale raw conversion reading.
> > + */
> > +struct ts5500_adc_platform_data {
> > + const char *name;
> > + struct {
> > + int data;
> > + int ctrl;
> > + } ioaddr;
> > + struct {
> > + u8 delay;
> > + u8 busy_mask;
> > + } read;
> > + u8 ctrl[2][2];
>
> const ?
>
> > + struct {
> > + int min[2][2];
> > + int max[2][2];
>
> Should those be const ?
>
> > + } ranges;
> > + int scale[2][2];
>
> const ?
>
> > +};
> > +
>
> I am a bit lost about this structure and its use. It appears as if you expect that
> there will be other uses besides the one defined here (otherwise the variables would
> not make much sense and you could use defines), yet it is all defined in this file
> and thus static. Please elaborate.
>
> > +#define ts5500_adc_test_bit(bit, map) (test_bit(bit, map) != 0)
> > +
> Why "!= 0" ? Isn't that implied ? And then why the define to start with ?
>
> > +/**
> > + * struct ts5500_adc_chip
> > + * @hwmon_dev: The hwmon device.
> > + * @lock: Read/Write mutex.
> > + * @spec: The mapped MAX197 platform data.
> > + * @polarity: bitmap for polarity.
> > + * @range: bitmap for range.
> > + */
> > +struct ts5500_adc_chip {
> > + struct device *hwmon_dev;
> > + struct mutex lock;
> > + struct ts5500_adc_platform_data spec;
> > + DECLARE_BITMAP(polarity, TS5500_ADC_CHANNELS_MAX);
> > + DECLARE_BITMAP(range, TS5500_ADC_CHANNELS_MAX);
> > +};
> > +
> > +static s32 ts5500_adc_scale(struct ts5500_adc_chip *chip, s16 raw,
> > + int polarity, int range)
> > +{
> > + s32 scaled = raw;
> > +
> > + scaled *= chip->spec.scale[polarity][range];
> > + scaled /= 10000;
> > +
> > + return scaled;
> > +}
> > +
> > +static int ts5500_adc_range(struct ts5500_adc_chip *chip, int is_min,
> > + int polarity, int range)
> > +{
> > + if (is_min)
> > + return chip->spec.ranges.min[polarity][range];
> > + return chip->spec.ranges.max[polarity][range];
> > +}
> > +
> > +static int ts5500_adc_strtol(const char *buf, long *value, int range1,
> > + int range2)
> > +{
> > + if (strict_strtol(buf, 10, value))
>
> checkpatch warning.
>
> > + return -EINVAL;
> > +
> > + if (range1 < range2)
> > + *value = SENSORS_LIMIT(*value, range1, range2);
> > + else
> > + *value = SENSORS_LIMIT(*value, range2, range1);
> > +
> > + return 0;
> > +}
> This function is called exactly once. Why not just embed it in the calling code ?
>
> > +
> > +static struct ts5500_adc_chip *ts5500_adc_get_drvdata(struct device *dev)
> > +{
> > + return platform_get_drvdata(to_platform_device(dev));
> > +}
> > +
> > +/**
> > + * ts5500_adc_show_range() - Display range on user output
> > + *
> > + * Function called on read access on
> > + * /sys/devices/platform/ts5500-adc/in{0,1,2,3,4,5,6,7}_{min,max}
> > + */
> > +static ssize_t ts5500_adc_show_range(struct device *dev,
> > + struct device_attribute *devattr, char *buf)
> > +{
> > + struct ts5500_adc_chip *chip = ts5500_adc_get_drvdata(dev);
> > + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> > + int is_min = attr->nr != 0;
> > + int polarity, range;
> > +
> > + if (mutex_lock_interruptible(&chip->lock))
> > + return -ERESTARTSYS;
> > +
> > + polarity = ts5500_adc_test_bit(attr->index, chip->polarity);
> > + range = ts5500_adc_test_bit(attr->index, chip->range);
> > +
> > + mutex_unlock(&chip->lock);
> > +
> > + return sprintf(buf, "%d\n",
> > + ts5500_adc_range(chip, is_min, polarity, range));
> > +}
> > +
> > +/**
> > + * ts5500_adc_store_range() - Write range from user input
> > + *
> > + * Function called on write access on
> > + * /sys/devices/platform/ts5500-adc/in{0,1,2,3,4,5,6,7}_{min,max}
> > + */
> > +static ssize_t ts5500_adc_store_range(struct device *dev,
> > + struct device_attribute *devattr,
> > + const char *buf, size_t count)
> > +{
> > + struct ts5500_adc_chip *chip = ts5500_adc_get_drvdata(dev);
> > + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> > + int is_min = attr->nr != 0;
> > + int range1 = ts5500_adc_range(chip, is_min, 0, 0);
> > + int range2 = ts5500_adc_range(chip, is_min, 1, 1);
> > + long value;
> > +
> > + if (ts5500_adc_strtol(buf, &value, range1, range2))
> > + return -EINVAL;
> > +
> > + if (mutex_lock_interruptible(&chip->lock))
> > + return -ERESTARTSYS;
> > +
> > + if (abs(value) > 5000)
> > + set_bit(attr->index, chip->range);
> > + else
> > + clear_bit(attr->index, chip->range);
> > +
> > + if (is_min) {
> > + if (value < 0)
> > + set_bit(attr->index, chip->polarity);
> > + else
> > + clear_bit(attr->index, chip->polarity);
> > + }
> > +
> > + mutex_unlock(&chip->lock);
> > +
> > + return count;
> > +}
> > +
> > +/**
> > + * ts5500_adc_show_input() - Show channel input
> > + *
> > + * Function called on read access on
> > + * /sys/devices/platform/ts5500-adc/in{0,1,2,3,4,5,6,7}_input
> > + */
> > +static ssize_t ts5500_adc_show_input(struct device *dev,
> > + struct device_attribute *devattr,
> > + char *buf)
> > +{
> > + struct ts5500_adc_chip *chip = ts5500_adc_get_drvdata(dev);
> > + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > + int polarity, range;
> > + int ret;
> > + u8 command;
> > +
> > + if (mutex_lock_interruptible(&chip->lock))
> > + return -ERESTARTSYS;
> > +
> > + polarity = ts5500_adc_test_bit(attr->index, chip->polarity);
> > + range = ts5500_adc_test_bit(attr->index, chip->range);
> > +
> > + command = attr->index | chip->spec.ctrl[polarity][range];
> > +
> > + outb(command, chip->spec.ioaddr.data);
> > +
> > + udelay(chip->spec.read.delay);
> > + ret = inb(chip->spec.ioaddr.ctrl);
> > +
> > + if (ret & chip->spec.read.busy_mask) {
> > + dev_err(dev, "device not ready (ret=0x0%x, try=%d)\n", ret,
> > + range);
>
> What does "try=" have to do with the displayed "range" ?
>
> > + ret = -EIO;
> > + } else {
> > + /* LSB of conversion is at 0x196 and MSB is at 0x197 */
> > + u8 lsb = inb(chip->spec.ioaddr.data);
> > + u8 msb = inb(chip->spec.ioaddr.data + 1);
> > + s16 raw = (msb << 8) | lsb;
> > + s32 scaled = ts5500_adc_scale(chip, raw, polarity, range);
> > +
> > + ret = sprintf(buf, "%d\n", scaled);
> > + }
> > +
> > + mutex_unlock(&chip->lock);
> > + return ret;
> > +}
> > +
> > +static ssize_t ts5500_adc_show_name(struct device *dev,
> > + struct device_attribute *devattr, char *buf)
> > +{
> > + return sprintf(buf, "%s\n", ts5500_adc_get_drvdata(dev)->spec.name);
> > +}
> > +
> > +#define TS5500_ADC_HWMON_CHANNEL(chan) \
> > + SENSOR_DEVICE_ATTR(in##chan##_input, S_IRUGO, \
> > + ts5500_adc_show_input, NULL, chan); \
> > + SENSOR_DEVICE_ATTR_2(in##chan##_max, S_IRUGO | S_IWUSR, \
> > + ts5500_adc_show_range, \
> > + ts5500_adc_store_range, 0, chan); \
> > + SENSOR_DEVICE_ATTR_2(in##chan##_min, S_IRUGO | S_IWUSR, \
> > + ts5500_adc_show_range, \
> > + ts5500_adc_store_range, 1, chan) \
> > +
> > +#define TS5500_ADC_SYSFS_CHANNEL(chan) \
> > + &sensor_dev_attr_in##chan##_input.dev_attr.attr, \
> > + &sensor_dev_attr_in##chan##_max.dev_attr.attr, \
> > + &sensor_dev_attr_in##chan##_min.dev_attr.attr
> > +
> > +static DEVICE_ATTR(name, S_IRUGO, ts5500_adc_show_name, NULL);
> > +
> > +static TS5500_ADC_HWMON_CHANNEL(0);
> > +static TS5500_ADC_HWMON_CHANNEL(1);
> > +static TS5500_ADC_HWMON_CHANNEL(2);
> > +static TS5500_ADC_HWMON_CHANNEL(3);
> > +static TS5500_ADC_HWMON_CHANNEL(4);
> > +static TS5500_ADC_HWMON_CHANNEL(5);
> > +static TS5500_ADC_HWMON_CHANNEL(6);
> > +static TS5500_ADC_HWMON_CHANNEL(7);
> > +
> Looks good at first glance, but unless I misunderstand something, each define generates
> three structures, yet only the first of those is declared static, the others are global.
>
> > +static const struct attribute_group ts5500_adc_sysfs_group = {
> > + .attrs = (struct attribute *[]) {
>
> checkpatch error.
>
> > + &dev_attr_name.attr,
> > + TS5500_ADC_SYSFS_CHANNEL(0),
> > + TS5500_ADC_SYSFS_CHANNEL(1),
> > + TS5500_ADC_SYSFS_CHANNEL(2),
> > + TS5500_ADC_SYSFS_CHANNEL(3),
> > + TS5500_ADC_SYSFS_CHANNEL(4),
> > + TS5500_ADC_SYSFS_CHANNEL(5),
> > + TS5500_ADC_SYSFS_CHANNEL(6),
> > + TS5500_ADC_SYSFS_CHANNEL(7),
> > + NULL
> > + }
> > +};
> > +
> > +static int __devinit ts5500_adc_probe(struct platform_device *pdev)
> > +{
> > + struct ts5500_adc_platform_data *pdata = pdev->dev.platform_data;
> > + struct ts5500_adc_chip *chip;
> > + int ret;
> > +
> > + if (pdata == NULL)
> > + return -ENODEV;
> > +
> > + chip = kzalloc(sizeof *chip, GFP_KERNEL);
> > + if (!chip)
> > + return -ENOMEM;
> > +
> > + chip->spec = *pdata;
> > +
> > + mutex_init(&chip->lock);
> > + mutex_lock(&chip->lock);
>
> Probe function does not need a lock if you rearrange the code below.
>
> > +
> > + ret = sysfs_create_group(&pdev->dev.kobj, &ts5500_adc_sysfs_group);
> > + if (ret) {
> > + dev_err(&pdev->dev, "sysfs_create_group failed.\n");
> > + goto error_unlock_and_free;
> > + }
> > +
> > + chip->hwmon_dev = hwmon_device_register(&pdev->dev);
> > + if (IS_ERR(chip->hwmon_dev)) {
> > + dev_err(&pdev->dev, "hwmon_device_register failed.\n");
> > + ret = PTR_ERR(chip->hwmon_dev);
> > + goto error_unregister_device;
> > + }
> > +
> > + platform_set_drvdata(pdev, chip);
>
> Set this before creating sysfs entries.
>
> > + mutex_unlock(&chip->lock);
> > + return 0;
> > +
> > +error_unregister_device:
> > + sysfs_remove_group(&pdev->dev.kobj, &ts5500_adc_sysfs_group);
> > +
> > +error_unlock_and_free:
> > + mutex_unlock(&chip->lock);
> > + kfree(chip);
> > + return ret;
> > +}
> > +
> > +static void ts5500_adc_release(struct device *dev)
> > +{
> > + /* noop */
> > +}
>
> Is this really needed ? Just wondering.
>
> > +
> > +static struct resource ts5500_adc_resources[] = {
> > + {
> > + .name = "ts5500_adc" "-data",
> > + .start = TS5500_ADC_INIT_LSB_REG,
> > + .end = TS5500_ADC_MSB_REG,
> > + .flags = IORESOURCE_IO,
> > + },
> > + {
> > + .name = "ts5500_adc" "-ctrl",
> > + .start = TS5500_ADC_CTRL_REG,
> > + .end = TS5500_ADC_CTRL_REG,
> > + .flags = IORESOURCE_IO,
> > + }
> > +};
> > +
> > +static struct ts5500_adc_platform_data ts5500_adc_platform_data = {
> > + .name = TS5500_ADC_NAME,
> > + .ioaddr = {
> > + .data = TS5500_ADC_INIT_LSB_REG,
> > + .ctrl = TS5500_ADC_CTRL_REG,
> > + },
> > + .read = {
> > + .delay = TS5500_ADC_READ_DELAY,
> > + .busy_mask = TS5500_ADC_READ_BUSY_MASK,
> > + },
> > + .ctrl = {
> > + { TS5500_ADC_UNIPOLAR | TS5500_ADC_RANGE_5V,
> > + TS5500_ADC_UNIPOLAR | TS5500_ADC_RANGE_10V },
> > + { TS5500_ADC_BIPOLAR | TS5500_ADC_RANGE_5V,
> > + TS5500_ADC_BIPOLAR | TS5500_ADC_RANGE_10V },
> > + },
> > + .ranges = {
> > + .min = {
> > + { 0, 0 },
> > + { -5000, -10000 },
> > + },
> > + .max = {
> > + { 5000, 10000 },
> > + { 5000, 10000 },
> > + },
> > + },
> > + .scale = {
> > + { 12207, 24414 },
> > + { 24414, 48828 },
> > + },
> > +};
> > +
> > +static struct platform_device ts5500_adc_device = {
> > + .name = "ts5500_adc",
> > + .id = -1,
> > + .resource = ts5500_adc_resources,
> > + .num_resources = ARRAY_SIZE(ts5500_adc_resources),
> > + .dev = {
> > + .platform_data = &ts5500_adc_platform_data,
> > + .release = ts5500_adc_release,
> > + },
> > +};
> > +
> Usually all the above would be in a platform file.
>
> > +static int __devexit ts5500_adc_remove(struct platform_device *pdev)
> > +{
> > + struct ts5500_adc_chip *chip = platform_get_drvdata(pdev);
> > +
> > + mutex_lock(&chip->lock);
> > + hwmon_device_unregister(chip->hwmon_dev);
> > + sysfs_remove_group(&pdev->dev.kobj, &ts5500_adc_sysfs_group);
> > + platform_set_drvdata(pdev, NULL);
> > + mutex_unlock(&chip->lock);
>
> Lock not needed here.
>
> > + kfree(chip);
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver ts5500_adc_driver = {
> > + .driver = {
> > + .name = "ts5500_adc",
> > + .owner = THIS_MODULE,
> > + },
> > + .probe = ts5500_adc_probe,
> > + .remove = __devexit_p(ts5500_adc_remove)
> > +};
> > +
> > +static int __init ts5500_adc_init(void)
> > +{
> > + int ret;
> > +
> > + ret = platform_driver_register(&ts5500_adc_driver);
> > + if (ret)
> > + goto error_out;
> > +
> > + ret = platform_device_register(&ts5500_adc_device);
> > + if (ret)
> > + goto error_device_register;
> > +
> > + return 0;
> > +
> > +error_device_register:
> > + platform_driver_unregister(&ts5500_adc_driver);
> > +error_out:
> > + return ret;
> > +}
> > +module_init(ts5500_adc_init);
> > +
> > +static void __exit ts5500_adc_exit(void)
> > +{
> > + platform_driver_unregister(&ts5500_adc_driver);
> > + platform_device_unregister(&ts5500_adc_device);
> > +}
> > +module_exit(ts5500_adc_exit);
> > +
> > +MODULE_DESCRIPTION("TS-5500 mapped MAX197 ADC device driver");
> > +MODULE_AUTHOR("Jonas Fonseca <jonas.fonseca@xxxxxxxxxxxxxxxxxxxx>");
> > +MODULE_LICENSE("GPL");
> > --
> > 1.7.6.5
> >
> >
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
