diff mbox series

[v1,1/2] iio: mtk-auxadc: add support IIO_CHAN_INFO_RAW case

Message ID 20210812054844.30575-2-hui.liu@mediatek.com
State Superseded
Headers show
Series AUXADC: Mediatek auxadc driver | expand

Commit Message

Hui Liu Aug. 12, 2021, 5:48 a.m. UTC 
Add support IIO_CHAN_INFO_RAW case.

Signed-off-by: Hui Liu <hui.liu@mediatek.com>
---
 drivers/iio/adc/mt6577_auxadc.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Comments

Jonathan Cameron Aug. 12, 2021, 6:07 p.m. UTC | #1 
On Thu, 12 Aug 2021 13:48:43 +0800
Hui Liu <hui.liu@mediatek.com> wrote:

> Add support IIO_CHAN_INFO_RAW case.
Why?

We almost never support both RAW and PROCESSED as userspace should be
fine to use either.  There are a few reasons we've let drivers do
this but I would like know why it matters to you and it definitely
needs to be in the patch description.

> 
> Signed-off-by: Hui Liu <hui.liu@mediatek.com>
> ---
>  drivers/iio/adc/mt6577_auxadc.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/mt6577_auxadc.c b/drivers/iio/adc/mt6577_auxadc.c
> index 79c1dd68b909..e995d43287b2 100644
> --- a/drivers/iio/adc/mt6577_auxadc.c
> +++ b/drivers/iio/adc/mt6577_auxadc.c
> @@ -60,7 +60,8 @@ static const struct mtk_auxadc_compatible mt6765_compat = {
>  		.type = IIO_VOLTAGE,				    \
>  		.indexed = 1,					    \
>  		.channel = (idx),				    \
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	    \
> +				      BIT(IIO_CHAN_INFO_PROCESSED), \
>  }
>  
>  static const struct iio_chan_spec mt6577_auxadc_iio_channels[] = {
> @@ -181,6 +182,19 @@ static int mt6577_auxadc_read_raw(struct iio_dev *indio_dev,
>  	struct mt6577_auxadc_device *adc_dev = iio_priv(indio_dev);
>  
>  	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		*val = mt6577_auxadc_read(indio_dev, chan);
> +		if (*val < 0) {
> +			dev_notice(indio_dev->dev.parent,
> +				"failed to sample data on channel[%d]\n",
> +				chan->channel);
> +			return *val;
> +		}
> +		if (adc_dev->dev_comp->sample_data_cali)
> +			*val = mt_auxadc_get_cali_data(*val, true);
> +
> +		return IIO_VAL_INT;
> +
>  	case IIO_CHAN_INFO_PROCESSED:
>  		*val = mt6577_auxadc_read(indio_dev, chan);
>  		if (*val < 0) {
Jonathan Cameron Aug. 14, 2021, 4:10 p.m. UTC | #2 
On Fri, 13 Aug 2021 11:46:24 +0800
hui.liu <hui.liu@mediatek.com> wrote:

> On Thu, 2021-08-12 at 19:07 +0100, Jonathan Cameron wrote:

> > On Thu, 12 Aug 2021 13:48:43 +0800

> > Hui Liu <hui.liu@mediatek.com> wrote:

> >   

> > > Add support IIO_CHAN_INFO_RAW case.  

> > 

> > Why?

> > 

> > We almost never support both RAW and PROCESSED as userspace should be

> > fine to use either.  There are a few reasons we've let drivers do

> > this but I would like know why it matters to you and it definitely

> > needs to be in the patch description.

> >   

> Hi Jonathan,

> 

> 1. To support ADC consumers' different types of requirement: some

> consumers want to call iio_read_channel_raw to get raw data, the others

> use iio_read_channel_processed to get voltage.


Give an example of the consumer using the raw channel readback (without
acess to any scaling information?)

> 2. In our origin driver, if consumer call iio_read_channel_processed,

> read back value is raw data. 

> 

> Could we use SCALE instead of PROCESSED in patch for next version, or

> what's your suggestion?


That would unfortunately be a userspace ABI change.  We can add interfaces
but taking them away is normally a problem :( 

Your reasons here are fine, subject to information on what consumer cares
about having _RAW, please resend the patch with this information added
to the description.

Thanks,

Jonathan
> 

> Thanks.

> 

> > > 

> > > Signed-off-by: Hui Liu <hui.liu@mediatek.com>

> > > ---

> > >  drivers/iio/adc/mt6577_auxadc.c | 16 +++++++++++++++-

> > >  1 file changed, 15 insertions(+), 1 deletion(-)

> > > 

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

> > > b/drivers/iio/adc/mt6577_auxadc.c

> > > index 79c1dd68b909..e995d43287b2 100644

> > > --- a/drivers/iio/adc/mt6577_auxadc.c

> > > +++ b/drivers/iio/adc/mt6577_auxadc.c

> > > @@ -60,7 +60,8 @@ static const struct mtk_auxadc_compatible

> > > mt6765_compat = {

> > >  		.type = IIO_VOLTAGE,				   

> > >  \

> > >  		.indexed = 1,					   

> > >  \

> > >  		.channel = (idx),				    \

> > > -		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \

> > > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	   

> > >  \

> > > +				      BIT(IIO_CHAN_INFO_PROCESSED), \

> > >  }

> > >  

> > >  static const struct iio_chan_spec mt6577_auxadc_iio_channels[] = {

> > > @@ -181,6 +182,19 @@ static int mt6577_auxadc_read_raw(struct

> > > iio_dev *indio_dev,

> > >  	struct mt6577_auxadc_device *adc_dev = iio_priv(indio_dev);

> > >  

> > >  	switch (info) {

> > > +	case IIO_CHAN_INFO_RAW:

> > > +		*val = mt6577_auxadc_read(indio_dev, chan);

> > > +		if (*val < 0) {

> > > +			dev_notice(indio_dev->dev.parent,

> > > +				"failed to sample data on

> > > channel[%d]\n",

> > > +				chan->channel);

> > > +			return *val;

> > > +		}

> > > +		if (adc_dev->dev_comp->sample_data_cali)

> > > +			*val = mt_auxadc_get_cali_data(*val, true);

> > > +

> > > +		return IIO_VAL_INT;

> > > +

> > >  	case IIO_CHAN_INFO_PROCESSED:

> > >  		*val = mt6577_auxadc_read(indio_dev, chan);

> > >  		if (*val < 0) {  

> > 

> >
Jonathan Cameron Aug. 17, 2021, 3:54 p.m. UTC | #3 
On Tue, 17 Aug 2021 16:37:52 +0800
hui.liu <hui.liu@mediatek.com> wrote:

> On Sat, 2021-08-14 at 17:10 +0100, Jonathan Cameron wrote:

> > On Fri, 13 Aug 2021 11:46:24 +0800

> > hui.liu <hui.liu@mediatek.com> wrote:

> >   

> > > On Thu, 2021-08-12 at 19:07 +0100, Jonathan Cameron wrote:  

> > > > On Thu, 12 Aug 2021 13:48:43 +0800

> > > > Hui Liu <hui.liu@mediatek.com> wrote:

> > > >     

> > > > > Add support IIO_CHAN_INFO_RAW case.    

> > > > 

> > > > Why?

> > > > 

> > > > We almost never support both RAW and PROCESSED as userspace

> > > > should be

> > > > fine to use either.  There are a few reasons we've let drivers do

> > > > this but I would like know why it matters to you and it

> > > > definitely

> > > > needs to be in the patch description.

> > > >     

> > > 

> > > Hi Jonathan,

> > > 

> > > 1. To support ADC consumers' different types of requirement: some

> > > consumers want to call iio_read_channel_raw to get raw data, the

> > > others

> > > use iio_read_channel_processed to get voltage.  

> > 

> > Give an example of the consumer using the raw channel readback

> > (without

> > acess to any scaling information?)

> >   

> 

> > > 2. In our origin driver, if consumer call

> > > iio_read_channel_processed,

> > > read back value is raw data. 

> > > 

> > > Could we use SCALE instead of PROCESSED in patch for next version,

> > > or

> > > what's your suggestion?  

> > 

> > That would unfortunately be a userspace ABI change.  We can add

> > interfaces

> > but taking them away is normally a problem :( 

> > 

> > Your reasons here are fine, subject to information on what consumer

> > cares

> > about having _RAW, please resend the patch with this information

> > added

> > to the description.

> > 

> > Thanks,

> > 

> > Jonathan  

> 

> 1. We found afe/iio-rescale.c, dac/dpot-dac.c and multiplexer/iio-mux.c 

> call iio_read_channel_raw to get raw data. If they use our ADC driver,

> I think we should support _RAW case. 

> If we support _RAW case, we will add more information in v2

> description.


iio-rescale has recently gained support for processed.
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/drivers/iio/afe/iio-rescale.c?h=testing&id=53ebee9499805add3eef630d998c40812e6a1c39
dpot-dac is a rather obscure special case, so I doubt you actually have one of those.
If iio-mux is relevant then we should add processed support to that driver as well.

I would rather see those users of the interface fixed than us
having to tweak lots of drivers to provide _raw when it isn't appropriate for
that piece of hardware.

Jonathan

> 

> 2. Since we change _PROCESSED readback value from raw data to voltage,

> our consumer will make the changes synchronously. 

> 

> > > 

> > > Thanks.

> > >   

> > > > > 

> > > > > Signed-off-by: Hui Liu <hui.liu@mediatek.com>

> > > > > ---

> > > > >  drivers/iio/adc/mt6577_auxadc.c | 16 +++++++++++++++-

> > > > >  1 file changed, 15 insertions(+), 1 deletion(-)

> > > > > 

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

> > > > > b/drivers/iio/adc/mt6577_auxadc.c

> > > > > index 79c1dd68b909..e995d43287b2 100644

> > > > > --- a/drivers/iio/adc/mt6577_auxadc.c

> > > > > +++ b/drivers/iio/adc/mt6577_auxadc.c

> > > > > @@ -60,7 +60,8 @@ static const struct mtk_auxadc_compatible

> > > > > mt6765_compat = {

> > > > >  		.type = IIO_VOLTAGE,				

> > > > >    

> > > > >  \

> > > > >  		.indexed = 1,					

> > > > >    

> > > > >  \

> > > > >  		.channel = (idx),				

> > > > >     \

> > > > > -		.info_mask_separate =

> > > > > BIT(IIO_CHAN_INFO_PROCESSED), \

> > > > > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	

> > > > >    

> > > > >  \

> > > > > +				      BIT(IIO_CHAN_INFO_PROCESS

> > > > > ED), \

> > > > >  }

> > > > >  

> > > > >  static const struct iio_chan_spec mt6577_auxadc_iio_channels[]

> > > > > = {

> > > > > @@ -181,6 +182,19 @@ static int mt6577_auxadc_read_raw(struct

> > > > > iio_dev *indio_dev,

> > > > >  	struct mt6577_auxadc_device *adc_dev =

> > > > > iio_priv(indio_dev);

> > > > >  

> > > > >  	switch (info) {

> > > > > +	case IIO_CHAN_INFO_RAW:

> > > > > +		*val = mt6577_auxadc_read(indio_dev, chan);

> > > > > +		if (*val < 0) {

> > > > > +			dev_notice(indio_dev->dev.parent,

> > > > > +				"failed to sample data on

> > > > > channel[%d]\n",

> > > > > +				chan->channel);

> > > > > +			return *val;

> > > > > +		}

> > > > > +		if (adc_dev->dev_comp->sample_data_cali)

> > > > > +			*val = mt_auxadc_get_cali_data(*val,

> > > > > true);

> > > > > +

> > > > > +		return IIO_VAL_INT;

> > > > > +

> > > > >  	case IIO_CHAN_INFO_PROCESSED:

> > > > >  		*val = mt6577_auxadc_read(indio_dev, chan);

> > > > >  		if (*val < 0) {    

> > > > 

> > > >     

> > 

> >
diff mbox series

Patch

diff --git a/drivers/iio/adc/mt6577_auxadc.c b/drivers/iio/adc/mt6577_auxadc.c
index 79c1dd68b909..e995d43287b2 100644
--- a/drivers/iio/adc/mt6577_auxadc.c
+++ b/drivers/iio/adc/mt6577_auxadc.c
@@ -60,7 +60,8 @@  static const struct mtk_auxadc_compatible mt6765_compat = {
 		.type = IIO_VOLTAGE,				    \
 		.indexed = 1,					    \
 		.channel = (idx),				    \
-		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	    \
+				      BIT(IIO_CHAN_INFO_PROCESSED), \
 }
 
 static const struct iio_chan_spec mt6577_auxadc_iio_channels[] = {
@@ -181,6 +182,19 @@  static int mt6577_auxadc_read_raw(struct iio_dev *indio_dev,
 	struct mt6577_auxadc_device *adc_dev = iio_priv(indio_dev);
 
 	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		*val = mt6577_auxadc_read(indio_dev, chan);
+		if (*val < 0) {
+			dev_notice(indio_dev->dev.parent,
+				"failed to sample data on channel[%d]\n",
+				chan->channel);
+			return *val;
+		}
+		if (adc_dev->dev_comp->sample_data_cali)
+			*val = mt_auxadc_get_cali_data(*val, true);
+
+		return IIO_VAL_INT;
+
 	case IIO_CHAN_INFO_PROCESSED:
 		*val = mt6577_auxadc_read(indio_dev, chan);
 		if (*val < 0) {