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

[John Linn]

> -----Original Message-----
> From: glikely at secretlab.ca [mailto:glikely at secretlab.ca] On Behalf Of Grant Likely
> Sent: Tuesday, November 10, 2009 9:45 AM
> To: Richard Röjfors
> Cc: spi-devel-general at lists.sourceforge.net; linuxppc-dev at ozlabs.org; Andrew Morton;
> dbrownell at users.sourceforge.net; John Linn
> Subject: 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ö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.

I agree.  

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

Thanks,
John

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


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