Re: [patch 2.6.14-git] SPI core, refresh

[dmitry pervushin <dpervushin@gmail.com>]

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