lkml.org 
[lkml]   [2011]   [Oct]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 5/6] IIO:hwmon interface client driver.
    On 10/20/11 16:30, Jonathan Cameron wrote:
    > On 10/20/11 16:12, Guenter Roeck wrote:
    >> On Thu, 2011-10-20 at 05:33 -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@cam.ac.uk>
    >>> ---
    >>> drivers/iio/Kconfig | 8 ++
    >>> drivers/iio/Makefile | 1 +
    >>> drivers/iio/iio_hwmon.c | 227 +++++++++++++++++++++++++++++++++++++++++++++++
    >>> 3 files changed, 236 insertions(+), 0 deletions(-)
    >>>
    >>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
    >>> index 308bc97..c2f0970 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"
    >>> + depends on HWMON
    >>> + 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.
    >>> +
    >>> 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..b3348ad
    >>> --- /dev/null
    >>> +++ b/drivers/iio/iio_hwmon.c
    >>> @@ -0,0 +1,227 @@
    >>> +/* 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.
    >>
    >> Just nitpicking ... this comment doesn't provide much value. It doesn't
    >> explain the limits, nor what could be improved.
    >>
    >>> + */
    >>> +
    >>> +#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);
    >>> + 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 = (long)val * (long)scaleint +
    >>> + (long)val * (long)scalepart / 1000000L;
    >>> + break;
    >>> + case IIO_VAL_INT_PLUS_NANO:
    >>> + result = (long)val * (long)scaleint +
    >>> + (long)val * (long)scalepart / 1000000000L;
    >>> + break;
    >>
    >> Still easy to imagine that val * scalepart gets larger than 2147483647L
    >> (on machines where sizeof(long) = 4) ... it will already happen if the
    >> result of (val * scalepart / 1000000000) is larger than 2.
    > Good point. I really ought to have done the calcs.
    > If we have maximum possible value in here things will be ugly.
    >
    > Worst case is scalepart is 9999999999. (could be done as 1 - 0.000000001
    > which would be nicer, but we don't specify a preference - from this
    > discussion I am suspecting we should!)
    >
    > Looks like 64 bits is going to be a requirement as you say.
    >>
    >> What value range do you expect to see here ?
    >>
    >> If (val * scaleint) is already the milli-unit, scalepart would possibly
    >> only address fractions of milli-units. If so, the result of (val *
    >> scalepart / 1000000000L) might always be smaller than 1, ie 0.
    > It certainly should be.
    >> If so, for the calculation to have any value, you might be better off using
    >> DIV_ROUND_CLOSEST(val * scalepart, 1000000000L).
    > Good idea.
    >>
    >> I am a bit confused by this anyway. Since hwmon in general reports
    >> milli-units, VAL_INT appears to reflect milli-units, VAL_INT_PLUS_MICRO
    >> really means nano-units, and IIO_VAL_INT_PLUS_NANO really means
    >> pico-units. Is this correct ?
    > Micro units of the scale factor.
    >
    > Take my test part a max1363...
    > Scale is actually 0.5 so each adc count (e.g. raw value) is 0.5millivolts.
    >
    > scale int here is 0,
    > scale part is 500,000 (so 0.5) and it returns IIO_VAL_INT_PLUS_MICRO.

    How about the following? It'll be extremely costly, but this isn't exactly
    a fast path!

    case IIO_VAL_INT_PLUS_MICRO:
    result = (s64)val * (s64)scaleint +
    div_s64((s64)val * (s64)scalepart, 1000000LL);
    break;
    case IIO_VAL_INT_PLUS_NANO:
    result = (s64)val * (s64)scaleint +
    div_s64((s64)val * (s64)scalepart, 1000000000LL);
    break;

    Everything should fit in there and it should give us pretty good precision.
    >
    >
    >>
    >>> + 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;
    >>> + 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) * (st->num_channels + 1),
    >>> + GFP_KERNEL);
    >>
    >> Why "+ 1" ?
    > Null terminated list for attribute groups. Hence the kzalloc.
    >>
    >> Unless I am missing something, you only use st->attrs[0] ..
    >> st->attrs[st->num_channels-1].
    >>
    >> Thanks,
    >> Guenter
    >>
    >>
    >>
    >
    > --
    > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
    > the body of a message to majordomo@vger.kernel.org
    > More majordomo info at http://vger.kernel.org/majordomo-info.html
    >



    \
     
     \ /
      Last update: 2011-10-24 12:13    [W:0.040 / U:153.068 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site