| Message ID | cover.1646060734.git.christophe.leroy@csgroup.eu |
|---|---|
| Headers | show |
| Series | Add support for components requiring trailing clock after transfer | expand |
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.
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
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
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
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.
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
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.