Re: [PATCH] iio: mtk-auxadc: update case IIO_CHAN_INFO_PROCESSED

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 24 Sep 2021 16:30:09 +0800
hui.liu <hui.liu@mediatek.com> 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@mediatek.com> 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@mediatek.com>  
> > 
> > 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:  
> > 
> >