Messages in this thread | | | Subject | Re: [patch 2.6.14-git] SPI core, refresh | From | dmitry pervushin <> | Date | Mon, 14 Nov 2005 20:32:10 +0300 |
| |
On Thu, 2005-11-10 at 23:55 -0800, David Brownell wrote: > I thought I'd send out a refresh of this simple SPI framework, > updated to build on recent kernels. The patch description > inludes a summary of what changed ... not much, though there > is now a Documentation/spi directory with a FAQ-ish writeup. I'd like to comment you framework; there are some places that still are suspicious to me. First of all, it is better to inline the patch; this makes easier commenting etc. My comments follow: + void *controller_state; + const void *controller_data; Why to use the separate controller_data/controller_state fields ? At learuesting qest one of them fits to the platform_data +struct spi_transfer { + /* it's ok if tx_buf == rx_buf (right?) + * for MicroWire, one buffer must be null + * buffers must work with dma_*map_single() calls + */ + const void *tx_buf; + void *rx_buf; + unsigned len; + + /* REVISIT for now, these are only for the controller driver's + * use, for recording dma mappings + */ + dma_addr_t tx_dma; + dma_addr_t rx_dma; OK, you requesting that tx/rx buffer must be DMA-capable. And why you are using stack-allocated buffers, for example, in spi_w8r8 ? If protocol driver is not enough smart to transfer small amouts of data not using DMA, this could crash the system... +struct spi_message { + struct spi_transfer *transfers; + unsigned n_transfer; + + struct spi_device *spi; + + /* completion is reported through a callback */ + void FASTCALL((*complete)(void *context)); As far as I understand, the protocol driver is requested to call complete somewhere after processing the message; even ignoring my preference to call this function as part of common message processing, I'd prefer to see exported/inline function like spi_message_complete (struct spi_message* msg). It is not clear that controller driver _must_ call the `complete' as part of processing message. +static inline int spi_w8r8(struct spi_device *spi, u8 cmd) +{ + int status; + u8 result; + + status = spi_write_then_read(spi, &cmd, 1, &result, 1); This breaks the statement that buffers must be dma_single_map'able :( Namely, result cannot be mapped. +/** + * spi_w8r16 - SPI synchronous 8 bit write followed by 16 bit read + * @spi: device with which data will be exchanged + * @cmd: command to be written before data is read back + * + * This returns the (unsigned) sixteen bit number returned by the + * device, or else a negative error code. Callable only from + * contexts that can sleep. + * + * The number is returned in wire-order, which is at least sometimes + * big-endian. + */ +static inline int spi_w8r16(struct spi_device *spi, u8 cmd) +{ + int status; + u16 result; + + status = spi_write_then_read(spi, &cmd, 1, (u8 *) &result, 2); + + /* return negative errno or unsigned value */ + return (status < 0) ? status : result; +} Incorrect mixing int (signed int!) and u16. What if spi_write_then_read read the value, say, 0xFFFF ? Is it the correct result or -1 indicating error ?
+ if (status < 0) { + dev_dbg(dev, "can't %s %s, status %d\n", + "add", proxy->dev.bus_id, status); +fail: + class_device_put(&master->cdev); + kfree(proxy); + return NULL; + } Using goto to the middle of compound statement... I personally do not like this. This can be easily moved out of block.
There will be more comments... -- cheers, dmitry pervushin
- 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/
| |