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

[Jonathan Cameron <jic23@kernel.org>]

On October 7, 2014 3:55:55 PM GMT+01:00, "Opensource [Adam Thomson]" <Adam.Thomson.Opensource@diasemi.com> wrote:
>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.
Oops got that wrong.  The fixed point maths to fix the in kernel interface isn't exactly
 difficult but indeed it does not handle this currently.
>
>> > +	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.
Nope just that such a new function might be useful.

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.