RE: [PATCH 3/8] iio: Add support for DA9150 GPADC

[Opensource [Adam Thomson]]

On September 27, 2014 11:50, Jonathan Cameron wrote:

> On 23/09/14 11:53, Adam Thomson wrote:
> > This patch adds support for DA9150 Charger & Fuel-Gauge IC GPADC.

> > +
> > +static inline int da9150_gpadc_gpio_6v_voltage_now(int raw_val)
> > +{
> > +	/* Convert to mV */
> > +	return (6 * ((raw_val * 1000) + 500)) / 1024;
> These could all be expressed as raw values with offsets
> and scales (and that would be preferred).
> E.g. This one has offset 500000 and scale 6000/1024 or even
> better use IIO_VAL_FRACTIONAL_LOG2 for scale with val1 = 6000
> and val2 = (log_2 1024) = 10.
> 

What you've suggested isn't correct. The problem here is that the offset is
added first to the raw ADC reading, without factoring the ADC value accordingly
to match the factor of the offset. If we take the original equation provided for
this channel of the ADC, the offset is actually 0.5 which should be added to the
raw ADC value. This doesn't fit into the implementation in the kernel as we
can't use floating point. If we multiply the offset but not the raw ADC value,
then add them before applying the scale factor, then the result is wrong at the
end. Basically you need a scale for the raw ADC value to match the offset scale
so you can achieve the correct results, which is what my calculation does.
But that seems impossible with the current raw|offset|scale method.

> > +	ret = iio_map_array_register(indio_dev, da9150_gpadc_default_maps);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to register IIO maps: %d\n", ret);
> > +		return ret;
> > +	}
> I'd suggest doing the devm_request_thread_irq before the iio_map_array
> stuff.  This is purely to avoid the order during remove not being
> obviously correct as it isn't the reverse of during probe.

Ok, should still work ok that way so can update.

> > +static int da9150_gpadc_remove(struct platform_device *pdev)
> > +{
> > +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > +
> > +	iio_map_array_unregister(indio_dev);
> Twice in one day.  I'm definitely thinking we should add a
> devm version of iio_map_array_register...

I assume you mean here that iio_device_unregister() should come first? Will
update.