Re: [PATCH v4 4/4] x86/platform: (TS-5500) add ADC support

[Guenter Roeck]

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
> > 
> > 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/