Re: [PATCH 5/6] IIO:hwmon interface client driver.

[Jonathan Cameron <jic23@cam.ac.uk>]

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!