diff mbox series

[1/3] iio: adc: ti-ads8344: properly byte swap value

Message ID 20200415212257.161238-2-alexandre.belloni@bootlin.com (mailing list archive)
State New, archived
Headers show
Series iio: adc: ti-ads8344: improve the driver | expand

Commit Message

Alexandre Belloni April 15, 2020, 9:22 p.m. UTC 
The first received byte is the MSB, followed by the LSB so the value needs
to be byte swapped.

Also, the ADC actually has a delay of one clock on the SPI bus. Read three
bytes to get the last bit.

Fixes: 8dd2d7c0fed7 ("iio: adc: Add driver for the TI ADS8344 A/DC chips")
Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/iio/adc/ti-ads8344.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

kernel test robot April 16, 2020, 6:22 a.m. UTC | #1 
Hi Alexandre,

I love your patch! Yet something to improve:

[auto build test ERROR on iio/togreg]
[also build test ERROR on v5.7-rc1 next-20200415]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Alexandre-Belloni/iio-adc-ti-ads8344-improve-the-driver/20200416-073357
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: c6x-allyesconfig (attached as .config)
compiler: c6x-elf-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=c6x 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):


vim +/302 +96 drivers/iio/adc/ti-ads8344.c

    72	
    73	static int ads8344_adc_conversion(struct ads8344 *adc, int channel,
    74					  bool differential)
    75	{
    76		struct spi_device *spi = adc->spi;
    77		int ret;
    78		u8 buf[3];
    79	
    80		adc->tx_buf = ADS8344_START;
    81		if (!differential)
    82			adc->tx_buf |= ADS8344_SINGLE_END;
    83		adc->tx_buf |= ADS8344_CHANNEL(channel);
    84		adc->tx_buf |= ADS8344_CLOCK_INTERNAL;
    85	
    86		ret = spi_write(spi, &adc->tx_buf, 1);
    87		if (ret)
    88			return ret;
    89	
    90		udelay(9);
    91	
    92		ret = spi_read(spi, buf, sizeof(buf));
    93		if (ret)
    94			return ret;
    95	
  > 96		return buf[0] << 9 | buf[1] << 1 | buf[2] >> 7;
    97	}
    98	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Lars-Peter Clausen April 16, 2020, 6:29 a.m. UTC | #2 
On 4/15/20 11:22 PM, Alexandre Belloni wrote:
> The first received byte is the MSB, followed by the LSB so the value needs
> to be byte swapped.
>
> Also, the ADC actually has a delay of one clock on the SPI bus. Read three
> bytes to get the last bit.
>
> Fixes: 8dd2d7c0fed7 ("iio: adc: Add driver for the TI ADS8344 A/DC chips")
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>   drivers/iio/adc/ti-ads8344.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/adc/ti-ads8344.c b/drivers/iio/adc/ti-ads8344.c
> index 9a460807d46d..6da50ea35217 100644
> --- a/drivers/iio/adc/ti-ads8344.c
> +++ b/drivers/iio/adc/ti-ads8344.c
> @@ -29,7 +29,6 @@ struct ads8344 {
>   	struct mutex lock;
>   
>   	u8 tx_buf ____cacheline_aligned;
> -	u16 rx_buf;
>   };
>   
>   #define ADS8344_VOLTAGE_CHANNEL(chan, si)				\
> @@ -76,6 +75,7 @@ static int ads8344_adc_conversion(struct ads8344 *adc, int channel,
>   {
>   	struct spi_device *spi = adc->spi;
>   	int ret;
> +	u8 buf[3];
Same as with spi_write(), spi_read() transfer buffers should not be on 
the stack in case the driver tries to use it for DMA.
>   
>   	adc->tx_buf = ADS8344_START;
>   	if (!differential)
> @@ -89,11 +89,11 @@ static int ads8344_adc_conversion(struct ads8344 *adc, int channel,
>   
>   	udelay(9);
>   
> -	ret = spi_read(spi, &adc->rx_buf, 2);
> +	ret = spi_read(spi, buf, sizeof(buf));
>   	if (ret)
>   		return ret;
>   
> -	return adc->rx_buf;
> +	return buf[0] << 9 | buf[1] << 1 | buf[2] >> 7;
>   }
>   
>   static int ads8344_read_raw(struct iio_dev *iio,
Alexandre Belloni April 16, 2020, 8:50 p.m. UTC | #3 
Hi,

On 16/04/2020 14:22:03+0800, kbuild test robot wrote:
> Hi Alexandre,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on iio/togreg]
> [also build test ERROR on v5.7-rc1 next-20200415]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> 
> url:    https://github.com/0day-ci/linux/commits/Alexandre-Belloni/iio-adc-ti-ads8344-improve-the-driver/20200416-073357
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> config: c6x-allyesconfig (attached as .config)
> compiler: c6x-elf-gcc (GCC) 9.3.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=c6x 
> 

I spent some time to reproduce and this is actually not that trivial
because your toolchains are linked with libisl22 and most distributions
still ship an older version. Maybe you can do something about that?

> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kbuild test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
> 
> vim +/302 +96 drivers/iio/adc/ti-ads8344.c
> 
>     72	
>     73	static int ads8344_adc_conversion(struct ads8344 *adc, int channel,
>     74					  bool differential)
>     75	{
>     76		struct spi_device *spi = adc->spi;
>     77		int ret;
>     78		u8 buf[3];
>     79	
>     80		adc->tx_buf = ADS8344_START;
>     81		if (!differential)
>     82			adc->tx_buf |= ADS8344_SINGLE_END;
>     83		adc->tx_buf |= ADS8344_CHANNEL(channel);
>     84		adc->tx_buf |= ADS8344_CLOCK_INTERNAL;
>     85	
>     86		ret = spi_write(spi, &adc->tx_buf, 1);
>     87		if (ret)
>     88			return ret;
>     89	
>     90		udelay(9);
>     91	
>     92		ret = spi_read(spi, buf, sizeof(buf));
>     93		if (ret)
>     94			return ret;
>     95	
>   > 96		return buf[0] << 9 | buf[1] << 1 | buf[2] >> 7;
>     97	}
>     98	
> 

I take it this is a false positive as I don't get any errors when
building this driver with the provided toolchain. However, I see a few
"internal compiler error: in priority, at haifa-sched.c:1599"
Philip Li April 19, 2020, 2:49 a.m. UTC | #4 
On Thu, Apr 16, 2020 at 10:50:23PM +0200, Alexandre Belloni wrote:
> Hi,
> 
> On 16/04/2020 14:22:03+0800, kbuild test robot wrote:
> > Hi Alexandre,
> > 
> > I love your patch! Yet something to improve:
> > 
> > [auto build test ERROR on iio/togreg]
> > [also build test ERROR on v5.7-rc1 next-20200415]
> > [if your patch is applied to the wrong git tree, please drop us a note to help
> > improve the system. BTW, we also suggest to use '--base' option to specify the
> > base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> > 
> > url:    https://github.com/0day-ci/linux/commits/Alexandre-Belloni/iio-adc-ti-ads8344-improve-the-driver/20200416-073357
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> > config: c6x-allyesconfig (attached as .config)
> > compiler: c6x-elf-gcc (GCC) 9.3.0
> > reproduce:
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # save the attached .config to linux build tree
> >         COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=c6x 
> > 
> 
> I spent some time to reproduce and this is actually not that trivial
> because your toolchains are linked with libisl22 and most distributions
> still ship an older version. Maybe you can do something about that?
Thanks for the feedback, we will resolve this to use old version in
earliest time.

> 
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kbuild test robot <lkp@intel.com>
> > 
> > All errors (new ones prefixed by >>):
> > 
> > 
> > vim +/302 +96 drivers/iio/adc/ti-ads8344.c
> > 
> >     72	
> >     73	static int ads8344_adc_conversion(struct ads8344 *adc, int channel,
> >     74					  bool differential)
> >     75	{
> >     76		struct spi_device *spi = adc->spi;
> >     77		int ret;
> >     78		u8 buf[3];
> >     79	
> >     80		adc->tx_buf = ADS8344_START;
> >     81		if (!differential)
> >     82			adc->tx_buf |= ADS8344_SINGLE_END;
> >     83		adc->tx_buf |= ADS8344_CHANNEL(channel);
> >     84		adc->tx_buf |= ADS8344_CLOCK_INTERNAL;
> >     85	
> >     86		ret = spi_write(spi, &adc->tx_buf, 1);
> >     87		if (ret)
> >     88			return ret;
> >     89	
> >     90		udelay(9);
> >     91	
> >     92		ret = spi_read(spi, buf, sizeof(buf));
> >     93		if (ret)
> >     94			return ret;
> >     95	
> >   > 96		return buf[0] << 9 | buf[1] << 1 | buf[2] >> 7;
> >     97	}
> >     98	
> > 
> 
> I take it this is a false positive as I don't get any errors when
> building this driver with the provided toolchain. However, I see a few
> "internal compiler error: in priority, at haifa-sched.c:1599"
> 
> -- 
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
Xia, Hui April 21, 2020, 7:25 a.m. UTC | #5 
>-----Original Message-----
>From: Li, Philip <philip.li@intel.com>
>Sent: 2020年4月19日 10:50
>To: Alexandre Belloni <alexandre.belloni@bootlin.com>
>Cc: lkp <lkp@intel.com>; Jonathan Cameron <jic23@kernel.org>; kbuild-
>all@lists.01.org; Hartmut Knaack <knaack.h@gmx.de>; Lars-Peter Clausen
><lars@metafoo.de>; Peter Meerwald-Stadler <pmeerw@pmeerw.net>; Gregory
>CLEMENT <gregory.clement@bootlin.com>; linux-iio@vger.kernel.org; linux-
>kernel@vger.kernel.org
>Subject: Re: [PATCH 1/3] iio: adc: ti-ads8344: properly byte swap value
>
>On Thu, Apr 16, 2020 at 10:50:23PM +0200, Alexandre Belloni wrote:
>> Hi,
>>
>> On 16/04/2020 14:22:03+0800, kbuild test robot wrote:
>> > Hi Alexandre,
>> >
>> > I love your patch! Yet something to improve:
>> >
>> > [auto build test ERROR on iio/togreg] [also build test ERROR on
>> > v5.7-rc1 next-20200415] [if your patch is applied to the wrong git
>> > tree, please drop us a note to help improve the system. BTW, we also
>> > suggest to use '--base' option to specify the base tree in git
>> > format-patch, please see https://stackoverflow.com/a/37406982]
>> >
>> > url:    https://github.com/0day-ci/linux/commits/Alexandre-Belloni/iio-adc-ti-
>ads8344-improve-the-driver/20200416-073357
>> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
>> > config: c6x-allyesconfig (attached as .config)
>> > compiler: c6x-elf-gcc (GCC) 9.3.0
>> > reproduce:
>> >         wget https://raw.githubusercontent.com/intel/lkp-
>tests/master/sbin/make.cross -O ~/bin/make.cross
>> >         chmod +x ~/bin/make.cross
>> >         # save the attached .config to linux build tree
>> >         COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0
>> > make.cross ARCH=c6x
>> >
>>
>> I spent some time to reproduce and this is actually not that trivial
>> because your toolchains are linked with libisl22 and most
>> distributions still ship an older version. Maybe you can do something about that?
>Thanks for the feedback, we will resolve this to use old version in earliest time.
The cross toolchains has been updated to link with libisl19 the stable version. Please remove the old toolchain and make.cross again.
Feel free to let us know if still have problem. Thanks.

>
>>
>> > If you fix the issue, kindly add following tag as appropriate
>> > Reported-by: kbuild test robot <lkp@intel.com>
>> >
>> > All errors (new ones prefixed by >>):
>> >
>> >
>> > vim +/302 +96 drivers/iio/adc/ti-ads8344.c
>> >
>> >     72
>> >     73	static int ads8344_adc_conversion(struct ads8344 *adc, int
>channel,
>> >     74					  bool differential)
>> >     75	{
>> >     76		struct spi_device *spi = adc->spi;
>> >     77		int ret;
>> >     78		u8 buf[3];
>> >     79
>> >     80		adc->tx_buf = ADS8344_START;
>> >     81		if (!differential)
>> >     82			adc->tx_buf |= ADS8344_SINGLE_END;
>> >     83		adc->tx_buf |= ADS8344_CHANNEL(channel);
>> >     84		adc->tx_buf |= ADS8344_CLOCK_INTERNAL;
>> >     85
>> >     86		ret = spi_write(spi, &adc->tx_buf, 1);
>> >     87		if (ret)
>> >     88			return ret;
>> >     89
>> >     90		udelay(9);
>> >     91
>> >     92		ret = spi_read(spi, buf, sizeof(buf));
>> >     93		if (ret)
>> >     94			return ret;
>> >     95
>> >   > 96		return buf[0] << 9 | buf[1] << 1 | buf[2] >> 7;
>> >     97	}
>> >     98
>> >
>>
>> I take it this is a false positive as I don't get any errors when
>> building this driver with the provided toolchain. However, I see a few
>> "internal compiler error: in priority, at haifa-sched.c:1599"
>>
>> --
>> Alexandre Belloni, Bootlin
>> Embedded Linux and Kernel engineering
>> https://bootlin.com
>>
Alexandre Belloni April 21, 2020, 12:24 p.m. UTC | #6 
Hi,

On 21/04/2020 07:25:01+0000, Xia, Hui wrote:
> >-----Original Message-----
> >From: Li, Philip <philip.li@intel.com>
> >Sent: 2020年4月19日 10:50
> >To: Alexandre Belloni <alexandre.belloni@bootlin.com>
> >Cc: lkp <lkp@intel.com>; Jonathan Cameron <jic23@kernel.org>; kbuild-
> >all@lists.01.org; Hartmut Knaack <knaack.h@gmx.de>; Lars-Peter Clausen
> ><lars@metafoo.de>; Peter Meerwald-Stadler <pmeerw@pmeerw.net>; Gregory
> >CLEMENT <gregory.clement@bootlin.com>; linux-iio@vger.kernel.org; linux-
> >kernel@vger.kernel.org
> >Subject: Re: [PATCH 1/3] iio: adc: ti-ads8344: properly byte swap value
> >
> >On Thu, Apr 16, 2020 at 10:50:23PM +0200, Alexandre Belloni wrote:
> >> Hi,
> >>
> >> On 16/04/2020 14:22:03+0800, kbuild test robot wrote:
> >> > Hi Alexandre,
> >> >
> >> > I love your patch! Yet something to improve:
> >> >
> >> > [auto build test ERROR on iio/togreg] [also build test ERROR on
> >> > v5.7-rc1 next-20200415] [if your patch is applied to the wrong git
> >> > tree, please drop us a note to help improve the system. BTW, we also
> >> > suggest to use '--base' option to specify the base tree in git
> >> > format-patch, please see https://stackoverflow.com/a/37406982]
> >> >
> >> > url:    https://github.com/0day-ci/linux/commits/Alexandre-Belloni/iio-adc-ti-
> >ads8344-improve-the-driver/20200416-073357
> >> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> >> > config: c6x-allyesconfig (attached as .config)
> >> > compiler: c6x-elf-gcc (GCC) 9.3.0
> >> > reproduce:
> >> >         wget https://raw.githubusercontent.com/intel/lkp-
> >tests/master/sbin/make.cross -O ~/bin/make.cross
> >> >         chmod +x ~/bin/make.cross
> >> >         # save the attached .config to linux build tree
> >> >         COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0
> >> > make.cross ARCH=c6x
> >> >
> >>
> >> I spent some time to reproduce and this is actually not that trivial
> >> because your toolchains are linked with libisl22 and most
> >> distributions still ship an older version. Maybe you can do something about that?
> >Thanks for the feedback, we will resolve this to use old version in earliest time.
> The cross toolchains has been updated to link with libisl19 the stable version. Please remove the old toolchain and make.cross again.
> Feel free to let us know if still have problem. Thanks.
> 

Thank you, this works properly on more machines now.
diff mbox series

Patch

diff --git a/drivers/iio/adc/ti-ads8344.c b/drivers/iio/adc/ti-ads8344.c
index 9a460807d46d..6da50ea35217 100644
--- a/drivers/iio/adc/ti-ads8344.c
+++ b/drivers/iio/adc/ti-ads8344.c
@@ -29,7 +29,6 @@  struct ads8344 {
 	struct mutex lock;
 
 	u8 tx_buf ____cacheline_aligned;
-	u16 rx_buf;
 };
 
 #define ADS8344_VOLTAGE_CHANNEL(chan, si)				\
@@ -76,6 +75,7 @@  static int ads8344_adc_conversion(struct ads8344 *adc, int channel,
 {
 	struct spi_device *spi = adc->spi;
 	int ret;
+	u8 buf[3];
 
 	adc->tx_buf = ADS8344_START;
 	if (!differential)
@@ -89,11 +89,11 @@  static int ads8344_adc_conversion(struct ads8344 *adc, int channel,
 
 	udelay(9);
 
-	ret = spi_read(spi, &adc->rx_buf, 2);
+	ret = spi_read(spi, buf, sizeof(buf));
 	if (ret)
 		return ret;
 
-	return adc->rx_buf;
+	return buf[0] << 9 | buf[1] << 1 | buf[2] >> 7;
 }
 
 static int ads8344_read_raw(struct iio_dev *iio,