| Message ID | 20190912144310.7458-1-andrea.merello@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fixes for ad7949 | expand |
On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote: > [External] > > This patch series fixes ad7949 driver incorrectly read data, simplify the > code, and enforces device timing constraints. > > This has been tested on a UltraZed SOM + a custom carrier equipped with > several AD7689 A/Ds. Patches have been developed on a Xilinx upstream > kernel and then rebased on linux-next kernel. > Hey, Thanks for the patches. Added Charles-Antoine to also take a look. Apologies for not thinking of adding him sooner. I typically try to review changes for ADI parts, but he wrote it, so he may have more input than I do. Jonathan will likely also take a look. If it's agreed, I would say to at least take the first patch ("iio: ad7949: kill pointless "readback"-handling code") now and see about the rest. The rest are a bit more open to discussion, so a v2 may happen. Thanks Alex > Andrea Merello (4): > iio: ad7949: kill pointless "readback"-handling code > iio: ad7949: fix incorrect SPI xfer len > iio: ad7949: fix SPI xfer delays > iio: ad7949: fix channels mixups > > drivers/iio/adc/ad7949.c | 64 +++++++++++++--------------------------- > 1 file changed, 21 insertions(+), 43 deletions(-) > > -- > 2.17.1
Le 13/09/2019 à 09:24, Ardelean, Alexandru a écrit : > On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote: >> [External] >> >> This patch series fixes ad7949 driver incorrectly read data, simplify the >> code, and enforces device timing constraints. >> >> This has been tested on a UltraZed SOM + a custom carrier equipped with >> several AD7689 A/Ds. Patches have been developed on a Xilinx upstream >> kernel and then rebased on linux-next kernel. >> > Thanks for the patches. > Added Charles-Antoine to also take a look. > Apologies for not thinking of adding him sooner. > > I typically try to review changes for ADI parts, but he wrote it, so he may have more input than I do. > Jonathan will likely also take a look. > > If it's agreed, I would say to at least take the first patch ("iio: ad7949: kill pointless "readback"-handling code") > now and see about the rest. > The rest are a bit more open to discussion, so a v2 may happen. Hi, Don't worry. Due to the fact I don't have on my mail client access to the whole discussions, I'm making a complete answer there based on the archive of the mailing list. Sorry for that. For the patch 1, I approve it too. This part of code is useless because the feature was removed. RIP my code. :D For the patch 2, the cache information was added due to comment from Jonathan Cameron when I developed the driver. The comment was: > Look very carefully at the requirements for a buffer being passed > to spi_sync. It needs to be DMA safe. This one is not. The usual > way to do that easily is to put a cacheline aligned buffer in your > ad7949_adc_chip structure. > > Lots of examples to copy, but it's also worth making sure you understand > why this is necessary. For the endianess thing, it shouldn't be required to make an explicit conversion into the driver. According to the spi.h documentation: > * In-memory data values are always in native CPU byte order, translated > * from the wire byte order (big-endian except with SPI_LSB_FIRST). So > * for example when bits_per_word is sixteen, buffers are 2N bytes long > * (@len = 2N) and hold N sixteen bit words in CPU byte order. So from my point of view the SPI subsystem always converts to the right endianess. We don't have to take care about it. For patch 3, I didn't use delay_usecs fiels due to the timings definition in the datasheet in "READ/WRITE SPANNING CONVERSION WITHOUT A BUSY INDICATOR" mode. During the delay, the chip select line must be released which is not the case when we use delay_usecs field. So I add the delay instruction after the write step to be compliant with these timings. For patch 4, I explained a bit in the other thread. Maybe we have a difference of behaviour due to the choice of the timings "modes"? BTW, from my point of view the datasheet is not totally clear about the timings and what is mandatory or not in the expected behaviour. Regards, Charles-Antoine Couret
On Fri, 13 Sep 2019 16:00:29 +0200 Couret Charles-Antoine <charles-antoine.couret@essensium.com> wrote: > Le 13/09/2019 à 09:24, Ardelean, Alexandru a écrit : > > On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote: > >> [External] > >> > >> This patch series fixes ad7949 driver incorrectly read data, simplify the > >> code, and enforces device timing constraints. > >> > >> This has been tested on a UltraZed SOM + a custom carrier equipped with > >> several AD7689 A/Ds. Patches have been developed on a Xilinx upstream > >> kernel and then rebased on linux-next kernel. > >> > > Thanks for the patches. > > Added Charles-Antoine to also take a look. > > Apologies for not thinking of adding him sooner. > > > > I typically try to review changes for ADI parts, but he wrote it, so he may have more input than I do. > > Jonathan will likely also take a look. > > > > If it's agreed, I would say to at least take the first patch ("iio: ad7949: kill pointless "readback"-handling code") > > now and see about the rest. > > The rest are a bit more open to discussion, so a v2 may happen. > > Hi, > > Don't worry. Due to the fact I don't have on my mail client access to > the whole discussions, I'm making a complete answer there based on the > archive of the mailing list. Sorry for that. > > > For the patch 1, I approve it too. This part of code is useless because > the feature was removed. RIP my code. :D > > For the patch 2, the cache information was added due to comment from > Jonathan Cameron when I developed the driver. The comment was: > > > Look very carefully at the requirements for a buffer being passed > > to spi_sync. It needs to be DMA safe. This one is not. The usual > > way to do that easily is to put a cacheline aligned buffer in your > > ad7949_adc_chip structure. The short version of this is best illustrated with an example. This only applies systems where the DMA engines are not coherent (i.e. a change made by a DMA engine is not automatically updated to all other places a copy is held in caches in the system, we have to do it by hand). We have a structure like struct bob { int initial_data; u8 buffer[8]; int magic_flags }; When a DMA transfer is setup involving 'buffer', the DMA engine may take up to a cacheline (typically 64 bytes) including buffer, make a copy of it and assume that the only bit of hardware working in this cacheline is itself. (Linux is 'guaranteeing' this when it tells the DMA engine to use this buffer.'. Whilst that DMA is going on, a CPU can write something in magic flags. That something might be important but unrelated to the DMA transfer going on. The DMA finishes, having put new data in the buffer element of the copy of the cacheline local to . It's guaranteed to not change it's copy of the cacheline (in this case containing the whole of bob). However, it's version of magic_flags is out of date so when we flush the caches at the end of the non coherent DMA transfer (to force the CPU to read it from main memory and get the new values in buffer), the value of magic_flags can be reset to the version the DMA engine has. So, upshot is to avoid any potential of such problems, DMA buffers 'must' always be in a cacheline containing nothing that might be changed by other activities. This can mean it is safe to put both TX and RX buffers in the same cacheline as we won't touch either during an SPI transfer. > > > > Lots of examples to copy, but it's also worth making sure you understand > > why this is necessary. > > For the endianess thing, it shouldn't be required to make an explicit > conversion into the driver. According to the spi.h documentation: > > > * In-memory data values are always in native CPU byte order, translated > > * from the wire byte order (big-endian except with SPI_LSB_FIRST). So > > * for example when bits_per_word is sixteen, buffers are 2N bytes long > > * (@len = 2N) and hold N sixteen bit words in CPU byte order. > So from my point of view the SPI subsystem always converts to the right > endianess. We don't have to take care about it. Correct, though as I commented on that patch, that's not always 'possible' and not all drivers set the word length 'correctly'. Wolfram's presentation on trying to implement DMA safety in I2C at ELCE2018 also touches on a lot of this. Thanks, Jonathan > > > For patch 3, I didn't use delay_usecs fiels due to the timings > definition in the datasheet in "READ/WRITE SPANNING CONVERSION WITHOUT A > BUSY INDICATOR" mode. During the delay, the chip select line must be > released which is not the case when we use delay_usecs field. So I add > the delay instruction after the write step to be compliant with these > timings. > > > For patch 4, I explained a bit in the other thread. Maybe we have a > difference of behaviour due to the choice of the timings "modes"? > > > BTW, from my point of view the datasheet is not totally clear about the > timings and what is mandatory or not in the expected behaviour. > > Regards, > > Charles-Antoine Couret >
Il giorno ven 13 set 2019 alle ore 16:00 Couret Charles-Antoine <charles-antoine.couret@essensium.com> ha scritto: > > For patch 3, I didn't use delay_usecs fiels due to the timings > definition in the datasheet in "READ/WRITE SPANNING CONVERSION WITHOUT A > BUSY INDICATOR" mode. During the delay, the chip select line must be > released which is not the case when we use delay_usecs field. So I add > the delay instruction after the write step to be compliant with these > timings. Ok, you are right, thank you for pointing this out.. It looks like that, for my original intent of being strict about acquisition time and conversion time (as per my original interpretation), both delays (before and after) CS change would be needed.. But since Alexandru pointed out that the single 2uS delay, as per current implementation, should be OK for the A/D chip, then I'd drop patch 3/4. > For patch 4, I explained a bit in the other thread. Maybe we have a > difference of behaviour due to the choice of the timings "modes"? I don't know.. is it "RAC" the intended working mode? If you want I can take some snapshot of scope images and share them.. > > BTW, from my point of view the datasheet is not totally clear about the > timings and what is mandatory or not in the expected behaviour. I agree with you that datasheet is not so clear about timings.. However I think it makes it pretty clear that we need the extra transfer in order to make sure that the read data is correct (except if the CFG doesn't change). IMO that's could be evinced by both diagrams (all modes I think) and by the following sentence in "READING/WRITING AFTER CONVERSION, ANY SPEED HOSTS" paragraph: "When reading/writing after conversion, or during acquisition(n), conversion results are for the previous (n − 1) conversion, and writing is for the (n + 1) acquisition". I interpret this as follows: - You write a new CFG in cycle n, you get (and discard) data for cycle n-1, and the new CFG will be used for next _acquisition_. - Once you release the CS (you finished to write) the conversion for the _currently_ acquired data (old cfg) starts. Once the A/D finishes the conversion, the acquisition starts with the new CFG - You read data, and the A/D returns results from the last _conversion_ that has been done, which is still related to the old configuration. Once you release the CS (finish to read) the conversion for the currently acquired data - which is now the one with the new CFG - is started. - Finally you read the result of the above conversion So IMO we really need three cycles to read from a random channel > Regards, > > Charles-Antoine Couret >
Il giorno dom 15 set 2019 alle ore 12:49 Jonathan Cameron <jic23@kernel.org> ha scritto: > > On Fri, 13 Sep 2019 16:00:29 +0200 > Couret Charles-Antoine <charles-antoine.couret@essensium.com> wrote: > > > Le 13/09/2019 à 09:24, Ardelean, Alexandru a écrit : > > > On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote: > > >> [External] > > >> > > >> This patch series fixes ad7949 driver incorrectly read data, simplify the > > >> code, and enforces device timing constraints. > > >> > > >> This has been tested on a UltraZed SOM + a custom carrier equipped with > > >> several AD7689 A/Ds. Patches have been developed on a Xilinx upstream > > >> kernel and then rebased on linux-next kernel. > > >> > > > Thanks for the patches. > > > Added Charles-Antoine to also take a look. > > > Apologies for not thinking of adding him sooner. > > > > > > I typically try to review changes for ADI parts, but he wrote it, so he may have more input than I do. > > > Jonathan will likely also take a look. > > > > > > If it's agreed, I would say to at least take the first patch ("iio: ad7949: kill pointless "readback"-handling code") > > > now and see about the rest. > > > The rest are a bit more open to discussion, so a v2 may happen. > > > > Hi, > > > > Don't worry. Due to the fact I don't have on my mail client access to > > the whole discussions, I'm making a complete answer there based on the > > archive of the mailing list. Sorry for that. > > > > > > For the patch 1, I approve it too. This part of code is useless because > > the feature was removed. RIP my code. :D > > > > For the patch 2, the cache information was added due to comment from > > Jonathan Cameron when I developed the driver. The comment was: > > > > > Look very carefully at the requirements for a buffer being passed > > > to spi_sync. It needs to be DMA safe. This one is not. The usual > > > way to do that easily is to put a cacheline aligned buffer in your > > > ad7949_adc_chip structure. > > The short version of this is best illustrated with an example. > This only applies systems where the DMA engines are not coherent > (i.e. a change made by a DMA engine is not automatically updated to > all other places a copy is held in caches in the system, we have to > do it by hand). > > We have a structure like > struct bob { > int initial_data; > u8 buffer[8]; > int magic_flags > }; > > When a DMA transfer is setup involving 'buffer', the DMA engine may take > up to a cacheline (typically 64 bytes) including buffer, make a copy of it > and assume that the only bit of hardware working in this cacheline is itself. > (Linux is 'guaranteeing' this when it tells the DMA engine to use this buffer.'. > Whilst that DMA is going on, a CPU can write something in magic flags. > That something might be important but unrelated to the DMA transfer going > on. > > The DMA finishes, having put new data in the buffer element of the copy > of the cacheline local to . It's guaranteed to not change it's copy of the > cacheline (in this case containing the whole of bob). However, it's version > of magic_flags is out of date so when we flush the caches at the end of the > non coherent DMA transfer (to force the CPU to read it from main memory and > get the new values in buffer), the value of magic_flags can be reset to the > version the DMA engine has. > > So, upshot is to avoid any potential of such problems, DMA buffers 'must' > always be in a cacheline containing nothing that might be changed by > other activities. This can mean it is safe to put both TX and RX buffers > in the same cacheline as we won't touch either during an SPI transfer. > > > > > > > Lots of examples to copy, but it's also worth making sure you understand > > > why this is necessary. > > > > For the endianess thing, it shouldn't be required to make an explicit > > conversion into the driver. According to the spi.h documentation: > > > > > * In-memory data values are always in native CPU byte order, translated > > > * from the wire byte order (big-endian except with SPI_LSB_FIRST). So > > > * for example when bits_per_word is sixteen, buffers are 2N bytes long > > > * (@len = 2N) and hold N sixteen bit words in CPU byte order. > > So from my point of view the SPI subsystem always converts to the right > > endianess. We don't have to take care about it. > > Correct, though as I commented on that patch, that's not always 'possible' > and not all drivers set the word length 'correctly'. Thank you both for the explanations about DMA and SPI endianess :) So indeed 2/4 seems OK to me, and it doesn't need any further endianess-related fix. > Wolfram's presentation on trying to implement DMA safety in I2C at ELCE2018 > also touches on a lot of this. > > Thanks, > > Jonathan > > > > > > > For patch 3, I didn't use delay_usecs fiels due to the timings > > definition in the datasheet in "READ/WRITE SPANNING CONVERSION WITHOUT A > > BUSY INDICATOR" mode. During the delay, the chip select line must be > > released which is not the case when we use delay_usecs field. So I add > > the delay instruction after the write step to be compliant with these > > timings. > > > > > > For patch 4, I explained a bit in the other thread. Maybe we have a > > difference of behaviour due to the choice of the timings "modes"? > > > > > > BTW, from my point of view the datasheet is not totally clear about the > > timings and what is mandatory or not in the expected behaviour. > > > > Regards, > > > > Charles-Antoine Couret > > >
On Mon, 2019-09-16 at 09:39 +0200, Andrea Merello wrote: > Il giorno dom 15 set 2019 alle ore 12:49 Jonathan Cameron > <jic23@kernel.org> ha scritto: > > On Fri, 13 Sep 2019 16:00:29 +0200 > > Couret Charles-Antoine <charles-antoine.couret@essensium.com> wrote: > > > > > Le 13/09/2019 à 09:24, Ardelean, Alexandru a écrit : > > > > On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote: > > > > > [External] > > > > > > > > > > This patch series fixes ad7949 driver incorrectly read data, simplify the > > > > > code, and enforces device timing constraints. > > > > > > > > > > This has been tested on a UltraZed SOM + a custom carrier equipped with > > > > > several AD7689 A/Ds. Patches have been developed on a Xilinx upstream > > > > > kernel and then rebased on linux-next kernel. > > > > > > > > > Thanks for the patches. > > > > Added Charles-Antoine to also take a look. > > > > Apologies for not thinking of adding him sooner. > > > > > > > > I typically try to review changes for ADI parts, but he wrote it, so he may have more input than I do. > > > > Jonathan will likely also take a look. > > > > > > > > If it's agreed, I would say to at least take the first patch ("iio: ad7949: kill pointless "readback"-handling > > > > code") > > > > now and see about the rest. > > > > The rest are a bit more open to discussion, so a v2 may happen. > > > > > > Hi, > > > > > > Don't worry. Due to the fact I don't have on my mail client access to > > > the whole discussions, I'm making a complete answer there based on the > > > archive of the mailing list. Sorry for that. > > > > > > > > > For the patch 1, I approve it too. This part of code is useless because > > > the feature was removed. RIP my code. :D > > > > > > For the patch 2, the cache information was added due to comment from > > > Jonathan Cameron when I developed the driver. The comment was: > > > > > > > Look very carefully at the requirements for a buffer being passed > > > > to spi_sync. It needs to be DMA safe. This one is not. The usual > > > > way to do that easily is to put a cacheline aligned buffer in your > > > > ad7949_adc_chip structure. > > > > The short version of this is best illustrated with an example. > > This only applies systems where the DMA engines are not coherent > > (i.e. a change made by a DMA engine is not automatically updated to > > all other places a copy is held in caches in the system, we have to > > do it by hand). > > > > We have a structure like > > struct bob { > > int initial_data; > > u8 buffer[8]; > > int magic_flags > > }; > > > > When a DMA transfer is setup involving 'buffer', the DMA engine may take > > up to a cacheline (typically 64 bytes) including buffer, make a copy of it > > and assume that the only bit of hardware working in this cacheline is itself. > > (Linux is 'guaranteeing' this when it tells the DMA engine to use this buffer.'. > > Whilst that DMA is going on, a CPU can write something in magic flags. > > That something might be important but unrelated to the DMA transfer going > > on. > > > > The DMA finishes, having put new data in the buffer element of the copy > > of the cacheline local to . It's guaranteed to not change it's copy of the > > cacheline (in this case containing the whole of bob). However, it's version > > of magic_flags is out of date so when we flush the caches at the end of the > > non coherent DMA transfer (to force the CPU to read it from main memory and > > get the new values in buffer), the value of magic_flags can be reset to the > > version the DMA engine has. > > > > So, upshot is to avoid any potential of such problems, DMA buffers 'must' > > always be in a cacheline containing nothing that might be changed by > > other activities. This can mean it is safe to put both TX and RX buffers > > in the same cacheline as we won't touch either during an SPI transfer. > > > > > > Lots of examples to copy, but it's also worth making sure you understand > > > > why this is necessary. > > > > > > For the endianess thing, it shouldn't be required to make an explicit > > > conversion into the driver. According to the spi.h documentation: > > > > > > > * In-memory data values are always in native CPU byte order, translated > > > > * from the wire byte order (big-endian except with SPI_LSB_FIRST). So > > > > * for example when bits_per_word is sixteen, buffers are 2N bytes long > > > > * (@len = 2N) and hold N sixteen bit words in CPU byte order. > > > So from my point of view the SPI subsystem always converts to the right > > > endianess. We don't have to take care about it. > > > > Correct, though as I commented on that patch, that's not always 'possible' > > and not all drivers set the word length 'correctly'. > > Thank you both for the explanations about DMA and SPI endianess :) > > So indeed 2/4 seems OK to me, and it doesn't need any further > endianess-related fix. Yep. With these explanations: Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > > Wolfram's presentation on trying to implement DMA safety in I2C at ELCE2018 > > also touches on a lot of this. > > > > Thanks, > > > > Jonathan > > > > > > > > For patch 3, I didn't use delay_usecs fiels due to the timings > > > definition in the datasheet in "READ/WRITE SPANNING CONVERSION WITHOUT A > > > BUSY INDICATOR" mode. During the delay, the chip select line must be > > > released which is not the case when we use delay_usecs field. So I add > > > the delay instruction after the write step to be compliant with these > > > timings. > > > > > > > > > For patch 4, I explained a bit in the other thread. Maybe we have a > > > difference of behaviour due to the choice of the timings "modes"? > > > > > > > > > BTW, from my point of view the datasheet is not totally clear about the > > > timings and what is mandatory or not in the expected behaviour. > > > > > > Regards, > > > > > > Charles-Antoine Couret > > >
On Mon, 2019-09-16 at 07:48 +0000, Ardelean, Alexandru wrote: > [External] > > On Mon, 2019-09-16 at 09:39 +0200, Andrea Merello wrote: > > Il giorno dom 15 set 2019 alle ore 12:49 Jonathan Cameron > > <jic23@kernel.org> ha scritto: > > > On Fri, 13 Sep 2019 16:00:29 +0200 > > > Couret Charles-Antoine <charles-antoine.couret@essensium.com> wrote: > > > > > > > Le 13/09/2019 à 09:24, Ardelean, Alexandru a écrit : > > > > > On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote: > > > > > > [External] > > > > > > > > > > > > This patch series fixes ad7949 driver incorrectly read data, simplify the > > > > > > code, and enforces device timing constraints. > > > > > > > > > > > > This has been tested on a UltraZed SOM + a custom carrier equipped with > > > > > > several AD7689 A/Ds. Patches have been developed on a Xilinx upstream > > > > > > kernel and then rebased on linux-next kernel. > > > > > > > > > > > Thanks for the patches. > > > > > Added Charles-Antoine to also take a look. > > > > > Apologies for not thinking of adding him sooner. > > > > > > > > > > I typically try to review changes for ADI parts, but he wrote it, so he may have more input than I do. > > > > > Jonathan will likely also take a look. > > > > > > > > > > If it's agreed, I would say to at least take the first patch ("iio: ad7949: kill pointless "readback"-handling > > > > > code") > > > > > now and see about the rest. > > > > > The rest are a bit more open to discussion, so a v2 may happen. > > > > > > > > Hi, > > > > > > > > Don't worry. Due to the fact I don't have on my mail client access to > > > > the whole discussions, I'm making a complete answer there based on the > > > > archive of the mailing list. Sorry for that. > > > > > > > > > > > > For the patch 1, I approve it too. This part of code is useless because > > > > the feature was removed. RIP my code. :D > > > > > > > > For the patch 2, the cache information was added due to comment from > > > > Jonathan Cameron when I developed the driver. The comment was: > > > > > > > > > Look very carefully at the requirements for a buffer being passed > > > > > to spi_sync. It needs to be DMA safe. This one is not. The usual > > > > > way to do that easily is to put a cacheline aligned buffer in your > > > > > ad7949_adc_chip structure. > > > > > > The short version of this is best illustrated with an example. > > > This only applies systems where the DMA engines are not coherent > > > (i.e. a change made by a DMA engine is not automatically updated to > > > all other places a copy is held in caches in the system, we have to > > > do it by hand). > > > > > > We have a structure like > > > struct bob { > > > int initial_data; > > > u8 buffer[8]; > > > int magic_flags > > > }; > > > > > > When a DMA transfer is setup involving 'buffer', the DMA engine may take > > > up to a cacheline (typically 64 bytes) including buffer, make a copy of it > > > and assume that the only bit of hardware working in this cacheline is itself. > > > (Linux is 'guaranteeing' this when it tells the DMA engine to use this buffer.'. > > > Whilst that DMA is going on, a CPU can write something in magic flags. > > > That something might be important but unrelated to the DMA transfer going > > > on. > > > > > > The DMA finishes, having put new data in the buffer element of the copy > > > of the cacheline local to . It's guaranteed to not change it's copy of the > > > cacheline (in this case containing the whole of bob). However, it's version > > > of magic_flags is out of date so when we flush the caches at the end of the > > > non coherent DMA transfer (to force the CPU to read it from main memory and > > > get the new values in buffer), the value of magic_flags can be reset to the > > > version the DMA engine has. > > > > > > So, upshot is to avoid any potential of such problems, DMA buffers 'must' > > > always be in a cacheline containing nothing that might be changed by > > > other activities. This can mean it is safe to put both TX and RX buffers > > > in the same cacheline as we won't touch either during an SPI transfer. > > > > > > > > Lots of examples to copy, but it's also worth making sure you understand > > > > > why this is necessary. > > > > > > > > For the endianess thing, it shouldn't be required to make an explicit > > > > conversion into the driver. According to the spi.h documentation: > > > > > > > > > * In-memory data values are always in native CPU byte order, translated > > > > > * from the wire byte order (big-endian except with SPI_LSB_FIRST). So > > > > > * for example when bits_per_word is sixteen, buffers are 2N bytes long > > > > > * (@len = 2N) and hold N sixteen bit words in CPU byte order. > > > > So from my point of view the SPI subsystem always converts to the right > > > > endianess. We don't have to take care about it. > > > > > > Correct, though as I commented on that patch, that's not always 'possible' > > > and not all drivers set the word length 'correctly'. > > > > Thank you both for the explanations about DMA and SPI endianess :) > > > > So indeed 2/4 seems OK to me, and it doesn't need any further > > endianess-related fix. > > Yep. > With these explanations: > > Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > this is for patch 2/4 > > > > > Wolfram's presentation on trying to implement DMA safety in I2C at ELCE2018 > > > also touches on a lot of this. > > > > > > Thanks, > > > > > > Jonathan > > > > > > > For patch 3, I didn't use delay_usecs fiels due to the timings > > > > definition in the datasheet in "READ/WRITE SPANNING CONVERSION WITHOUT A > > > > BUSY INDICATOR" mode. During the delay, the chip select line must be > > > > released which is not the case when we use delay_usecs field. So I add > > > > the delay instruction after the write step to be compliant with these > > > > timings. > > > > > > > > > > > > For patch 4, I explained a bit in the other thread. Maybe we have a > > > > difference of behaviour due to the choice of the timings "modes"? > > > > > > > > > > > > BTW, from my point of view the datasheet is not totally clear about the > > > > timings and what is mandatory or not in the expected behaviour. > > > > > > > > Regards, > > > > > > > > Charles-Antoine Couret > > > >