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

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

[Jonathan Cameron]

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) {


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

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

[Jonathan Cameron]

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) {  
> > 
> >   


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

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

[Jonathan Cameron]

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) {    
> > > > 
> > > >     
> > 
> >   


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

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

[Jonathan Cameron]

On Thu, 26 Aug 2021 10:27:34 +0800
hui.liu <hui.liu@mediatek.com> wrote:

> Hi Jonathan,
> 
> Our internal Kernel module CCCI use _RAW case, we described in the
> previous mail. Could you give your suggestion?

Fix your internal module?  I would have assumed it would want
calibrated values if available, so it seems a little odd to use _RAW.
If there is a good reason for it doing so that you can talk about here
then perhaps we can consider it.

As a general rule we don't put any significant effort into supporting
out of tree drivers so I'm not very keen on adding _RAW on the basis
one uses it.

Jonathan


> 
> Thanks.
> Hui
> 
> > 
> > On Tue, 2021-08-17 at 16:54 +0100, Jonathan Cameron wrote:  
> > > 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
> > >   
> > 
> > Hi Jonathan,
> > 
> > For we have an internal use scenario:
> > Our internal Kernel module CCCI(Communication interface between AP
> > Core and Modem) would like get raw data from ADC, but CCCI driver
> > have not do upstream now.
> > 
> > If we no need add _RAW, we will only change _PROCESSED readback value
> > in the next version of patch.
> >   
> > > > 
> > > > 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) {      
> > > > > > > 
> > > > > > >       
> > > > > 
> > > > >     
> > > 
> > >   
> > 
> >   


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