> On Thu, Sep 30, 2010 at 12:08 PM, Wolfgang Denk <wd@xxxxxxx> wrote:
> [snip other valid review comments]
> >
> > This is a header file, yet you add here literally thousands of lines
> of
> > code.
>
> Yes, these functions are entirely too large to be inlined. It looks
> like you are trying to borrow too heavily from the iop-adma model.
> The differences between iop13xx and iop33x from a adma perspective are
> just in descriptor format and channel capabilities. If you look at
> the routines implemented in:
> arch/arm/include/asm/hardware/iop3xx-adma.h
> arch/arm/mach-iop13xx/include/mach/adma.h
> ...they are just simple helpers that abstract the descriptor details.
> For example:
>
> iop_adma_prep_dma_xor()
> {
> [snip]
> spin_lock_bh(&iop_chan->lock);
> slot_cnt = iop_chan_xor_slot_count(len, src_cnt,
> &slots_per_op);
> sw_desc = iop_adma_alloc_slots(iop_chan, slot_cnt,
> slots_per_op);
> if (sw_desc) {
> grp_start = sw_desc->group_head;
> iop_desc_init_xor(grp_start, src_cnt, flags);
> iop_desc_set_byte_count(grp_start, iop_chan, len);
> iop_desc_set_dest_addr(grp_start, iop_chan, dma_dest);
> sw_desc->unmap_src_cnt = src_cnt;
> sw_desc->unmap_len = len;
> sw_desc->async_tx.flags = flags;
> while (src_cnt--)
> iop_desc_set_xor_src_addr(grp_start, src_cnt,
> dma_src[src_cnt]);
> }
> spin_unlock_bh(&iop_chan->lock);
>
> return sw_desc ? &sw_desc->async_tx : NULL;
> }
>
> Where iop_adma_alloc_slots() is implemented differently between
> iop13xx and iop3xx. In this case why does ppc440spe-adma.h exist? If
> it has code specific to ppe440spe it should just live in the ppe440spe
> C file. If it is truly generic it should move to the base adma.c
> implementation. If you want to reuse a ppe440spe routine just link to
> it.
[Marri]That is how I started changing the code. And I see tons of warnings
Saying "Used but not defined" or "Defined but not used". How should I
suppress
Some functions from adma.c are used in ppc440spe-adma.c and some from
ppc440spe-adma.c
Are used in adma.c. So I created intermediate file ppc440spe-adma.h with
inlined
Functions. In future this will be converted into ppc4xx_adma.h and move
existing
SoC specific stuff to ppc440spe-adma.c file.
>
> > Selecting the architecture at build time is bad as it prevents using
> a
> > sinlge kernel image across a wide range of boards. You only replied
> > "We select the architecture at build time." without any explanation
> if
> > there is a pressing technical reason to do it this way, or if this
> was
> > just a arbitrary decision.
>
> I agree I have yet to see any indication that this driver needs to
> have an architecture selected at build time.
>
> A potential compromise a first step would be to have a common C file
> that is shared between two driver modules until such point that they
> can be unified into a common module.
As I responded to Mr. Wolfgang in previous email, similar SoC DMA engines
will
Be resolved at run time. Whereas completely different architectures will
be
Resolved at build time.
Regards,
Marri
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
