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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
