[PATCH 0/3] iio: adc: ti-ads8344: improve the driver

[PATCH 0/3] iio: adc: ti-ads8344: improve the driver

[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

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

[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

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

[kbuild test robot]

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

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

[Alexandre Belloni]

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

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

[Philip Li]

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
> 

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

[Xia, Hui]

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

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

[Alexandre Belloni]

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

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

[Lars-Peter Clausen]

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,

[PATCH 2/3] iio: adc: ti-ads8344: remove tx_buf from driver data

[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

Re: [PATCH 2/3] iio: adc: ti-ads8344: remove tx_buf from driver data

[Lars-Peter Clausen]

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

Re: [PATCH 2/3] iio: adc: ti-ads8344: remove tx_buf from driver data

[Andy Shevchenko]

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

[PATCH 3/3] iio: adc: ti-ads8344: optimize consumption

[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