Re: [PATCH v3 3/3] iio: adc: add new lp8788 adc driver

[Lars-Peter Clausen <lars@metafoo.de>]

On 08/16/2012 09:39 AM, Kim, Milo wrote:
> Patch v3.
> (a) Delete unnecessary blank line of description
> (b) Sort alphabetical order for header
> (c) Replace udelay() with usleep_range()
> (d) Change read_raw() in case of scale and offset
>     : result can be calculated as raw * (scaleint + scalepart * 1000000) + offset.
>       (scale: micro unit)

For IIO it should actually be (raw + offset) * scale and the result should be
in the unit which is specified in the IIO sysfs ABI document. E.g. milivolts
for voltages.

[...]
> +
> +	if (lp8788_read_multi_bytes(adc->lp, LP8788_ADC_RAW, rawdata, size))
> +		goto err;
> +
> +	msb = (rawdata[0] << 4) & 0x00000ff0;
> +	lsb = (rawdata[1] >> 4) & 0x0000000f;
> +	result = msb | lsb;
> +
> +	/* iio consumer: result = raw * scale + offset */
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		*val = result;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = lp8788_scale[id];
> +		*val2 = 0;

Scale should be the number of millivolts per bit. Given the number in the table
above I somehow doubt that this is what you return here. Well or maybe this
part is actually used to measure up to a few kilo volts ;)

> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = lp8788_scale[id] / 2;
> +		return IIO_VAL_INT;

As said above offset should be in the same unit as the raw result. I'd expect
it to be same for each channel.

Btw. how does the voltage mapping look in you case?

0x000 raw -> ? V
0x800 raw -> ? V
0xfff raw -> ? V

> +	default:
> +		break;
> +	}
> +
> +err:
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info lp8788_adc_info = {
> +	.read_raw = &lp8788_adc_read_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +#define LP8788_CHAN(_id, _type) {				\
> +		.type = _type,					\
> +		.indexed = 1,					\
> +		.channel = LPADC_##_id,				\
> +		.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |	\
> +			IIO_CHAN_INFO_SCALE_SEPARATE_BIT,	\
> +		.address = LP8788_ADC_RAW,			\
> +		.scan_type = IIO_ST('u', 8, 12, 4),		\

This says your sample has 8 bits and you want to store them in a 12 bit word.
This seems wrong.


> +		.scan_index = 1,				\
> +		.datasheet_name = #_id,				\
> +}
> +