On Fri, Aug 22, 2014 at 04:33:09PM +0300, Grygorii Strashko wrote: > On 08/21/2014 09:20 PM, Mark Brown wrote: > > On Thu, Aug 21, 2014 at 06:25:06PM +0300, Grygorii Strashko wrote: > >> +- ti,davinci-spi-wdelay : delay between transmissions. > > I don't understand why this is here - there is already standard support > > in the SPI core for client drivers specifying inter-transfer delays. If > > there is a need to provide platform configuration for this in addition > > to that then it should also be a standard property, there is nothing > > device specific about these. > Sorry, may be I've missed smth, because I was not able to find such common > property in SPI bindings document and code. Could you point me on, pls? It's not a property, it's what the delay_usecs in the transfer does. There is no obvious reason to apply something like this unconditionally on every transfer in DT - either it's needed only at particular points in the transfer for device reasons (in which case the device driver needs to know about it) or we need some sort of association with what the device is doing since the way the client driver splits up transfers is entirely up to it. > >> +- ti,davinci-spi-odd-parity : odd partity enabled > >> + OR > >> + ti,davinci-spi-even-parity : even parity enabled > > What do these mean? > Supported by OMAP-L138/da830: > " > SPIFMTn[23].PARPOL - Parity polarity: even or odd. PARPOL can be modified in privilege mode only. > 0 An even parity flag is added at the end of the transmit data stream. > 1 An odd parity flag is added at the end of the transmit data stream. Why would these be specified in DT and not with runtime flags enabled by the device? It looks like they affect the data stream generated by the controller so the client device needs to know about them; I'd expect that it's device driver would be controlling when they are enabled if the controller supports them. > >> +- ti,davinci-spi-io-type: io type (check platform_data/spi-davinci.h) > > The bindings should be independent of the kernel, the values need to be > > included here (and the defines moved to include/dt-bindings so they can > > be used when writing DTs). > Allowed values here are: > #define SPI_IO_TYPE_INTR 0 > #define SPI_IO_TYPE_POLL 1 > #define SPI_IO_TYPE_DMA 2 > I'll update. Now I see these it's not at all clear why this is configurable at all - I would expect the controller driver to automatically select the most appropriate scheme for the transfers it's doing in order to obtain the best performance rather than having a global switch for this. For almost all devices the best results will be obtained by doing a mix of these modes. Typically for each level of transfer there will be some minimal size below which it doesn't make sense to use that control type, for example it's common to only DMA if there's more than a FIFO worth of data to transfer.
Attachment:
signature.asc
Description: Digital signature
