[U-Boot] [PATCH 2/6] mx6q: Add support for ECSPI through mxc_spi driver

[Marek Vasut]

> On 18/01/2012 02:44, Eric Nelson wrote:
> > On 01/17/2012 06:27 PM, Marek Vasut wrote:
> >>> On 01/17/2012 04:19 PM, Marek Vasut wrote:
> >>>>> Signed-off-by: Eric Nelson<eric.nelson at boundarydevices.com>
> >>>>> +/* ECSPI registers */
> >>>>> +struct cspi_regs {
> >>>>> +    u32 rxdata;
> >>>>> +    u32 txdata;
> >>>>> +    u32 ctrl;
> >>>>> +    u32 cfg;
> >>>>> +    u32 intr;
> >>>>> +    u32 dma;
> >>>>> +    u32 stat;
> >>>>> +    u32 period;
> >>>>> +};
> >>>> 
> >>>> Sigh ... it's no fun I can have only one remark :-)
> >>>> 
> >>>> Is this part common for all imx-es ?
> >>> 
> >>> All i.MX6's
> >>> 
> >>> This is a cut&  paste from MX51.
> >>> 
> >>> I was tempted to introduce an 'mxc_ecspi.h' to merge the declaration
> >>> for i.MX5x and i.MX6 which share the ECSPI peripheral and 'mxc_cspi.h'
> >>> for i.MX31 and i.MX35 that share the CSPI peripheral.
> >> 
> >> But you don't even need this outside of the spi driver so just put it
> >> into the
> >> spi driver and be done with it. That'll solve your duplication issue.
> >> 
> >> M
> > 
> > I'll defer to Stefano on this one, since I did this in response
> 
> > to his request:
> Yes, I admit I am guilty about this !
> 
> The layout of the CSPI registers is not exactly the same for all SOCs.
> For example, the MXC_CSPICTRL_TC has a different position, as well as
> the BITCOUNT (because the MX31 can send less bits in one shot) and
> MXC_CSPICTRL_CHIPSELECT.
> 
> So they are similar, but not exactly the same.
> 
> Then we have the MX5 registers. Even if we check the CSPI (not eCSPI)
> controller, the layout of the registers is different compared to the MX3
> SOCs.
> 
> > The struct cspi_regs is already present in mx31, mx35, and mx51 headers,
> > so I'm not breaking new ground here, only in the bitfield declarations.
> 
> Right, I see the same. The cspi_regs structure is already defined into
> the imx_regs.h, only the bit layout was moved. And as the bit layout is
> SOC dependent, I think the right place for it is inside the imx-regs.h
> registers.
> 
> > My interpretation of Stefano's intent is to clean up the driver at the
> > expense
> > of extra defines in the arch-specific headers.
> 
> Yes, you're right - of course, I am open also to other solutions if they
> are proofed to be better ;-).
> 
> Best regards,
> Stefano

Ok guys, I see ... Stefano, you're ok with putting the reg structures into these 
header files?

M