lkml.org 
[lkml]   [2011]   [Oct]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCHv2 4/7] input/cma3000_d0x: Add CMA3000 spi support
    Hello Jonathan

    I just send the mail with the latest patch version. I will wait for
    hermanthv to see if he wants to replace the msg from the read/write
    functions, and I will follow his style for the cmr3000. I have testest
    the latest changes on my board and it looks ok.

    Thanks for your feedback, and I hope it is ok now :)

    On Tue, Oct 18, 2011 at 17:27, Jonathan Cameron <jic23@cam.ac.uk> wrote:
    > 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 :)
    >>>>
    >>>>
    >>>
    >>>
    >>
    >>
    >>
    >
    >



    --
    Ricardo Ribalda
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2011-10-18 17:53    [W:2.622 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site