[RESEND][PATCH 0/2] McSPI Slave and DMA ,FIFO support

[RESEND][PATCH 0/2] McSPI Slave and DMA ,FIFO support

[Hemanth V]

This series of 2 patches adds support for McSPI slave
and DMA,FIFO. It incorporates review comments from
Kevin Hilman and Tony Lindgren


PATCH[1/2]: Changes to arch specific files
PATCH[2/2]: omap2_mcspi.c file changes

 arch/arm/mach-omap2/devices.c           |    5
 arch/arm/plat-omap/include/mach/mcspi.h |   16 +
 drivers/spi/omap2_mcspi.c               |  353
++++++++++++++++++++++++++++++++++++++++------

Re: [RESEND][PATCH 0/2] McSPI Slave and DMA ,FIFO support

[David Brownell]

On Tuesday 23 June 2009, Hemanth V wrote:
> This series of 2 patches adds support for McSPI slave
> and DMA,FIFO. It incorporates review comments from
> Kevin Hilman and Tony Lindgren
> 
> 
> PATCH[1/2]: Changes to arch specific files
> PATCH[2/2]: omap2_mcspi.c file changes

Needs some changes yet:

  - Split out slave support as a separate patch.
    That's not really supported by the interface yet.

  - You sent the two patches with the same $SUBJECT;
    which indicates they are the *SAME* patch ...
    either make it be one patch, or change $SUBJECT
    (perhaps to match what you used above).

The stuff in #1 should have Tony's ack.


> 
>  arch/arm/mach-omap2/devices.c           |    5
>  arch/arm/plat-omap/include/mach/mcspi.h |   16 +
>  drivers/spi/omap2_mcspi.c               |  353
> ++++++++++++++++++++++++++++++++++++++++------
> 
> 
> 

Re: [RESEND][PATCH 0/2] McSPI Slave and DMA ,FIFO support

[Hemanth V]

----- Original Message ----- 
From: "David Brownell" <david-b@pacbell.net>


> On Tuesday 23 June 2009, Hemanth V wrote:
>> This series of 2 patches adds support for McSPI slave
>> and DMA,FIFO. It incorporates review comments from
>> Kevin Hilman and Tony Lindgren
>> 
>> 
>> PATCH[1/2]: Changes to arch specific files
>> PATCH[2/2]: omap2_mcspi.c file changes
> 
> Needs some changes yet:
> 
>  - Split out slave support as a separate patch.
>    That's not really supported by the interface yet.

Do you see any major changes required to support
slave mode in the SPI core driver. We are able to
use the existing interface for slave mode also, but
some APIs/ Structures could be made generic.

Re: [spi-devel-general] [RESEND][PATCH 0/2] McSPI Slave and DMA , FIFO support

[David Brownell]

On Sunday 05 July 2009, Hemanth V wrote:
> Do you see any major changes required to support
> slave mode in the SPI core driver.

There *is* no such thing as a "SPI core driver"...


> We are able to 
> use the existing interface for slave mode also, but
> some APIs/ Structures could be made generic.

Three things are obvious:

 - A spi_master is not a slave side driver!

 - Control model would need to be inverted
    * Chip select would be one input,
      not N outputs
    * No clock rate controls at all
    * latency issues ... driver can't necessarily
      respond quickly enough to guarantee no data
      loss (FIFOs help)
    * Can't re-use /dev/spidev*
    * Request queue would have a very different role

 - Some primitives are likely missing, for flow
   control (when hardware has a READY handshake)

I think a few proposals for how to handle slave
side have been circulated.

Re: [spi-devel-general] [RESEND][PATCH 0/2] McSPI Slave and DMA , FIFO support

[Hemanth V]

---- Original Message ----- 
From: "David Brownell" <david-b@pacbell.net>


> On Sunday 05 July 2009, Hemanth V wrote:
>> Do you see any major changes required to support
>> slave mode in the SPI core driver.
> 
> There *is* no such thing as a "SPI core driver"...
> 
I was referring to

spi.c - SPI init/core code

> 
>> We are able to 
>> use the existing interface for slave mode also, but
>> some APIs/ Structures could be made generic.
> 
> Three things are obvious:
> 
> - A spi_master is not a slave side driver!

Yes a more generic or new structure would be good,
but the current one does work for slave mode also.

> 
> - Control model would need to be inverted
>    * Chip select would be one input,
>      not N outputs
>    * No clock rate controls at all

Conditional checks for slave/master mode have been added in
the driver where required.

>    * latency issues ... driver can't necessarily
>      respond quickly enough to guarantee no data
>      loss (FIFOs help)

We currently support both FIFO and DMA for
slave mode

>    * Can't re-use /dev/spidev*
>    * Request queue would have a very different role
> 
> - Some primitives are likely missing, for flow
>   control (when hardware has a READY handshake)
> 
> I think a few proposals for how to handle slave
> side have been circulated.

So would it be possible to merge the slave support also for
now, and could be modified to support the new slave interface
as and when available.

Re: [spi-devel-general] [RESEND][PATCH 0/2] McSPI Slave and DMA , FIFO support

[Tony Lindgren]

* Hemanth V <hemanthv@ti.com> [090828 05:05]:
> ---- Original Message ----- From: "David Brownell" <david-b@pacbell.net>
>
>
>> On Sunday 05 July 2009, Hemanth V wrote:
>>> Do you see any major changes required to support
>>> slave mode in the SPI core driver.
>>
>> There *is* no such thing as a "SPI core driver"...
>>
> I was referring to
>
> spi.c - SPI init/core code
>
>>
>>> We are able to use the existing interface for slave mode also, but
>>> some APIs/ Structures could be made generic.
>>
>> Three things are obvious:
>>
>> - A spi_master is not a slave side driver!
>
> Yes a more generic or new structure would be good,
> but the current one does work for slave mode also.
>
>>
>> - Control model would need to be inverted
>>    * Chip select would be one input,
>>      not N outputs
>>    * No clock rate controls at all
>
> Conditional checks for slave/master mode have been added in
> the driver where required.
>
>>    * latency issues ... driver can't necessarily
>>      respond quickly enough to guarantee no data
>>      loss (FIFOs help)
>
> We currently support both FIFO and DMA for
> slave mode
>
>>    * Can't re-use /dev/spidev*
>>    * Request queue would have a very different role
>>
>> - Some primitives are likely missing, for flow
>>   control (when hardware has a READY handshake)
>>
>> I think a few proposals for how to handle slave
>> side have been circulated.
>
> So would it be possible to merge the slave support also for
> now, and could be modified to support the new slave interface
> as and when available.

In general it's not good to merge half implemented patches.

That's because it usually means that the work on getting
things cleaned up stops right there.

And then the community is left to deal with the mess.

So I recommend doing it clean from the start rather than
merging in hacks.

Tony

Re: [spi-devel-general] [RESEND][PATCH 0/2] McSPI Slave and DMA , FIFO support

[David Brownell]

On Friday 28 August 2009, Hemanth V wrote:
> So would it be possible to merge the slave support also for
> now, and could be modified to support the new slave interface
> as and when available.

Not really.  You still haven't split the non-slave stuff into
a patch, note ..