On Fri, 24 Sep 2021 16:30:09 +0800 hui.liu <hui.liu@xxxxxxxxxxxx> 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: > > On Tue, 14 Sep 2021 21:09:01 +0800 > > Hui Liu <hui.liu@xxxxxxxxxxxx> wrote: > > > > > Convert raw data to processed data. > > > > > > Fixes: ace4cdfe67be ("iio: adc: mt2701: Add Mediatek auxadc driver > > > for > > > mt2701.") > > > Signed-off-by: Hui Liu <hui.liu@xxxxxxxxxxxx> > > > > 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 > > > > > --- > > > 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: > > > >
