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

On Tue, Nov 10, 2009 at 9:19 AM, Richard R=F6jfors
[off-list ref] wrote:

Grant Likely wrote:

quoted

Oops, I replied to the original version, but missed the subsequent
versions. =A0Looks like some of my comments still apply though.
Overall, the patch changes too many things all at once. =A0You should
look at splitting it up. =A0At 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.

quoted

quoted

+struct xilinx_spi {
+ =A0 =A0 =A0 /* bitbang has to be first */
+ =A0 =A0 =A0 struct spi_bitbang bitbang;
+ =A0 =A0 =A0 struct completion done;
+ =A0 =A0 =A0 struct resource mem; /* phys mem */
+ =A0 =A0 =A0 void __iomem =A0 =A0*regs; =A0/* virt. address of the con=

trol registers */

quoted

quoted

+ =A0 =A0 =A0 u32 irq;
+ =A0 =A0 =A0 u8 *rx_ptr; =A0 =A0 =A0 =A0 =A0 =A0 /* pointer in the Tx =

buffer */

quoted

quoted

+ =A0 =A0 =A0 const u8 *tx_ptr; =A0 =A0 =A0 /* pointer in the Rx buffer=

 */

quoted

quoted

+ =A0 =A0 =A0 int remaining_bytes; =A0 =A0/* the number of bytes left t=

o transfer */

quoted

quoted

+ =A0 =A0 =A0 /* offset to the XSPI regs, these might vary... */
+ =A0 =A0 =A0 u8 bits_per_word;
+ =A0 =A0 =A0 bool big_endian; =A0 =A0 =A0 =A0/* The device could be ac=

cessed big or little

quoted

quoted

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* endi=

an

quoted

quoted

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
+};
+

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.

quoted

Ah, you changed these to functions instead of macros. =A0I like.
However, as you suggested elsewhere in this thread, you could change
these to callbacks and then eliminate the if/else statements. =A0I think
that is the approach you should use. =A0I don't think you need to worry
about it being slower. =A0Any 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, wo=

uld 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.

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