| Message ID | 20161211200350.13590-2-hauke@hauke-m.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Dec 11, 2016 at 09:03:49PM +0100, Hauke Mehrtens wrote: > This callback checks if the transfer really finished. This allows a > driver to directly call the completion list in the irq handler and it > does not have to bushy wait till the hardware is really finished in the > IRQ handler. This is needed for the Lantiq driver. This is really hard to understand and does not seem like a good interface, it's going to be confusing for anyone who doesn't have hardware which is broken in whatever way your hardware is broken and it sounds like it's misnamed for your hardware since my guess is that you actually want to wait for something in the function rather than just check to see if the transfer completed. Why not just have your interrupt handler schedule something on a workqueue to check whatever it is needs checking here?
On 12/14/2016 04:21 PM, Mark Brown wrote: > On Sun, Dec 11, 2016 at 09:03:49PM +0100, Hauke Mehrtens wrote: > >> This callback checks if the transfer really finished. This allows a >> driver to directly call the completion list in the irq handler and it >> does not have to bushy wait till the hardware is really finished in the >> IRQ handler. This is needed for the Lantiq driver. > > This is really hard to understand and does not seem like a good > interface, it's going to be confusing for anyone who doesn't have > hardware which is broken in whatever way your hardware is broken and it > sounds like it's misnamed for your hardware since my guess is that you > actually want to wait for something in the function rather than just > check to see if the transfer completed. > > Why not just have your interrupt handler schedule something on a > workqueue to check whatever it is needs checking here? Ok, I will try to find a different way for this problem. I tried to avoid this extra workqueue by usig the normal spi workqueue. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/14/2016 04:21 PM, Mark Brown wrote: > On Sun, Dec 11, 2016 at 09:03:49PM +0100, Hauke Mehrtens wrote: > >> This callback checks if the transfer really finished. This allows a >> driver to directly call the completion list in the irq handler and it >> does not have to bushy wait till the hardware is really finished in the >> IRQ handler. This is needed for the Lantiq driver. > > This is really hard to understand and does not seem like a good > interface, it's going to be confusing for anyone who doesn't have > hardware which is broken in whatever way your hardware is broken and it > sounds like it's misnamed for your hardware since my guess is that you > actually want to wait for something in the function rather than just > check to see if the transfer completed. > > Why not just have your interrupt handler schedule something on a > workqueue to check whatever it is needs checking here? I have two problems. 1. My SPI controller does not signal an IRQ when it is idle, it can only send an interrupt based on the filling of the rx and tx fifo or after every word (2 to 32 bits). The data from the fifo is written to a shift register in the hardware, so when the fifo is empty it is still working on the last word and should not be reconfigured. I could make it send an IRQ after every word, but that would be a lot of IRQs. To solve this problem I would convert it to threaded IRQs and directly do the busy waiting int the IRQ handler. 2. When something went wrong and I got an error IRQ for example, I would like to add this information to the spi message that was transfered, but the only way I see it to not wake the completion and let it run into a timeout. I would like to have a function in which I can do some busy waiting till the last word is really written to the wire and also return if the transfer was successful or what error occurred. With the check_finished callback I was albe to solve both problems. Should I rename it to get_transfer_result() ? Hauke -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Dec 17, 2016 at 11:16:17PM +0100, Hauke Mehrtens wrote: > On 12/14/2016 04:21 PM, Mark Brown wrote: > > Why not just have your interrupt handler schedule something on a > > workqueue to check whatever it is needs checking here? > 2. When something went wrong and I got an error IRQ for example, I would > like to add this information to the spi message that was transfered, but > the only way I see it to not wake the completion and let it run into a > timeout. Just set an error status in the message and then complete the current transfer or the message. > I would like to have a function in which I can do some busy waiting till > the last word is really written to the wire and also return if the > transfer was successful or what error occurred. The busy waiting really is pretty specialist.
On 01/10/2017 01:34 PM, Mark Brown wrote: > On Sat, Dec 17, 2016 at 11:16:17PM +0100, Hauke Mehrtens wrote: >> On 12/14/2016 04:21 PM, Mark Brown wrote: > >>> Why not just have your interrupt handler schedule something on a >>> workqueue to check whatever it is needs checking here? > >> 2. When something went wrong and I got an error IRQ for example, I would >> like to add this information to the spi message that was transfered, but >> the only way I see it to not wake the completion and let it run into a >> timeout. > > Just set an error status in the message and then complete the current > transfer or the message. > >> I would like to have a function in which I can do some busy waiting till >> the last word is really written to the wire and also return if the >> transfer was successful or what error occurred. > > The busy waiting really is pretty specialist. What should I do now? I can add the transfer status in some other way, no problem, but how do I tell Linux that the transfer finished when it really finished. I also tried to just ignore the last word, but when the controller is configured for the next transfer, the old one gets corrupted. There is a per transfered word IRQ, I can use that, but it is probably triggered very often and I do not know if it works in all situations and it has to get synced with the fifo IRQs which is probably complicated. I can also start my own driver thread to do the busy waiting and trigger the spi_finalize_current_transfer() later after busy waiting in the driver. I can also use transfer_one_message() I would prefer to add an additional busy waiting after the wait_for_completion_timeout() like I did in this patch, but how should this look like? Hauke -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
ping Mark, What should I do in the driver now? Hauke On 01/10/2017 11:44 PM, Hauke Mehrtens wrote: > On 01/10/2017 01:34 PM, Mark Brown wrote: >> On Sat, Dec 17, 2016 at 11:16:17PM +0100, Hauke Mehrtens wrote: >>> On 12/14/2016 04:21 PM, Mark Brown wrote: >> >>>> Why not just have your interrupt handler schedule something on a >>>> workqueue to check whatever it is needs checking here? >> >>> 2. When something went wrong and I got an error IRQ for example, I would >>> like to add this information to the spi message that was transfered, but >>> the only way I see it to not wake the completion and let it run into a >>> timeout. >> >> Just set an error status in the message and then complete the current >> transfer or the message. >> >>> I would like to have a function in which I can do some busy waiting till >>> the last word is really written to the wire and also return if the >>> transfer was successful or what error occurred. >> >> The busy waiting really is pretty specialist. > > What should I do now? > > I can add the transfer status in some other way, no problem, but how do > I tell Linux that the transfer finished when it really finished. I also > tried to just ignore the last word, but when the controller is > configured for the next transfer, the old one gets corrupted. > > There is a per transfered word IRQ, I can use that, but it is probably > triggered very often and I do not know if it works in all situations and > it has to get synced with the fifo IRQs which is probably complicated. > > I can also start my own driver thread to do the busy waiting and trigger > the spi_finalize_current_transfer() later after busy waiting in the driver. > > I can also use transfer_one_message() > > I would prefer to add an additional busy waiting after the > wait_for_completion_timeout() like I did in this patch, but how should > this look like? > > Hauke > -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Feb 04, 2017 at 02:45:57PM +0100, Hauke Mehrtens wrote: > ping Mark, > > What should I do in the driver now? Please don't top post, reply in line with needed context. This allows readers to readily follow the flow of conversation and understand what you are talking about and also helps ensure that everything in the discussion is being addressed. Please don't send content free pings and please allow a reasonable time for review. People get busy, go on holiday, attend conferences and so on so unless there is some reason for urgency (like critical bug fixes) please allow at least a couple of weeks for review. If there have been review comments then people may be waiting for those to be addressed. Sending content free pings just adds to the mail volume (if they are seen at all) and if something has gone wrong you'll have to resend the patches anyway. > > I can add the transfer status in some other way, no problem, but how do > > I tell Linux that the transfer finished when it really finished. I also > > tried to just ignore the last word, but when the controller is > > configured for the next transfer, the old one gets corrupted. As I've now said three or four times this sounds like you should use a workqueue or something similar to do whatever is needed wait for the hardware to finish whatever it's doing.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 838783c..8702cdf 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1013,6 +1013,16 @@ static int spi_transfer_one_message(struct spi_master *master, msecs_to_jiffies(ms)); } + if (master->check_finished) { + ret = master->check_finished(master, ms); + if (ret) { + dev_err(&msg->spi->dev, + "SPI transfer not finished: %i\n", + ret); + msg->status = ret; + } + } + if (ms == 0) { SPI_STATISTICS_INCREMENT_FIELD(statm, timedout); diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 4b743ac..2b851a6 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -370,6 +370,9 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv) * transfer_one_message are mutually exclusive; when both * are set, the generic subsystem does not call your * transfer_one callback. + * @check_finished: This callback allows the driver to check if the message + * was fully transferred. return a negative value in case + * of an error. * @handle_err: the subsystem calls the driver to handle an error that occurs * in the generic implementation of transfer_one_message(). * @unprepare_message: undo any work done by prepare_message(). @@ -546,6 +549,7 @@ struct spi_master { void (*set_cs)(struct spi_device *spi, bool enable); int (*transfer_one)(struct spi_master *master, struct spi_device *spi, struct spi_transfer *transfer); + int (*check_finished)(struct spi_master *master, unsigned long timeout); void (*handle_err)(struct spi_master *master, struct spi_message *message);
This callback checks if the transfer really finished. This allows a driver to directly call the completion list in the irq handler and it does not have to bushy wait till the hardware is really finished in the IRQ handler. This is needed for the Lantiq driver. Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> --- drivers/spi/spi.c | 10 ++++++++++ include/linux/spi/spi.h | 4 ++++ 2 files changed, 14 insertions(+)