linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] iio: adc: ti-ads8344: improve the driver
@ 2020-04-15 21:22 Alexandre Belloni
  2020-04-15 21:22 ` [PATCH 1/3] iio: adc: ti-ads8344: properly byte swap value Alexandre Belloni
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Alexandre Belloni @ 2020-04-15 21:22 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Gregory CLEMENT, linux-iio, linux-kernel, Alexandre Belloni

Hello,

This series improves the ads8344 driver.

The first patch is fix and can be backported.

The second patch removes the dubious ____cacheline_aligned

The last one is improving power consumption by shutting down the ADC
while it is not used.

Alexandre Belloni (3):
  iio: adc: ti-ads8344: properly byte swap value
  iio: adc: ti-ads8344: remove tx_buf from driver data
  iio: adc: ti-ads8344: optimize consumption

 drivers/iio/adc/ti-ads8344.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

-- 
2.25.2


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/3] iio: adc: ti-ads8344: properly byte swap value
  2020-04-15 21:22 [PATCH 0/3] iio: adc: ti-ads8344: improve the driver Alexandre Belloni
@ 2020-04-15 21:22 ` Alexandre Belloni
  2020-04-16  6:22   ` kbuild test robot
  2020-04-16  6:29   ` Lars-Peter Clausen
  2020-04-15 21:22 ` [PATCH 2/3] iio: adc: ti-ads8344: remove tx_buf from driver data Alexandre Belloni
  2020-04-15 21:22 ` [PATCH 3/3] iio: adc: ti-ads8344: optimize consumption Alexandre Belloni
  2 siblings, 2 replies; 12+ messages in thread
From: Alexandre Belloni @ 2020-04-15 21:22 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Gregory CLEMENT, linux-iio, linux-kernel, Alexandre Belloni

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];
 
 	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,
-- 
2.25.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/3] iio: adc: ti-ads8344: remove tx_buf from driver data
  2020-04-15 21:22 [PATCH 0/3] iio: adc: ti-ads8344: improve the driver Alexandre Belloni
  2020-04-15 21:22 ` [PATCH 1/3] iio: adc: ti-ads8344: properly byte swap value Alexandre Belloni
@ 2020-04-15 21:22 ` Alexandre Belloni
  2020-04-16  6:26   ` Lars-Peter Clausen
  2020-04-17 10:24   ` Andy Shevchenko
  2020-04-15 21:22 ` [PATCH 3/3] iio: adc: ti-ads8344: optimize consumption Alexandre Belloni
  2 siblings, 2 replies; 12+ messages in thread
From: Alexandre Belloni @ 2020-04-15 21:22 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Gregory CLEMENT, linux-iio, linux-kernel, Alexandre Belloni

There is no need to keep tx_buf around, it is only used for the conversion.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/iio/adc/ti-ads8344.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/adc/ti-ads8344.c b/drivers/iio/adc/ti-ads8344.c
index 6da50ea35217..9b2d3a8ea6bd 100644
--- a/drivers/iio/adc/ti-ads8344.c
+++ b/drivers/iio/adc/ti-ads8344.c
@@ -22,13 +22,7 @@
 struct ads8344 {
 	struct spi_device *spi;
 	struct regulator *reg;
-	/*
-	 * Lock protecting access to adc->tx_buff and rx_buff,
-	 * especially from concurrent read on sysfs file.
-	 */
-	struct mutex lock;
-
-	u8 tx_buf ____cacheline_aligned;
+	struct mutex lock; /* protect from concurrent conversions */
 };
 
 #define ADS8344_VOLTAGE_CHANNEL(chan, si)				\
@@ -77,13 +71,13 @@ static int ads8344_adc_conversion(struct ads8344 *adc, int channel,
 	int ret;
 	u8 buf[3];
 
-	adc->tx_buf = ADS8344_START;
+	buf[0] = ADS8344_START;
 	if (!differential)
-		adc->tx_buf |= ADS8344_SINGLE_END;
-	adc->tx_buf |= ADS8344_CHANNEL(channel);
-	adc->tx_buf |= ADS8344_CLOCK_INTERNAL;
+		buf[0] |= ADS8344_SINGLE_END;
+	buf[0] |= ADS8344_CHANNEL(channel);
+	buf[0] |= ADS8344_CLOCK_INTERNAL;
 
-	ret = spi_write(spi, &adc->tx_buf, 1);
+	ret = spi_write(spi, buf, 1);
 	if (ret)
 		return ret;
 
-- 
2.25.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/3] iio: adc: ti-ads8344: optimize consumption
  2020-04-15 21:22 [PATCH 0/3] iio: adc: ti-ads8344: improve the driver Alexandre Belloni
  2020-04-15 21:22 ` [PATCH 1/3] iio: adc: ti-ads8344: properly byte swap value Alexandre Belloni
  2020-04-15 21:22 ` [PATCH 2/3] iio: adc: ti-ads8344: remove tx_buf from driver data Alexandre Belloni
@ 2020-04-15 21:22 ` Alexandre Belloni
  2 siblings, 0 replies; 12+ messages in thread
From: Alexandre Belloni @ 2020-04-15 21:22 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Gregory CLEMENT, linux-iio, linux-kernel, Alexandre Belloni

Set the clock mode only once, at probe time and then keep the ADC powered
down between conversions.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/iio/adc/ti-ads8344.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ti-ads8344.c b/drivers/iio/adc/ti-ads8344.c
index 9b2d3a8ea6bd..0804650d8d8a 100644
--- a/drivers/iio/adc/ti-ads8344.c
+++ b/drivers/iio/adc/ti-ads8344.c
@@ -65,7 +65,7 @@ static const struct iio_chan_spec ads8344_channels[] = {
 };
 
 static int ads8344_adc_conversion(struct ads8344 *adc, int channel,
-				  bool differential)
+				  bool differential, u8 clock)
 {
 	struct spi_device *spi = adc->spi;
 	int ret;
@@ -75,7 +75,7 @@ static int ads8344_adc_conversion(struct ads8344 *adc, int channel,
 	if (!differential)
 		buf[0] |= ADS8344_SINGLE_END;
 	buf[0] |= ADS8344_CHANNEL(channel);
-	buf[0] |= ADS8344_CLOCK_INTERNAL;
+	buf[0] |= clock;
 
 	ret = spi_write(spi, buf, 1);
 	if (ret)
@@ -100,7 +100,7 @@ static int ads8344_read_raw(struct iio_dev *iio,
 	case IIO_CHAN_INFO_RAW:
 		mutex_lock(&adc->lock);
 		*value = ads8344_adc_conversion(adc, channel->scan_index,
-						channel->differential);
+						channel->differential, 0);
 		mutex_unlock(&adc->lock);
 		if (*value < 0)
 			return *value;
@@ -155,6 +155,11 @@ static int ads8344_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
+	/* Do a dummy read and set external clock mode */
+	ret = ads8344_adc_conversion(adc, 0, 0, ADS8344_CLOCK_INTERNAL);
+	if (ret < 0)
+		return ret;
+
 	spi_set_drvdata(spi, indio_dev);
 
 	ret = iio_device_register(indio_dev);
-- 
2.25.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] iio: adc: ti-ads8344: properly byte swap value
  2020-04-15 21:22 ` [PATCH 1/3] iio: adc: ti-ads8344: properly byte swap value Alexandre Belloni
@ 2020-04-16  6:22   ` kbuild test robot
  2020-04-16 20:50     ` Alexandre Belloni
  2020-04-16  6:29   ` Lars-Peter Clausen
  1 sibling, 1 reply; 12+ messages in thread
From: kbuild test robot @ 2020-04-16  6:22 UTC (permalink / raw)
  To: Alexandre Belloni, Jonathan Cameron
  Cc: kbuild-all, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Gregory CLEMENT, linux-iio, linux-kernel,
	Alexandre Belloni

[-- Attachment #1: Type: text/plain, Size: 1972 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51700 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] iio: adc: ti-ads8344: remove tx_buf from driver data
  2020-04-15 21:22 ` [PATCH 2/3] iio: adc: ti-ads8344: remove tx_buf from driver data Alexandre Belloni
@ 2020-04-16  6:26   ` Lars-Peter Clausen
  2020-04-17 10:24   ` Andy Shevchenko
  1 sibling, 0 replies; 12+ messages in thread
From: Lars-Peter Clausen @ 2020-04-16  6:26 UTC (permalink / raw)
  To: Alexandre Belloni, Jonathan Cameron
  Cc: Hartmut Knaack, Peter Meerwald-Stadler, Gregory CLEMENT,
	linux-iio, linux-kernel

On 4/15/20 11:22 PM, Alexandre Belloni wrote:
> There is no need to keep tx_buf around, it is only used for the conversion.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>   drivers/iio/adc/ti-ads8344.c | 18 ++++++------------
>   1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iio/adc/ti-ads8344.c b/drivers/iio/adc/ti-ads8344.c
> index 6da50ea35217..9b2d3a8ea6bd 100644
> --- a/drivers/iio/adc/ti-ads8344.c
> +++ b/drivers/iio/adc/ti-ads8344.c
> @@ -22,13 +22,7 @@
>   struct ads8344 {
>   	struct spi_device *spi;
>   	struct regulator *reg;
> -	/*
> -	 * Lock protecting access to adc->tx_buff and rx_buff,
> -	 * especially from concurrent read on sysfs file.
> -	 */
> -	struct mutex lock;
> -
> -	u8 tx_buf ____cacheline_aligned;
> +	struct mutex lock; /* protect from concurrent conversions */
>   };
>   
>   #define ADS8344_VOLTAGE_CHANNEL(chan, si)				\
> @@ -77,13 +71,13 @@ static int ads8344_adc_conversion(struct ads8344 *adc, int channel,
>   	int ret;
>   	u8 buf[3];

spi_write() might use the buffer in a DMA transfer, so it can't be on 
the stack and needs to be in its own cacheline.

>   
> -	adc->tx_buf = ADS8344_START;
> +	buf[0] = ADS8344_START;
>   	if (!differential)
> -		adc->tx_buf |= ADS8344_SINGLE_END;
> -	adc->tx_buf |= ADS8344_CHANNEL(channel);
> -	adc->tx_buf |= ADS8344_CLOCK_INTERNAL;
> +		buf[0] |= ADS8344_SINGLE_END;
> +	buf[0] |= ADS8344_CHANNEL(channel);
> +	buf[0] |= ADS8344_CLOCK_INTERNAL;
>   
> -	ret = spi_write(spi, &adc->tx_buf, 1);
> +	ret = spi_write(spi, buf, 1);
>   	if (ret)
>   		return ret;
>   



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] iio: adc: ti-ads8344: properly byte swap value
  2020-04-15 21:22 ` [PATCH 1/3] iio: adc: ti-ads8344: properly byte swap value Alexandre Belloni
  2020-04-16  6:22   ` kbuild test robot
@ 2020-04-16  6:29   ` Lars-Peter Clausen
  1 sibling, 0 replies; 12+ messages in thread
From: Lars-Peter Clausen @ 2020-04-16  6:29 UTC (permalink / raw)
  To: Alexandre Belloni, Jonathan Cameron
  Cc: Hartmut Knaack, Peter Meerwald-Stadler, Gregory CLEMENT,
	linux-iio, linux-kernel

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,



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] iio: adc: ti-ads8344: properly byte swap value
  2020-04-16  6:22   ` kbuild test robot
@ 2020-04-16 20:50     ` Alexandre Belloni
  2020-04-19  2:49       ` Philip Li
  0 siblings, 1 reply; 12+ messages in thread
From: Alexandre Belloni @ 2020-04-16 20:50 UTC (permalink / raw)
  To: kbuild test robot
  Cc: Jonathan Cameron, kbuild-all, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Gregory CLEMENT, linux-iio, linux-kernel

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"

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] iio: adc: ti-ads8344: remove tx_buf from driver data
  2020-04-15 21:22 ` [PATCH 2/3] iio: adc: ti-ads8344: remove tx_buf from driver data Alexandre Belloni
  2020-04-16  6:26   ` Lars-Peter Clausen
@ 2020-04-17 10:24   ` Andy Shevchenko
  1 sibling, 0 replies; 12+ messages in thread
From: Andy Shevchenko @ 2020-04-17 10:24 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Gregory CLEMENT, linux-iio,
	Linux Kernel Mailing List

On Thu, Apr 16, 2020 at 4:08 AM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> There is no need to keep tx_buf around, it is only used for the conversion.
>

As Lars said. And some SPI controllers may want to DMA even one byte, so, NAK.


> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>  drivers/iio/adc/ti-ads8344.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iio/adc/ti-ads8344.c b/drivers/iio/adc/ti-ads8344.c
> index 6da50ea35217..9b2d3a8ea6bd 100644
> --- a/drivers/iio/adc/ti-ads8344.c
> +++ b/drivers/iio/adc/ti-ads8344.c
> @@ -22,13 +22,7 @@
>  struct ads8344 {
>         struct spi_device *spi;
>         struct regulator *reg;
> -       /*
> -        * Lock protecting access to adc->tx_buff and rx_buff,
> -        * especially from concurrent read on sysfs file.
> -        */
> -       struct mutex lock;
> -
> -       u8 tx_buf ____cacheline_aligned;
> +       struct mutex lock; /* protect from concurrent conversions */
>  };
>
>  #define ADS8344_VOLTAGE_CHANNEL(chan, si)                              \
> @@ -77,13 +71,13 @@ static int ads8344_adc_conversion(struct ads8344 *adc, int channel,
>         int ret;
>         u8 buf[3];
>
> -       adc->tx_buf = ADS8344_START;
> +       buf[0] = ADS8344_START;
>         if (!differential)
> -               adc->tx_buf |= ADS8344_SINGLE_END;
> -       adc->tx_buf |= ADS8344_CHANNEL(channel);
> -       adc->tx_buf |= ADS8344_CLOCK_INTERNAL;
> +               buf[0] |= ADS8344_SINGLE_END;
> +       buf[0] |= ADS8344_CHANNEL(channel);
> +       buf[0] |= ADS8344_CLOCK_INTERNAL;
>
> -       ret = spi_write(spi, &adc->tx_buf, 1);
> +       ret = spi_write(spi, buf, 1);
>         if (ret)
>                 return ret;
>
> --
> 2.25.2
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] iio: adc: ti-ads8344: properly byte swap value
  2020-04-16 20:50     ` Alexandre Belloni
@ 2020-04-19  2:49       ` Philip Li
  2020-04-21  7:25         ` Xia, Hui
  0 siblings, 1 reply; 12+ messages in thread
From: Philip Li @ 2020-04-19  2:49 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: kbuild test robot, Jonathan Cameron, kbuild-all, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, Gregory CLEMENT,
	linux-iio, linux-kernel

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
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH 1/3] iio: adc: ti-ads8344: properly byte swap value
  2020-04-19  2:49       ` Philip Li
@ 2020-04-21  7:25         ` Xia, Hui
  2020-04-21 12:24           ` Alexandre Belloni
  0 siblings, 1 reply; 12+ messages in thread
From: Xia, Hui @ 2020-04-21  7:25 UTC (permalink / raw)
  To: Li, Philip, Alexandre Belloni
  Cc: lkp, Jonathan Cameron, kbuild-all, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, Gregory CLEMENT,
	linux-iio, linux-kernel



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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] iio: adc: ti-ads8344: properly byte swap value
  2020-04-21  7:25         ` Xia, Hui
@ 2020-04-21 12:24           ` Alexandre Belloni
  0 siblings, 0 replies; 12+ messages in thread
From: Alexandre Belloni @ 2020-04-21 12:24 UTC (permalink / raw)
  To: Xia, Hui
  Cc: Li, Philip, lkp, Jonathan Cameron, kbuild-all, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, Gregory CLEMENT,
	linux-iio, linux-kernel

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.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-04-21 12:24 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-15 21:22 [PATCH 0/3] iio: adc: ti-ads8344: improve the driver Alexandre Belloni
2020-04-15 21:22 ` [PATCH 1/3] iio: adc: ti-ads8344: properly byte swap value Alexandre Belloni
2020-04-16  6:22   ` kbuild test robot
2020-04-16 20:50     ` Alexandre Belloni
2020-04-19  2:49       ` Philip Li
2020-04-21  7:25         ` Xia, Hui
2020-04-21 12:24           ` Alexandre Belloni
2020-04-16  6:29   ` Lars-Peter Clausen
2020-04-15 21:22 ` [PATCH 2/3] iio: adc: ti-ads8344: remove tx_buf from driver data Alexandre Belloni
2020-04-16  6:26   ` Lars-Peter Clausen
2020-04-17 10:24   ` Andy Shevchenko
2020-04-15 21:22 ` [PATCH 3/3] iio: adc: ti-ads8344: optimize consumption Alexandre Belloni

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).