On October 7, 2014 20:37, Jonathan Cameron wrote: > On October 7, 2014 3:55:55 PM GMT+01:00, "Opensource [Adam Thomson]" > <Adam.Thomson.Opensource@xxxxxxxxxxx> 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. 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. > > > >> > + 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. :) Ok fair enough. > > -- > Sent from my Android phone with K-9 Mail. Please excuse my brevity. ��.n��������+%������w��{.n�����{��(��)��jg��������ݢj����G�������j:+v���w�m������w�������h�����٥