[lm-sensors] HWMON: S3C24XX series ADC driver

[lm-sensors] HWMON: S3C24XX series ADC driver

[Ben Dooks]

Add support for the ADC controller on the S3C
series of processors to drivers/hwmon for use with
hardware monitoring systems.

Signed-off-by: Ben Dooks <ben@simtec.co.uk>

Index: linux.git/drivers/hwmon/Kconfig
=================================--- linux.git.orig/drivers/hwmon/Kconfig	2009-05-20 22:02:06.000000000 +0100
+++ linux.git/drivers/hwmon/Kconfig	2009-05-20 22:02:43.000000000 +0100
@@ -702,6 +702,16 @@ config SENSORS_SHT15
 	  This driver can also be built as a module.  If so, the module
 	  will be called sht15.
 
+config SENSORS_S3C_ADC
+	tristate "S3C24XX/S3C64XX Inbuilt ADC"
+	depends on HWMON && (ARCH_S3C2410 || ARCH_S3C64XX)
+	help
+	  If you say yes here you get support for the on-board ADCs of
+	  the Samsung S3C24XX or S3C64XX series of SoC
+
+	  This driver can also be built as a module. If so, the module
+	  will be called s3c-adc.
+
 config SENSORS_SIS5595
 	tristate "Silicon Integrated Systems Corp. SiS5595"
 	depends on PCI
Index: linux.git/drivers/hwmon/Makefile
=================================--- linux.git.orig/drivers/hwmon/Makefile	2009-05-20 22:02:06.000000000 +0100
+++ linux.git/drivers/hwmon/Makefile	2009-05-20 22:02:43.000000000 +0100
@@ -76,6 +76,7 @@ obj-$(CONFIG_SENSORS_MAX6650)	+= max6650
 obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
 obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
+obj-$(CONFIG_SENSORS_S3C_ADC)	+= s3c-adc.o
 obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
 obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595.o
 obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
Index: linux.git/drivers/hwmon/s3c-adc.c
=================================--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux.git/drivers/hwmon/s3c-adc.c	2009-05-20 22:20:32.000000000 +0100
@@ -0,0 +1,371 @@
+/* linux/drivers/hwmon/s3c-adc.c
+ *
+ * Copyright (C) 2005, 2008, 2009 Simtec Electronics
+ *	http://armlinux.simtec.co.uk/
+ *	Ben Dooks <ben@simtec.co.uk>
+ *
+ * S3C24XX/S3C64XX ADC hwmon support
+ *
+ * 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.
+ *
+ * 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., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+*/
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+
+#include <plat/adc.h>
+#include <plat/hwmon.h>
+
+/**
+ * struct s3c_hwmon - ADC hwmon client information
+ * @lock: Access lock for conversion.
+ * @wait: Wait queue for conversions to complete.
+ * @client: The client we registered with the S3C ADC core.
+ * @dev: The platform device we bound to.
+ * @hwmon_dev: The hwmon device we created.
+ * @in_attr: The device attributes we created.
+*/
+struct s3c_hwmon {
+	struct semaphore	lock;
+	wait_queue_head_t	wait;
+	int			val;
+
+	struct s3c_hwmon_client	*client;
+	struct platform_device	*dev;
+	struct device		*hwmon_dev;
+
+	struct sensor_device_attribute *in_attr[8];
+};
+
+static inline struct s3c_hwmon *dev_to_ourhwmon(struct platform_device *dev)
+{
+	return (struct s3c_hwmon *)platform_get_drvdata(dev);
+}
+
+static struct s3c_hwmon *done_adc;
+
+/**
+ * s3c_hwmon_adcdone - ADC core callback
+ * @value: The value that we got from the ADC core
+ * @ignore: Only used for the touchscreen client.
+ * @left: The number of conversions left (not used here).
+ *
+ * This is called when the ADC has finished its conversion to
+ * inform us of the result.
+ */
+static void s3c_hwmon_adcdone(unsigned value, unsigned ignore, unsigned *left)
+{
+	struct s3c_hwmon *adc = done_adc;
+
+	hwmon->val = value;
+	wake_up(&hwmon->wait);
+}
+
+/**
+ * s3c_hwmon_read_ch - read a value from a given adc channel.
+ * @hwmon: Our state.
+ * @channel: The channel we're reading from.
+ *
+ * Read a value from the @channel with the proper locking and sleep until
+ * either the read completes or we timeout awaiting the ADC core to get
+ * back to us.
+ */
+static int s3c_hwmon_read_ch(struct s3c_hwmon *hwmon, int channel)
+{
+	unsigned long timeout;
+	int ret;
+
+	ret = down_interruptible(&hwmon->lock);
+	if (ret < 0)
+		return ret;
+
+	dev_dbg(&hwmon->dev->dev, "reading channel %d\n", channel);
+
+	hwmon->val = -1;
+	done_adc = adc;
+
+	ret = s3c_adc_start(hwmon->client, channel, 1);
+	if (ret < 0)
+		goto err;
+
+	timeout = wait_event_timeout(hwmon->wait, hwmon->val >= 0, HZ / 2);
+	ret = (timeout = 0) ? -ETIMEDOUT : hwmon->val;
+
+ err:
+	up(&hwmon->lock);
+	return ret;
+}
+
+/**
+ * s3c_hwmon_show_raw - show a conversion from the raw channel number.
+ * @dev: The device that the attribute belongs to.
+ * @attr: The attribute being read.
+ * @buf: The result buffer.
+ *
+ * This show deals with the raw attribute, registered for each possible
+ * ADC channel. This does a conversion and returns the raw (un-scaled)
+ * value returned from the hardware.
+ */
+static ssize_t s3c_hwmon_show_raw(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct s3c_hwmon *adc = dev_to_ourhwmon(to_platform_device(dev));
+	int ret, nr;
+
+	nr = attr->attr.name[3] - '0';
+	ret = s3c_hwmon_read_ch(adc, nr);
+
+	return  (ret < 0) ? ret : snprintf(buf, PAGE_SIZE, "%d\n", ret);
+}
+
+#define DEF_ADC_ATTR(x)	\
+	static DEVICE_ATTR(adc##x##_raw, S_IRUGO, s3c_hwmon_show_raw, NULL)
+
+DEF_ADC_ATTR(0);
+DEF_ADC_ATTR(1);
+DEF_ADC_ATTR(2);
+DEF_ADC_ATTR(3);
+DEF_ADC_ATTR(4);
+DEF_ADC_ATTR(5);
+DEF_ADC_ATTR(6);
+DEF_ADC_ATTR(7);
+
+static struct device_attribute *s3c_hwmon_attrs[8] = {
+	&dev_attr_adc0_raw,
+	&dev_attr_adc1_raw,
+	&dev_attr_adc2_raw,
+	&dev_attr_adc3_raw,
+	&dev_attr_adc4_raw,
+	&dev_attr_adc5_raw,
+	&dev_attr_adc6_raw,
+	&dev_attr_adc7_raw,
+};
+
+/**
+ * s3c_hwmon_ch_show - show value of a given channel
+ * @dev: The device that the attribute belongs to.
+ * @attr: The attribute being read.
+ * @buf: The result buffer.
+ *
+ * Read a value from the ADC and scale it before returning it to the
+ * caller. The scale factor is gained from the channel configuration
+ * passed via the platform data when the device was registered.
+ */
+static ssize_t s3c_hwmon_ch_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct sensor_device_attribute *sen_attr = to_sensor_dev_attr(attr);
+	struct s3c_hwmon_pdata *pdata = dev->platform_data;
+	struct s3c_hwmon *adc = dev_to_ourhwmon(to_platform_device(dev));
+	struct s3c_hwmon_chcfg *cfg;
+
+	int ret;
+
+	cfg = pdata->in[sen_attr->index];
+	if (!cfg)
+		return -EINVAL;
+
+	ret = s3c_hwmon_read_ch(adc,sen_attr->index);
+	if (ret < 0)
+		return ret;
+
+	ret *= cfg->mult;
+	ret /= cfg->div;
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
+}
+
+#define MAX_LABEL (16)
+
+/**
+ * s3c_hwmon_create_attr - create hwmon attribute for given channel.
+ * @hwmon: Our hwmon instance.
+ * @pdata: The platform data provided by the device.
+ * @channel: The ADC channel number to process.
+ *
+ * Create the scaled attribute for use with hwmon from the specified
+ * platform data in @pdata. The sysfs entry is handled by the routine
+ * s3c_hwmon_ch_show().
+ *
+ * The attribute name is taken from the configuration data if present
+ * otherwise the name is taken by concatenating in_ with the channel
+ * number.
+ */
+static int s3c_hwmon_create_attr(struct s3c_hwmon *hwmon,
+				 struct s3c_hwmon_pdata *pdata,
+				 int channel)
+{
+	struct s3c_hwmon_chcfg *cfg = pdata->in[channel];
+	struct sensor_device_attribute *attr;
+	const char *name;
+
+	if (!cfg)
+		return 0;
+
+	attr = kzalloc(sizeof(*attr) + MAX_LABEL, GFP_KERNEL);
+	if (attr = NULL)
+		return -ENOMEM;
+
+	if (cfg->name)
+		name = cfg->name;
+	else {
+		name = (char *)(attr+1);
+		snprintf((char *)(attr+1), MAX_LABEL, "in_%d", channel);
+	}
+
+	attr->dev_attr.attr.name  = name;
+	attr->dev_attr.attr.mode  = S_IRUGO;
+	attr->dev_attr.attr.owner = THIS_MODULE;
+	attr->dev_attr.show = s3c_hwmon_ch_show;
+
+	attr->index = channel;
+	hwmon->in_attr[channel] = attr;
+
+	return device_create_file(&hwmon->dev->dev, &attr->dev_attr);
+}
+
+/**
+ * s3c_hwmon_probe - device probe entry.
+ * @dev: The device being probed.
+*/
+static int s3c_hwmon_probe(struct platform_device *dev)
+{
+	struct s3c_hwmon_pdata *pdata = dev->dev.platform_data;
+	struct s3c_hwmon *hwmon;
+	int ret = 0;
+	int i;
+
+	adc = kzalloc(sizeof(struct s3c_hwmon), GFP_KERNEL);
+	if (adc = NULL) {
+		dev_err(&dev->dev, "no memory\n");
+		return -ENOMEM;
+	}
+
+	platform_set_drvdata(dev, adc);
+	hwmon->dev = dev;
+
+	/* only enable the clock when we are actually using the adc */
+
+	init_waitqueue_head(&hwmon->wait);
+	init_MUTEX(&hwmon->lock);
+
+	/* Register with the core ADC driver. */
+
+	hwmon->client = s3c_adc_register(dev, NULL, s3c_hwmon_adcdone, 0);
+	if (IS_ERR(hwmon->client)) {
+		dev_err(&dev->dev, "cannot register adc\n");
+		ret = PTR_ERR(hwmon->client);
+		goto err_mem;
+	}
+
+	/* add attributes for our adc devices. */
+
+	for (i = 0; i < ARRAY_SIZE(s3c_hwmon_attrs); i++) {
+		ret = device_create_file(&dev->dev, s3c_hwmon_attrs[i]);
+		if (ret) {
+			dev_err(&dev->dev, "error creating channel %d\n", i);
+
+			for (i--; i >= 0; i--)
+				device_remove_file(&dev->dev, s3c_hwmon_attrs[i]);
+
+			goto err_registered;
+		}
+	}
+
+	/* register with the hwmon core */
+
+	hwmon->hwmon_dev = hwmon_device_register(&dev->dev);
+	if (IS_ERR(hwmon->hwmon_dev)) {
+		dev_err(&dev->dev, "error registering with hwmon\n");
+		ret = PTR_ERR(hwmon->hwmon_dev);
+		goto err_attribute;
+	}
+
+	if (pdata) {
+		for (i = 0; i < ARRAY_SIZE(pdata->in); i++)
+			s3c_hwmon_create_attr(adc, pdata, i);
+	}
+
+	return 0;
+
+ err_attribute:
+	for (i = 0; i < ARRAY_SIZE(s3c_hwmon_attrs); i++)
+		device_remove_file(&dev->dev, s3c_hwmon_attrs[i]);
+
+ err_registered:
+	s3c_adc_release(hwmon->client);
+
+ err_mem:
+	kfree(adc);
+	return ret;
+}
+
+static int __devexit s3c_hwmon_remove(struct platform_device *dev)
+{
+	struct s3c_hwmon *hwmon = dev_to_ourhwmon(dev);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(s3c_hwmon_attrs); i++)
+		device_remove_file(&dev->dev, s3c_hwmon_attrs[i]);
+
+	for (i = 0; i < ARRAY_SIZE(hwmon->in_attr); i++) {
+		if (!hwmon->in_attr[i])
+			continue;
+
+		device_remove_file(&dev->dev, &hwmon->in_attr[i]->dev_attr);
+		kfree(hwmon->in_attr[i]);
+	}
+
+	hwmon_device_unregister(hwmon->hwmon_dev);
+	s3c_adc_release(hwmon->client);
+
+	return 0;
+}
+
+static struct platform_driver s3c_hwmon_driver = {
+	.driver	= {
+		.name		= "s3c-hwmon",
+		.owner		= THIS_MODULE,
+	},
+	.probe		= s3c_hwmon_probe,
+	.remove		= __devexit_p(s3c_hwmon_remove),
+};
+
+static int __init s3c_adc_init(void)
+{
+	return platform_driver_register(&s3c_hwmon_driver);
+}
+
+static void __exit s3c_adc_exit(void)
+{
+	platform_driver_unregister(&s3c_hwmon_driver);
+}
+
+module_init(s3c_adc_init);
+module_exit(s3c_adc_exit);
+
+MODULE_AUTHOR("Ben Dooks <ben@simtec.co.uk>");
+MODULE_DESCRIPTION("S3C ADC HWMon driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:s3c-hwmon");
Index: linux.git/arch/arm/plat-s3c/include/plat/hwmon.h
=================================--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux.git/arch/arm/plat-s3c/include/plat/hwmon.h	2009-05-20 22:02:43.000000000 +0100
@@ -0,0 +1,43 @@
+/* linux/arch/arm/plat-s3c/include/plat/hwmon.h
+ *
+ * Copyright 2005 Simtec Electronics
+ *	Ben Dooks <ben@simtec.co.uk>
+ *	http://armlinux.simtec.co.uk/
+ *
+ * S3C - HWMon interface for ADC
+ *
+ * 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.
+*/
+
+#ifndef __ASM_ARCH_ADC_HWMON_H
+#define __ASM_ARCH_ADC_HWMON_H __FILE__
+
+/**
+ * s3c_hwmon_chcfg - channel configuration
+ * @name: The name to give this channel.
+ * @mult: Multiply the ADC value read by this.
+ * @div: Divide the value from the ADC by this.
+ *
+ * The value read from the ADC is converted to a value that
+ * hwmon expects (mV) by result = (value_read * @mult) / @div.
+ */
+struct s3c_hwmon_chcfg {
+	const char	*name;
+	unsigned int	min;
+	unsigned int	max;
+	unsigned int	mult;
+	unsigned int	div;
+};
+
+/**
+ * s3c_hwmon_pdata - HWMON platform data
+ * @in: One configuration for each possible channel used.
+ */
+struct s3c_hwmon_pdata {
+	struct s3c_hwmon_chcfg	*in[8];
+};
+
+#endif /* __ASM_ARCH_ADC_HWMON_H */
+

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] HWMON: S3C24XX series ADC driver

[Ramax Lo]

2009/5/21 Ben Dooks <ben@simtec.co.uk>
>
> Add support for the ADC controller on the S3C
> series of processors to drivers/hwmon for use with
> hardware monitoring systems.
>
> Signed-off-by: Ben Dooks <ben@simtec.co.uk>
>
> Index: linux.git/drivers/hwmon/Kconfig
> =================================> --- linux.git.orig/drivers/hwmon/Kconfig        2009-05-20 22:02:06.000000000 +0100
> +++ linux.git/drivers/hwmon/Kconfig     2009-05-20 22:02:43.000000000 +0100
> @@ -702,6 +702,16 @@ config SENSORS_SHT15
>          This driver can also be built as a module.  If so, the module
>          will be called sht15.
>
> +config SENSORS_S3C_ADC
> +       tristate "S3C24XX/S3C64XX Inbuilt ADC"
> +       depends on HWMON && (ARCH_S3C2410 || ARCH_S3C64XX)
> +       help
> +         If you say yes here you get support for the on-board ADCs of
> +         the Samsung S3C24XX or S3C64XX series of SoC
> +
> +         This driver can also be built as a module. If so, the module
> +         will be called s3c-adc.
> +
>  config SENSORS_SIS5595
>        tristate "Silicon Integrated Systems Corp. SiS5595"
>        depends on PCI
> Index: linux.git/drivers/hwmon/Makefile
> =================================> --- linux.git.orig/drivers/hwmon/Makefile       2009-05-20 22:02:06.000000000 +0100
> +++ linux.git/drivers/hwmon/Makefile    2009-05-20 22:02:43.000000000 +0100
> @@ -76,6 +76,7 @@ obj-$(CONFIG_SENSORS_MAX6650) += max6650
>  obj-$(CONFIG_SENSORS_PC87360)  += pc87360.o
>  obj-$(CONFIG_SENSORS_PC87427)  += pc87427.o
>  obj-$(CONFIG_SENSORS_PCF8591)  += pcf8591.o
> +obj-$(CONFIG_SENSORS_S3C_ADC)  += s3c-adc.o
>  obj-$(CONFIG_SENSORS_SHT15)    += sht15.o
>  obj-$(CONFIG_SENSORS_SIS5595)  += sis5595.o
>  obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
> Index: linux.git/drivers/hwmon/s3c-adc.c
> =================================> --- /dev/null   1970-01-01 00:00:00.000000000 +0000
> +++ linux.git/drivers/hwmon/s3c-adc.c   2009-05-20 22:20:32.000000000 +0100
> @@ -0,0 +1,371 @@
> +/* linux/drivers/hwmon/s3c-adc.c
> + *
> + * Copyright (C) 2005, 2008, 2009 Simtec Electronics
> + *     http://armlinux.simtec.co.uk/
> + *     Ben Dooks <ben@simtec.co.uk>
> + *
> + * S3C24XX/S3C64XX ADC hwmon support
> + *
> + * 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.
> + *
> + * 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., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> +*/
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +
> +#include <plat/adc.h>
> +#include <plat/hwmon.h>
> +
> +/**
> + * struct s3c_hwmon - ADC hwmon client information
> + * @lock: Access lock for conversion.
> + * @wait: Wait queue for conversions to complete.
> + * @client: The client we registered with the S3C ADC core.
> + * @dev: The platform device we bound to.
> + * @hwmon_dev: The hwmon device we created.
> + * @in_attr: The device attributes we created.
> +*/
> +struct s3c_hwmon {
> +       struct semaphore        lock;
> +       wait_queue_head_t       wait;
> +       int                     val;
> +
> +       struct s3c_hwmon_client *client;
> +       struct platform_device  *dev;
> +       struct device           *hwmon_dev;
> +
> +       struct sensor_device_attribute *in_attr[8];
> +};
> +
> +static inline struct s3c_hwmon *dev_to_ourhwmon(struct platform_device *dev)
> +{
> +       return (struct s3c_hwmon *)platform_get_drvdata(dev);
> +}
> +
> +static struct s3c_hwmon *done_adc;
> +
> +/**
> + * s3c_hwmon_adcdone - ADC core callback
> + * @value: The value that we got from the ADC core
> + * @ignore: Only used for the touchscreen client.
> + * @left: The number of conversions left (not used here).
> + *
> + * This is called when the ADC has finished its conversion to
> + * inform us of the result.
> + */
> +static void s3c_hwmon_adcdone(unsigned value, unsigned ignore, unsigned *left)
> +{
> +       struct s3c_hwmon *adc = done_adc;
> +
> +       hwmon->val = value;
> +       wake_up(&hwmon->wait);

The variable should be 'adc'?

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] HWMON: S3C24XX series ADC driver

[Ben Dooks]

Ramax Lo wrote:
> 2009/5/21 Ben Dooks <ben@simtec.co.uk>
>> Add support for the ADC controller on the S3C
>> series of processors to drivers/hwmon for use with
>> hardware monitoring systems.
>>
>> Signed-off-by: Ben Dooks <ben@simtec.co.uk>
>>
>> Index: linux.git/drivers/hwmon/Kconfig
>> =================================
[snip]

>> +/**
>> + * s3c_hwmon_adcdone - ADC core callback
>> + * @value: The value that we got from the ADC core
>> + * @ignore: Only used for the touchscreen client.
>> + * @left: The number of conversions left (not used here).
>> + *
>> + * This is called when the ADC has finished its conversion to
>> + * inform us of the result.
>> + */
>> +static void s3c_hwmon_adcdone(unsigned value, unsigned ignore, unsigned *left)
>> +{
>> +       struct s3c_hwmon *adc = done_adc;
>> +
>> +       hwmon->val = value;
>> +       wake_up(&hwmon->wait);
> 
> The variable should be 'adc'?

Yes, you're right. It seems I failed to refresh the patch before
sending it from the quilt it was in. Will check and repost.

-- 
Ben Dooks, Software Engineer, Simtec Electronics

http://www.simtec.co.uk/

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[lm-sensors] HWMON: S3C24XX series ADC driver

[Ben Dooks]

Add support for the ADC controller on the S3C
series of processors to drivers/hwmon for use with
hardware monitoring systems.

Signed-off-by: Ben Dooks <ben@simtec.co.uk>

Index: linux.git/drivers/hwmon/Kconfig
=================================--- linux.git.orig/drivers/hwmon/Kconfig	2009-05-20 22:02:06.000000000 +0100
+++ linux.git/drivers/hwmon/Kconfig	2009-05-20 22:02:43.000000000 +0100
@@ -702,6 +702,16 @@ config SENSORS_SHT15
 	  This driver can also be built as a module.  If so, the module
 	  will be called sht15.
 
+config SENSORS_S3C_ADC
+	tristate "S3C24XX/S3C64XX Inbuilt ADC"
+	depends on HWMON && (ARCH_S3C2410 || ARCH_S3C64XX)
+	help
+	  If you say yes here you get support for the on-board ADCs of
+	  the Samsung S3C24XX or S3C64XX series of SoC
+
+	  This driver can also be built as a module. If so, the module
+	  will be called s3c-adc.
+
 config SENSORS_SIS5595
 	tristate "Silicon Integrated Systems Corp. SiS5595"
 	depends on PCI
Index: linux.git/drivers/hwmon/Makefile
=================================--- linux.git.orig/drivers/hwmon/Makefile	2009-05-20 22:02:06.000000000 +0100
+++ linux.git/drivers/hwmon/Makefile	2009-05-20 22:02:43.000000000 +0100
@@ -76,6 +76,7 @@ obj-$(CONFIG_SENSORS_MAX6650)	+= max6650
 obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
 obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
+obj-$(CONFIG_SENSORS_S3C_ADC)	+= s3c-adc.o
 obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
 obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595.o
 obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
Index: linux.git/drivers/hwmon/s3c-adc.c
=================================--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux.git/drivers/hwmon/s3c-adc.c	2009-05-26 09:00:22.000000000 +0100
@@ -0,0 +1,371 @@
+/* linux/drivers/hwmon/s3c-adc.c
+ *
+ * Copyright (C) 2005, 2008, 2009 Simtec Electronics
+ *	http://armlinux.simtec.co.uk/
+ *	Ben Dooks <ben@simtec.co.uk>
+ *
+ * S3C24XX/S3C64XX ADC hwmon support
+ *
+ * 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.
+ *
+ * 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., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+*/
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+
+#include <plat/adc.h>
+#include <plat/hwmon.h>
+
+/**
+ * struct s3c_hwmon - ADC hwmon client information
+ * @lock: Access lock for conversion.
+ * @wait: Wait queue for conversions to complete.
+ * @client: The client we registered with the S3C ADC core.
+ * @dev: The platform device we bound to.
+ * @hwmon_dev: The hwmon device we created.
+ * @in_attr: The device attributes we created.
+*/
+struct s3c_hwmon {
+	struct semaphore	lock;
+	wait_queue_head_t	wait;
+	int			val;
+
+	struct s3c_adc_client	*client;
+	struct platform_device	*dev;
+	struct device		*hwmon_dev;
+
+	struct sensor_device_attribute *in_attr[8];
+};
+
+static inline struct s3c_hwmon *dev_to_ourhwmon(struct platform_device *dev)
+{
+	return (struct s3c_hwmon *)platform_get_drvdata(dev);
+}
+
+static struct s3c_hwmon *done_adc;
+
+/**
+ * s3c_hwmon_adcdone - ADC core callback
+ * @value: The value that we got from the ADC core
+ * @ignore: Only used for the touchscreen client.
+ * @left: The number of conversions left (not used here).
+ *
+ * This is called when the ADC has finished its conversion to
+ * inform us of the result.
+ */
+static void s3c_hwmon_adcdone(unsigned value, unsigned ignore, unsigned *left)
+{
+	struct s3c_hwmon *hwmon = done_adc;
+
+	hwmon->val = value;
+	wake_up(&hwmon->wait);
+}
+
+/**
+ * s3c_hwmon_read_ch - read a value from a given adc channel.
+ * @hwmon: Our state.
+ * @channel: The channel we're reading from.
+ *
+ * Read a value from the @channel with the proper locking and sleep until
+ * either the read completes or we timeout awaiting the ADC core to get
+ * back to us.
+ */
+static int s3c_hwmon_read_ch(struct s3c_hwmon *hwmon, int channel)
+{
+	unsigned long timeout;
+	int ret;
+
+	ret = down_interruptible(&hwmon->lock);
+	if (ret < 0)
+		return ret;
+
+	dev_dbg(&hwmon->dev->dev, "reading channel %d\n", channel);
+
+	hwmon->val = -1;
+	done_adc = hwmon;
+
+	ret = s3c_adc_start(hwmon->client, channel, 1);
+	if (ret < 0)
+		goto err;
+
+	timeout = wait_event_timeout(hwmon->wait, hwmon->val >= 0, HZ / 2);
+	ret = (timeout = 0) ? -ETIMEDOUT : hwmon->val;
+
+ err:
+	up(&hwmon->lock);
+	return ret;
+}
+
+/**
+ * s3c_hwmon_show_raw - show a conversion from the raw channel number.
+ * @dev: The device that the attribute belongs to.
+ * @attr: The attribute being read.
+ * @buf: The result buffer.
+ *
+ * This show deals with the raw attribute, registered for each possible
+ * ADC channel. This does a conversion and returns the raw (un-scaled)
+ * value returned from the hardware.
+ */
+static ssize_t s3c_hwmon_show_raw(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct s3c_hwmon *adc = dev_to_ourhwmon(to_platform_device(dev));
+	int ret, nr;
+
+	nr = attr->attr.name[3] - '0';
+	ret = s3c_hwmon_read_ch(adc, nr);
+
+	return  (ret < 0) ? ret : snprintf(buf, PAGE_SIZE, "%d\n", ret);
+}
+
+#define DEF_ADC_ATTR(x)	\
+	static DEVICE_ATTR(adc##x##_raw, S_IRUGO, s3c_hwmon_show_raw, NULL)
+
+DEF_ADC_ATTR(0);
+DEF_ADC_ATTR(1);
+DEF_ADC_ATTR(2);
+DEF_ADC_ATTR(3);
+DEF_ADC_ATTR(4);
+DEF_ADC_ATTR(5);
+DEF_ADC_ATTR(6);
+DEF_ADC_ATTR(7);
+
+static struct device_attribute *s3c_hwmon_attrs[8] = {
+	&dev_attr_adc0_raw,
+	&dev_attr_adc1_raw,
+	&dev_attr_adc2_raw,
+	&dev_attr_adc3_raw,
+	&dev_attr_adc4_raw,
+	&dev_attr_adc5_raw,
+	&dev_attr_adc6_raw,
+	&dev_attr_adc7_raw,
+};
+
+/**
+ * s3c_hwmon_ch_show - show value of a given channel
+ * @dev: The device that the attribute belongs to.
+ * @attr: The attribute being read.
+ * @buf: The result buffer.
+ *
+ * Read a value from the ADC and scale it before returning it to the
+ * caller. The scale factor is gained from the channel configuration
+ * passed via the platform data when the device was registered.
+ */
+static ssize_t s3c_hwmon_ch_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct sensor_device_attribute *sen_attr = to_sensor_dev_attr(attr);
+	struct s3c_hwmon_pdata *pdata = dev->platform_data;
+	struct s3c_hwmon *hwmon = dev_to_ourhwmon(to_platform_device(dev));
+	struct s3c_hwmon_chcfg *cfg;
+
+	int ret;
+
+	cfg = pdata->in[sen_attr->index];
+	if (!cfg)
+		return -EINVAL;
+
+	ret = s3c_hwmon_read_ch(hwmon, sen_attr->index);
+	if (ret < 0)
+		return ret;
+
+	ret *= cfg->mult;
+	ret /= cfg->div;
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
+}
+
+#define MAX_LABEL (16)
+
+/**
+ * s3c_hwmon_create_attr - create hwmon attribute for given channel.
+ * @hwmon: Our hwmon instance.
+ * @pdata: The platform data provided by the device.
+ * @channel: The ADC channel number to process.
+ *
+ * Create the scaled attribute for use with hwmon from the specified
+ * platform data in @pdata. The sysfs entry is handled by the routine
+ * s3c_hwmon_ch_show().
+ *
+ * The attribute name is taken from the configuration data if present
+ * otherwise the name is taken by concatenating in_ with the channel
+ * number.
+ */
+static int s3c_hwmon_create_attr(struct s3c_hwmon *hwmon,
+				 struct s3c_hwmon_pdata *pdata,
+				 int channel)
+{
+	struct s3c_hwmon_chcfg *cfg = pdata->in[channel];
+	struct sensor_device_attribute *attr;
+	const char *name;
+
+	if (!cfg)
+		return 0;
+
+	attr = kzalloc(sizeof(*attr) + MAX_LABEL, GFP_KERNEL);
+	if (attr = NULL)
+		return -ENOMEM;
+
+	if (cfg->name)
+		name = cfg->name;
+	else {
+		name = (char *)(attr+1);
+		snprintf((char *)(attr+1), MAX_LABEL, "in_%d", channel);
+	}
+
+	attr->dev_attr.attr.name  = name;
+	attr->dev_attr.attr.mode  = S_IRUGO;
+	attr->dev_attr.attr.owner = THIS_MODULE;
+	attr->dev_attr.show = s3c_hwmon_ch_show;
+
+	attr->index = channel;
+	hwmon->in_attr[channel] = attr;
+
+	return device_create_file(&hwmon->dev->dev, &attr->dev_attr);
+}
+
+/**
+ * s3c_hwmon_probe - device probe entry.
+ * @dev: The device being probed.
+*/
+static int s3c_hwmon_probe(struct platform_device *dev)
+{
+	struct s3c_hwmon_pdata *pdata = dev->dev.platform_data;
+	struct s3c_hwmon *hwmon;
+	int ret = 0;
+	int i;
+
+	hwmon = kzalloc(sizeof(struct s3c_hwmon), GFP_KERNEL);
+	if (hwmon = NULL) {
+		dev_err(&dev->dev, "no memory\n");
+		return -ENOMEM;
+	}
+
+	platform_set_drvdata(dev, hwmon);
+	hwmon->dev = dev;
+
+	/* only enable the clock when we are actually using the adc */
+
+	init_waitqueue_head(&hwmon->wait);
+	init_MUTEX(&hwmon->lock);
+
+	/* Register with the core ADC driver. */
+
+	hwmon->client = s3c_adc_register(dev, NULL, s3c_hwmon_adcdone, 0);
+	if (IS_ERR(hwmon->client)) {
+		dev_err(&dev->dev, "cannot register adc\n");
+		ret = PTR_ERR(hwmon->client);
+		goto err_mem;
+	}
+
+	/* add attributes for our adc devices. */
+
+	for (i = 0; i < ARRAY_SIZE(s3c_hwmon_attrs); i++) {
+		ret = device_create_file(&dev->dev, s3c_hwmon_attrs[i]);
+		if (ret) {
+			dev_err(&dev->dev, "error creating channel %d\n", i);
+
+			for (i--; i >= 0; i--)
+				device_remove_file(&dev->dev, s3c_hwmon_attrs[i]);
+
+			goto err_registered;
+		}
+	}
+
+	/* register with the hwmon core */
+
+	hwmon->hwmon_dev = hwmon_device_register(&dev->dev);
+	if (IS_ERR(hwmon->hwmon_dev)) {
+		dev_err(&dev->dev, "error registering with hwmon\n");
+		ret = PTR_ERR(hwmon->hwmon_dev);
+		goto err_attribute;
+	}
+
+	if (pdata) {
+		for (i = 0; i < ARRAY_SIZE(pdata->in); i++)
+			s3c_hwmon_create_attr(hwmon, pdata, i);
+	}
+
+	return 0;
+
+ err_attribute:
+	for (i = 0; i < ARRAY_SIZE(s3c_hwmon_attrs); i++)
+		device_remove_file(&dev->dev, s3c_hwmon_attrs[i]);
+
+ err_registered:
+	s3c_adc_release(hwmon->client);
+
+ err_mem:
+	kfree(hwmon);
+	return ret;
+}
+
+static int __devexit s3c_hwmon_remove(struct platform_device *dev)
+{
+	struct s3c_hwmon *hwmon = dev_to_ourhwmon(dev);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(s3c_hwmon_attrs); i++)
+		device_remove_file(&dev->dev, s3c_hwmon_attrs[i]);
+
+	for (i = 0; i < ARRAY_SIZE(hwmon->in_attr); i++) {
+		if (!hwmon->in_attr[i])
+			continue;
+
+		device_remove_file(&dev->dev, &hwmon->in_attr[i]->dev_attr);
+		kfree(hwmon->in_attr[i]);
+	}
+
+	hwmon_device_unregister(hwmon->hwmon_dev);
+	s3c_adc_release(hwmon->client);
+
+	return 0;
+}
+
+static struct platform_driver s3c_hwmon_driver = {
+	.driver	= {
+		.name		= "s3c-hwmon",
+		.owner		= THIS_MODULE,
+	},
+	.probe		= s3c_hwmon_probe,
+	.remove		= __devexit_p(s3c_hwmon_remove),
+};
+
+static int __init s3c_hwmon_init(void)
+{
+	return platform_driver_register(&s3c_hwmon_driver);
+}
+
+static void __exit s3c_hwmon_exit(void)
+{
+	platform_driver_unregister(&s3c_hwmon_driver);
+}
+
+module_init(s3c_hwmon_init);
+module_exit(s3c_hwmon_exit);
+
+MODULE_AUTHOR("Ben Dooks <ben@simtec.co.uk>");
+MODULE_DESCRIPTION("S3C ADC HWMon driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:s3c-hwmon");
Index: linux.git/arch/arm/plat-s3c/include/plat/hwmon.h
=================================--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux.git/arch/arm/plat-s3c/include/plat/hwmon.h	2009-05-20 22:02:43.000000000 +0100
@@ -0,0 +1,43 @@
+/* linux/arch/arm/plat-s3c/include/plat/hwmon.h
+ *
+ * Copyright 2005 Simtec Electronics
+ *	Ben Dooks <ben@simtec.co.uk>
+ *	http://armlinux.simtec.co.uk/
+ *
+ * S3C - HWMon interface for ADC
+ *
+ * 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.
+*/
+
+#ifndef __ASM_ARCH_ADC_HWMON_H
+#define __ASM_ARCH_ADC_HWMON_H __FILE__
+
+/**
+ * s3c_hwmon_chcfg - channel configuration
+ * @name: The name to give this channel.
+ * @mult: Multiply the ADC value read by this.
+ * @div: Divide the value from the ADC by this.
+ *
+ * The value read from the ADC is converted to a value that
+ * hwmon expects (mV) by result = (value_read * @mult) / @div.
+ */
+struct s3c_hwmon_chcfg {
+	const char	*name;
+	unsigned int	min;
+	unsigned int	max;
+	unsigned int	mult;
+	unsigned int	div;
+};
+
+/**
+ * s3c_hwmon_pdata - HWMON platform data
+ * @in: One configuration for each possible channel used.
+ */
+struct s3c_hwmon_pdata {
+	struct s3c_hwmon_chcfg	*in[8];
+};
+
+#endif /* __ASM_ARCH_ADC_HWMON_H */
+

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] HWMON: S3C24XX series ADC driver

[Hector Oron]

Hi,

2009/5/26 Ben Dooks <ben@simtec.co.uk>:
> +       /* Register with the core ADC driver. */
> +
> +       hwmon->client = s3c_adc_register(dev, NULL, s3c_hwmon_adcdone, 0);

I have been trying out this code and when compiling I get a warning,

  CC [M]  drivers/hwmon/s3c-adc.o
drivers/hwmon/s3c-adc.c: In function 's3c_hwmon_probe':
drivers/hwmon/s3c-adc.c:275: warning: passing argument 3 of
's3c_adc_register' from
er type

s3c_hwmon_adcdone is a callback function and register function is
expecting something like "void (*conv)(unsigned d0, unsigned d1)"

I quite do not understand it very well to propose a patch.

Cheers

-- 
 Héctor Orón

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] HWMON: S3C24XX series ADC driver

[Hector Oron]

Hi,

  Nelson Castillo's contribution was missing, those were the missing
bits, hence the warning,
[PATCH 2/2] s3c-adc: Expose number of remaining conversions to convert
callback[1]

BTW, do you know any user application I can use to test the driver?

Cheers =)

[1] http://marc.info/?l=linux-arm-kernel&m\x124250942823013&w=2

--

 arch/arm/plat-s3c/include/plat/adc.h |    3 ++-
 arch/arm/plat-s3c24xx/adc.c          |   11 +++++++----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/arm/plat-s3c/include/plat/adc.h
b/arch/arm/plat-s3c/include/plat/adc.h
index 43df2a4..a3e34b7 100644
--- a/arch/arm/plat-s3c/include/plat/adc.h
+++ b/arch/arm/plat-s3c/include/plat/adc.h
@@ -21,7 +21,8 @@ extern int s3c_adc_start(struct s3c_adc_client *client,

 extern struct s3c_adc_client *s3c_adc_register(struct platform_device *pdev,
 					       void (*select)(unsigned selected),
-					       void (*conv)(unsigned d0, unsigned d1),
+					       void (*conv)(unsigned d0, unsigned d1,
+					       unsigned *samples_left),
 					       unsigned int is_ts);

 extern void s3c_adc_release(struct s3c_adc_client *client);
diff --git a/arch/arm/plat-s3c24xx/adc.c b/arch/arm/plat-s3c24xx/adc.c
index 9a5c767..0613eed 100644
--- a/arch/arm/plat-s3c24xx/adc.c
+++ b/arch/arm/plat-s3c24xx/adc.c
@@ -45,7 +45,8 @@ struct s3c_adc_client {
 	unsigned char		 channel;

 	void	(*select_cb)(unsigned selected);
-	void	(*convert_cb)(unsigned val1, unsigned val2);
+	void	(*convert_cb)(unsigned val1, unsigned val2,
+                                unsigned *samples_left);
 };

 struct adc_device {
@@ -158,7 +159,8 @@ static void s3c_adc_default_select(unsigned select)

 struct s3c_adc_client *s3c_adc_register(struct platform_device *pdev,
 					void (*select)(unsigned int selected),
-					void (*conv)(unsigned d0, unsigned d1),
+					void (*conv)(unsigned d0, unsigned d1,
+                                        unsigned *samples_left),
 					unsigned int is_ts)
 {
 	struct s3c_adc_client *client;
@@ -210,9 +212,10 @@ static irqreturn_t s3c_adc_irq(int irq, void *pw)
 	data1 = readl(adc->regs + S3C2410_ADCDAT1);
 	adc_dbg(adc, "read %d: 0x%04x, 0x%04x\n", client->nr_samples, data0, data1);

-	(client->convert_cb)(data0 & 0x3ff, data1 & 0x3ff);
+	client->nr_samples--;
+	(client->convert_cb)(data0 & 0x3ff, data1 & 0x3ff, &client->nr_samples);

-	if (--client->nr_samples > 0) {
+	if (client->nr_samples > 0) {
 		/* fire another conversion for this */

 		client->select_cb(1);
-- 
1.6.3.1

-- 
 Héctor Orón

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] HWMON: S3C24XX series ADC driver

[Ben Dooks]

Hector Oron wrote:
> Hi,
> 
> 2009/5/26 Ben Dooks <ben@simtec.co.uk>:
>> +       /* Register with the core ADC driver. */
>> +
>> +       hwmon->client = s3c_adc_register(dev, NULL, s3c_hwmon_adcdone, 0);
> 
> I have been trying out this code and when compiling I get a warning,
> 
>   CC [M]  drivers/hwmon/s3c-adc.o
> drivers/hwmon/s3c-adc.c: In function 's3c_hwmon_probe':
> drivers/hwmon/s3c-adc.c:275: warning: passing argument 3 of
> 's3c_adc_register' from
> er type
> 
> s3c_hwmon_adcdone is a callback function and register function is
> expecting something like "void (*conv)(unsigned d0, unsigned d1)"

by the time it is merged, it'll be expecting a third argument
to be passed.

-- 
Ben Dooks, Software Engineer, Simtec Electronics

http://www.simtec.co.uk/

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] HWMON: S3C24XX series ADC driver

[Jean Delvare]

Hi Ben,

On Tue, 26 May 2009 09:07:01 +0100, Ben Dooks wrote:
> Add support for the ADC controller on the S3C
> series of processors to drivers/hwmon for use with
> hardware monitoring systems.

Random comments:

> 
> Signed-off-by: Ben Dooks <ben@simtec.co.uk>
> 
> Index: linux.git/drivers/hwmon/Kconfig
> =================================> --- linux.git.orig/drivers/hwmon/Kconfig	2009-05-20 22:02:06.000000000 +0100
> +++ linux.git/drivers/hwmon/Kconfig	2009-05-20 22:02:43.000000000 +0100
> @@ -702,6 +702,16 @@ config SENSORS_SHT15
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called sht15.
>  
> +config SENSORS_S3C_ADC
> +	tristate "S3C24XX/S3C64XX Inbuilt ADC"
> +	depends on HWMON && (ARCH_S3C2410 || ARCH_S3C64XX)

This item is inside a big "if HWMON" block, no need to repeat the
condition.

> +	help
> +	  If you say yes here you get support for the on-board ADCs of
> +	  the Samsung S3C24XX or S3C64XX series of SoC
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called s3c-adc.
> +
>  config SENSORS_SIS5595
>  	tristate "Silicon Integrated Systems Corp. SiS5595"
>  	depends on PCI
> Index: linux.git/drivers/hwmon/Makefile
> =================================> --- linux.git.orig/drivers/hwmon/Makefile	2009-05-20 22:02:06.000000000 +0100
> +++ linux.git/drivers/hwmon/Makefile	2009-05-20 22:02:43.000000000 +0100
> @@ -76,6 +76,7 @@ obj-$(CONFIG_SENSORS_MAX6650)	+= max6650
>  obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
>  obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
>  obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
> +obj-$(CONFIG_SENSORS_S3C_ADC)	+= s3c-adc.o
>  obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
>  obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595.o
>  obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
> Index: linux.git/drivers/hwmon/s3c-adc.c
> =================================> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux.git/drivers/hwmon/s3c-adc.c	2009-05-26 09:00:22.000000000 +0100
> @@ -0,0 +1,371 @@
> +/* linux/drivers/hwmon/s3c-adc.c
> + *
> + * Copyright (C) 2005, 2008, 2009 Simtec Electronics
> + *	http://armlinux.simtec.co.uk/
> + *	Ben Dooks <ben@simtec.co.uk>
> + *
> + * S3C24XX/S3C64XX ADC hwmon support
> + *
> + * 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.
> + *
> + * 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., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> +*/
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +
> +#include <plat/adc.h>
> +#include <plat/hwmon.h>
> +
> +/**
> + * struct s3c_hwmon - ADC hwmon client information
> + * @lock: Access lock for conversion.

This gives us zero idea what the lock is protecting.

> + * @wait: Wait queue for conversions to complete.
> + * @client: The client we registered with the S3C ADC core.
> + * @dev: The platform device we bound to.
> + * @hwmon_dev: The hwmon device we created.
> + * @in_attr: The device attributes we created.
> +*/
> +struct s3c_hwmon {
> +	struct semaphore	lock;


> +	wait_queue_head_t	wait;
> +	int			val;

This is an horribly vague struct member name, and undocumented at that.

> +
> +	struct s3c_adc_client	*client;
> +	struct platform_device	*dev;

The fact that you need this suggests a wrong calling convention
somewhere in the code.

> +	struct device		*hwmon_dev;
> +
> +	struct sensor_device_attribute *in_attr[8];

What is the benefit of allocating these attributes dynamically? You are
likely to fragment your memory and require ugly code to make it work.

> +};
> +
> +static inline struct s3c_hwmon *dev_to_ourhwmon(struct platform_device *dev)
> +{
> +	return (struct s3c_hwmon *)platform_get_drvdata(dev);

Useless cast. Not to mention that this function is pretty much
pointless... just call platform_get_drvdata directly.

> +}
> +
> +static struct s3c_hwmon *done_adc;

What is this?

> +
> +/**
> + * s3c_hwmon_adcdone - ADC core callback
> + * @value: The value that we got from the ADC core
> + * @ignore: Only used for the touchscreen client.
> + * @left: The number of conversions left (not used here).
> + *
> + * This is called when the ADC has finished its conversion to
> + * inform us of the result.
> + */
> +static void s3c_hwmon_adcdone(unsigned value, unsigned ignore, unsigned *left)
> +{
> +	struct s3c_hwmon *hwmon = done_adc;
> +
> +	hwmon->val = value;
> +	wake_up(&hwmon->wait);
> +}
> +
> +/**
> + * s3c_hwmon_read_ch - read a value from a given adc channel.
> + * @hwmon: Our state.
> + * @channel: The channel we're reading from.
> + *
> + * Read a value from the @channel with the proper locking and sleep until
> + * either the read completes or we timeout awaiting the ADC core to get
> + * back to us.
> + */
> +static int s3c_hwmon_read_ch(struct s3c_hwmon *hwmon, int channel)
> +{
> +	unsigned long timeout;
> +	int ret;
> +
> +	ret = down_interruptible(&hwmon->lock);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_dbg(&hwmon->dev->dev, "reading channel %d\n", channel);
> +
> +	hwmon->val = -1;
> +	done_adc = hwmon;
> +
> +	ret = s3c_adc_start(hwmon->client, channel, 1);
> +	if (ret < 0)
> +		goto err;
> +
> +	timeout = wait_event_timeout(hwmon->wait, hwmon->val >= 0, HZ / 2);
> +	ret = (timeout = 0) ? -ETIMEDOUT : hwmon->val;
> +
> + err:
> +	up(&hwmon->lock);
> +	return ret;
> +}
> +
> +/**
> + * s3c_hwmon_show_raw - show a conversion from the raw channel number.
> + * @dev: The device that the attribute belongs to.
> + * @attr: The attribute being read.
> + * @buf: The result buffer.
> + *
> + * This show deals with the raw attribute, registered for each possible
> + * ADC channel. This does a conversion and returns the raw (un-scaled)
> + * value returned from the hardware.
> + */
> +static ssize_t s3c_hwmon_show_raw(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct s3c_hwmon *adc = dev_to_ourhwmon(to_platform_device(dev));

aka dev_get_drvdata().

> +	int ret, nr;
> +
> +	nr = attr->attr.name[3] - '0';

This is pretty fragile. Please use SENSOR_ATTR() instead.

> +	ret = s3c_hwmon_read_ch(adc, nr);
> +
> +	return  (ret < 0) ? ret : snprintf(buf, PAGE_SIZE, "%d\n", ret);
> +}
> +
> +#define DEF_ADC_ATTR(x)	\
> +	static DEVICE_ATTR(adc##x##_raw, S_IRUGO, s3c_hwmon_show_raw, NULL)
> +
> +DEF_ADC_ATTR(0);
> +DEF_ADC_ATTR(1);
> +DEF_ADC_ATTR(2);
> +DEF_ADC_ATTR(3);
> +DEF_ADC_ATTR(4);
> +DEF_ADC_ATTR(5);
> +DEF_ADC_ATTR(6);
> +DEF_ADC_ATTR(7);
> +
> +static struct device_attribute *s3c_hwmon_attrs[8] = {
> +	&dev_attr_adc0_raw,
> +	&dev_attr_adc1_raw,
> +	&dev_attr_adc2_raw,
> +	&dev_attr_adc3_raw,
> +	&dev_attr_adc4_raw,
> +	&dev_attr_adc5_raw,
> +	&dev_attr_adc6_raw,
> +	&dev_attr_adc7_raw,
> +};

This looks like debugging stuff. Does it really need to stay in the
final code? I would make it conditional, at least.

> +
> +/**
> + * s3c_hwmon_ch_show - show value of a given channel
> + * @dev: The device that the attribute belongs to.
> + * @attr: The attribute being read.
> + * @buf: The result buffer.
> + *
> + * Read a value from the ADC and scale it before returning it to the
> + * caller. The scale factor is gained from the channel configuration
> + * passed via the platform data when the device was registered.
> + */
> +static ssize_t s3c_hwmon_ch_show(struct device *dev,
> +				 struct device_attribute *attr,
> +				 char *buf)
> +{
> +	struct sensor_device_attribute *sen_attr = to_sensor_dev_attr(attr);
> +	struct s3c_hwmon_pdata *pdata = dev->platform_data;
> +	struct s3c_hwmon *hwmon = dev_to_ourhwmon(to_platform_device(dev));
> +	struct s3c_hwmon_chcfg *cfg;
> +
> +	int ret;
> +
> +	cfg = pdata->in[sen_attr->index];
> +	if (!cfg)
> +		return -EINVAL;
> +
> +	ret = s3c_hwmon_read_ch(hwmon, sen_attr->index);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret *= cfg->mult;
> +	ret /= cfg->div;

Division lacks rounding.

> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> +}
> +
> +#define MAX_LABEL (16)
> +
> +/**
> + * s3c_hwmon_create_attr - create hwmon attribute for given channel.
> + * @hwmon: Our hwmon instance.
> + * @pdata: The platform data provided by the device.
> + * @channel: The ADC channel number to process.
> + *
> + * Create the scaled attribute for use with hwmon from the specified
> + * platform data in @pdata. The sysfs entry is handled by the routine
> + * s3c_hwmon_ch_show().
> + *
> + * The attribute name is taken from the configuration data if present
> + * otherwise the name is taken by concatenating in_ with the channel
> + * number.
> + */
> +static int s3c_hwmon_create_attr(struct s3c_hwmon *hwmon,
> +				 struct s3c_hwmon_pdata *pdata,
> +				 int channel)
> +{
> +	struct s3c_hwmon_chcfg *cfg = pdata->in[channel];
> +	struct sensor_device_attribute *attr;
> +	const char *name;
> +
> +	if (!cfg)
> +		return 0;
> +
> +	attr = kzalloc(sizeof(*attr) + MAX_LABEL, GFP_KERNEL);
> +	if (attr = NULL)
> +		return -ENOMEM;
> +
> +	if (cfg->name)
> +		name = cfg->name;
> +	else {
> +		name = (char *)(attr+1);
> +		snprintf((char *)(attr+1), MAX_LABEL, "in_%d", channel);

This name doesn't match what is described in
Documentation/hwmon/sysfs-interface, which means that your driver won't
work with libsensors. This is a blocker.

> +	}
> +
> +	attr->dev_attr.attr.name  = name;
> +	attr->dev_attr.attr.mode  = S_IRUGO;
> +	attr->dev_attr.attr.owner = THIS_MODULE;
> +	attr->dev_attr.show = s3c_hwmon_ch_show;
> +
> +	attr->index = channel;
> +	hwmon->in_attr[channel] = attr;
> +
> +	return device_create_file(&hwmon->dev->dev, &attr->dev_attr);
> +}
> +
> +/**
> + * s3c_hwmon_probe - device probe entry.
> + * @dev: The device being probed.
> +*/
> +static int s3c_hwmon_probe(struct platform_device *dev)

No __devinit?

> +{
> +	struct s3c_hwmon_pdata *pdata = dev->dev.platform_data;
> +	struct s3c_hwmon *hwmon;
> +	int ret = 0;
> +	int i;
> +
> +	hwmon = kzalloc(sizeof(struct s3c_hwmon), GFP_KERNEL);
> +	if (hwmon = NULL) {
> +		dev_err(&dev->dev, "no memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	platform_set_drvdata(dev, hwmon);
> +	hwmon->dev = dev;
> +
> +	/* only enable the clock when we are actually using the adc */
> +
> +	init_waitqueue_head(&hwmon->wait);
> +	init_MUTEX(&hwmon->lock);
> +
> +	/* Register with the core ADC driver. */
> +
> +	hwmon->client = s3c_adc_register(dev, NULL, s3c_hwmon_adcdone, 0);
> +	if (IS_ERR(hwmon->client)) {
> +		dev_err(&dev->dev, "cannot register adc\n");
> +		ret = PTR_ERR(hwmon->client);
> +		goto err_mem;
> +	}
> +
> +	/* add attributes for our adc devices. */
> +
> +	for (i = 0; i < ARRAY_SIZE(s3c_hwmon_attrs); i++) {
> +		ret = device_create_file(&dev->dev, s3c_hwmon_attrs[i]);
> +		if (ret) {
> +			dev_err(&dev->dev, "error creating channel %d\n", i);
> +
> +			for (i--; i >= 0; i--)
> +				device_remove_file(&dev->dev, s3c_hwmon_attrs[i]);
> +
> +			goto err_registered;
> +		}
> +	}
> +
> +	/* register with the hwmon core */
> +
> +	hwmon->hwmon_dev = hwmon_device_register(&dev->dev);
> +	if (IS_ERR(hwmon->hwmon_dev)) {
> +		dev_err(&dev->dev, "error registering with hwmon\n");
> +		ret = PTR_ERR(hwmon->hwmon_dev);
> +		goto err_attribute;
> +	}
> +
> +	if (pdata) {
> +		for (i = 0; i < ARRAY_SIZE(pdata->in); i++)
> +			s3c_hwmon_create_attr(hwmon, pdata, i);

No error check?

> +	}
> +
> +	return 0;
> +
> + err_attribute:
> +	for (i = 0; i < ARRAY_SIZE(s3c_hwmon_attrs); i++)
> +		device_remove_file(&dev->dev, s3c_hwmon_attrs[i]);
> +
> + err_registered:
> +	s3c_adc_release(hwmon->client);
> +
> + err_mem:
> +	kfree(hwmon);
> +	return ret;
> +}
> +
> +static int __devexit s3c_hwmon_remove(struct platform_device *dev)
> +{
> +	struct s3c_hwmon *hwmon = dev_to_ourhwmon(dev);
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(s3c_hwmon_attrs); i++)
> +		device_remove_file(&dev->dev, s3c_hwmon_attrs[i]);
> +
> +	for (i = 0; i < ARRAY_SIZE(hwmon->in_attr); i++) {
> +		if (!hwmon->in_attr[i])
> +			continue;
> +
> +		device_remove_file(&dev->dev, &hwmon->in_attr[i]->dev_attr);
> +		kfree(hwmon->in_attr[i]);
> +	}
> +
> +	hwmon_device_unregister(hwmon->hwmon_dev);
> +	s3c_adc_release(hwmon->client);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver s3c_hwmon_driver = {
> +	.driver	= {
> +		.name		= "s3c-hwmon",

Please make this match the module name.

> +		.owner		= THIS_MODULE,
> +	},
> +	.probe		= s3c_hwmon_probe,
> +	.remove		= __devexit_p(s3c_hwmon_remove),
> +};
> +
> +static int __init s3c_hwmon_init(void)
> +{
> +	return platform_driver_register(&s3c_hwmon_driver);
> +}
> +
> +static void __exit s3c_hwmon_exit(void)
> +{
> +	platform_driver_unregister(&s3c_hwmon_driver);
> +}
> +
> +module_init(s3c_hwmon_init);
> +module_exit(s3c_hwmon_exit);
> +
> +MODULE_AUTHOR("Ben Dooks <ben@simtec.co.uk>");
> +MODULE_DESCRIPTION("S3C ADC HWMon driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:s3c-hwmon");
> Index: linux.git/arch/arm/plat-s3c/include/plat/hwmon.h
> =================================> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux.git/arch/arm/plat-s3c/include/plat/hwmon.h	2009-05-20 22:02:43.000000000 +0100
> @@ -0,0 +1,43 @@
> +/* linux/arch/arm/plat-s3c/include/plat/hwmon.h
> + *
> + * Copyright 2005 Simtec Electronics
> + *	Ben Dooks <ben@simtec.co.uk>
> + *	http://armlinux.simtec.co.uk/
> + *
> + * S3C - HWMon interface for ADC
> + *
> + * 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.
> +*/
> +
> +#ifndef __ASM_ARCH_ADC_HWMON_H
> +#define __ASM_ARCH_ADC_HWMON_H __FILE__
> +
> +/**
> + * s3c_hwmon_chcfg - channel configuration
> + * @name: The name to give this channel.
> + * @mult: Multiply the ADC value read by this.
> + * @div: Divide the value from the ADC by this.
> + *
> + * The value read from the ADC is converted to a value that
> + * hwmon expects (mV) by result = (value_read * @mult) / @div.
> + */
> +struct s3c_hwmon_chcfg {
> +	const char	*name;
> +	unsigned int	min;
> +	unsigned int	max;

Unused attributes?

> +	unsigned int	mult;
> +	unsigned int	div;
> +};
> +
> +/**
> + * s3c_hwmon_pdata - HWMON platform data
> + * @in: One configuration for each possible channel used.
> + */
> +struct s3c_hwmon_pdata {
> +	struct s3c_hwmon_chcfg	*in[8];
> +};
> +
> +#endif /* __ASM_ARCH_ADC_HWMON_H */
> +
> 


-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] HWMON: S3C24XX series ADC driver

[Ben Dooks]

Jean Delvare wrote:
> Hi Ben,
> 
> On Tue, 26 May 2009 09:07:01 +0100, Ben Dooks wrote:
>> Add support for the ADC controller on the S3C
>> series of processors to drivers/hwmon for use with
>> hardware monitoring systems.
> 
> Random comments:
> 
>> Signed-off-by: Ben Dooks <ben@simtec.co.uk>
>>
>> Index: linux.git/drivers/hwmon/Kconfig
>> =================================>> --- linux.git.orig/drivers/hwmon/Kconfig	2009-05-20 22:02:06.000000000 +0100
>> +++ linux.git/drivers/hwmon/Kconfig	2009-05-20 22:02:43.000000000 +0100
>> @@ -702,6 +702,16 @@ config SENSORS_SHT15
>>  	  This driver can also be built as a module.  If so, the module
>>  	  will be called sht15.
>>  
>> +config SENSORS_S3C_ADC
>> +	tristate "S3C24XX/S3C64XX Inbuilt ADC"
>> +	depends on HWMON && (ARCH_S3C2410 || ARCH_S3C64XX)
> 
> This item is inside a big "if HWMON" block, no need to repeat the
> condition.
> 
>> +	help
>> +	  If you say yes here you get support for the on-board ADCs of
>> +	  the Samsung S3C24XX or S3C64XX series of SoC
>> +
>> +	  This driver can also be built as a module. If so, the module
>> +	  will be called s3c-adc.
>> +
>>  config SENSORS_SIS5595
>>  	tristate "Silicon Integrated Systems Corp. SiS5595"
>>  	depends on PCI
>> Index: linux.git/drivers/hwmon/Makefile
>> =================================>> --- linux.git.orig/drivers/hwmon/Makefile	2009-05-20 22:02:06.000000000 +0100
>> +++ linux.git/drivers/hwmon/Makefile	2009-05-20 22:02:43.000000000 +0100
>> @@ -76,6 +76,7 @@ obj-$(CONFIG_SENSORS_MAX6650)	+= max6650
>>  obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
>>  obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
>>  obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
>> +obj-$(CONFIG_SENSORS_S3C_ADC)	+= s3c-adc.o
>>  obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
>>  obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595.o
>>  obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
>> Index: linux.git/drivers/hwmon/s3c-adc.c
>> =================================>> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
>> +++ linux.git/drivers/hwmon/s3c-adc.c	2009-05-26 09:00:22.000000000 +0100
>> @@ -0,0 +1,371 @@
>> +/* linux/drivers/hwmon/s3c-adc.c
>> + *
>> + * Copyright (C) 2005, 2008, 2009 Simtec Electronics
>> + *	http://armlinux.simtec.co.uk/
>> + *	Ben Dooks <ben@simtec.co.uk>
>> + *
>> + * S3C24XX/S3C64XX ADC hwmon support
>> + *
>> + * 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.
>> + *
>> + * 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., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>> +*/
>> +
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/init.h>
>> +#include <linux/err.h>
>> +#include <linux/clk.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +
>> +#include <plat/adc.h>
>> +#include <plat/hwmon.h>
>> +
>> +/**
>> + * struct s3c_hwmon - ADC hwmon client information
>> + * @lock: Access lock for conversion.
> 
> This gives us zero idea what the lock is protecting.

changed to:
  @lock: Access lock to serialise the conversions.

>> + * @wait: Wait queue for conversions to complete.
>> + * @client: The client we registered with the S3C ADC core.
>> + * @dev: The platform device we bound to.
>> + * @hwmon_dev: The hwmon device we created.
>> + * @in_attr: The device attributes we created.
>> +*/
>> +struct s3c_hwmon {
>> +	struct semaphore	lock;
> 
> 
>> +	wait_queue_head_t	wait;
>> +	int			val;
> 
> This is an horribly vague struct member name, and undocumented at that.

Added documentation and renamed to ret_val.

  * @ret_val: Value returned from current conversion to return to caller.

>> +
>> +	struct s3c_adc_client	*client;
>> +	struct platform_device	*dev;
> 
> The fact that you need this suggests a wrong calling convention
> somewhere in the code.
> 
>> +	struct device		*hwmon_dev;
>> +
>> +	struct sensor_device_attribute *in_attr[8];
> 
> What is the benefit of allocating these attributes dynamically? You are
> likely to fragment your memory and require ugly code to make it work.
> 
>> +};
>> +
>> +static inline struct s3c_hwmon *dev_to_ourhwmon(struct platform_device *dev)
>> +{
>> +	return (struct s3c_hwmon *)platform_get_drvdata(dev);
> 
> Useless cast. Not to mention that this function is pretty much
> pointless... just call platform_get_drvdata directly.

ok, removed.

>> +}
>> +
>> +static struct s3c_hwmon *done_adc;
> 
> What is this?

The core adc driver doesn't keep any private data, so we need
this to get back to our state when the conversion ends.

>> +
>> +/**
>> + * s3c_hwmon_adcdone - ADC core callback
>> + * @value: The value that we got from the ADC core
>> + * @ignore: Only used for the touchscreen client.
>> + * @left: The number of conversions left (not used here).
>> + *
>> + * This is called when the ADC has finished its conversion to
>> + * inform us of the result.
>> + */
>> +static void s3c_hwmon_adcdone(unsigned value, unsigned ignore, unsigned *left)
>> +{
>> +	struct s3c_hwmon *hwmon = done_adc;
>> +
>> +	hwmon->val = value;
>> +	wake_up(&hwmon->wait);
>> +}
>> +
>> +/**
>> + * s3c_hwmon_read_ch - read a value from a given adc channel.
>> + * @hwmon: Our state.
>> + * @channel: The channel we're reading from.
>> + *
>> + * Read a value from the @channel with the proper locking and sleep until
>> + * either the read completes or we timeout awaiting the ADC core to get
>> + * back to us.
>> + */
>> +static int s3c_hwmon_read_ch(struct s3c_hwmon *hwmon, int channel)
>> +{
>> +	unsigned long timeout;
>> +	int ret;
>> +
>> +	ret = down_interruptible(&hwmon->lock);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	dev_dbg(&hwmon->dev->dev, "reading channel %d\n", channel);
>> +
>> +	hwmon->val = -1;
>> +	done_adc = hwmon;
>> +
>> +	ret = s3c_adc_start(hwmon->client, channel, 1);
>> +	if (ret < 0)
>> +		goto err;
>> +
>> +	timeout = wait_event_timeout(hwmon->wait, hwmon->val >= 0, HZ / 2);
>> +	ret = (timeout = 0) ? -ETIMEDOUT : hwmon->val;
>> +
>> + err:
>> +	up(&hwmon->lock);
>> +	return ret;
>> +}
>> +
>> +/**
>> + * s3c_hwmon_show_raw - show a conversion from the raw channel number.
>> + * @dev: The device that the attribute belongs to.
>> + * @attr: The attribute being read.
>> + * @buf: The result buffer.
>> + *
>> + * This show deals with the raw attribute, registered for each possible
>> + * ADC channel. This does a conversion and returns the raw (un-scaled)
>> + * value returned from the hardware.
>> + */
>> +static ssize_t s3c_hwmon_show_raw(struct device *dev,
>> +				  struct device_attribute *attr, char *buf)
>> +{
>> +	struct s3c_hwmon *adc = dev_to_ourhwmon(to_platform_device(dev));
> 
> aka dev_get_drvdata().

removed.

>> +	int ret, nr;
>> +
>> +	nr = attr->attr.name[3] - '0';
> 
> This is pretty fragile. Please use SENSOR_ATTR() instead.

ok, done.

>> +	ret = s3c_hwmon_read_ch(adc, nr);
>> +
>> +	return  (ret < 0) ? ret : snprintf(buf, PAGE_SIZE, "%d\n", ret);
>> +}
>> +
>> +#define DEF_ADC_ATTR(x)	\
>> +	static DEVICE_ATTR(adc##x##_raw, S_IRUGO, s3c_hwmon_show_raw, NULL)
>> +
>> +DEF_ADC_ATTR(0);
>> +DEF_ADC_ATTR(1);
>> +DEF_ADC_ATTR(2);
>> +DEF_ADC_ATTR(3);
>> +DEF_ADC_ATTR(4);
>> +DEF_ADC_ATTR(5);
>> +DEF_ADC_ATTR(6);
>> +DEF_ADC_ATTR(7);
>> +
>> +static struct device_attribute *s3c_hwmon_attrs[8] = {
>> +	&dev_attr_adc0_raw,
>> +	&dev_attr_adc1_raw,
>> +	&dev_attr_adc2_raw,
>> +	&dev_attr_adc3_raw,
>> +	&dev_attr_adc4_raw,
>> +	&dev_attr_adc5_raw,
>> +	&dev_attr_adc6_raw,
>> +	&dev_attr_adc7_raw,
>> +};
> 
> This looks like debugging stuff. Does it really need to stay in the
> final code? I would make it conditional, at least.

Added config to include them.

>> +
>> +/**
>> + * s3c_hwmon_ch_show - show value of a given channel
>> + * @dev: The device that the attribute belongs to.
>> + * @attr: The attribute being read.
>> + * @buf: The result buffer.
>> + *
>> + * Read a value from the ADC and scale it before returning it to the
>> + * caller. The scale factor is gained from the channel configuration
>> + * passed via the platform data when the device was registered.
>> + */
>> +static ssize_t s3c_hwmon_ch_show(struct device *dev,
>> +				 struct device_attribute *attr,
>> +				 char *buf)
>> +{
>> +	struct sensor_device_attribute *sen_attr = to_sensor_dev_attr(attr);
>> +	struct s3c_hwmon_pdata *pdata = dev->platform_data;
>> +	struct s3c_hwmon *hwmon = dev_to_ourhwmon(to_platform_device(dev));
>> +	struct s3c_hwmon_chcfg *cfg;
>> +
>> +	int ret;
>> +
>> +	cfg = pdata->in[sen_attr->index];
>> +	if (!cfg)
>> +		return -EINVAL;
>> +
>> +	ret = s3c_hwmon_read_ch(hwmon, sen_attr->index);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret *= cfg->mult;
>> +	ret /= cfg->div;
> 
> Division lacks rounding.
>

Which rounding should be used?

>> +
>> +	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
>> +}
>> +
>> +#define MAX_LABEL (16)
>> +
>> +/**
>> + * s3c_hwmon_create_attr - create hwmon attribute for given channel.
>> + * @hwmon: Our hwmon instance.
>> + * @pdata: The platform data provided by the device.
>> + * @channel: The ADC channel number to process.
>> + *
>> + * Create the scaled attribute for use with hwmon from the specified
>> + * platform data in @pdata. The sysfs entry is handled by the routine
>> + * s3c_hwmon_ch_show().
>> + *
>> + * The attribute name is taken from the configuration data if present
>> + * otherwise the name is taken by concatenating in_ with the channel
>> + * number.
>> + */
>> +static int s3c_hwmon_create_attr(struct s3c_hwmon *hwmon,
>> +				 struct s3c_hwmon_pdata *pdata,
>> +				 int channel)
>> +{
>> +	struct s3c_hwmon_chcfg *cfg = pdata->in[channel];
>> +	struct sensor_device_attribute *attr;
>> +	const char *name;
>> +
>> +	if (!cfg)
>> +		return 0;
>> +
>> +	attr = kzalloc(sizeof(*attr) + MAX_LABEL, GFP_KERNEL);
>> +	if (attr = NULL)
>> +		return -ENOMEM;
>> +
>> +	if (cfg->name)
>> +		name = cfg->name;
>> +	else {
>> +		name = (char *)(attr+1);
>> +		snprintf((char *)(attr+1), MAX_LABEL, "in_%d", channel);
> 
> This name doesn't match what is described in
> Documentation/hwmon/sysfs-interface, which means that your driver won't
> work with libsensors. This is a blocker.

ok, fixed.

>> +	}
>> +
>> +	attr->dev_attr.attr.name  = name;
>> +	attr->dev_attr.attr.mode  = S_IRUGO;
>> +	attr->dev_attr.attr.owner = THIS_MODULE;
>> +	attr->dev_attr.show = s3c_hwmon_ch_show;
>> +
>> +	attr->index = channel;
>> +	hwmon->in_attr[channel] = attr;
>> +
>> +	return device_create_file(&hwmon->dev->dev, &attr->dev_attr);
>> +}
>> +
>> +/**
>> + * s3c_hwmon_probe - device probe entry.
>> + * @dev: The device being probed.
>> +*/
>> +static int s3c_hwmon_probe(struct platform_device *dev)
> 
> No __devinit?

fixed.

>> +{
>> +	struct s3c_hwmon_pdata *pdata = dev->dev.platform_data;
>> +	struct s3c_hwmon *hwmon;
>> +	int ret = 0;
>> +	int i;
>> +
>> +	hwmon = kzalloc(sizeof(struct s3c_hwmon), GFP_KERNEL);
>> +	if (hwmon = NULL) {
>> +		dev_err(&dev->dev, "no memory\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	platform_set_drvdata(dev, hwmon);
>> +	hwmon->dev = dev;
>> +
>> +	/* only enable the clock when we are actually using the adc */
>> +
>> +	init_waitqueue_head(&hwmon->wait);
>> +	init_MUTEX(&hwmon->lock);
>> +
>> +	/* Register with the core ADC driver. */
>> +
>> +	hwmon->client = s3c_adc_register(dev, NULL, s3c_hwmon_adcdone, 0);
>> +	if (IS_ERR(hwmon->client)) {
>> +		dev_err(&dev->dev, "cannot register adc\n");
>> +		ret = PTR_ERR(hwmon->client);
>> +		goto err_mem;
>> +	}
>> +
>> +	/* add attributes for our adc devices. */
>> +
>> +	for (i = 0; i < ARRAY_SIZE(s3c_hwmon_attrs); i++) {
>> +		ret = device_create_file(&dev->dev, s3c_hwmon_attrs[i]);
>> +		if (ret) {
>> +			dev_err(&dev->dev, "error creating channel %d\n", i);
>> +
>> +			for (i--; i >= 0; i--)
>> +				device_remove_file(&dev->dev, s3c_hwmon_attrs[i]);
>> +
>> +			goto err_registered;
>> +		}
>> +	}
>> +
>> +	/* register with the hwmon core */
>> +
>> +	hwmon->hwmon_dev = hwmon_device_register(&dev->dev);
>> +	if (IS_ERR(hwmon->hwmon_dev)) {
>> +		dev_err(&dev->dev, "error registering with hwmon\n");
>> +		ret = PTR_ERR(hwmon->hwmon_dev);
>> +		goto err_attribute;
>> +	}
>> +
>> +	if (pdata) {
>> +		for (i = 0; i < ARRAY_SIZE(pdata->in); i++)
>> +			s3c_hwmon_create_attr(hwmon, pdata, i);
> 
> No error check?

added

>> +	}
>> +
>> +	return 0;
>> +
>> + err_attribute:
>> +	for (i = 0; i < ARRAY_SIZE(s3c_hwmon_attrs); i++)
>> +		device_remove_file(&dev->dev, s3c_hwmon_attrs[i]);
>> +
>> + err_registered:
>> +	s3c_adc_release(hwmon->client);
>> +
>> + err_mem:
>> +	kfree(hwmon);
>> +	return ret;
>> +}
>> +
>> +static int __devexit s3c_hwmon_remove(struct platform_device *dev)
>> +{
>> +	struct s3c_hwmon *hwmon = dev_to_ourhwmon(dev);
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(s3c_hwmon_attrs); i++)
>> +		device_remove_file(&dev->dev, s3c_hwmon_attrs[i]);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(hwmon->in_attr); i++) {
>> +		if (!hwmon->in_attr[i])
>> +			continue;
>> +
>> +		device_remove_file(&dev->dev, &hwmon->in_attr[i]->dev_attr);
>> +		kfree(hwmon->in_attr[i]);
>> +	}
>> +
>> +	hwmon_device_unregister(hwmon->hwmon_dev);
>> +	s3c_adc_release(hwmon->client);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver s3c_hwmon_driver = {
>> +	.driver	= {
>> +		.name		= "s3c-hwmon",
> 
> Please make this match the module name.

right, will rename driver file to s3c-hwmon.c

>> +		.owner		= THIS_MODULE,
>> +	},
>> +	.probe		= s3c_hwmon_probe,
>> +	.remove		= __devexit_p(s3c_hwmon_remove),
>> +};
>> +
>> +static int __init s3c_hwmon_init(void)
>> +{
>> +	return platform_driver_register(&s3c_hwmon_driver);
>> +}
>> +
>> +static void __exit s3c_hwmon_exit(void)
>> +{
>> +	platform_driver_unregister(&s3c_hwmon_driver);
>> +}
>> +
>> +module_init(s3c_hwmon_init);
>> +module_exit(s3c_hwmon_exit);
>> +
>> +MODULE_AUTHOR("Ben Dooks <ben@simtec.co.uk>");
>> +MODULE_DESCRIPTION("S3C ADC HWMon driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:s3c-hwmon");
>> Index: linux.git/arch/arm/plat-s3c/include/plat/hwmon.h
>> =================================>> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
>> +++ linux.git/arch/arm/plat-s3c/include/plat/hwmon.h	2009-05-20 22:02:43.000000000 +0100
>> @@ -0,0 +1,43 @@
>> +/* linux/arch/arm/plat-s3c/include/plat/hwmon.h
>> + *
>> + * Copyright 2005 Simtec Electronics
>> + *	Ben Dooks <ben@simtec.co.uk>
>> + *	http://armlinux.simtec.co.uk/
>> + *
>> + * S3C - HWMon interface for ADC
>> + *
>> + * 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.
>> +*/
>> +
>> +#ifndef __ASM_ARCH_ADC_HWMON_H
>> +#define __ASM_ARCH_ADC_HWMON_H __FILE__
>> +
>> +/**
>> + * s3c_hwmon_chcfg - channel configuration
>> + * @name: The name to give this channel.
>> + * @mult: Multiply the ADC value read by this.
>> + * @div: Divide the value from the ADC by this.
>> + *
>> + * The value read from the ADC is converted to a value that
>> + * hwmon expects (mV) by result = (value_read * @mult) / @div.
>> + */
>> +struct s3c_hwmon_chcfg {
>> +	const char	*name;
>> +	unsigned int	min;
>> +	unsigned int	max;
> 
> Unused attributes?

will remove.

>> +	unsigned int	mult;
>> +	unsigned int	div;
>> +};
>> +
>> +/**
>> + * s3c_hwmon_pdata - HWMON platform data
>> + * @in: One configuration for each possible channel used.
>> + */
>> +struct s3c_hwmon_pdata {
>> +	struct s3c_hwmon_chcfg	*in[8];
>> +};
>> +
>> +#endif /* __ASM_ARCH_ADC_HWMON_H */

-- 
Ben Dooks, Software Engineer, Simtec Electronics

http://www.simtec.co.uk/

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] HWMON: S3C24XX series ADC driver

[Jean Delvare]

On Thu, 28 May 2009 12:22:41 +0100, Ben Dooks wrote:
> Jean Delvare wrote:
> > On Tue, 26 May 2009 09:07:01 +0100, Ben Dooks wrote:
> >> +	int			val;
> > 
> > This is an horribly vague struct member name, and undocumented at that.
> 
> Added documentation and renamed to ret_val.
> 
>   * @ret_val: Value returned from current conversion to return to caller.

This seems artificially complex. You need to wait for the conversion to
complete anyway, so why bother with an asynchronous mechanism?


> >> +static struct s3c_hwmon *done_adc;
> > 
> > What is this?
> 
> The core adc driver doesn't keep any private data, so we need
> this to get back to our state when the conversion ends.

This will break if you have two "s3c-hwmon" devices on the same system.
Why don't you just fix the core adc driver?


> >> +	ret *= cfg->mult;
> >> +	ret /= cfg->div;
> > 
> > Division lacks rounding.
> 
> Which rounding should be used?

DIV_ROUND_CLOSEST()

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] HWMON: S3C24XX series ADC driver

[Ben Dooks]

Jean Delvare wrote:
> On Thu, 28 May 2009 12:22:41 +0100, Ben Dooks wrote:
>> Jean Delvare wrote:
>>> On Tue, 26 May 2009 09:07:01 +0100, Ben Dooks wrote:
>>>> +	int			val;
>>> This is an horribly vague struct member name, and undocumented at that.
>> Added documentation and renamed to ret_val.
>>
>>   * @ret_val: Value returned from current conversion to return to caller.
> 
> This seems artificially complex. You need to wait for the conversion to
> complete anyway, so why bother with an asynchronous mechanism?

I'll add a sync conversion call to the adc core driver
and this'll get rid of both of these.

> 
>>>> +static struct s3c_hwmon *done_adc;
>>> What is this?
>> The core adc driver doesn't keep any private data, so we need
>> this to get back to our state when the conversion ends.
> 
> This will break if you have two "s3c-hwmon" devices on the same system.
> Why don't you just fix the core adc driver?
> 
> 
>>>> +	ret *= cfg->mult;
>>>> +	ret /= cfg->div;
>>> Division lacks rounding.
>> Which rounding should be used?
> 
> DIV_ROUND_CLOSEST()

ok, will change.

-- 
Ben Dooks, Software Engineer, Simtec Electronics

http://www.simtec.co.uk/

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] HWMON: S3C24XX series ADC driver

[Ben Dooks]

Jean Delvare wrote:
> Hi Ben,
> 

[snip]

>> +	struct device		*hwmon_dev;
>> +
>> +	struct sensor_device_attribute *in_attr[8];
> 
> What is the benefit of allocating these attributes dynamically? You are
> likely to fragment your memory and require ugly code to make it work.

I suppose if we're only looking at producing a single in
or in and label attribute then it is only 24+name bytes
for each attribute.

[snip]

>> +static int s3c_hwmon_create_attr(struct s3c_hwmon *hwmon,
>> +				 struct s3c_hwmon_pdata *pdata,
>> +				 int channel)
>> +{
>> +	struct s3c_hwmon_chcfg *cfg = pdata->in[channel];
>> +	struct sensor_device_attribute *attr;
>> +	const char *name;
>> +
>> +	if (!cfg)
>> +		return 0;
>> +
>> +	attr = kzalloc(sizeof(*attr) + MAX_LABEL, GFP_KERNEL);
>> +	if (attr = NULL)
>> +		return -ENOMEM;
>> +
>> +	if (cfg->name)
>> +		name = cfg->name;
>> +	else {
>> +		name = (char *)(attr+1);
>> +		snprintf((char *)(attr+1), MAX_LABEL, "in_%d", channel);
> 
> This name doesn't match what is described in
> Documentation/hwmon/sysfs-interface, which means that your driver won't
> work with libsensors. This is a blocker.

Ah, having re-read it, it seems the name should go in in%d_label sysfs 
field as well as changing in_%d to in%d_input.

Ben Dooks, Software Engineer, Simtec Electronics

http://www.simtec.co.uk/

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] HWMON: S3C24XX series ADC driver

[Jean Delvare]

On Thu, 28 May 2009 16:21:34 +0100, Ben Dooks wrote:
> Jean Delvare wrote:
> > Hi Ben,
> > 
> 
> [snip]
> 
> >> +	struct device		*hwmon_dev;
> >> +
> >> +	struct sensor_device_attribute *in_attr[8];
> > 
> > What is the benefit of allocating these attributes dynamically? You are
> > likely to fragment your memory and require ugly code to make it work.
> 
> I suppose if we're only looking at producing a single in
> or in and label attribute then it is only 24+name bytes
> for each attribute.
> 
> [snip]
> 
> >> +static int s3c_hwmon_create_attr(struct s3c_hwmon *hwmon,
> >> +				 struct s3c_hwmon_pdata *pdata,
> >> +				 int channel)
> >> +{
> >> +	struct s3c_hwmon_chcfg *cfg = pdata->in[channel];
> >> +	struct sensor_device_attribute *attr;
> >> +	const char *name;
> >> +
> >> +	if (!cfg)
> >> +		return 0;
> >> +
> >> +	attr = kzalloc(sizeof(*attr) + MAX_LABEL, GFP_KERNEL);
> >> +	if (attr = NULL)
> >> +		return -ENOMEM;
> >> +
> >> +	if (cfg->name)
> >> +		name = cfg->name;
> >> +	else {
> >> +		name = (char *)(attr+1);
> >> +		snprintf((char *)(attr+1), MAX_LABEL, "in_%d", channel);
> > 
> > This name doesn't match what is described in
> > Documentation/hwmon/sysfs-interface, which means that your driver won't
> > work with libsensors. This is a blocker.
> 
> Ah, having re-read it, it seems the name should go in in%d_label sysfs 
> field as well as changing in_%d to in%d_input.

Correct. Then you no longer need dynamic attribute names, which will
make your code much more simple.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[lm-sensors] HWMON: S3C24XX series ADC driver

[Ben Dooks]

Add support for the ADC controller on the S3C
series of processors to drivers/hwmon for use with
hardware monitoring systems.

Signed-off-by: Ben Dooks <ben@simtec.co.uk>

Index: linux.git/drivers/hwmon/Kconfig
=================================--- linux.git.orig/drivers/hwmon/Kconfig	2009-05-28 15:28:32.000000000 +0100
+++ linux.git/drivers/hwmon/Kconfig	2009-05-28 15:39:44.000000000 +0100
@@ -702,6 +702,23 @@ config SENSORS_SHT15
 	  This driver can also be built as a module.  If so, the module
 	  will be called sht15.
 
+config SENSORS_S3C
+	tristate "S3C24XX/S3C64XX Inbuilt ADC"
+	depends on ARCH_S3C2410 || ARCH_S3C64XX
+	help
+	  If you say yes here you get support for the on-board ADCs of
+	  the Samsung S3C24XX or S3C64XX series of SoC
+
+	  This driver can also be built as a module. If so, the module
+	  will be called s3c-adc.
+
+config SENSORS_S3C_RAW
+	bool "Include raw channel attributes in sysfs"
+	depends on SENSORS_S3C
+	help
+	  Say Y here if you want to include raw copies of all the ADC
+	  channels in sysfs.
+
 config SENSORS_SIS5595
 	tristate "Silicon Integrated Systems Corp. SiS5595"
 	depends on PCI
Index: linux.git/drivers/hwmon/Makefile
=================================--- linux.git.orig/drivers/hwmon/Makefile	2009-05-28 15:28:32.000000000 +0100
+++ linux.git/drivers/hwmon/Makefile	2009-05-28 15:39:44.000000000 +0100
@@ -76,6 +76,7 @@ obj-$(CONFIG_SENSORS_MAX6650)	+= max6650
 obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
 obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
+obj-$(CONFIG_SENSORS_S3C)	+= s3c-hwmon.o
 obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
 obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595.o
 obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
Index: linux.git/drivers/hwmon/s3c-hwmon.c
=================================--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux.git/drivers/hwmon/s3c-hwmon.c	2009-05-28 21:09:39.000000000 +0100
@@ -0,0 +1,413 @@
+/* linux/drivers/hwmon/s3c-hwmon.c
+ *
+ * Copyright (C) 2005, 2008, 2009 Simtec Electronics
+ *	http://armlinux.simtec.co.uk/
+ *	Ben Dooks <ben@simtec.co.uk>
+ *
+ * S3C24XX/S3C64XX ADC hwmon support
+ *
+ * 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.
+ *
+ * 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., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+*/
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+
+#include <plat/adc.h>
+#include <plat/hwmon.h>
+
+struct s3c_hwmon_attr {
+	struct sensor_device_attribute	in;
+	struct sensor_device_attribute	label;
+	char				in_name[12];
+	char				label_name[12];
+};
+
+/**
+ * struct s3c_hwmon - ADC hwmon client information
+ * @lock: Access lock to serialise the conversions.
+ * @client: The client we registered with the S3C ADC core.
+ * @hwmon_dev: The hwmon device we created.
+ * @attr: The holders for the channel attributes.
+*/
+struct s3c_hwmon {
+	struct semaphore	lock;
+	struct s3c_adc_client	*client;
+	struct device		*hwmon_dev;
+
+	struct s3c_hwmon_attr	attrs[8];
+};
+
+/**
+ * s3c_hwmon_read_ch - read a value from a given adc channel.
+ * @dev: The device.
+ * @hwmon: Our state.
+ * @channel: The channel we're reading from.
+ *
+ * Read a value from the @channel with the proper locking and sleep until
+ * either the read completes or we timeout awaiting the ADC core to get
+ * back to us.
+ */
+static int s3c_hwmon_read_ch(struct device *dev,
+			     struct s3c_hwmon *hwmon, int channel)
+{
+	int ret;
+
+	ret = down_interruptible(&hwmon->lock);
+	if (ret < 0)
+		return ret;
+
+	dev_dbg(dev, "reading channel %d\n", channel);
+
+	ret = s3c_adc_read(hwmon->client, channel);
+	up(&hwmon->lock);
+
+	return ret;
+}
+
+#ifdef CONFIG_SENSORS_S3C_RAW
+/**
+ * s3c_hwmon_show_raw - show a conversion from the raw channel number.
+ * @dev: The device that the attribute belongs to.
+ * @attr: The attribute being read.
+ * @buf: The result buffer.
+ *
+ * This show deals with the raw attribute, registered for each possible
+ * ADC channel. This does a conversion and returns the raw (un-scaled)
+ * value returned from the hardware.
+ */
+static ssize_t s3c_hwmon_show_raw(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct s3c_hwmon *adc = platform_get_drvdata(to_platform_device(dev));
+	struct sensor_device_attribute *sa = to_sensor_dev_attr(attr);
+	int ret;
+
+	ret = s3c_hwmon_read_ch(dev, adc, sa->index);
+
+	return  (ret < 0) ? ret : snprintf(buf, PAGE_SIZE, "%d\n", ret);
+}
+
+#define DEF_ADC_ATTR(x)	\
+	static SENSOR_DEVICE_ATTR(adc##x##_raw, S_IRUGO, s3c_hwmon_show_raw, NULL, x)
+
+DEF_ADC_ATTR(0);
+DEF_ADC_ATTR(1);
+DEF_ADC_ATTR(2);
+DEF_ADC_ATTR(3);
+DEF_ADC_ATTR(4);
+DEF_ADC_ATTR(5);
+DEF_ADC_ATTR(6);
+DEF_ADC_ATTR(7);
+
+static struct device_attribute *s3c_hwmon_attrs[8] = {
+	&sensor_dev_attr_adc0_raw.dev_attr,
+	&sensor_dev_attr_adc1_raw.dev_attr,
+	&sensor_dev_attr_adc2_raw.dev_attr,
+	&sensor_dev_attr_adc3_raw.dev_attr,
+	&sensor_dev_attr_adc4_raw.dev_attr,
+	&sensor_dev_attr_adc5_raw.dev_attr,
+	&sensor_dev_attr_adc6_raw.dev_attr,
+	&sensor_dev_attr_adc7_raw.dev_attr,
+};
+
+static inline int s3c_hwmon_add_raw(struct device *dev)
+{
+	int ret, i;
+
+	for (i = 0; i < ARRAY_SIZE(s3c_hwmon_attrs); i++) {
+		ret = device_create_file(dev, s3c_hwmon_attrs[i]);
+		if (ret) {
+			dev_err(dev, "error creating channel %d\n", i);
+
+			for (i--; i >= 0; i--)
+				device_remove_file(dev, s3c_hwmon_attrs[i]);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static inline void s3c_hwmon_remove_raw(struct device *dev)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(s3c_hwmon_attrs); i++)
+		device_remove_file(dev, s3c_hwmon_attrs[i]);
+}
+
+#else
+
+static inline int s3c_hwmon_add_raw(struct device *dev) { return 0; }
+static inline void s3c_hwmon_remove_raw(struct device *dev) { }
+
+#endif /* CONFIG_SENSORS_S3C_RAW */
+
+/**
+ * s3c_hwmon_ch_show - show value of a given channel
+ * @dev: The device that the attribute belongs to.
+ * @attr: The attribute being read.
+ * @buf: The result buffer.
+ *
+ * Read a value from the ADC and scale it before returning it to the
+ * caller. The scale factor is gained from the channel configuration
+ * passed via the platform data when the device was registered.
+ */
+static ssize_t s3c_hwmon_ch_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct sensor_device_attribute *sen_attr = to_sensor_dev_attr(attr);
+	struct s3c_hwmon *hwmon = platform_get_drvdata(to_platform_device(dev));
+	struct s3c_hwmon_pdata *pdata = dev->platform_data;
+	struct s3c_hwmon_chcfg *cfg;
+	int ret;
+
+	cfg = pdata->in[sen_attr->index];
+	if (!cfg)
+		return -EINVAL;
+
+	ret = s3c_hwmon_read_ch(dev, hwmon, sen_attr->index);
+	if (ret < 0)
+		return ret;
+
+	ret *= cfg->mult;
+	ret = DIV_ROUND_CLOSEST(ret, cfg->div);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
+}
+
+/**
+ * s3c_hwmon_label_show - show label name of the given channel.
+ * @dev: The device that the attribute belongs to.
+ * @attr: The attribute being read.
+ * @buf: The result buffer.
+ *
+ * Return the label name of a given channel
+ */
+static ssize_t s3c_hwmon_label_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	struct sensor_device_attribute *sen_attr = to_sensor_dev_attr(attr);
+	struct s3c_hwmon_pdata *pdata = dev->platform_data;
+	struct s3c_hwmon_chcfg *cfg;
+
+	cfg = pdata->in[sen_attr->index];
+	if (!cfg || !cfg->name)
+		return -EINVAL;
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", cfg->name);
+}
+
+
+/**
+ * s3c_hwmon_create_attr - create hwmon attribute for given channel.
+ * @dev: The device to create the attribute on.
+ * @cfg: The channel configuration passed from the platform data.
+ * @channel: The ADC channel number to process.
+ *
+ * Create the scaled attribute for use with hwmon from the specified
+ * platform data in @pdata. The sysfs entry is handled by the routine
+ * s3c_hwmon_ch_show().
+ *
+ * The attribute name is taken from the configuration data if present
+ * otherwise the name is taken by concatenating in_ with the channel
+ * number.
+ */
+static int s3c_hwmon_create_attr(struct device *dev,
+				 struct s3c_hwmon_chcfg *cfg,
+				 struct s3c_hwmon_attr *attrs,
+				 int channel)
+{
+	struct sensor_device_attribute *attr;
+	int ret;
+
+	if (!cfg)
+		return 0;
+
+	snprintf(attrs->in_name, sizeof(attrs->in_name), "in%d_input", channel);
+
+	attr = &attrs->in;
+	attr->index = channel;
+	attr->dev_attr.attr.name  = attrs->in_name;
+	attr->dev_attr.attr.mode  = S_IRUGO;
+	attr->dev_attr.attr.owner = THIS_MODULE;
+	attr->dev_attr.show = s3c_hwmon_ch_show;
+
+	ret =  device_create_file(dev, &attr->dev_attr);
+	if (ret < 0) {
+		dev_err(dev, "failed to create input attribute\n");
+		return ret;
+	}
+
+	/* if this has a name, add a label */
+	if (cfg->name) {
+		snprintf(attrs->label_name, sizeof(attrs->label_name),
+			 "in%d_label", channel);
+
+		attr = &attrs->label;
+		attr->index = channel;
+		attr->dev_attr.attr.name  = attrs->label_name;
+		attr->dev_attr.attr.mode  = S_IRUGO;
+		attr->dev_attr.attr.owner = THIS_MODULE;
+		attr->dev_attr.show = s3c_hwmon_label_show;
+
+		ret = device_create_file(dev, &attr->dev_attr);
+		if (ret < 0) {
+			device_remove_file(dev, &attrs->in.dev_attr);
+			dev_err(dev, "failed to create label attribute\n");
+		}
+	}
+
+	return ret;
+}
+
+static void s3c_hwmon_remove_attr(struct device *dev,
+				  struct s3c_hwmon_attr *attrs)
+{
+	device_remove_file(dev, &attrs->in.dev_attr);
+	device_remove_file(dev, &attrs->label.dev_attr);
+}
+
+/**
+ * s3c_hwmon_probe - device probe entry.
+ * @dev: The device being probed.
+*/
+static int __devinit s3c_hwmon_probe(struct platform_device *dev)
+{
+	struct s3c_hwmon_pdata *pdata = dev->dev.platform_data;
+	struct s3c_hwmon *hwmon;
+	int ret = 0;
+	int i;
+
+	hwmon = kzalloc(sizeof(struct s3c_hwmon), GFP_KERNEL);
+	if (hwmon = NULL) {
+		dev_err(&dev->dev, "no memory\n");
+		return -ENOMEM;
+	}
+
+	platform_set_drvdata(dev, hwmon);
+
+	init_MUTEX(&hwmon->lock);
+
+	/* Register with the core ADC driver. */
+
+	hwmon->client = s3c_adc_register(dev, NULL, NULL, NULL, 0);
+	if (IS_ERR(hwmon->client)) {
+		dev_err(&dev->dev, "cannot register adc\n");
+		ret = PTR_ERR(hwmon->client);
+		goto err_mem;
+	}
+
+	/* add attributes for our adc devices. */
+
+	ret = s3c_hwmon_add_raw(&dev->dev);
+	if (ret)
+		goto err_registered;
+
+	/* register with the hwmon core */
+
+	hwmon->hwmon_dev = hwmon_device_register(&dev->dev);
+	if (IS_ERR(hwmon->hwmon_dev)) {
+		dev_err(&dev->dev, "error registering with hwmon\n");
+		ret = PTR_ERR(hwmon->hwmon_dev);
+		goto err_raw_attribute;
+	}
+
+	if (pdata) {
+		for (i = 0; i < ARRAY_SIZE(pdata->in); i++) {
+			ret = s3c_hwmon_create_attr(&dev->dev, pdata->in[i],
+						    &hwmon->attrs[i], i);
+			if (ret) {
+				dev_err(&dev->dev,
+					"error creating channel %d\n", i);
+
+				for (i--; i >= 0; i--)
+					s3c_hwmon_remove_attr(&dev->dev,
+							      &hwmon->attrs[i]);
+
+				goto err_hwmon_register;
+			}
+		}
+	}
+
+	return 0;
+
+ err_hwmon_register:
+	hwmon_device_unregister(hwmon->hwmon_dev);
+
+ err_raw_attribute:
+	s3c_hwmon_remove_raw(&dev->dev);
+
+ err_registered:
+	s3c_adc_release(hwmon->client);
+
+ err_mem:
+	kfree(hwmon);
+	return ret;
+}
+
+static int __devexit s3c_hwmon_remove(struct platform_device *dev)
+{
+	struct s3c_hwmon *hwmon = platform_get_drvdata(dev);
+	int i;
+
+	s3c_hwmon_remove_raw(&dev->dev);
+
+	for (i = 0; i < ARRAY_SIZE(hwmon->attrs); i++)
+		s3c_hwmon_remove_attr(&dev->dev, &hwmon->attrs[i]);
+
+	hwmon_device_unregister(hwmon->hwmon_dev);
+	s3c_adc_release(hwmon->client);
+
+	return 0;
+}
+
+static struct platform_driver s3c_hwmon_driver = {
+	.driver	= {
+		.name		= "s3c-hwmon",
+		.owner		= THIS_MODULE,
+	},
+	.probe		= s3c_hwmon_probe,
+	.remove		= __devexit_p(s3c_hwmon_remove),
+};
+
+static int __init s3c_hwmon_init(void)
+{
+	return platform_driver_register(&s3c_hwmon_driver);
+}
+
+static void __exit s3c_hwmon_exit(void)
+{
+	platform_driver_unregister(&s3c_hwmon_driver);
+}
+
+module_init(s3c_hwmon_init);
+module_exit(s3c_hwmon_exit);
+
+MODULE_AUTHOR("Ben Dooks <ben@simtec.co.uk>");
+MODULE_DESCRIPTION("S3C ADC HWMon driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:s3c-hwmon");
Index: linux.git/arch/arm/plat-s3c/include/plat/hwmon.h
=================================--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux.git/arch/arm/plat-s3c/include/plat/hwmon.h	2009-05-28 15:39:44.000000000 +0100
@@ -0,0 +1,41 @@
+/* linux/arch/arm/plat-s3c/include/plat/hwmon.h
+ *
+ * Copyright 2005 Simtec Electronics
+ *	Ben Dooks <ben@simtec.co.uk>
+ *	http://armlinux.simtec.co.uk/
+ *
+ * S3C - HWMon interface for ADC
+ *
+ * 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.
+*/
+
+#ifndef __ASM_ARCH_ADC_HWMON_H
+#define __ASM_ARCH_ADC_HWMON_H __FILE__
+
+/**
+ * s3c_hwmon_chcfg - channel configuration
+ * @name: The name to give this channel.
+ * @mult: Multiply the ADC value read by this.
+ * @div: Divide the value from the ADC by this.
+ *
+ * The value read from the ADC is converted to a value that
+ * hwmon expects (mV) by result = (value_read * @mult) / @div.
+ */
+struct s3c_hwmon_chcfg {
+	const char	*name;
+	unsigned int	mult;
+	unsigned int	div;
+};
+
+/**
+ * s3c_hwmon_pdata - HWMON platform data
+ * @in: One configuration for each possible channel used.
+ */
+struct s3c_hwmon_pdata {
+	struct s3c_hwmon_chcfg	*in[8];
+};
+
+#endif /* __ASM_ARCH_ADC_HWMON_H */
+

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] HWMON: S3C24XX series ADC driver

[Jean Delvare]

Hi Ben,

On Thu, 28 May 2009 21:42:20 +0100, Ben Dooks wrote:
> Add support for the ADC controller on the S3C
> series of processors to drivers/hwmon for use with
> hardware monitoring systems.
> 
> Signed-off-by: Ben Dooks <ben@simtec.co.uk>

Second review:

> 
> Index: linux.git/drivers/hwmon/Kconfig
> =================================> --- linux.git.orig/drivers/hwmon/Kconfig	2009-05-28 15:28:32.000000000 +0100
> +++ linux.git/drivers/hwmon/Kconfig	2009-05-28 15:39:44.000000000 +0100
> @@ -702,6 +702,23 @@ config SENSORS_SHT15
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called sht15.
>  
> +config SENSORS_S3C
> +	tristate "S3C24XX/S3C64XX Inbuilt ADC"
> +	depends on ARCH_S3C2410 || ARCH_S3C64XX
> +	help
> +	  If you say yes here you get support for the on-board ADCs of
> +	  the Samsung S3C24XX or S3C64XX series of SoC
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called s3c-adc.

Actually not, as you meanwhile renamed the driver to s3c-hwmon (I think
I preferred s3c-adc, but what really matter is to be consistent.)

> +
> +config SENSORS_S3C_RAW
> +	bool "Include raw channel attributes in sysfs"
> +	depends on SENSORS_S3C
> +	help
> +	  Say Y here if you want to include raw copies of all the ADC
> +	  channels in sysfs.

Why not just make that code depend on DEBUG (which is set by
CONFIG_HWMON_DEBUG_CHIP)? I'd rather avoid multiplying the
configuration options.

> +
>  config SENSORS_SIS5595
>  	tristate "Silicon Integrated Systems Corp. SiS5595"
>  	depends on PCI
> Index: linux.git/drivers/hwmon/Makefile
> =================================> --- linux.git.orig/drivers/hwmon/Makefile	2009-05-28 15:28:32.000000000 +0100
> +++ linux.git/drivers/hwmon/Makefile	2009-05-28 15:39:44.000000000 +0100
> @@ -76,6 +76,7 @@ obj-$(CONFIG_SENSORS_MAX6650)	+= max6650
>  obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
>  obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
>  obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
> +obj-$(CONFIG_SENSORS_S3C)	+= s3c-hwmon.o
>  obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
>  obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595.o
>  obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
> Index: linux.git/drivers/hwmon/s3c-hwmon.c
> =================================> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux.git/drivers/hwmon/s3c-hwmon.c	2009-05-28 21:09:39.000000000 +0100
> @@ -0,0 +1,413 @@
> +/* linux/drivers/hwmon/s3c-hwmon.c
> + *
> + * Copyright (C) 2005, 2008, 2009 Simtec Electronics
> + *	http://armlinux.simtec.co.uk/
> + *	Ben Dooks <ben@simtec.co.uk>
> + *
> + * S3C24XX/S3C64XX ADC hwmon support
> + *
> + * 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.
> + *
> + * 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., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> +*/
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +
> +#include <plat/adc.h>
> +#include <plat/hwmon.h>
> +
> +struct s3c_hwmon_attr {
> +	struct sensor_device_attribute	in;
> +	struct sensor_device_attribute	label;
> +	char				in_name[12];
> +	char				label_name[12];
> +};
> +
> +/**
> + * struct s3c_hwmon - ADC hwmon client information
> + * @lock: Access lock to serialise the conversions.
> + * @client: The client we registered with the S3C ADC core.
> + * @hwmon_dev: The hwmon device we created.
> + * @attr: The holders for the channel attributes.
> +*/
> +struct s3c_hwmon {
> +	struct semaphore	lock;
> +	struct s3c_adc_client	*client;
> +	struct device		*hwmon_dev;
> +
> +	struct s3c_hwmon_attr	attrs[8];
> +};
> +
> +/**
> + * s3c_hwmon_read_ch - read a value from a given adc channel.
> + * @dev: The device.
> + * @hwmon: Our state.
> + * @channel: The channel we're reading from.
> + *
> + * Read a value from the @channel with the proper locking and sleep until
> + * either the read completes or we timeout awaiting the ADC core to get
> + * back to us.
> + */
> +static int s3c_hwmon_read_ch(struct device *dev,
> +			     struct s3c_hwmon *hwmon, int channel)
> +{
> +	int ret;
> +
> +	ret = down_interruptible(&hwmon->lock);
> +	if (ret < 0)
> +		return ret;

Why do you need locking here? If any locking is needed, I am reasonably
certain it should be handled by the s3c adc core rather than drivers
using it.

> +
> +	dev_dbg(dev, "reading channel %d\n", channel);
> +
> +	ret = s3c_adc_read(hwmon->client, channel);
> +	up(&hwmon->lock);
> +
> +	return ret;
> +}
> +
> +#ifdef CONFIG_SENSORS_S3C_RAW
> +/**
> + * s3c_hwmon_show_raw - show a conversion from the raw channel number.
> + * @dev: The device that the attribute belongs to.
> + * @attr: The attribute being read.
> + * @buf: The result buffer.
> + *
> + * This show deals with the raw attribute, registered for each possible
> + * ADC channel. This does a conversion and returns the raw (un-scaled)
> + * value returned from the hardware.
> + */
> +static ssize_t s3c_hwmon_show_raw(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct s3c_hwmon *adc = platform_get_drvdata(to_platform_device(dev));
> +	struct sensor_device_attribute *sa = to_sensor_dev_attr(attr);
> +	int ret;
> +
> +	ret = s3c_hwmon_read_ch(dev, adc, sa->index);
> +
> +	return  (ret < 0) ? ret : snprintf(buf, PAGE_SIZE, "%d\n", ret);
> +}
> +
> +#define DEF_ADC_ATTR(x)	\
> +	static SENSOR_DEVICE_ATTR(adc##x##_raw, S_IRUGO, s3c_hwmon_show_raw, NULL, x)
> +
> +DEF_ADC_ATTR(0);
> +DEF_ADC_ATTR(1);
> +DEF_ADC_ATTR(2);
> +DEF_ADC_ATTR(3);
> +DEF_ADC_ATTR(4);
> +DEF_ADC_ATTR(5);
> +DEF_ADC_ATTR(6);
> +DEF_ADC_ATTR(7);
> +
> +static struct device_attribute *s3c_hwmon_attrs[8] = {
> +	&sensor_dev_attr_adc0_raw.dev_attr,
> +	&sensor_dev_attr_adc1_raw.dev_attr,
> +	&sensor_dev_attr_adc2_raw.dev_attr,
> +	&sensor_dev_attr_adc3_raw.dev_attr,
> +	&sensor_dev_attr_adc4_raw.dev_attr,
> +	&sensor_dev_attr_adc5_raw.dev_attr,
> +	&sensor_dev_attr_adc6_raw.dev_attr,
> +	&sensor_dev_attr_adc7_raw.dev_attr,
> +};
> +
> +static inline int s3c_hwmon_add_raw(struct device *dev)
> +{
> +	int ret, i;
> +
> +	for (i = 0; i < ARRAY_SIZE(s3c_hwmon_attrs); i++) {
> +		ret = device_create_file(dev, s3c_hwmon_attrs[i]);

Is it OK to create entries for channels which do not exist?

> +		if (ret) {
> +			dev_err(dev, "error creating channel %d\n", i);
> +
> +			for (i--; i >= 0; i--)
> +				device_remove_file(dev, s3c_hwmon_attrs[i]);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static inline void s3c_hwmon_remove_raw(struct device *dev)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(s3c_hwmon_attrs); i++)
> +		device_remove_file(dev, s3c_hwmon_attrs[i]);
> +}
> +
> +#else
> +
> +static inline int s3c_hwmon_add_raw(struct device *dev) { return 0; }
> +static inline void s3c_hwmon_remove_raw(struct device *dev) { }
> +
> +#endif /* CONFIG_SENSORS_S3C_RAW */
> +
> +/**
> + * s3c_hwmon_ch_show - show value of a given channel
> + * @dev: The device that the attribute belongs to.
> + * @attr: The attribute being read.
> + * @buf: The result buffer.
> + *
> + * Read a value from the ADC and scale it before returning it to the
> + * caller. The scale factor is gained from the channel configuration
> + * passed via the platform data when the device was registered.
> + */
> +static ssize_t s3c_hwmon_ch_show(struct device *dev,
> +				 struct device_attribute *attr,
> +				 char *buf)
> +{
> +	struct sensor_device_attribute *sen_attr = to_sensor_dev_attr(attr);
> +	struct s3c_hwmon *hwmon = platform_get_drvdata(to_platform_device(dev));
> +	struct s3c_hwmon_pdata *pdata = dev->platform_data;
> +	struct s3c_hwmon_chcfg *cfg;
> +	int ret;
> +
> +	cfg = pdata->in[sen_attr->index];
> +	if (!cfg)
> +		return -EINVAL;
> +
> +	ret = s3c_hwmon_read_ch(dev, hwmon, sen_attr->index);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret *= cfg->mult;

Just thinking about it: you might want to make ret a long, to avoid
overflows if large multipliers are specified.

> +	ret = DIV_ROUND_CLOSEST(ret, cfg->div);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> +}
> +
> +/**
> + * s3c_hwmon_label_show - show label name of the given channel.
> + * @dev: The device that the attribute belongs to.
> + * @attr: The attribute being read.
> + * @buf: The result buffer.
> + *
> + * Return the label name of a given channel
> + */
> +static ssize_t s3c_hwmon_label_show(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *buf)
> +{
> +	struct sensor_device_attribute *sen_attr = to_sensor_dev_attr(attr);
> +	struct s3c_hwmon_pdata *pdata = dev->platform_data;
> +	struct s3c_hwmon_chcfg *cfg;
> +
> +	cfg = pdata->in[sen_attr->index];
> +	if (!cfg || !cfg->name)
> +		return -EINVAL;
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n", cfg->name);
> +}
> +
> +
> +/**
> + * s3c_hwmon_create_attr - create hwmon attribute for given channel.
> + * @dev: The device to create the attribute on.
> + * @cfg: The channel configuration passed from the platform data.
> + * @channel: The ADC channel number to process.
> + *
> + * Create the scaled attribute for use with hwmon from the specified
> + * platform data in @pdata. The sysfs entry is handled by the routine
> + * s3c_hwmon_ch_show().
> + *
> + * The attribute name is taken from the configuration data if present
> + * otherwise the name is taken by concatenating in_ with the channel
> + * number.
> + */
> +static int s3c_hwmon_create_attr(struct device *dev,
> +				 struct s3c_hwmon_chcfg *cfg,
> +				 struct s3c_hwmon_attr *attrs,
> +				 int channel)
> +{
> +	struct sensor_device_attribute *attr;
> +	int ret;
> +
> +	if (!cfg)
> +		return 0;

I'd rather see you not call this function at all for unused channels.

> +
> +	snprintf(attrs->in_name, sizeof(attrs->in_name), "in%d_input", channel);
> +
> +	attr = &attrs->in;
> +	attr->index = channel;
> +	attr->dev_attr.attr.name  = attrs->in_name;
> +	attr->dev_attr.attr.mode  = S_IRUGO;
> +	attr->dev_attr.attr.owner = THIS_MODULE;
> +	attr->dev_attr.show = s3c_hwmon_ch_show;
> +
> +	ret =  device_create_file(dev, &attr->dev_attr);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to create input attribute\n");
> +		return ret;
> +	}
> +
> +	/* if this has a name, add a label */
> +	if (cfg->name) {
> +		snprintf(attrs->label_name, sizeof(attrs->label_name),
> +			 "in%d_label", channel);
> +
> +		attr = &attrs->label;
> +		attr->index = channel;
> +		attr->dev_attr.attr.name  = attrs->label_name;
> +		attr->dev_attr.attr.mode  = S_IRUGO;
> +		attr->dev_attr.attr.owner = THIS_MODULE;
> +		attr->dev_attr.show = s3c_hwmon_label_show;
> +
> +		ret = device_create_file(dev, &attr->dev_attr);
> +		if (ret < 0) {
> +			device_remove_file(dev, &attrs->in.dev_attr);
> +			dev_err(dev, "failed to create label attribute\n");
> +		}
> +	}
> +
> +	return ret;
> +}

I don't understand why you insist on creating these attributes
dynamically. Almost all other hwmon drivers declare the attributes in a
static manner, and only the decision to create each attribute for a
given device is decided at runtime. This makes the code much smaller.

> +
> +static void s3c_hwmon_remove_attr(struct device *dev,
> +				  struct s3c_hwmon_attr *attrs)
> +{
> +	device_remove_file(dev, &attrs->in.dev_attr);
> +	device_remove_file(dev, &attrs->label.dev_attr);
> +}
> +
> +/**
> + * s3c_hwmon_probe - device probe entry.
> + * @dev: The device being probed.
> +*/
> +static int __devinit s3c_hwmon_probe(struct platform_device *dev)
> +{
> +	struct s3c_hwmon_pdata *pdata = dev->dev.platform_data;
> +	struct s3c_hwmon *hwmon;
> +	int ret = 0;
> +	int i;
> +
> +	hwmon = kzalloc(sizeof(struct s3c_hwmon), GFP_KERNEL);
> +	if (hwmon = NULL) {
> +		dev_err(&dev->dev, "no memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	platform_set_drvdata(dev, hwmon);
> +
> +	init_MUTEX(&hwmon->lock);
> +
> +	/* Register with the core ADC driver. */
> +
> +	hwmon->client = s3c_adc_register(dev, NULL, NULL, NULL, 0);
> +	if (IS_ERR(hwmon->client)) {
> +		dev_err(&dev->dev, "cannot register adc\n");
> +		ret = PTR_ERR(hwmon->client);
> +		goto err_mem;
> +	}
> +
> +	/* add attributes for our adc devices. */
> +
> +	ret = s3c_hwmon_add_raw(&dev->dev);
> +	if (ret)
> +		goto err_registered;
> +
> +	/* register with the hwmon core */
> +
> +	hwmon->hwmon_dev = hwmon_device_register(&dev->dev);
> +	if (IS_ERR(hwmon->hwmon_dev)) {
> +		dev_err(&dev->dev, "error registering with hwmon\n");
> +		ret = PTR_ERR(hwmon->hwmon_dev);
> +		goto err_raw_attribute;
> +	}
> +
> +	if (pdata) {

If !pdata the device is useless, so we can be reasonably certain this
can't happen. If you really want to deal with this case then you'd
rather move the check at the beginning of the function.

> +		for (i = 0; i < ARRAY_SIZE(pdata->in); i++) {
> +			ret = s3c_hwmon_create_attr(&dev->dev, pdata->in[i],
> +						    &hwmon->attrs[i], i);
> +			if (ret) {
> +				dev_err(&dev->dev,
> +					"error creating channel %d\n", i);
> +
> +				for (i--; i >= 0; i--)
> +					s3c_hwmon_remove_attr(&dev->dev,
> +							      &hwmon->attrs[i]);
> +
> +				goto err_hwmon_register;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +
> + err_hwmon_register:
> +	hwmon_device_unregister(hwmon->hwmon_dev);
> +
> + err_raw_attribute:
> +	s3c_hwmon_remove_raw(&dev->dev);
> +
> + err_registered:
> +	s3c_adc_release(hwmon->client);
> +
> + err_mem:
> +	kfree(hwmon);
> +	return ret;
> +}
> +
> +static int __devexit s3c_hwmon_remove(struct platform_device *dev)
> +{
> +	struct s3c_hwmon *hwmon = platform_get_drvdata(dev);
> +	int i;
> +
> +	s3c_hwmon_remove_raw(&dev->dev);
> +
> +	for (i = 0; i < ARRAY_SIZE(hwmon->attrs); i++)
> +		s3c_hwmon_remove_attr(&dev->dev, &hwmon->attrs[i]);
> +
> +	hwmon_device_unregister(hwmon->hwmon_dev);
> +	s3c_adc_release(hwmon->client);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver s3c_hwmon_driver = {
> +	.driver	= {
> +		.name		= "s3c-hwmon",
> +		.owner		= THIS_MODULE,
> +	},
> +	.probe		= s3c_hwmon_probe,
> +	.remove		= __devexit_p(s3c_hwmon_remove),
> +};
> +
> +static int __init s3c_hwmon_init(void)
> +{
> +	return platform_driver_register(&s3c_hwmon_driver);
> +}
> +
> +static void __exit s3c_hwmon_exit(void)
> +{
> +	platform_driver_unregister(&s3c_hwmon_driver);
> +}
> +
> +module_init(s3c_hwmon_init);
> +module_exit(s3c_hwmon_exit);
> +
> +MODULE_AUTHOR("Ben Dooks <ben@simtec.co.uk>");
> +MODULE_DESCRIPTION("S3C ADC HWMon driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:s3c-hwmon");
> Index: linux.git/arch/arm/plat-s3c/include/plat/hwmon.h
> =================================> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux.git/arch/arm/plat-s3c/include/plat/hwmon.h	2009-05-28 15:39:44.000000000 +0100
> @@ -0,0 +1,41 @@
> +/* linux/arch/arm/plat-s3c/include/plat/hwmon.h
> + *
> + * Copyright 2005 Simtec Electronics
> + *	Ben Dooks <ben@simtec.co.uk>
> + *	http://armlinux.simtec.co.uk/
> + *
> + * S3C - HWMon interface for ADC
> + *
> + * 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.
> +*/
> +
> +#ifndef __ASM_ARCH_ADC_HWMON_H
> +#define __ASM_ARCH_ADC_HWMON_H __FILE__
> +
> +/**
> + * s3c_hwmon_chcfg - channel configuration
> + * @name: The name to give this channel.
> + * @mult: Multiply the ADC value read by this.
> + * @div: Divide the value from the ADC by this.
> + *
> + * The value read from the ADC is converted to a value that
> + * hwmon expects (mV) by result = (value_read * @mult) / @div.
> + */
> +struct s3c_hwmon_chcfg {
> +	const char	*name;
> +	unsigned int	mult;
> +	unsigned int	div;
> +};
> +
> +/**
> + * s3c_hwmon_pdata - HWMON platform data
> + * @in: One configuration for each possible channel used.
> + */
> +struct s3c_hwmon_pdata {
> +	struct s3c_hwmon_chcfg	*in[8];
> +};
> +
> +#endif /* __ASM_ARCH_ADC_HWMON_H */
> +
> 


-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] HWMON: S3C24XX series ADC driver

[Ben Dooks]

Jean Delvare wrote:
> Hi Ben,
> 
> On Thu, 28 May 2009 21:42:20 +0100, Ben Dooks wrote:
>> Add support for the ADC controller on the S3C
>> series of processors to drivers/hwmon for use with
>> hardware monitoring systems.
>>
>> Signed-off-by: Ben Dooks <ben@simtec.co.uk>
> 
> Second review:
> 
>> Index: linux.git/drivers/hwmon/Kconfig
>> =================================>> --- linux.git.orig/drivers/hwmon/Kconfig	2009-05-28 15:28:32.000000000 +0100
>> +++ linux.git/drivers/hwmon/Kconfig	2009-05-28 15:39:44.000000000 +0100
>> @@ -702,6 +702,23 @@ config SENSORS_SHT15
>>  	  This driver can also be built as a module.  If so, the module
>>  	  will be called sht15.
>>  
>> +config SENSORS_S3C
>> +	tristate "S3C24XX/S3C64XX Inbuilt ADC"
>> +	depends on ARCH_S3C2410 || ARCH_S3C64XX
>> +	help
>> +	  If you say yes here you get support for the on-board ADCs of
>> +	  the Samsung S3C24XX or S3C64XX series of SoC
>> +
>> +	  This driver can also be built as a module. If so, the module
>> +	  will be called s3c-adc.
> 
> Actually not, as you meanwhile renamed the driver to s3c-hwmon (I think
> I preferred s3c-adc, but what really matter is to be consistent.)

s3c-adc sounds too much like the core driver, i'll change this to
s3c-hwmon.

>> +
>> +config SENSORS_S3C_RAW
>> +	bool "Include raw channel attributes in sysfs"
>> +	depends on SENSORS_S3C
>> +	help
>> +	  Say Y here if you want to include raw copies of all the ADC
>> +	  channels in sysfs.
> 
> Why not just make that code depend on DEBUG (which is set by
> CONFIG_HWMON_DEBUG_CHIP)? I'd rather avoid multiplying the
> configuration options.

>> +
>>  config SENSORS_SIS5595
>>  	tristate "Silicon Integrated Systems Corp. SiS5595"
>>  	depends on PCI
>> Index: linux.git/drivers/hwmon/Makefile
>> =================================>> --- linux.git.orig/drivers/hwmon/Makefile	2009-05-28 15:28:32.000000000 +0100
>> +++ linux.git/drivers/hwmon/Makefile	2009-05-28 15:39:44.000000000 +0100
>> @@ -76,6 +76,7 @@ obj-$(CONFIG_SENSORS_MAX6650)	+= max6650
>>  obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
>>  obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
>>  obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
>> +obj-$(CONFIG_SENSORS_S3C)	+= s3c-hwmon.o
>>  obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
>>  obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595.o
>>  obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
>> Index: linux.git/drivers/hwmon/s3c-hwmon.c
>> =================================>> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
>> +++ linux.git/drivers/hwmon/s3c-hwmon.c	2009-05-28 21:09:39.000000000 +0100
>> @@ -0,0 +1,413 @@
>> +/* linux/drivers/hwmon/s3c-hwmon.c
>> + *
>> + * Copyright (C) 2005, 2008, 2009 Simtec Electronics
>> + *	http://armlinux.simtec.co.uk/
>> + *	Ben Dooks <ben@simtec.co.uk>
>> + *
>> + * S3C24XX/S3C64XX ADC hwmon support
>> + *
>> + * 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.
>> + *
>> + * 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., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>> +*/
>> +
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/init.h>
>> +#include <linux/err.h>
>> +#include <linux/clk.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +
>> +#include <plat/adc.h>
>> +#include <plat/hwmon.h>
>> +
>> +struct s3c_hwmon_attr {
>> +	struct sensor_device_attribute	in;
>> +	struct sensor_device_attribute	label;
>> +	char				in_name[12];
>> +	char				label_name[12];
>> +};
>> +
>> +/**
>> + * struct s3c_hwmon - ADC hwmon client information
>> + * @lock: Access lock to serialise the conversions.
>> + * @client: The client we registered with the S3C ADC core.
>> + * @hwmon_dev: The hwmon device we created.
>> + * @attr: The holders for the channel attributes.
>> +*/
>> +struct s3c_hwmon {
>> +	struct semaphore	lock;
>> +	struct s3c_adc_client	*client;
>> +	struct device		*hwmon_dev;
>> +
>> +	struct s3c_hwmon_attr	attrs[8];
>> +};
>> +
>> +/**
>> + * s3c_hwmon_read_ch - read a value from a given adc channel.
>> + * @dev: The device.
>> + * @hwmon: Our state.
>> + * @channel: The channel we're reading from.
>> + *
>> + * Read a value from the @channel with the proper locking and sleep until
>> + * either the read completes or we timeout awaiting the ADC core to get
>> + * back to us.
>> + */
>> +static int s3c_hwmon_read_ch(struct device *dev,
>> +			     struct s3c_hwmon *hwmon, int channel)
>> +{
>> +	int ret;
>> +
>> +	ret = down_interruptible(&hwmon->lock);
>> +	if (ret < 0)
>> +		return ret;
> 
> Why do you need locking here? If any locking is needed, I am reasonably
> certain it should be handled by the s3c adc core rather than drivers
> using it.

I'd rather leave the locking to the client, as in this case
we use down_interruptible() so the user can interrupt the
call if necessary.

>> +
>> +	dev_dbg(dev, "reading channel %d\n", channel);
>> +
>> +	ret = s3c_adc_read(hwmon->client, channel);
>> +	up(&hwmon->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +#ifdef CONFIG_SENSORS_S3C_RAW
>> +/**
>> + * s3c_hwmon_show_raw - show a conversion from the raw channel number.
>> + * @dev: The device that the attribute belongs to.
>> + * @attr: The attribute being read.
>> + * @buf: The result buffer.
>> + *
>> + * This show deals with the raw attribute, registered for each possible
>> + * ADC channel. This does a conversion and returns the raw (un-scaled)
>> + * value returned from the hardware.
>> + */
>> +static ssize_t s3c_hwmon_show_raw(struct device *dev,
>> +				  struct device_attribute *attr, char *buf)
>> +{
>> +	struct s3c_hwmon *adc = platform_get_drvdata(to_platform_device(dev));
>> +	struct sensor_device_attribute *sa = to_sensor_dev_attr(attr);
>> +	int ret;
>> +
>> +	ret = s3c_hwmon_read_ch(dev, adc, sa->index);
>> +
>> +	return  (ret < 0) ? ret : snprintf(buf, PAGE_SIZE, "%d\n", ret);
>> +}
>> +
>> +#define DEF_ADC_ATTR(x)	\
>> +	static SENSOR_DEVICE_ATTR(adc##x##_raw, S_IRUGO, s3c_hwmon_show_raw, NULL, x)
>> +
>> +DEF_ADC_ATTR(0);
>> +DEF_ADC_ATTR(1);
>> +DEF_ADC_ATTR(2);
>> +DEF_ADC_ATTR(3);
>> +DEF_ADC_ATTR(4);
>> +DEF_ADC_ATTR(5);
>> +DEF_ADC_ATTR(6);
>> +DEF_ADC_ATTR(7);
>> +
>> +static struct device_attribute *s3c_hwmon_attrs[8] = {
>> +	&sensor_dev_attr_adc0_raw.dev_attr,
>> +	&sensor_dev_attr_adc1_raw.dev_attr,
>> +	&sensor_dev_attr_adc2_raw.dev_attr,
>> +	&sensor_dev_attr_adc3_raw.dev_attr,
>> +	&sensor_dev_attr_adc4_raw.dev_attr,
>> +	&sensor_dev_attr_adc5_raw.dev_attr,
>> +	&sensor_dev_attr_adc6_raw.dev_attr,
>> +	&sensor_dev_attr_adc7_raw.dev_attr,
>> +};
>> +
>> +static inline int s3c_hwmon_add_raw(struct device *dev)
>> +{
>> +	int ret, i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(s3c_hwmon_attrs); i++) {
>> +		ret = device_create_file(dev, s3c_hwmon_attrs[i]);
> 
> Is it OK to create entries for channels which do not exist?

The channels always exist, the pdata specifies which channels
the board wants to export to userspace via hwmon.

>> +		if (ret) {
>> +			dev_err(dev, "error creating channel %d\n", i);
>> +
>> +			for (i--; i >= 0; i--)
>> +				device_remove_file(dev, s3c_hwmon_attrs[i]);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static inline void s3c_hwmon_remove_raw(struct device *dev)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(s3c_hwmon_attrs); i++)
>> +		device_remove_file(dev, s3c_hwmon_attrs[i]);
>> +}
>> +
>> +#else
>> +
>> +static inline int s3c_hwmon_add_raw(struct device *dev) { return 0; }
>> +static inline void s3c_hwmon_remove_raw(struct device *dev) { }
>> +
>> +#endif /* CONFIG_SENSORS_S3C_RAW */
>> +
>> +/**
>> + * s3c_hwmon_ch_show - show value of a given channel
>> + * @dev: The device that the attribute belongs to.
>> + * @attr: The attribute being read.
>> + * @buf: The result buffer.
>> + *
>> + * Read a value from the ADC and scale it before returning it to the
>> + * caller. The scale factor is gained from the channel configuration
>> + * passed via the platform data when the device was registered.
>> + */
>> +static ssize_t s3c_hwmon_ch_show(struct device *dev,
>> +				 struct device_attribute *attr,
>> +				 char *buf)
>> +{
>> +	struct sensor_device_attribute *sen_attr = to_sensor_dev_attr(attr);
>> +	struct s3c_hwmon *hwmon = platform_get_drvdata(to_platform_device(dev));
>> +	struct s3c_hwmon_pdata *pdata = dev->platform_data;
>> +	struct s3c_hwmon_chcfg *cfg;
>> +	int ret;
>> +
>> +	cfg = pdata->in[sen_attr->index];
>> +	if (!cfg)
>> +		return -EINVAL;
>> +
>> +	ret = s3c_hwmon_read_ch(dev, hwmon, sen_attr->index);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret *= cfg->mult;
> 
> Just thinking about it: you might want to make ret a long, to avoid
> overflows if large multipliers are specified.

long and int are the same on arm, the result is currently 12bit, so it 
would have to be a huge multiplier to get an overflow.

I'll add a warning print in the creation loop if mult is over 16bit.

>> +	ret = DIV_ROUND_CLOSEST(ret, cfg->div);
>> +
>> +	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
>> +}
>> +
>> +/**
>> + * s3c_hwmon_label_show - show label name of the given channel.
>> + * @dev: The device that the attribute belongs to.
>> + * @attr: The attribute being read.
>> + * @buf: The result buffer.
>> + *
>> + * Return the label name of a given channel
>> + */
>> +static ssize_t s3c_hwmon_label_show(struct device *dev,
>> +				    struct device_attribute *attr,
>> +				    char *buf)
>> +{
>> +	struct sensor_device_attribute *sen_attr = to_sensor_dev_attr(attr);
>> +	struct s3c_hwmon_pdata *pdata = dev->platform_data;
>> +	struct s3c_hwmon_chcfg *cfg;
>> +
>> +	cfg = pdata->in[sen_attr->index];
>> +	if (!cfg || !cfg->name)
>> +		return -EINVAL;
>> +
>> +	return snprintf(buf, PAGE_SIZE, "%s\n", cfg->name);
>> +}
>> +
>> +
>> +/**
>> + * s3c_hwmon_create_attr - create hwmon attribute for given channel.
>> + * @dev: The device to create the attribute on.
>> + * @cfg: The channel configuration passed from the platform data.
>> + * @channel: The ADC channel number to process.
>> + *
>> + * Create the scaled attribute for use with hwmon from the specified
>> + * platform data in @pdata. The sysfs entry is handled by the routine
>> + * s3c_hwmon_ch_show().
>> + *
>> + * The attribute name is taken from the configuration data if present
>> + * otherwise the name is taken by concatenating in_ with the channel
>> + * number.
>> + */
>> +static int s3c_hwmon_create_attr(struct device *dev,
>> +				 struct s3c_hwmon_chcfg *cfg,
>> +				 struct s3c_hwmon_attr *attrs,
>> +				 int channel)
>> +{
>> +	struct sensor_device_attribute *attr;
>> +	int ret;
>> +
>> +	if (!cfg)
>> +		return 0;
> 
> I'd rather see you not call this function at all for unused channels.

ok, will change the loop.

>> +
>> +	snprintf(attrs->in_name, sizeof(attrs->in_name), "in%d_input", channel);
>> +
>> +	attr = &attrs->in;
>> +	attr->index = channel;
>> +	attr->dev_attr.attr.name  = attrs->in_name;
>> +	attr->dev_attr.attr.mode  = S_IRUGO;
>> +	attr->dev_attr.attr.owner = THIS_MODULE;
>> +	attr->dev_attr.show = s3c_hwmon_ch_show;
>> +
>> +	ret =  device_create_file(dev, &attr->dev_attr);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to create input attribute\n");
>> +		return ret;
>> +	}
>> +
>> +	/* if this has a name, add a label */
>> +	if (cfg->name) {
>> +		snprintf(attrs->label_name, sizeof(attrs->label_name),
>> +			 "in%d_label", channel);
>> +
>> +		attr = &attrs->label;
>> +		attr->index = channel;
>> +		attr->dev_attr.attr.name  = attrs->label_name;
>> +		attr->dev_attr.attr.mode  = S_IRUGO;
>> +		attr->dev_attr.attr.owner = THIS_MODULE;
>> +		attr->dev_attr.show = s3c_hwmon_label_show;
>> +
>> +		ret = device_create_file(dev, &attr->dev_attr);
>> +		if (ret < 0) {
>> +			device_remove_file(dev, &attrs->in.dev_attr);
>> +			dev_err(dev, "failed to create label attribute\n");
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
> 
> I don't understand why you insist on creating these attributes
> dynamically. Almost all other hwmon drivers declare the attributes in a
> static manner, and only the decision to create each attribute for a
        you probably wanted to use add here instead of reusing create.
> given device is decided at runtime. This makes the code much smaller.

I've always prefered to avoid static data when there's a possiblity
the device may not be instantiated for the particular machine the
kernel is being booted on.

The code itself is 256 bytes, wheras having two arrays of attributes
would turn up at 384 bytes not including the code to call
device_create_file() as appropriate.

>> +
>> +static void s3c_hwmon_remove_attr(struct device *dev,
>> +				  struct s3c_hwmon_attr *attrs)
>> +{
>> +	device_remove_file(dev, &attrs->in.dev_attr);
>> +	device_remove_file(dev, &attrs->label.dev_attr);
>> +}
>> +
>> +/**
>> + * s3c_hwmon_probe - device probe entry.
>> + * @dev: The device being probed.
>> +*/
>> +static int __devinit s3c_hwmon_probe(struct platform_device *dev)
>> +{
>> +	struct s3c_hwmon_pdata *pdata = dev->dev.platform_data;
>> +	struct s3c_hwmon *hwmon;
>> +	int ret = 0;
>> +	int i;
>> +
>> +	hwmon = kzalloc(sizeof(struct s3c_hwmon), GFP_KERNEL);
>> +	if (hwmon = NULL) {
>> +		dev_err(&dev->dev, "no memory\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	platform_set_drvdata(dev, hwmon);
>> +
>> +	init_MUTEX(&hwmon->lock);
>> +
>> +	/* Register with the core ADC driver. */
>> +
>> +	hwmon->client = s3c_adc_register(dev, NULL, NULL, NULL, 0);
>> +	if (IS_ERR(hwmon->client)) {
>> +		dev_err(&dev->dev, "cannot register adc\n");
>> +		ret = PTR_ERR(hwmon->client);
>> +		goto err_mem;
>> +	}
>> +
>> +	/* add attributes for our adc devices. */
>> +
>> +	ret = s3c_hwmon_add_raw(&dev->dev);
>> +	if (ret)
>> +		goto err_registered;
>> +
>> +	/* register with the hwmon core */
>> +
>> +	hwmon->hwmon_dev = hwmon_device_register(&dev->dev);
>> +	if (IS_ERR(hwmon->hwmon_dev)) {
>> +		dev_err(&dev->dev, "error registering with hwmon\n");
>> +		ret = PTR_ERR(hwmon->hwmon_dev);
>> +		goto err_raw_attribute;
>> +	}
>> +
>> +	if (pdata) {
> 
> If !pdata the device is useless, so we can be reasonably certain this
> can't happen. If you really want to deal with this case then you'd
> rather move the check at the beginning of the function.

In the original, having no pdata just meant you had the raw attributes
available. I'll make the device bail if there's no pdata.

>> +		for (i = 0; i < ARRAY_SIZE(pdata->in); i++) {
>> +			ret = s3c_hwmon_create_attr(&dev->dev, pdata->in[i],
>> +						    &hwmon->attrs[i], i);
>> +			if (ret) {
>> +				dev_err(&dev->dev,
>> +					"error creating channel %d\n", i);
>> +
>> +				for (i--; i >= 0; i--)
>> +					s3c_hwmon_remove_attr(&dev->dev,
>> +							      &hwmon->attrs[i]);
>> +
>> +				goto err_hwmon_register;
>> +			}
>> +		}
>> +	}
>> +
>> +	return 0;
>> +
>> + err_hwmon_register:
>> +	hwmon_device_unregister(hwmon->hwmon_dev);
>> +
>> + err_raw_attribute:
>> +	s3c_hwmon_remove_raw(&dev->dev);
>> +
>> + err_registered:
>> +	s3c_adc_release(hwmon->client);
>> +
>> + err_mem:
>> +	kfree(hwmon);
>> +	return ret;
>> +}
>> +
>> +static int __devexit s3c_hwmon_remove(struct platform_device *dev)
>> +{
>> +	struct s3c_hwmon *hwmon = platform_get_drvdata(dev);
>> +	int i;
>> +
>> +	s3c_hwmon_remove_raw(&dev->dev);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(hwmon->attrs); i++)
>> +		s3c_hwmon_remove_attr(&dev->dev, &hwmon->attrs[i]);
>> +
>> +	hwmon_device_unregister(hwmon->hwmon_dev);
>> +	s3c_adc_release(hwmon->client);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver s3c_hwmon_driver = {
>> +	.driver	= {
>> +		.name		= "s3c-hwmon",
>> +		.owner		= THIS_MODULE,
>> +	},
>> +	.probe		= s3c_hwmon_probe,
>> +	.remove		= __devexit_p(s3c_hwmon_remove),
>> +};
>> +
>> +static int __init s3c_hwmon_init(void)
>> +{
>> +	return platform_driver_register(&s3c_hwmon_driver);
>> +}
>> +
>> +static void __exit s3c_hwmon_exit(void)
>> +{
>> +	platform_driver_unregister(&s3c_hwmon_driver);
>> +}
>> +
>> +module_init(s3c_hwmon_init);
>> +module_exit(s3c_hwmon_exit);
>> +
>> +MODULE_AUTHOR("Ben Dooks <ben@simtec.co.uk>");
>> +MODULE_DESCRIPTION("S3C ADC HWMon driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:s3c-hwmon");
>> Index: linux.git/arch/arm/plat-s3c/include/plat/hwmon.h
>> =================================>> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
>> +++ linux.git/arch/arm/plat-s3c/include/plat/hwmon.h	2009-05-28 15:39:44.000000000 +0100
>> @@ -0,0 +1,41 @@
>> +/* linux/arch/arm/plat-s3c/include/plat/hwmon.h
>> + *
>> + * Copyright 2005 Simtec Electronics
>> + *	Ben Dooks <ben@simtec.co.uk>
>> + *	http://armlinux.simtec.co.uk/
>> + *
>> + * S3C - HWMon interface for ADC
>> + *
>> + * 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.
>> +*/
>> +
>> +#ifndef __ASM_ARCH_ADC_HWMON_H
>> +#define __ASM_ARCH_ADC_HWMON_H __FILE__
>> +
>> +/**
>> + * s3c_hwmon_chcfg - channel configuration
>> + * @name: The name to give this channel.
>> + * @mult: Multiply the ADC value read by this.
>> + * @div: Divide the value from the ADC by this.
>> + *
>> + * The value read from the ADC is converted to a value that
>> + * hwmon expects (mV) by result = (value_read * @mult) / @div.
>> + */
>> +struct s3c_hwmon_chcfg {
>> +	const char	*name;
>> +	unsigned int	mult;
>> +	unsigned int	div;
>> +};
>> +
>> +/**
>> + * s3c_hwmon_pdata - HWMON platform data
>> + * @in: One configuration for each possible channel used.
>> + */
>> +struct s3c_hwmon_pdata {
>> +	struct s3c_hwmon_chcfg	*in[8];
>> +};
>> +
>> +#endif /* __ASM_ARCH_ADC_HWMON_H */
>> +

-- 
Ben Dooks, Software Engineer, Simtec Electronics

http://www.simtec.co.uk/

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] HWMON: S3C24XX series ADC driver

[Jean Delvare]

On Sat, 30 May 2009 14:47:03 +0100, Ben Dooks wrote:
> Jean Delvare wrote:
> > On Thu, 28 May 2009 21:42:20 +0100, Ben Dooks wrote:
> >> +static int s3c_hwmon_read_ch(struct device *dev,
> >> +			     struct s3c_hwmon *hwmon, int channel)
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = down_interruptible(&hwmon->lock);
> >> +	if (ret < 0)
> >> +		return ret;
> > 
> > Why do you need locking here? If any locking is needed, I am reasonably
> > certain it should be handled by the s3c adc core rather than drivers
> > using it.
> 
> I'd rather leave the locking to the client, as in this case
> we use down_interruptible() so the user can interrupt the
> call if necessary.

What are you protecting? And how are you protecting it with a
per-client lock, if there are several clients?

> >> (...)
> >> +static inline int s3c_hwmon_add_raw(struct device *dev)
> >> +{
> >> +	int ret, i;
> >> +
> >> +	for (i = 0; i < ARRAY_SIZE(s3c_hwmon_attrs); i++) {
> >> +		ret = device_create_file(dev, s3c_hwmon_attrs[i]);  
> > 
> > Is it OK to create entries for channels which do not exist?  
> 
> The channels always exist, the pdata specifies which channels
> the board wants to export to userspace via hwmon.

OK. Then you may want to consider declaring a group of attribute and
register it all at once. This saves you the hassle of handling the
errors manually.

> >> (...)
> >> +static int s3c_hwmon_create_attr(struct device *dev,
> >> +				 struct s3c_hwmon_chcfg *cfg,
> >> +				 struct s3c_hwmon_attr *attrs,
> >> +				 int channel)
> >> +{
> >> +	struct sensor_device_attribute *attr;
> >> +	int ret;
> >> +
> >> +	if (!cfg)
> >> +		return 0;
> >> +
> >> +	snprintf(attrs->in_name, sizeof(attrs->in_name), "in%d_input", channel);
> >> +
> >> +	attr = &attrs->in;
> >> +	attr->index = channel;
> >> +	attr->dev_attr.attr.name  = attrs->in_name;
> >> +	attr->dev_attr.attr.mode  = S_IRUGO;
> >> +	attr->dev_attr.attr.owner = THIS_MODULE;
> >> +	attr->dev_attr.show = s3c_hwmon_ch_show;
> >> +
> >> +	ret =  device_create_file(dev, &attr->dev_attr);
> >> +	if (ret < 0) {
> >> +		dev_err(dev, "failed to create input attribute\n");
> >> +		return ret;
> >> +	}
> >> +
> >> +	/* if this has a name, add a label */
> >> +	if (cfg->name) {
> >> +		snprintf(attrs->label_name, sizeof(attrs->label_name),
> >> +			 "in%d_label", channel);
> >> +
> >> +		attr = &attrs->label;
> >> +		attr->index = channel;
> >> +		attr->dev_attr.attr.name  = attrs->label_name;
> >> +		attr->dev_attr.attr.mode  = S_IRUGO;
> >> +		attr->dev_attr.attr.owner = THIS_MODULE;
> >> +		attr->dev_attr.show = s3c_hwmon_label_show;
> >> +
> >> +		ret = device_create_file(dev, &attr->dev_attr);
> >> +		if (ret < 0) {
> >> +			device_remove_file(dev, &attrs->in.dev_attr);
> >> +			dev_err(dev, "failed to create label attribute\n");
> >> +		}
> >> +	}
> >> +
> >> +	return ret;
> >> +}
> > 
> > I don't understand why you insist on creating these attributes
> > dynamically. Almost all other hwmon drivers declare the attributes in a
> > static manner, and only the decision to create each attribute for a
>         you probably wanted to use add here instead of reusing create.
> > given device is decided at runtime. This makes the code much smaller.
> 
> I've always prefered to avoid static data when there's a possiblity
> the device may not be instantiated for the particular machine the
> kernel is being booted on.
> 
> The code itself is 256 bytes, wheras having two arrays of attributes
> would turn up at 384 bytes not including the code to call
> device_create_file() as appropriate.

You didn't count the memory you have to allocate in the dynamic case,
so the comparison is hardly fair. That can't be less than the static
data if all 8 channels are used, and it's even likely to be somewhat
more due to memory fragmentation. Then of course the exact computation
depends on how many channels you expect to have on average.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] HWMON: S3C24XX series ADC driver

[Ben Dooks]

Jean Delvare wrote:
> On Sat, 30 May 2009 14:47:03 +0100, Ben Dooks wrote:
>> Jean Delvare wrote:
>>> On Thu, 28 May 2009 21:42:20 +0100, Ben Dooks wrote:
>>>> +static int s3c_hwmon_read_ch(struct device *dev,
>>>> +			     struct s3c_hwmon *hwmon, int channel)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = down_interruptible(&hwmon->lock);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>> Why do you need locking here? If any locking is needed, I am reasonably
>>> certain it should be handled by the s3c adc core rather than drivers
>>> using it.
>> I'd rather leave the locking to the client, as in this case
>> we use down_interruptible() so the user can interrupt the
>> call if necessary.
> 
> What are you protecting? And how are you protecting it with a
> per-client lock, if there are several clients?

The ADC core code has a simple lock on it to handle
multiple clients, but doesn't handle the same client
trying >1 conversions at a time.

In this case the caller ensures the exclusivity of one
call per client in the way that best suits the caller,
ie something that can be interrupted by the user if
necessary.

>>>> (...)
>>>> +static inline int s3c_hwmon_add_raw(struct device *dev)
>>>> +{
>>>> +	int ret, i;
>>>> +
>>>> +	for (i = 0; i < ARRAY_SIZE(s3c_hwmon_attrs); i++) {
>>>> +		ret = device_create_file(dev, s3c_hwmon_attrs[i]);  
>>> Is it OK to create entries for channels which do not exist?  
>> The channels always exist, the pdata specifies which channels
>> the board wants to export to userspace via hwmon.
> 
> OK. Then you may want to consider declaring a group of attribute and
> register it all at once. This saves you the hassle of handling the
> errors manually.
> 
>>>> (...)
>>>> +static int s3c_hwmon_create_attr(struct device *dev,
>>>> +				 struct s3c_hwmon_chcfg *cfg,
>>>> +				 struct s3c_hwmon_attr *attrs,
>>>> +				 int channel)
>>>> +{
>>>> +	struct sensor_device_attribute *attr;
>>>> +	int ret;
>>>> +
>>>> +	if (!cfg)
>>>> +		return 0;
>>>> +
>>>> +	snprintf(attrs->in_name, sizeof(attrs->in_name), "in%d_input", channel);
>>>> +
>>>> +	attr = &attrs->in;
>>>> +	attr->index = channel;
>>>> +	attr->dev_attr.attr.name  = attrs->in_name;
>>>> +	attr->dev_attr.attr.mode  = S_IRUGO;
>>>> +	attr->dev_attr.attr.owner = THIS_MODULE;
>>>> +	attr->dev_attr.show = s3c_hwmon_ch_show;
>>>> +
>>>> +	ret =  device_create_file(dev, &attr->dev_attr);
>>>> +	if (ret < 0) {
>>>> +		dev_err(dev, "failed to create input attribute\n");
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	/* if this has a name, add a label */
>>>> +	if (cfg->name) {
>>>> +		snprintf(attrs->label_name, sizeof(attrs->label_name),
>>>> +			 "in%d_label", channel);
>>>> +
>>>> +		attr = &attrs->label;
>>>> +		attr->index = channel;
>>>> +		attr->dev_attr.attr.name  = attrs->label_name;
>>>> +		attr->dev_attr.attr.mode  = S_IRUGO;
>>>> +		attr->dev_attr.attr.owner = THIS_MODULE;
>>>> +		attr->dev_attr.show = s3c_hwmon_label_show;
>>>> +
>>>> +		ret = device_create_file(dev, &attr->dev_attr);
>>>> +		if (ret < 0) {
>>>> +			device_remove_file(dev, &attrs->in.dev_attr);
>>>> +			dev_err(dev, "failed to create label attribute\n");
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return ret;
>>>> +}
>>> I don't understand why you insist on creating these attributes
>>> dynamically. Almost all other hwmon drivers declare the attributes in a
>>> static manner, and only the decision to create each attribute for a
>>         you probably wanted to use add here instead of reusing create.
>>> given device is decided at runtime. This makes the code much smaller.
>> I've always prefered to avoid static data when there's a possiblity
>> the device may not be instantiated for the particular machine the
>> kernel is being booted on.
>>
>> The code itself is 256 bytes, wheras having two arrays of attributes
>> would turn up at 384 bytes not including the code to call
>> device_create_file() as appropriate.
> 
> You didn't count the memory you have to allocate in the dynamic case,
> so the comparison is hardly fair. That can't be less than the static
> data if all 8 channels are used, and it's even likely to be somewhat
> more due to memory fragmentation. Then of course the exact computation
> depends on how many channels you expect to have on average.

I removed the per-channel allocation, so it will either allocate
enough for 8 channels no matter how many are used, or not allocate
anything if the device is not instantiated. This removes the frag
argument, as we use kmalloc() to instantiate the device context
from the platform device.

The dynamic case makes the built kernel smaller, it doesn't make
the memory usage smaller (the probe code will get thrown away after
initialisation time).

-- 
Ben Dooks, Software Engineer, Simtec Electronics

http://www.simtec.co.uk/

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] HWMON: S3C24XX series ADC driver

[Hector Oron]

Hi Ben,

Where can I find those changes to test?
I have a clone of your git repo located at aeryn.fluff.org.uk, is the
s3c-hwmon (adc core) driver in this repo? any branch?

Cheers =)

-- 
 Héctor Orón

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[lm-sensors] HWMON: S3C24XX series ADC driver

[Ben Dooks]

Add support for the ADC controller on the S3C
series of processors to drivers/hwmon for use with
hardware monitoring systems.

Signed-off-by: Ben Dooks <ben@simtec.co.uk>

Index: linux.git/drivers/hwmon/Kconfig
=================================--- linux.git.orig/drivers/hwmon/Kconfig	2009-05-28 15:28:32.000000000 +0100
+++ linux.git/drivers/hwmon/Kconfig	2009-05-30 14:34:11.000000000 +0100
@@ -702,6 +702,23 @@ config SENSORS_SHT15
 	  This driver can also be built as a module.  If so, the module
 	  will be called sht15.
 
+config SENSORS_S3C
+	tristate "S3C24XX/S3C64XX Inbuilt ADC"
+	depends on ARCH_S3C2410 || ARCH_S3C64XX
+	help
+	  If you say yes here you get support for the on-board ADCs of
+	  the Samsung S3C24XX or S3C64XX series of SoC
+
+	  This driver can also be built as a module. If so, the module
+	  will be called s3c-hwmo.
+
+config SENSORS_S3C_RAW
+	bool "Include raw channel attributes in sysfs"
+	depends on SENSORS_S3C
+	help
+	  Say Y here if you want to include raw copies of all the ADC
+	  channels in sysfs.
+
 config SENSORS_SIS5595
 	tristate "Silicon Integrated Systems Corp. SiS5595"
 	depends on PCI
Index: linux.git/drivers/hwmon/Makefile
=================================--- linux.git.orig/drivers/hwmon/Makefile	2009-05-28 15:28:32.000000000 +0100
+++ linux.git/drivers/hwmon/Makefile	2009-05-28 15:39:44.000000000 +0100
@@ -76,6 +76,7 @@ obj-$(CONFIG_SENSORS_MAX6650)	+= max6650
 obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
 obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
+obj-$(CONFIG_SENSORS_S3C)	+= s3c-hwmon.o
 obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
 obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595.o
 obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
Index: linux.git/drivers/hwmon/s3c-hwmon.c
=================================--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux.git/drivers/hwmon/s3c-hwmon.c	2009-06-08 12:24:26.000000000 +0100
@@ -0,0 +1,405 @@
+/* linux/drivers/hwmon/s3c-hwmon.c
+ *
+ * Copyright (C) 2005, 2008, 2009 Simtec Electronics
+ *	http://armlinux.simtec.co.uk/
+ *	Ben Dooks <ben@simtec.co.uk>
+ *
+ * S3C24XX/S3C64XX ADC hwmon support
+ *
+ * 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.
+ *
+ * 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., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+*/
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+
+#include <plat/adc.h>
+#include <plat/hwmon.h>
+
+struct s3c_hwmon_attr {
+	struct sensor_device_attribute	in;
+	struct sensor_device_attribute	label;
+	char				in_name[12];
+	char				label_name[12];
+};
+
+/**
+ * struct s3c_hwmon - ADC hwmon client information
+ * @lock: Access lock to serialise the conversions.
+ * @client: The client we registered with the S3C ADC core.
+ * @hwmon_dev: The hwmon device we created.
+ * @attr: The holders for the channel attributes.
+*/
+struct s3c_hwmon {
+	struct semaphore	lock;
+	struct s3c_adc_client	*client;
+	struct device		*hwmon_dev;
+
+	struct s3c_hwmon_attr	attrs[8];
+};
+
+/**
+ * s3c_hwmon_read_ch - read a value from a given adc channel.
+ * @dev: The device.
+ * @hwmon: Our state.
+ * @channel: The channel we're reading from.
+ *
+ * Read a value from the @channel with the proper locking and sleep until
+ * either the read completes or we timeout awaiting the ADC core to get
+ * back to us.
+ */
+static int s3c_hwmon_read_ch(struct device *dev,
+			     struct s3c_hwmon *hwmon, int channel)
+{
+	int ret;
+
+	ret = down_interruptible(&hwmon->lock);
+	if (ret < 0)
+		return ret;
+
+	dev_dbg(dev, "reading channel %d\n", channel);
+
+	ret = s3c_adc_read(hwmon->client, channel);
+	up(&hwmon->lock);
+
+	return ret;
+}
+
+#ifdef CONFIG_SENSORS_S3C_RAW
+/**
+ * s3c_hwmon_show_raw - show a conversion from the raw channel number.
+ * @dev: The device that the attribute belongs to.
+ * @attr: The attribute being read.
+ * @buf: The result buffer.
+ *
+ * This show deals with the raw attribute, registered for each possible
+ * ADC channel. This does a conversion and returns the raw (un-scaled)
+ * value returned from the hardware.
+ */
+static ssize_t s3c_hwmon_show_raw(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct s3c_hwmon *adc = platform_get_drvdata(to_platform_device(dev));
+	struct sensor_device_attribute *sa = to_sensor_dev_attr(attr);
+	int ret;
+
+	ret = s3c_hwmon_read_ch(dev, adc, sa->index);
+
+	return  (ret < 0) ? ret : snprintf(buf, PAGE_SIZE, "%d\n", ret);
+}
+
+#define DEF_ADC_ATTR(x)	\
+	static SENSOR_DEVICE_ATTR(adc##x##_raw, S_IRUGO, s3c_hwmon_show_raw, NULL, x)
+
+DEF_ADC_ATTR(0);
+DEF_ADC_ATTR(1);
+DEF_ADC_ATTR(2);
+DEF_ADC_ATTR(3);
+DEF_ADC_ATTR(4);
+DEF_ADC_ATTR(5);
+DEF_ADC_ATTR(6);
+DEF_ADC_ATTR(7);
+
+static struct attribute *s3c_hwmon_attrs[9] = {
+	&sensor_dev_attr_adc0_raw.dev_attr.attr,
+	&sensor_dev_attr_adc1_raw.dev_attr.attr,
+	&sensor_dev_attr_adc2_raw.dev_attr.attr,
+	&sensor_dev_attr_adc3_raw.dev_attr.attr,
+	&sensor_dev_attr_adc4_raw.dev_attr.attr,
+	&sensor_dev_attr_adc5_raw.dev_attr.attr,
+	&sensor_dev_attr_adc6_raw.dev_attr.attr,
+	&sensor_dev_attr_adc7_raw.dev_attr.attr,
+	NULL,
+};
+
+static struct attribute_group s3c_hwmon_attrgroup = {
+	.attrs	= s3c_hwmon_attrs,
+};
+
+static inline int s3c_hwmon_add_raw(struct device *dev)
+{
+	return sysfs_create_group(&dev->kobj, &s3c_hwmon_attrgroup);
+}
+
+static inline void s3c_hwmon_remove_raw(struct device *dev)
+{
+	sysfs_remove_group(&dev->kobj, &s3c_hwmon_attrgroup);
+}
+
+#else
+
+static inline int s3c_hwmon_add_raw(struct device *dev) { return 0; }
+static inline void s3c_hwmon_remove_raw(struct device *dev) { }
+
+#endif /* CONFIG_SENSORS_S3C_RAW */
+
+/**
+ * s3c_hwmon_ch_show - show value of a given channel
+ * @dev: The device that the attribute belongs to.
+ * @attr: The attribute being read.
+ * @buf: The result buffer.
+ *
+ * Read a value from the ADC and scale it before returning it to the
+ * caller. The scale factor is gained from the channel configuration
+ * passed via the platform data when the device was registered.
+ */
+static ssize_t s3c_hwmon_ch_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct sensor_device_attribute *sen_attr = to_sensor_dev_attr(attr);
+	struct s3c_hwmon *hwmon = platform_get_drvdata(to_platform_device(dev));
+	struct s3c_hwmon_pdata *pdata = dev->platform_data;
+	struct s3c_hwmon_chcfg *cfg;
+	int ret;
+
+	cfg = pdata->in[sen_attr->index];
+
+	ret = s3c_hwmon_read_ch(dev, hwmon, sen_attr->index);
+	if (ret < 0)
+		return ret;
+
+	ret *= cfg->mult;
+	ret = DIV_ROUND_CLOSEST(ret, cfg->div);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
+}
+
+/**
+ * s3c_hwmon_label_show - show label name of the given channel.
+ * @dev: The device that the attribute belongs to.
+ * @attr: The attribute being read.
+ * @buf: The result buffer.
+ *
+ * Return the label name of a given channel
+ */
+static ssize_t s3c_hwmon_label_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	struct sensor_device_attribute *sen_attr = to_sensor_dev_attr(attr);
+	struct s3c_hwmon_pdata *pdata = dev->platform_data;
+	struct s3c_hwmon_chcfg *cfg;
+
+	cfg = pdata->in[sen_attr->index];
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", cfg->name);
+}
+
+/**
+ * s3c_hwmon_create_attr - create hwmon attribute for given channel.
+ * @dev: The device to create the attribute on.
+ * @cfg: The channel configuration passed from the platform data.
+ * @channel: The ADC channel number to process.
+ *
+ * Create the scaled attribute for use with hwmon from the specified
+ * platform data in @pdata. The sysfs entry is handled by the routine
+ * s3c_hwmon_ch_show().
+ *
+ * The attribute name is taken from the configuration data if present
+ * otherwise the name is taken by concatenating in_ with the channel
+ * number.
+ */
+static int s3c_hwmon_create_attr(struct device *dev,
+				 struct s3c_hwmon_chcfg *cfg,
+				 struct s3c_hwmon_attr *attrs,
+				 int channel)
+{
+	struct sensor_device_attribute *attr;
+	int ret;
+
+	snprintf(attrs->in_name, sizeof(attrs->in_name), "in%d_input", channel);
+
+	attr = &attrs->in;
+	attr->index = channel;
+	attr->dev_attr.attr.name  = attrs->in_name;
+	attr->dev_attr.attr.mode  = S_IRUGO;
+	attr->dev_attr.attr.owner = THIS_MODULE;
+	attr->dev_attr.show = s3c_hwmon_ch_show;
+
+	ret =  device_create_file(dev, &attr->dev_attr);
+	if (ret < 0) {
+		dev_err(dev, "failed to create input attribute\n");
+		return ret;
+	}
+
+	/* if this has a name, add a label */
+	if (cfg->name) {
+		snprintf(attrs->label_name, sizeof(attrs->label_name),
+			 "in%d_label", channel);
+
+		attr = &attrs->label;
+		attr->index = channel;
+		attr->dev_attr.attr.name  = attrs->label_name;
+		attr->dev_attr.attr.mode  = S_IRUGO;
+		attr->dev_attr.attr.owner = THIS_MODULE;
+		attr->dev_attr.show = s3c_hwmon_label_show;
+
+		ret = device_create_file(dev, &attr->dev_attr);
+		if (ret < 0) {
+			device_remove_file(dev, &attrs->in.dev_attr);
+			dev_err(dev, "failed to create label attribute\n");
+		}
+	}
+
+	return ret;
+}
+
+static void s3c_hwmon_remove_attr(struct device *dev,
+				  struct s3c_hwmon_attr *attrs)
+{
+	device_remove_file(dev, &attrs->in.dev_attr);
+	device_remove_file(dev, &attrs->label.dev_attr);
+}
+
+/**
+ * s3c_hwmon_probe - device probe entry.
+ * @dev: The device being probed.
+*/
+static int __devinit s3c_hwmon_probe(struct platform_device *dev)
+{
+	struct s3c_hwmon_pdata *pdata = dev->dev.platform_data;
+	struct s3c_hwmon *hwmon;
+	int ret = 0;
+	int i;
+
+	if (!pdata) {
+		dev_err(&dev->dev, "no platform data supplied\n");
+		return -EINVAL;
+	}
+
+	hwmon = kzalloc(sizeof(struct s3c_hwmon), GFP_KERNEL);
+	if (hwmon = NULL) {
+		dev_err(&dev->dev, "no memory\n");
+		return -ENOMEM;
+	}
+
+	platform_set_drvdata(dev, hwmon);
+
+	init_MUTEX(&hwmon->lock);
+
+	/* Register with the core ADC driver. */
+
+	hwmon->client = s3c_adc_register(dev, NULL, NULL, NULL, 0);
+	if (IS_ERR(hwmon->client)) {
+		dev_err(&dev->dev, "cannot register adc\n");
+		ret = PTR_ERR(hwmon->client);
+		goto err_mem;
+	}
+
+	/* add attributes for our adc devices. */
+
+	ret = s3c_hwmon_add_raw(&dev->dev);
+	if (ret)
+		goto err_registered;
+
+	/* register with the hwmon core */
+
+	hwmon->hwmon_dev = hwmon_device_register(&dev->dev);
+	if (IS_ERR(hwmon->hwmon_dev)) {
+		dev_err(&dev->dev, "error registering with hwmon\n");
+		ret = PTR_ERR(hwmon->hwmon_dev);
+		goto err_raw_attribute;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(pdata->in); i++) {
+		if (!pdata->in[i])
+			continue;
+
+		if (pdata->in[i]->mult >= 0x10000)
+			dev_warn(&dev->dev,
+				 "channel %d multiplier too large\n",
+				 i);
+
+		ret = s3c_hwmon_create_attr(&dev->dev, pdata->in[i],
+					    &hwmon->attrs[i], i);
+		if (ret) {
+			dev_err(&dev->dev,
+					"error creating channel %d\n", i);
+
+			for (i--; i >= 0; i--)
+				s3c_hwmon_remove_attr(&dev->dev,
+							      &hwmon->attrs[i]);
+
+			goto err_hwmon_register;
+		}
+	}
+
+	return 0;
+
+ err_hwmon_register:
+	hwmon_device_unregister(hwmon->hwmon_dev);
+
+ err_raw_attribute:
+	s3c_hwmon_remove_raw(&dev->dev);
+
+ err_registered:
+	s3c_adc_release(hwmon->client);
+
+ err_mem:
+	kfree(hwmon);
+	return ret;
+}
+
+static int __devexit s3c_hwmon_remove(struct platform_device *dev)
+{
+	struct s3c_hwmon *hwmon = platform_get_drvdata(dev);
+	int i;
+
+	s3c_hwmon_remove_raw(&dev->dev);
+
+	for (i = 0; i < ARRAY_SIZE(hwmon->attrs); i++)
+		s3c_hwmon_remove_attr(&dev->dev, &hwmon->attrs[i]);
+
+	hwmon_device_unregister(hwmon->hwmon_dev);
+	s3c_adc_release(hwmon->client);
+
+	return 0;
+}
+
+static struct platform_driver s3c_hwmon_driver = {
+	.driver	= {
+		.name		= "s3c-hwmon",
+		.owner		= THIS_MODULE,
+	},
+	.probe		= s3c_hwmon_probe,
+	.remove		= __devexit_p(s3c_hwmon_remove),
+};
+
+static int __init s3c_hwmon_init(void)
+{
+	return platform_driver_register(&s3c_hwmon_driver);
+}
+
+static void __exit s3c_hwmon_exit(void)
+{
+	platform_driver_unregister(&s3c_hwmon_driver);
+}
+
+module_init(s3c_hwmon_init);
+module_exit(s3c_hwmon_exit);
+
+MODULE_AUTHOR("Ben Dooks <ben@simtec.co.uk>");
+MODULE_DESCRIPTION("S3C ADC HWMon driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:s3c-hwmon");
Index: linux.git/arch/arm/plat-s3c/include/plat/hwmon.h
=================================--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux.git/arch/arm/plat-s3c/include/plat/hwmon.h	2009-05-28 15:39:44.000000000 +0100
@@ -0,0 +1,41 @@
+/* linux/arch/arm/plat-s3c/include/plat/hwmon.h
+ *
+ * Copyright 2005 Simtec Electronics
+ *	Ben Dooks <ben@simtec.co.uk>
+ *	http://armlinux.simtec.co.uk/
+ *
+ * S3C - HWMon interface for ADC
+ *
+ * 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.
+*/
+
+#ifndef __ASM_ARCH_ADC_HWMON_H
+#define __ASM_ARCH_ADC_HWMON_H __FILE__
+
+/**
+ * s3c_hwmon_chcfg - channel configuration
+ * @name: The name to give this channel.
+ * @mult: Multiply the ADC value read by this.
+ * @div: Divide the value from the ADC by this.
+ *
+ * The value read from the ADC is converted to a value that
+ * hwmon expects (mV) by result = (value_read * @mult) / @div.
+ */
+struct s3c_hwmon_chcfg {
+	const char	*name;
+	unsigned int	mult;
+	unsigned int	div;
+};
+
+/**
+ * s3c_hwmon_pdata - HWMON platform data
+ * @in: One configuration for each possible channel used.
+ */
+struct s3c_hwmon_pdata {
+	struct s3c_hwmon_chcfg	*in[8];
+};
+
+#endif /* __ASM_ARCH_ADC_HWMON_H */
+

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] HWMON: S3C24XX series ADC driver

[Jean Delvare]

Hi Ben,

On Mon, 08 Jun 2009 12:35:21 +0100, Ben Dooks wrote:
> Add support for the ADC controller on the S3C
> series of processors to drivers/hwmon for use with
> hardware monitoring systems.
> 
> Signed-off-by: Ben Dooks <ben@simtec.co.uk>

Looks OK to me now, you can add:

Acked-by: Jean Delvare <khali@linux-fr.org>

As this patch depends on changes from the s3c tree, I guess you prefer
to merge it yourself via that tree?

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] HWMON: S3C24XX series ADC driver

[Ben Dooks]

Jean Delvare wrote:
> Hi Ben,
> 
> On Mon, 08 Jun 2009 12:35:21 +0100, Ben Dooks wrote:
>> Add support for the ADC controller on the S3C
>> series of processors to drivers/hwmon for use with
>> hardware monitoring systems.
>>
>> Signed-off-by: Ben Dooks <ben@simtec.co.uk>
> 
> Looks OK to me now, you can add:
> 
> Acked-by: Jean Delvare <khali@linux-fr.org>
> 
> As this patch depends on changes from the s3c tree, I guess you prefer
> to merge it yourself via that tree?

RMK currently has a large number of patches queued,
it would be nicer to him to merge this directly and
then pass him only the s3c specific bits to reduce
the chance of any conflicts. It isn't as if we've
currently got this enabled in the .config, so it
won't affect the autobuilders.

-- 
Ben Dooks, Software Engineer, Simtec Electronics

http://www.simtec.co.uk/

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] HWMON: S3C24XX series ADC driver

[Jean Delvare]

On Fri, 12 Jun 2009 10:04:01 +0100, Ben Dooks wrote:
> Jean Delvare wrote:
> > Hi Ben,
> > 
> > On Mon, 08 Jun 2009 12:35:21 +0100, Ben Dooks wrote:
> >> Add support for the ADC controller on the S3C
> >> series of processors to drivers/hwmon for use with
> >> hardware monitoring systems.
> >>
> >> Signed-off-by: Ben Dooks <ben@simtec.co.uk>
> > 
> > Looks OK to me now, you can add:
> > 
> > Acked-by: Jean Delvare <khali@linux-fr.org>
> > 
> > As this patch depends on changes from the s3c tree, I guess you prefer
> > to merge it yourself via that tree?
> 
> RMK currently has a large number of patches queued,
> it would be nicer to him to merge this directly and

Confused. Do you mean me, not him?

> then pass him only the s3c specific bits to reduce
> the chance of any conflicts. It isn't as if we've
> currently got this enabled in the .config, so it
> won't affect the autobuilders.

So basically you're asking me to merge code which you know doesn't
build? I can't do that, sorry.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors