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"? 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@xxxxxxxxx> 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-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
