Re: [PATCHv2 4/7] input/cma3000_d0x: Add CMA3000 spi support

[Jonathan Cameron <jic23@cam.ac.uk>]

On 10/18/11 16:18, Ricardo Ribalda Delgado wrote:
> Hello Jonathan
> 
>  I have separated the read and write commands... but when I was
> implementing the spi_write I found that most of the drivers just use
> stack variables for spi_write. Like drivers/hwmon/ads7871.c,
> drivers/gpio/max7301.c and others... Am I missing something or it is a
> general "bug"?
bug unless I am also missing where they prevent dma
transfers.  spi_read_then_write is fine
as it does a copy, but spi_write doesn't. 
Documentation/spi/spi-summary

"Note that there are two types of memory your driver must manage as part
of interacting with SPI devices.

  - I/O buffers use the usual Linux rules, and must be DMA-safe.
    You'd normally allocate them from the heap or free page pool.
    Don't use the stack, or anything that's declared "static".
"

This was an issue many people (including me) weren't aware of until
a year or two ago when it was picked up in reviews of a number of drivers.
It was around that time the ___cacheline_aligned trick was used by
Michael Hennerich and everyone else picked up on that as often the
easiest way of doing this in a driver.

Don't suppose there is anything stopping you doing 
spi_write_then_read(tx, 2, NULL, 0)
and using the buffers helpfully allocated in the spi core?
This'll allocate extra space if someone else is using the core
bounce buffers though...

> 
> As you say, it is better to not rewrite the cma3000 driver until
> Hemanth says so, lets leave it for a future patch.
> 
> 
> 
> 
> On Tue, Oct 18, 2011 at 15:50, Jonathan Cameron <jic23@cam.ac.uk> wrote:
>> On 10/18/11 14:43, Ricardo Ribalda Delgado wrote:
>>> Hello Jonathan
>>>
>>>   First of all, thanks for your messages :).
>>>
>>>> To make my point about these functions being more complex than needed
>>>> in more detail....
>>>>
>>>> If this were two functions and you drop the zero and 1 mask
>>>> (which I'm not convinced make any sense. I've also killed the message.
>>>> We both agree it is the wrong way to go, so post a patch fixing the i2c
>>>> interface as well.
>>>
>>> Of course your functions are much more simpler and beautiful than the
>>> fat one I wrote, no doubt about it :). Just three comments
>>>
>>> - Checking the one mask and the zero mask is the only way we have to
>>> know if the chip is still there, The absense of that reply should
>>> trigger an IO error or at least a retry. As you point out, the
>>> zero/one mask is only violated on startup.  I just wanted to make it
>>> more risk free, but if you believe it is more clear that way, lets
>>> remove it
>> It's somewhat unconventional to verify the existence of a chip like this.
>> Usually you assume that if it was there once it still is unless there
>> is a very good reason to think otherwise.  Worth doing an initial check
>> in your spi_probe and indeed verify there against these known bits.
>> No need to do it every time though.
>>>
>>> - I am not very fun of kmallocing data per write, specially when it is
>>> part of the irq handler, and you expect this to be low latency. What
>>> about allocating a buffer on init time, and use it with a mutex?
>> That's absolutely fine and the right way to do it. You could poke it
>> into the cma3000_accl_data then use the cachline aligned magic. Its
>> is tiny so I doubt anyone will mind the overhead for the i2c side of
>> things.
>>>
>>> -I dont like the push error message to the bottom, but that will mean
>>> a rewrite of the cma3000 driver, shall I go for it?
>> I would. Though probably worth getting Hemanth to say if he minds first
>> given it's his driver!
>>>
>>>
>>> Thanks again, and  I will post the new version when you reply this :)
>>>
>>>
>>
>>
> 
> 
>