On Fri, 24 Sep 2021 16:30:09 +0800
hui.liu [off-list ref] wrote:

Hi Jonathan,

In the previous mail(maybe 2021/08/15), we want to support two case in
our driver: _RAW and _SCALE. In _SCALE case:

_RAW and _PROCESSED I think?  (scale is something else entirely)

	*val = *val * VOLTAGE_FULL_RANGE;
	*val2 = AUXADC_PRECISE;
	return IIO_VAL_FRACTIONAL_LOG2;

If user call read_raw, will get raw data; If user call read_processed,
will get processed voltage.

What's your opinion?

I'm sorry, but I don't understand the question.

I looked back at that earlier discussion and the discussion was about also
adding _RAW.  I think we concluded that, as we already had _PROCESSED and
generally do not support both, we would just continue to have processed.

The suggestion in this review is to use a different choice from the various
ways that IIO can represent values in order to potentially maintain slightly
higher precision.  Whether that actually helps depends a bit of how the
value is being used in your out of tree driver.  If that driver wants
a different scaling (perhaps micro volts rather than millivolts) then,
if I recall correctly slightly higher precision is maintained
https://elixir.bootlin.com/linux/latest/source/drivers/iio/inkern.c#L625

If you prefer to keep the code as is, just address the request for a little
more information in the patch description.

Thanks,

Jonathan
 

Thanks.


On Sat, 2021-09-18 at 19:20 +0100, Jonathan Cameron wrote:

quoted

On Tue, 14 Sep 2021 21:09:01 +0800
Hui Liu [off-list ref] wrote:
  

quoted

Convert raw data to processed data.

Fixes: ace4cdfe67be ("iio: adc: mt2701: Add Mediatek auxadc driver
for
mt2701.")
Signed-off-by: Hui Liu <redacted>  

Hi Hui Liu

This fix is obviously correct but I think we can improve it a little.

1) Add a bit more detail to the patch description.  Perhaps change it
to something like
Previously the driver did not apply the scaling necessary to take the
voltage range of this ADC into account. 

2) If you change to

	*val = *val * VOLTAGE_FULL_RANGE;
	*val2 = 12;
	return IIO_VAL_FRACTIONAL_LOG2;

then you should get a more precise answer.  (Please check that
though!)
This might be an issue if you have consumers drivers though that can
not
cope with this particular type.  If so please state that in the patch
description
and add a comment to the code to say that so we don't end up
'improving' this
in future without taking those consumers into account.

Thanks,

Jonathan
  

quoted

---
 drivers/iio/adc/mt6577_auxadc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/iio/adc/mt6577_auxadc.c

b/drivers/iio/adc/mt6577_auxadc.c
index 79c1dd68b909..d4fccd52ef08 100644

--- a/drivers/iio/adc/mt6577_auxadc.c
+++ b/drivers/iio/adc/mt6577_auxadc.c

@@ -82,6 +82,10 @@ static const struct iio_chan_spec

mt6577_auxadc_iio_channels[] = {
 	MT6577_AUXADC_CHANNEL(15),
 };
 
+/* For Voltage calculation */
+#define VOLTAGE_FULL_RANGE  1500	/* VA voltage */
+#define AUXADC_PRECISE      4096	/* 12 bits */
+
 static int mt_auxadc_get_cali_data(int rawdata, bool enable_cali)
 {
 	return rawdata;

@@ -191,6 +195,10 @@ static int mt6577_auxadc_read_raw(struct

iio_dev *indio_dev,
 		}
 		if (adc_dev->dev_comp->sample_data_cali)
 			*val = mt_auxadc_get_cali_data(*val, true);
+
+		/* Convert adc raw data to voltage: 0 - 1500 mV */
+		*val = *val * VOLTAGE_FULL_RANGE / AUXADC_PRECISE;
+
 		return IIO_VAL_INT;
 
 	default:  

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel