[spi-devel-general] [PATCH v4] xilinx_spi: Splitted into generic, of and platform driver, added support for DS570

[Grant Likely]

On Tue, Nov 10, 2009 at 9:19 AM, Richard Röjfors
<richard.rojfors at mocean-labs.com> wrote:
> Grant Likely wrote:
>> Oops, I replied to the original version, but missed the subsequent
>> versions.  Looks like some of my comments still apply though.
>> Overall, the patch changes too many things all at once.  You should
>> look at splitting it up.  At the very least the io accessor changes
>> should be done in a separate patch.

Hi Richard.  Please do another spin of this patch.  I don't have any
particular problem with the changes, but it needs to be in a more
granular form so I can review it properly.

>>> +struct xilinx_spi {
>>> +       /* bitbang has to be first */
>>> +       struct spi_bitbang bitbang;
>>> +       struct completion done;
>>> +       struct resource mem; /* phys mem */
>>> +       void __iomem    *regs;  /* virt. address of the control registers */
>>> +       u32 irq;
>>> +       u8 *rx_ptr;             /* pointer in the Tx buffer */
>>> +       const u8 *tx_ptr;       /* pointer in the Rx buffer */
>>> +       int remaining_bytes;    /* the number of bytes left to transfer */
>>> +       /* offset to the XSPI regs, these might vary... */
>>> +       u8 bits_per_word;
>>> +       bool big_endian;        /* The device could be accessed big or little
>>> +                                * endian
>>> +                                */
>>> +};
>>> +
>>
>> Why is the definition of xilinx_spi moved?
>
> I liked the idea of heaving the struct defined in the top of the file.

... which is completely unrelated to the patch purpose, and is not
mentioned in the patch header.  If you really want to move it then put
it in a completely separate patch and describe the change properly.
As it is right now it is just noise that makes the stated purpose of
the patch hard to review.

>> Ah, you changed these to functions instead of macros.  I like.
>> However, as you suggested elsewhere in this thread, you could change
>> these to callbacks and then eliminate the if/else statements.  I think
>> that is the approach you should use.  I don't think you need to worry
>> about it being slower.  Any extra cycles for jumping to a callback
>> will be far dwarfed by the number of cycles it takes to complete an
>> SPI transfer.
>
> Sure that can be updated. I prefer to do that in an incremental patch, would be great to get this
> big one merged first.

As already commented on, this patch is too big and does too many
unrelated things.  Please split into discrete changes so it can be
reviewed properly.

Thanks,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.