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/24/11 17:10, Guenter Roeck wrote:
    > On Mon, 2011-10-24 at 11:58 -0400, Jonathan Cameron wrote:
    >> On 10/24/11 16:39, Guenter Roeck wrote:
    >>> On Mon, 2011-10-24 at 06:09 -0400, Jonathan Cameron wrote:
    >>> [ ... ]
    >>>>>>> +/*
    >>>>>>> + * 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;
    >>>
    >>> Is div_s64 really necessary, or would
    >>>
    >>> result = (long)val * (long)scaleint +
    >>> DIV_ROUND_CLOSEST((s64)val * (s64)scalepart,
    >>> 1000000000LL);
    >>>
    >>> work as well ?
    >> Not if you want it to compile on arm v5 by the look of it.
    >>
    >> ERROR: "__aeabi_ldivmod" [drivers/staging/iio/iio_hwmon.ko] undefined!
    >>
    > Annoying. Ok, I don't have a better idea than using div_s64. You don't
    > need s64 for the first part of the operation (val * scaleint), though,
    > since the result is a long.
    True enough. Pretty unlikely we are going to have 2 MV hwmon devices any
    time soon. I'll pop that back down to int * int I think!



    \
     
     \ /
      Last update: 2011-10-24 18:19    [W:0.041 / U:59.536 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site