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

-----Original Message-----
From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On Behalf Of Gra=

nt Likely

Sent: Tuesday, November 10, 2009 9:45 AM
To: Richard R=F6jfors
Cc: spi-devel-general@lists.sourceforge.net; linuxppc-dev@ozlabs.org; And=

rew Morton;

dbrownell@users.sourceforge.net; John Linn
Subject: Re: [spi-devel-general] [PATCH v4] xilinx_spi: Splitted into gen=

eric, of and platform

driver, added support for DS570
=

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

quoted

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.

I agree.  =


We have a functioning driver such that I need to make sure it doesn't regre=
ss. More granular changes will also allow us to better isolate any problems=
 if they occur.

Thanks,
John

=

quoted

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 c=

ontrol registers */

quoted

quoted

quoted

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

x buffer */

quoted

quoted

quoted

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

er */

quoted

quoted

quoted

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

 to transfer */

quoted

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 =

accessed big or little

quoted

quoted

quoted

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* en=

dian

quoted

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

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 thi=

nk

quoted

quoted

that is the approach you should use. =A0I don't think you need to worr=

y

quoted

quoted

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, =

would be great to get this

quoted

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.

This email and any attachments are intended for the sole use of the named r=
ecipient(s) and contain(s) confidential information that may be proprietary=
, privileged or copyrighted under applicable law. If you are not the intend=
ed recipient, do not read, copy, or forward this email message or any attac=
hments. Delete this email message and any attachments immediately.