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

[Guenter Roeck <guenter.roeck@ericsson.com>]

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 ?

Thanks,
Guenter