From: Mark Brown <broonie@kernel.org> To: Martin Sperl <kernel@martin.sperl.org> Cc: Atsushi Nemoto <anemo@mba.ocn.ne.jp>, iivanov@mm-sol.com, gsi@denx.de, linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] spi: core: make zero length transfer valid again Date: Mon, 3 Mar 2014 10:05:26 +0900 [thread overview] Message-ID: <20140303010526.GV29849@sirena.org.uk> (raw) In-Reply-To: <4D0588EB-C20C-4B24-B656-6175C2309352@martin.sperl.org> [-- Attachment #1: Type: text/plain, Size: 1827 bytes --] On Sat, Mar 01, 2014 at 12:40:39PM +0100, Martin Sperl wrote: > I am not sure if it might make some bus-drivers more complicated > /inefficient just to support this zero length. For most of them it should be relatively straightforward, especially as we factor things out into the core so that the drivers don't need to implement the delays at all and it's all handled in the core. > For example: the spi-bcm2835.c driver would do the following with a > spi_transfer.len == 0 in the transfer_on method: > * enables SPI and wait for interrupt completion > * the above which will trigger an interrupt > ** in the interrupt we find out that there is nothing to transfer, > so we signal completion to transfer_one, so it may continue. > * the main transfer_one will get woken up > ** it will do a delay_usecs > ** it will handle CS_CHANGE > ** it will disable SPI/reset HW again > So this implementation shows that there is a lot of inefficient > overhead/delay just to trigger a delay... You really ought to be deferring to task context to implement the delays anyway - delaying in interrupt context is rude (though doable for very short delays). I'd have expected that the enable/disable to be bypassable, unless the hardware needs to be reset between transfers this should only be happening when the device goes idle. > I guess other drivers will show similar code-artefacts and > some may even make the implicit assumption it has to be non-zero, > which would break functionality those odd devices. There's lots of stuff that's broken with individual drivers - anything that relies on cs_change is going to break with half the drivers out there. Some of this is legitimate hardware limitations (where the /CS control is out of the control of software) but a lot of it is just bugs due to people open coding things. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> To: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> Cc: Atsushi Nemoto <anemo-7JcRY8pycbNHfZP73Gtkiw@public.gmane.org>, iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org, gsi-ynQEQJNshbs@public.gmane.org, linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Subject: Re: [PATCH] spi: core: make zero length transfer valid again Date: Mon, 3 Mar 2014 10:05:26 +0900 [thread overview] Message-ID: <20140303010526.GV29849@sirena.org.uk> (raw) In-Reply-To: <4D0588EB-C20C-4B24-B656-6175C2309352-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> [-- Attachment #1: Type: text/plain, Size: 1827 bytes --] On Sat, Mar 01, 2014 at 12:40:39PM +0100, Martin Sperl wrote: > I am not sure if it might make some bus-drivers more complicated > /inefficient just to support this zero length. For most of them it should be relatively straightforward, especially as we factor things out into the core so that the drivers don't need to implement the delays at all and it's all handled in the core. > For example: the spi-bcm2835.c driver would do the following with a > spi_transfer.len == 0 in the transfer_on method: > * enables SPI and wait for interrupt completion > * the above which will trigger an interrupt > ** in the interrupt we find out that there is nothing to transfer, > so we signal completion to transfer_one, so it may continue. > * the main transfer_one will get woken up > ** it will do a delay_usecs > ** it will handle CS_CHANGE > ** it will disable SPI/reset HW again > So this implementation shows that there is a lot of inefficient > overhead/delay just to trigger a delay... You really ought to be deferring to task context to implement the delays anyway - delaying in interrupt context is rude (though doable for very short delays). I'd have expected that the enable/disable to be bypassable, unless the hardware needs to be reset between transfers this should only be happening when the device goes idle. > I guess other drivers will show similar code-artefacts and > some may even make the implicit assumption it has to be non-zero, > which would break functionality those odd devices. There's lots of stuff that's broken with individual drivers - anything that relies on cs_change is going to break with half the drivers out there. Some of this is legitimate hardware limitations (where the /CS control is out of the control of software) but a lot of it is just bugs due to people open coding things. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --]
next prev parent reply other threads:[~2014-03-03 1:59 UTC|newest] Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-02-28 14:03 [PATCH] spi: core: make zero length transfer valid again Atsushi Nemoto 2014-02-28 14:03 ` Atsushi Nemoto 2014-03-01 4:13 ` Mark Brown 2014-03-01 4:13 ` Mark Brown 2014-03-01 11:40 ` Martin Sperl 2014-03-01 11:40 ` Martin Sperl 2014-03-03 1:05 ` Mark Brown [this message] 2014-03-03 1:05 ` Mark Brown 2014-03-03 8:26 ` Thierry Reding
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20140303010526.GV29849@sirena.org.uk \ --to=broonie@kernel.org \ --cc=anemo@mba.ocn.ne.jp \ --cc=gsi@denx.de \ --cc=iivanov@mm-sol.com \ --cc=kernel@martin.sperl.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-spi@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.