diff mbox series

[v2,2/2] spi: fsl-spi: Implement trailing bits

Message ID fe4a3946a66ede73f6d6871700f2aaf0171372a1.1646060734.git.christophe.leroy@csgroup.eu (mailing list archive)
State New, archived
Headers show
Series Add support for components requiring trailing clock after transfer | expand

Commit Message

Christophe Leroy Feb. 28, 2022, 3:15 p.m. UTC 
In order to support IDT 801034 QUAD PCM CODEC, implement
trailing clock for the amount of requested bits.

On fsl SPI, the minimum we can implement is a 4 bits shot.
If the requested number is greated than 8, use 16.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 drivers/spi/spi-fsl-spi.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

Comments

Mark Brown Feb. 28, 2022, 3:29 p.m. UTC | #1 
On Mon, Feb 28, 2022 at 04:15:46PM +0100, Christophe Leroy wrote:

> +	if (!status && spi->trailing_bits) {
> +		struct spi_transfer t = {
> +			.len = 1,
> +			.tx_buf = empty_zero_page,
> +		};
> +
> +		if (spi->trailing_bits < 4)
> +			t.bits_per_word = 4;
> +		else if (spi->trailing_bits > 8)
> +			t.bits_per_word = 16;
> +		else
> +			t.bits_per_word = spi->trailing_bits;
> +
> +		status = fsl_spi_setup_transfer(spi, &t);
> +		if (!status)
> +			status = fsl_spi_bufs(spi, &t, 0);
> +	}
> +	m->status = status;

The binding looks good now but this is still driver specific code when
it looks like it could easily be implemented in the core - like I said
on the previous version you'd need to update drivers to advertise less
than 8 bits but there's basically nothing driver specific I can see here
so any driver using transfer_one() would get support that way.
Christophe Leroy Feb. 28, 2022, 4:02 p.m. UTC | #2 
Le 28/02/2022 à 16:29, Mark Brown a écrit :
> On Mon, Feb 28, 2022 at 04:15:46PM +0100, Christophe Leroy wrote:
> 
>> +	if (!status && spi->trailing_bits) {
>> +		struct spi_transfer t = {
>> +			.len = 1,
>> +			.tx_buf = empty_zero_page,
>> +		};
>> +
>> +		if (spi->trailing_bits < 4)
>> +			t.bits_per_word = 4;
>> +		else if (spi->trailing_bits > 8)
>> +			t.bits_per_word = 16;
>> +		else
>> +			t.bits_per_word = spi->trailing_bits;
>> +
>> +		status = fsl_spi_setup_transfer(spi, &t);
>> +		if (!status)
>> +			status = fsl_spi_bufs(spi, &t, 0);
>> +	}
>> +	m->status = status;
> 
> The binding looks good now but this is still driver specific code when
> it looks like it could easily be implemented in the core - like I said
> on the previous version you'd need to update drivers to advertise less
> than 8 bits but there's basically nothing driver specific I can see here
> so any driver using transfer_one() would get support that way.

Argh ! Sorry your comment to the previous version ended up in Junk 
mails. I see it now.

We discussed that back in 2016 in 
https://lore.kernel.org/linux-spi/20160824112701.GE22076@sirena.org.uk/ 
and my understanding at that time was that it was not something that 
could be done at core level.

But maybe things have changed since then ?

By the way, fsl-spi driver doesn't implement transfer_one() but 
transfer_one_message() so it takes care of the chipselect changes and 
therefore the final dummy transfer with CS off is to be done there as 
far as I understand.

Would it mean changing fsl-spi driver to implement transfer_one() first ?

Thanks
Christophe
Mark Brown Feb. 28, 2022, 4:14 p.m. UTC | #3 
On Mon, Feb 28, 2022 at 04:02:30PM +0000, Christophe Leroy wrote:
> Le 28/02/2022 à 16:29, Mark Brown a écrit :

> > The binding looks good now but this is still driver specific code when
> > it looks like it could easily be implemented in the core - like I said
> > on the previous version you'd need to update drivers to advertise less
> > than 8 bits but there's basically nothing driver specific I can see here
> > so any driver using transfer_one() would get support that way.

> Argh ! Sorry your comment to the previous version ended up in Junk 
> mails. I see it now.

No problem.

> We discussed that back in 2016 in 
> https://lore.kernel.org/linux-spi/20160824112701.GE22076@sirena.org.uk/ 
> and my understanding at that time was that it was not something that 
> could be done at core level.

> But maybe things have changed since then ?

What I said then was "it would need a new core feature" which is what
the binding does, I'm suggesting that you also do that for the handling
of the implementation as well.

Actually now I think about it perhaps this shouldn't be a binding at all
but rather something specified by the client driver - presumably any
system using an affected device is going to need these extra clock
cycles so they'll all need to add the same property.

> By the way, fsl-spi driver doesn't implement transfer_one() but 
> transfer_one_message() so it takes care of the chipselect changes and 
> therefore the final dummy transfer with CS off is to be done there as 
> far as I understand.

> Would it mean changing fsl-spi driver to implement transfer_one() first ?

Well, if it can implement transfer_one() without any negative
consequences whichh
kernel test robot Feb. 28, 2022, 6:28 p.m. UTC | #4 
Hi Christophe,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v5.17-rc6]
[cannot apply to broonie-spi/for-next next-20220228]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Christophe-Leroy/Add-support-for-components-requiring-trailing-clock-after-transfer/20220228-231740
base:    7e57714cd0ad2d5bb90e50b5096a0e671dec1ef3
config: hexagon-buildonly-randconfig-r004-20220228 (https://download.01.org/0day-ci/archive/20220301/202203010254.tIHIltE2-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/7eb07c4d26401389204fcc6cf685c18c89b64ef8
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Christophe-Leroy/Add-support-for-components-requiring-trailing-clock-after-transfer/20220228-231740
        git checkout 7eb07c4d26401389204fcc6cf685c18c89b64ef8
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/spi/

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

All warnings (new ones prefixed by >>):

>> drivers/spi/spi-fsl-spi.c:435:14: warning: incompatible integer to pointer conversion initializing 'const void *' with an expression of type 'unsigned long' [-Wint-conversion]
                           .tx_buf = empty_zero_page,
                                     ^~~~~~~~~~~~~~~
   1 warning generated.


vim +435 drivers/spi/spi-fsl-spi.c

   356	
   357	static int fsl_spi_do_one_msg(struct spi_master *master,
   358				      struct spi_message *m)
   359	{
   360		struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(master);
   361		struct spi_device *spi = m->spi;
   362		struct spi_transfer *t, *first;
   363		unsigned int cs_change;
   364		const int nsecs = 50;
   365		int status, last_bpw;
   366	
   367		/*
   368		 * In CPU mode, optimize large byte transfers to use larger
   369		 * bits_per_word values to reduce number of interrupts taken.
   370		 */
   371		if (!(mpc8xxx_spi->flags & SPI_CPM_MODE)) {
   372			list_for_each_entry(t, &m->transfers, transfer_list) {
   373				if (t->len < 256 || t->bits_per_word != 8)
   374					continue;
   375				if ((t->len & 3) == 0)
   376					t->bits_per_word = 32;
   377				else if ((t->len & 1) == 0)
   378					t->bits_per_word = 16;
   379			}
   380		}
   381	
   382		/* Don't allow changes if CS is active */
   383		cs_change = 1;
   384		list_for_each_entry(t, &m->transfers, transfer_list) {
   385			if (cs_change)
   386				first = t;
   387			cs_change = t->cs_change;
   388			if (first->speed_hz != t->speed_hz) {
   389				dev_err(&spi->dev,
   390					"speed_hz cannot change while CS is active\n");
   391				return -EINVAL;
   392			}
   393		}
   394	
   395		last_bpw = -1;
   396		cs_change = 1;
   397		status = -EINVAL;
   398		list_for_each_entry(t, &m->transfers, transfer_list) {
   399			if (cs_change || last_bpw != t->bits_per_word)
   400				status = fsl_spi_setup_transfer(spi, t);
   401			if (status < 0)
   402				break;
   403			last_bpw = t->bits_per_word;
   404	
   405			if (cs_change) {
   406				fsl_spi_chipselect(spi, BITBANG_CS_ACTIVE);
   407				ndelay(nsecs);
   408			}
   409			cs_change = t->cs_change;
   410			if (t->len)
   411				status = fsl_spi_bufs(spi, t, m->is_dma_mapped);
   412			if (status) {
   413				status = -EMSGSIZE;
   414				break;
   415			}
   416			m->actual_length += t->len;
   417	
   418			spi_transfer_delay_exec(t);
   419	
   420			if (cs_change) {
   421				ndelay(nsecs);
   422				fsl_spi_chipselect(spi, BITBANG_CS_INACTIVE);
   423				ndelay(nsecs);
   424			}
   425		}
   426	
   427		if (status || !cs_change) {
   428			ndelay(nsecs);
   429			fsl_spi_chipselect(spi, BITBANG_CS_INACTIVE);
   430		}
   431	
   432		if (!status && spi->trailing_bits) {
   433			struct spi_transfer t = {
   434				.len = 1,
 > 435				.tx_buf = empty_zero_page,
   436			};
   437	
   438			if (spi->trailing_bits < 4)
   439				t.bits_per_word = 4;
   440			else if (spi->trailing_bits > 8)
   441				t.bits_per_word = 16;
   442			else
   443				t.bits_per_word = spi->trailing_bits;
   444	
   445			status = fsl_spi_setup_transfer(spi, &t);
   446			if (!status)
   447				status = fsl_spi_bufs(spi, &t, 0);
   448		}
   449		m->status = status;
   450	
   451		fsl_spi_setup_transfer(spi, NULL);
   452		spi_finalize_current_message(master);
   453		return 0;
   454	}
   455	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot Feb. 28, 2022, 6:38 p.m. UTC | #5 
Hi Christophe,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v5.17-rc6]
[cannot apply to broonie-spi/for-next next-20220228]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Christophe-Leroy/Add-support-for-components-requiring-trailing-clock-after-transfer/20220228-231740
base:    7e57714cd0ad2d5bb90e50b5096a0e671dec1ef3
config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20220301/202203010220.Yzk53fCj-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/7eb07c4d26401389204fcc6cf685c18c89b64ef8
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Christophe-Leroy/Add-support-for-components-requiring-trailing-clock-after-transfer/20220228-231740
        git checkout 7eb07c4d26401389204fcc6cf685c18c89b64ef8
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=mips SHELL=/bin/bash drivers/

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

All warnings (new ones prefixed by >>):

   drivers/spi/spi-fsl-spi.c: In function 'fsl_spi_do_one_msg':
>> drivers/spi/spi-fsl-spi.c:435:35: warning: initialization of 'const void *' from 'long unsigned int' makes pointer from integer without a cast [-Wint-conversion]
     435 |                         .tx_buf = empty_zero_page,
         |                                   ^~~~~~~~~~~~~~~
   drivers/spi/spi-fsl-spi.c:435:35: note: (near initialization for 't.tx_buf')


vim +435 drivers/spi/spi-fsl-spi.c

   356	
   357	static int fsl_spi_do_one_msg(struct spi_master *master,
   358				      struct spi_message *m)
   359	{
   360		struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(master);
   361		struct spi_device *spi = m->spi;
   362		struct spi_transfer *t, *first;
   363		unsigned int cs_change;
   364		const int nsecs = 50;
   365		int status, last_bpw;
   366	
   367		/*
   368		 * In CPU mode, optimize large byte transfers to use larger
   369		 * bits_per_word values to reduce number of interrupts taken.
   370		 */
   371		if (!(mpc8xxx_spi->flags & SPI_CPM_MODE)) {
   372			list_for_each_entry(t, &m->transfers, transfer_list) {
   373				if (t->len < 256 || t->bits_per_word != 8)
   374					continue;
   375				if ((t->len & 3) == 0)
   376					t->bits_per_word = 32;
   377				else if ((t->len & 1) == 0)
   378					t->bits_per_word = 16;
   379			}
   380		}
   381	
   382		/* Don't allow changes if CS is active */
   383		cs_change = 1;
   384		list_for_each_entry(t, &m->transfers, transfer_list) {
   385			if (cs_change)
   386				first = t;
   387			cs_change = t->cs_change;
   388			if (first->speed_hz != t->speed_hz) {
   389				dev_err(&spi->dev,
   390					"speed_hz cannot change while CS is active\n");
   391				return -EINVAL;
   392			}
   393		}
   394	
   395		last_bpw = -1;
   396		cs_change = 1;
   397		status = -EINVAL;
   398		list_for_each_entry(t, &m->transfers, transfer_list) {
   399			if (cs_change || last_bpw != t->bits_per_word)
   400				status = fsl_spi_setup_transfer(spi, t);
   401			if (status < 0)
   402				break;
   403			last_bpw = t->bits_per_word;
   404	
   405			if (cs_change) {
   406				fsl_spi_chipselect(spi, BITBANG_CS_ACTIVE);
   407				ndelay(nsecs);
   408			}
   409			cs_change = t->cs_change;
   410			if (t->len)
   411				status = fsl_spi_bufs(spi, t, m->is_dma_mapped);
   412			if (status) {
   413				status = -EMSGSIZE;
   414				break;
   415			}
   416			m->actual_length += t->len;
   417	
   418			spi_transfer_delay_exec(t);
   419	
   420			if (cs_change) {
   421				ndelay(nsecs);
   422				fsl_spi_chipselect(spi, BITBANG_CS_INACTIVE);
   423				ndelay(nsecs);
   424			}
   425		}
   426	
   427		if (status || !cs_change) {
   428			ndelay(nsecs);
   429			fsl_spi_chipselect(spi, BITBANG_CS_INACTIVE);
   430		}
   431	
   432		if (!status && spi->trailing_bits) {
   433			struct spi_transfer t = {
   434				.len = 1,
 > 435				.tx_buf = empty_zero_page,
   436			};
   437	
   438			if (spi->trailing_bits < 4)
   439				t.bits_per_word = 4;
   440			else if (spi->trailing_bits > 8)
   441				t.bits_per_word = 16;
   442			else
   443				t.bits_per_word = spi->trailing_bits;
   444	
   445			status = fsl_spi_setup_transfer(spi, &t);
   446			if (!status)
   447				status = fsl_spi_bufs(spi, &t, 0);
   448		}
   449		m->status = status;
   450	
   451		fsl_spi_setup_transfer(spi, NULL);
   452		spi_finalize_current_message(master);
   453		return 0;
   454	}
   455	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot Feb. 28, 2022, 6:48 p.m. UTC | #6 
Hi Christophe,

I love your patch! Yet something to improve:

[auto build test ERROR on v5.17-rc6]
[cannot apply to broonie-spi/for-next next-20220228]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Christophe-Leroy/Add-support-for-components-requiring-trailing-clock-after-transfer/20220228-231740
base:    7e57714cd0ad2d5bb90e50b5096a0e671dec1ef3
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20220301/202203010212.oQMKRiPl-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/7eb07c4d26401389204fcc6cf685c18c89b64ef8
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Christophe-Leroy/Add-support-for-components-requiring-trailing-clock-after-transfer/20220228-231740
        git checkout 7eb07c4d26401389204fcc6cf685c18c89b64ef8
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=alpha SHELL=/bin/bash

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

All errors (new ones prefixed by >>):

   drivers/spi/spi-fsl-spi.c: In function 'fsl_spi_do_one_msg':
>> drivers/spi/spi-fsl-spi.c:435:35: error: 'empty_zero_page' undeclared (first use in this function)
     435 |                         .tx_buf = empty_zero_page,
         |                                   ^~~~~~~~~~~~~~~
   drivers/spi/spi-fsl-spi.c:435:35: note: each undeclared identifier is reported only once for each function it appears in


vim +/empty_zero_page +435 drivers/spi/spi-fsl-spi.c

   356	
   357	static int fsl_spi_do_one_msg(struct spi_master *master,
   358				      struct spi_message *m)
   359	{
   360		struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(master);
   361		struct spi_device *spi = m->spi;
   362		struct spi_transfer *t, *first;
   363		unsigned int cs_change;
   364		const int nsecs = 50;
   365		int status, last_bpw;
   366	
   367		/*
   368		 * In CPU mode, optimize large byte transfers to use larger
   369		 * bits_per_word values to reduce number of interrupts taken.
   370		 */
   371		if (!(mpc8xxx_spi->flags & SPI_CPM_MODE)) {
   372			list_for_each_entry(t, &m->transfers, transfer_list) {
   373				if (t->len < 256 || t->bits_per_word != 8)
   374					continue;
   375				if ((t->len & 3) == 0)
   376					t->bits_per_word = 32;
   377				else if ((t->len & 1) == 0)
   378					t->bits_per_word = 16;
   379			}
   380		}
   381	
   382		/* Don't allow changes if CS is active */
   383		cs_change = 1;
   384		list_for_each_entry(t, &m->transfers, transfer_list) {
   385			if (cs_change)
   386				first = t;
   387			cs_change = t->cs_change;
   388			if (first->speed_hz != t->speed_hz) {
   389				dev_err(&spi->dev,
   390					"speed_hz cannot change while CS is active\n");
   391				return -EINVAL;
   392			}
   393		}
   394	
   395		last_bpw = -1;
   396		cs_change = 1;
   397		status = -EINVAL;
   398		list_for_each_entry(t, &m->transfers, transfer_list) {
   399			if (cs_change || last_bpw != t->bits_per_word)
   400				status = fsl_spi_setup_transfer(spi, t);
   401			if (status < 0)
   402				break;
   403			last_bpw = t->bits_per_word;
   404	
   405			if (cs_change) {
   406				fsl_spi_chipselect(spi, BITBANG_CS_ACTIVE);
   407				ndelay(nsecs);
   408			}
   409			cs_change = t->cs_change;
   410			if (t->len)
   411				status = fsl_spi_bufs(spi, t, m->is_dma_mapped);
   412			if (status) {
   413				status = -EMSGSIZE;
   414				break;
   415			}
   416			m->actual_length += t->len;
   417	
   418			spi_transfer_delay_exec(t);
   419	
   420			if (cs_change) {
   421				ndelay(nsecs);
   422				fsl_spi_chipselect(spi, BITBANG_CS_INACTIVE);
   423				ndelay(nsecs);
   424			}
   425		}
   426	
   427		if (status || !cs_change) {
   428			ndelay(nsecs);
   429			fsl_spi_chipselect(spi, BITBANG_CS_INACTIVE);
   430		}
   431	
   432		if (!status && spi->trailing_bits) {
   433			struct spi_transfer t = {
   434				.len = 1,
 > 435				.tx_buf = empty_zero_page,
   436			};
   437	
   438			if (spi->trailing_bits < 4)
   439				t.bits_per_word = 4;
   440			else if (spi->trailing_bits > 8)
   441				t.bits_per_word = 16;
   442			else
   443				t.bits_per_word = spi->trailing_bits;
   444	
   445			status = fsl_spi_setup_transfer(spi, &t);
   446			if (!status)
   447				status = fsl_spi_bufs(spi, &t, 0);
   448		}
   449		m->status = status;
   450	
   451		fsl_spi_setup_transfer(spi, NULL);
   452		spi_finalize_current_message(master);
   453		return 0;
   454	}
   455	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Christophe Leroy March 1, 2022, 12:53 p.m. UTC | #7 
Le 28/02/2022 à 17:14, Mark Brown a écrit :
> On Mon, Feb 28, 2022 at 04:02:30PM +0000, Christophe Leroy wrote:
>> Le 28/02/2022 à 16:29, Mark Brown a écrit :
> 
>>> The binding looks good now but this is still driver specific code when
>>> it looks like it could easily be implemented in the core - like I said
>>> on the previous version you'd need to update drivers to advertise less
>>> than 8 bits but there's basically nothing driver specific I can see here
>>> so any driver using transfer_one() would get support that way.
> 
>> Argh ! Sorry your comment to the previous version ended up in Junk
>> mails. I see it now.
> 
> No problem.
> 
>> We discussed that back in 2016 in
>> https://lore.kernel.org/linux-spi/20160824112701.GE22076@sirena.org.uk/
>> and my understanding at that time was that it was not something that
>> could be done at core level.
> 
>> But maybe things have changed since then ?
> 
> What I said then was "it would need a new core feature" which is what
> the binding does, I'm suggesting that you also do that for the handling
> of the implementation as well.
> 
> Actually now I think about it perhaps this shouldn't be a binding at all
> but rather something specified by the client driver - presumably any
> system using an affected device is going to need these extra clock
> cycles so they'll all need to add the same property.
> 
>> By the way, fsl-spi driver doesn't implement transfer_one() but
>> transfer_one_message() so it takes care of the chipselect changes and
>> therefore the final dummy transfer with CS off is to be done there as
>> far as I understand.
> 
>> Would it mean changing fsl-spi driver to implement transfer_one() first ?
> 
> Well, if it can implement transfer_one() without any negative
> consequences whichh

Seems like your sentence is truncated.

My understanding today is that this trailing transfer with chipselect 
OFF is to be added at the end of transfer_one_message().

It can be implemented in the core transfer_one_message() for drivers 
implementing transfer_one(). For the other drivers not having 
transfer_one() but having transfer_one_message(), it must be implemented 
in the driver's transfer_one_message().

Am I right ?


fsl-spi driver is the one I need to support this new functionnality and 
it has its own transfer_one_message().

What would you expect ?


Thanks
Christophe
Mark Brown March 1, 2022, 1:25 p.m. UTC | #8 
On Tue, Mar 01, 2022 at 12:53:58PM +0000, Christophe Leroy wrote:

> My understanding today is that this trailing transfer with chipselect 
> OFF is to be added at the end of transfer_one_message().

Yes.

> It can be implemented in the core transfer_one_message() for drivers 
> implementing transfer_one(). For the other drivers not having 
> transfer_one() but having transfer_one_message(), it must be implemented 
> in the driver's transfer_one_message().

> Am I right ?

Yes.

> fsl-spi driver is the one I need to support this new functionnality and 
> it has its own transfer_one_message().

> What would you expect ?

Well, if the fsl-spi driver has a good reason for open coding
transfer_message() it could continue to do it, however looking at the
driver it seems like it's only the little bit at the start to optimise
the trasfer width in CPU mode which looks like it could easily be moved
to a prepare_message() callback and save a bunch of code so it'd be good
to convert it.  I guess it shouldn't be strictly essential though.
Christophe Leroy Aug. 18, 2022, 6:35 p.m. UTC | #9 
Le 28/02/2022 à 17:14, Mark Brown a écrit :
> 
>> We discussed that back in 2016 in
>> https://lore.kernel.org/linux-spi/20160824112701.GE22076@sirena.org.uk/
>> and my understanding at that time was that it was not something that
>> could be done at core level.
> 
>> But maybe things have changed since then ?
> 
> What I said then was "it would need a new core feature" which is what
> the binding does, I'm suggesting that you also do that for the handling
> of the implementation as well.
> 
> Actually now I think about it perhaps this shouldn't be a binding at all
> but rather something specified by the client driver - presumably any
> system using an affected device is going to need these extra clock
> cycles so they'll all need to add the same property.

Yes indeed. Therefore in v3 I took a different approach : a flag .cs_off 
tells to spi_transfer_one_message() that a given transfer has to be 
performed with chipselect OFF, therefore the consumer has full control 
of how and when to add those additional fake clock cycles during a 
transfer, and can eventually add one at anyplace during the transfer.

Here an exemple of what will do the consumer.


static int idt821034_8bit_write(struct idt821034 *idt821034, u8 val)
{
	struct spi_transfer xfer[] = {{
		.tx_buf = &idt821034->spi_tx_buf,
		.len = 1,
	},{
		.tx_buf = &idt821034->spi_tx_buf,
		.len = 1,
		.cs_off = 1,
	}};
	int ret;

	idt821034->spi_tx_buf = val;

	ret = spi_sync_transfer(idt821034->spi, xfer, ARRAY_SIZE(xfer));
	if (ret)
		return ret;

	return 0;
}



Christophe
Mark Brown Aug. 22, 2022, 5:15 p.m. UTC | #10 
On Thu, Aug 18, 2022 at 06:35:39PM +0000, Christophe Leroy wrote:

> Yes indeed. Therefore in v3 I took a different approach : a flag .cs_off 
> tells to spi_transfer_one_message() that a given transfer has to be 
> performed with chipselect OFF, therefore the consumer has full control 
> of how and when to add those additional fake clock cycles during a 
> transfer, and can eventually add one at anyplace during the transfer.

> Here an exemple of what will do the consumer.

Hrm, we should already be able to synthesize that with cs_change though
there's usability challenges there and AFAICT it doesn't work for the
first transfer which your proposal would so there's a functional benefit
even if you don't need it for your device right now.  It would be good
if you could have a look at using cs_change for your use case.  Sorry, I
don't think I'd fully realised what you were looking to accomplish here
until I saw your proposal.
Christophe Leroy Aug. 22, 2022, 6:38 p.m. UTC | #11 
Le 22/08/2022 à 19:15, Mark Brown a écrit :
> On Thu, Aug 18, 2022 at 06:35:39PM +0000, Christophe Leroy wrote:
> 
>> Yes indeed. Therefore in v3 I took a different approach : a flag .cs_off
>> tells to spi_transfer_one_message() that a given transfer has to be
>> performed with chipselect OFF, therefore the consumer has full control
>> of how and when to add those additional fake clock cycles during a
>> transfer, and can eventually add one at anyplace during the transfer.
> 
>> Here an exemple of what will do the consumer.
> 
> Hrm, we should already be able to synthesize that with cs_change though
> there's usability challenges there and AFAICT it doesn't work for the
> first transfer which your proposal would so there's a functional benefit
> even if you don't need it for your device right now.  It would be good
> if you could have a look at using cs_change for your use case.  Sorry, I
> don't think I'd fully realised what you were looking to accomplish here
> until I saw your proposal.

I think we already addressed this possibility back in 2016, see 
https://lore.kernel.org/linux-spi/20160824111206.GD22076@sirena.org.uk/

The conclusion was that it was not possible to accomplish it with cs_change.

Or did we miss something at that time ?

My understanding is that if you set cs_change for any transfer of a 
message except the last, then CS goes OFF then ON again between the said 
transfer and the following one. If you set cs_change for the last 
transfer of a message, then CS stays ON until next message instead of 
going OFF as usual.

My need is to transfer a fake byte with CS off at the end of a message. 
I still can't see how cs_change can achieve that.

Thanks
Christophe
Mark Brown Aug. 22, 2022, 7:29 p.m. UTC | #12 
On Mon, Aug 22, 2022 at 06:38:22PM +0000, Christophe Leroy wrote:
> Le 22/08/2022 à 19:15, Mark Brown a écrit :

> I think we already addressed this possibility back in 2016, see 
> https://lore.kernel.org/linux-spi/20160824111206.GD22076@sirena.org.uk/

> The conclusion was that it was not possible to accomplish it with cs_change.

> Or did we miss something at that time ?

No, I think that's fine - your proposal has some overlap but is fine.
diff mbox series

Patch

diff --git a/drivers/spi/spi-fsl-spi.c b/drivers/spi/spi-fsl-spi.c
index bdf94cc7be1a..aefaea439672 100644
--- a/drivers/spi/spi-fsl-spi.c
+++ b/drivers/spi/spi-fsl-spi.c
@@ -424,13 +424,30 @@  static int fsl_spi_do_one_msg(struct spi_master *master,
 		}
 	}
 
-	m->status = status;
-
 	if (status || !cs_change) {
 		ndelay(nsecs);
 		fsl_spi_chipselect(spi, BITBANG_CS_INACTIVE);
 	}
 
+	if (!status && spi->trailing_bits) {
+		struct spi_transfer t = {
+			.len = 1,
+			.tx_buf = empty_zero_page,
+		};
+
+		if (spi->trailing_bits < 4)
+			t.bits_per_word = 4;
+		else if (spi->trailing_bits > 8)
+			t.bits_per_word = 16;
+		else
+			t.bits_per_word = spi->trailing_bits;
+
+		status = fsl_spi_setup_transfer(spi, &t);
+		if (!status)
+			status = fsl_spi_bufs(spi, &t, 0);
+	}
+	m->status = status;
+
 	fsl_spi_setup_transfer(spi, NULL);
 	spi_finalize_current_message(master);
 	return 0;