On Wed, Oct 19, 2011 at 10:47:07AM -0400, Jonathan Cameron wrote: > Should move to drivers/hwmon once people are happy with it. > > Minimal support of simple in, curr and temp attributes > so far. > > Signed-off-by: Jonathan Cameron <jic23@xxxxxxxxx> Couple of comments below. > --- > drivers/iio/Kconfig | 8 ++ > drivers/iio/Makefile | 1 + > drivers/iio/iio_hwmon.c | 214 +++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 223 insertions(+), 0 deletions(-) > > diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig > index 308bc97..fb6d0a1 100644 > --- a/drivers/iio/Kconfig > +++ b/drivers/iio/Kconfig > @@ -11,6 +11,14 @@ menuconfig IIO > > if IIO > > +config IIO_HWMON > + tristate "Hwmon driver that uses channels specified via iio maps" > + help > + This is a platform driver that in combination with a suitable > + map allows IIO devices to provide basic hwmon functionality > + for those channels specified in the map. > + Should probably also depend on HWMON > + > source "drivers/iio/adc/Kconfig" > source "drivers/iio/imu/Kconfig" > source "drivers/iio/light/Kconfig" > diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile > index cfb588a..5f9c01a 100644 > --- a/drivers/iio/Makefile > +++ b/drivers/iio/Makefile > @@ -6,6 +6,7 @@ obj-y = inkern.o > obj-$(CONFIG_IIO) += iio.o > industrialio-y := core.o > > +obj-$(CONFIG_IIO_HWMON) += iio_hwmon.o > obj-y += adc/ > obj-y += imu/ > obj-y += light/ > diff --git a/drivers/iio/iio_hwmon.c b/drivers/iio/iio_hwmon.c > new file mode 100644 > index 0000000..6eeceeb > --- /dev/null > +++ b/drivers/iio/iio_hwmon.c > @@ -0,0 +1,214 @@ > +/* Hwmon client for industrial I/O devices > + * > + * Copyright (c) 2011 Jonathan Cameron > + * > + * 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. > + * > + * Limited functionality currently supported. > + */ > + > +#include <linux/kernel.h> > +#include <linux/slab.h> > +#include <linux/module.h> > +#include <linux/err.h> > +#include <linux/platform_device.h> > +#include <linux/iio/inkern.h> > +#include <linux/hwmon.h> > +#include <linux/hwmon-sysfs.h> > + > +/** > + * struct iio_hwmon_state - device instance state > + * @channels: filled with null terminated array of channels from iio > + * @num_channels: number of channels in channels (saves counting twice) > + * @hwmon_dev: associated hwmon device > + * @attr_group: the group of attributes > + * @attrs: null terminated array of attribute pointers. > + */ > +struct iio_hwmon_state { > + struct iio_channel **channels; > + int num_channels; > + struct device *hwmon_dev; > + struct attribute_group attr_group; > + struct attribute **attrs; > +}; > + > +/* > + * Assumes that IIO and hwmon operate in the same base units. > + * This is supposed to be true, but needs verification for > + * new channel types. > + */ > +static ssize_t iio_hwmon_read_val(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + long result; > + int val, ret, scaleint, scalepart; > + struct sensor_device_attribute *sattr = > + to_sensor_dev_attr(attr); Does this need more than one line ? > + struct iio_hwmon_state *state = dev_get_drvdata(dev); > + > + /* > + * No locking between this pair, so theoretically possible > + * the scale has changed. > + */ > + ret = iio_read_channel_raw(state->channels[sattr->index], > + &val); > + if (ret < 0) > + return ret; > + > + ret = iio_read_channel_scale(state->channels[sattr->index], > + &scaleint, &scalepart); > + if (ret < 0) > + return ret; > + switch (ret) { > + case IIO_VAL_INT: > + result = val * scaleint; > + break; > + case IIO_VAL_INT_PLUS_MICRO: > + result = val * (scaleint * 1000000 + scalepart)/1000000; > + break; > + case IIO_VAL_INT_PLUS_NANO: > + result = val * (scaleint * 1000000000 + scalepart)/1000000000; I think you might want to use long or even long long for all variables here, or at least typecast to long or long long. Concern is that for example "scaleint * 1000000000" may be calculated as int and would easily overflow; even as long it would easily overflow. There should be blanks around '/' (why doesn't checkpatch complain about this anymore ?). > + break; > + default: > + return -EINVAL; > + } > + return sprintf(buf, "%ld\n", result); > +} > + > +static void iio_hwmon_free_attrs(struct iio_hwmon_state *st) > +{ > + int i; > + struct sensor_device_attribute *a; > + for (i = 0; i < st->num_channels; i++) > + if (st->attrs[i]) { > + a = to_sensor_dev_attr( > + container_of(st->attrs[i], > + struct device_attribute, > + attr)); > + kfree(a); > + } > +} > + > +static int __devinit iio_hwmon_probe(struct platform_device *pdev) > +{ > + struct iio_hwmon_state *st; > + struct sensor_device_attribute *a; > + int ret, i = 0; > + int in_i = 1, temp_i = 1, curr_i = 1; > + > + st = kzalloc(sizeof(*st), GFP_KERNEL); > + if (st == NULL) { > + ret = -ENOMEM; > + goto error_ret; > + } > + > + st->channels = iio_channel_get_all(&pdev->dev, NULL); > + if (IS_ERR(st->channels)) { > + ret = PTR_ERR(st->channels); > + goto error_free_state; > + } > + > + /* count how many attributes we have */ > + while (st->channels[st->num_channels]) > + st->num_channels++; > + > + st->attrs = kzalloc(sizeof(st->attrs)*(i+1), GFP_KERNEL); Blanks around '*' and '+'. i == 0 here, so what is the point, and what are you trying to do ? Should it be st->num_channels instead of i + 1 ? > + if (st->attrs == NULL) { > + ret = -ENOMEM; > + goto error_release_channels; > + } > + for (i = 0; i < st->num_channels; i++) { > + a = kzalloc(sizeof(*a), GFP_KERNEL); What if this fails ? > + sysfs_attr_init(&a->dev_attr.attr); > + switch (iio_get_channel_type(st->channels[i])) { > + case IIO_VOLTAGE: > + a->dev_attr.attr.name = kasprintf(GFP_KERNEL, > + "in%d_input", > + in_i++); > + break; > + case IIO_TEMP: > + a->dev_attr.attr.name = kasprintf(GFP_KERNEL, > + "temp%d_input", > + temp_i++); > + break; > + case IIO_CURRENT: > + a->dev_attr.attr.name = kasprintf(GFP_KERNEL, > + "curr%d_input", > + curr_i++); > + break; > + default: > + ret = -EINVAL; > + goto error_free_attrs; > + } > + if (a->dev_attr.attr.name == NULL) { > + ret = -ENOMEM; > + goto error_free_attrs; > + } I think this and the above EINVAL will result in leaking "a" since you did not store it in st->attrs[i] yet. > + a->dev_attr.show = iio_hwmon_read_val; > + a->dev_attr.attr.mode = S_IRUGO; > + a->index = i; > + st->attrs[i] = &a->dev_attr.attr; > + } > + > + st->attr_group.attrs = st->attrs; > + st->hwmon_dev = hwmon_device_register(&pdev->dev); > + Would be better to do this after successfully creating the sysfs attributes. > + ret = sysfs_create_group(&pdev->dev.kobj, &st->attr_group); > + if (ret < 0) > + goto error_free_attrs; > + > + platform_set_drvdata(pdev, st); > + You need to do this prior to creating the sysfs entries, since the read functions expect it to be there. > + return 0; > + > +error_free_attrs: This doesn't unregister the hwmon device. > + iio_hwmon_free_attrs(st); > + kfree(st->attrs); > +error_release_channels: > + iio_channel_release_all(st->channels); > +error_free_state: > + kfree(st); > +error_ret: > + return ret; > +} > + > +static int __devexit iio_hwmon_remove(struct platform_device *pdev) > +{ > + struct iio_hwmon_state *st = platform_get_drvdata(pdev); > + > + sysfs_remove_group(&pdev->dev.kobj, &st->attr_group); > + iio_hwmon_free_attrs(st); > + kfree(st->attrs); > + hwmon_device_unregister(st->hwmon_dev); > + iio_channel_release_all(st->channels); > + > + return 0; > +} > + > +static struct platform_driver __refdata iio_hwmon_driver = { > + .driver = { > + .name = "iio_hwmon", > + .owner = THIS_MODULE, > + }, > + .probe = iio_hwmon_probe, > + .remove = __devexit_p(iio_hwmon_remove), > +}; > + > +static int iio_inkern_init(void) > +{ > + return platform_driver_register(&iio_hwmon_driver); > +} > +module_init(iio_inkern_init); > + > +static void iio_inkern_exit(void) > +{ > + platform_driver_unregister(&iio_hwmon_driver); > +} > +module_exit(iio_inkern_exit); > + > +MODULE_AUTHOR("Jonathan Cameron <jic23@xxxxxxxxx>"); > +MODULE_DESCRIPTION("IIO to hwmon driver"); > +MODULE_LICENSE("GPL v2"); > -- > 1.7.7 > -- Guenter Roeck Distinguished Engineer PDU IP Systems -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html