Message-ID: <20111019163816.GC2264@ericsson.com>
Date:	Wed, 19 Oct 2011 09:38:16 -0700
From:	Guenter Roeck <guenter.roeck@...csson.com>
To:	Jonathan Cameron <jic23@....ac.uk>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
	"linus.ml.walleij@...il.com" <linus.ml.walleij@...il.com>,
	"zdevai@...il.com" <zdevai@...il.com>,
	"linux@....linux.org.uk" <linux@....linux.org.uk>,
	"arnd@...db.de" <arnd@...db.de>,
	"broonie@...nsource.wolfsonmicro.com" 
	<broonie@...nsource.wolfsonmicro.com>,
	"gregkh@...e.de" <gregkh@...e.de>,
	"lm-sensors@...sensors.org" <lm-sensors@...sensors.org>,
	"khali@...ux-fr.org" <khali@...ux-fr.org>
Subject: Re: [PATCH 5/6] IIO:hwmon interface client driver.

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@....ac.uk>

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@....ac.uk>");
> +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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists