On October 7, 2014 20:37, Jonathan Cameron wrote:

On October 7, 2014 3:55:55 PM GMT+01:00, "Opensource [Adam Thomson]"
[off-list ref] wrote:

quoted

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

quoted

On 23/09/14 11:53, Adam Thomson wrote:

quoted

This patch adds support for DA9150 Charger & Fuel-Gauge IC GPADC.

quoted

quoted

+
+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.

I did have a quick look when I had a spare moment, and I guess you could do
something like having an offset scale/factor which can be applied to the raw
reading, prior to adding the offset. May be an option but I think this would
also have to be exposed to user-space as I believe the same problem would reside
there as well.

quoted

quoted

quoted

+	ret = iio_map_array_register(indio_dev,

da9150_gpadc_default_maps);

quoted

quoted

+	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

quoted

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.

quoted

quoted

+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.

:) Ok fair enough.

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