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 at 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 at 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 at 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 at 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
