[PATCH 2/6] S3C: DMA: Add api driver for PL330

[Jassi Brar]

On Mon, May 17, 2010 at 5:40 PM, Ben Dooks <ben-linux at fluff.org> wrote:
> On Mon, May 17, 2010 at 10:35:39AM +0900, Jassi Brar wrote:

>> +/*
>> + * The platforms just need to provide this info
>> + * to the S3C DMA API driver for PL330.
>
> would be heklpful to describe what this information actually
> means, including how it helps to configure the hardware appropriately.

Ok

......

>> +struct s3c_pl330_dmac {
>> +     struct pl330_info *pi;
>> +     /* Number of channels currently busy */
>> +     unsigned busy_chan;
>> +     /* To attach to the global list of DMACs */
>> +     struct list_head node;
>> +     /* List of IDs of peripherals this DMAC can work with */
>> +     enum dma_ch *peri;
>> +     /* Pool to quickly allocate xfers for all channels in the dmac */
>> +     struct kmem_cache *kmcache;
>> +};
>
> I would prefer to see kerneldoc style documentation for this
> if possible.
Ok.

......

>> +     ch = kmalloc(sizeof(*ch), GFP_KERNEL);
>> +     /* Return silently to work with other channels */
>> +     if (!ch)
>> +             return;
>> +
>> +     ch->id = id;
>> +     ch->dmac = NULL;
>> +
>
> should you init the list entry before adding it?
sorry, I am unable to see any problem with what I do.
Am I overlooking something?

.....

>> +/*
>> + * Measure of suitability of 'dmac' handling 'ch'
>> + *
>> + * 0 indicates 'dmac' can not handle 'ch' either
>> + * because it is not supported by the hardware or
>> + * because all dmac channels are currently busy.
>> + *
>> + * >0 vlaue indicates 'dmac' has the capability.
>> + * The bigger the value the more suitable the dmac.
>> + */
>> +typedef u8 suit;
>> +#define MAX_SUIT     255
>> +#define MIN_SUIT     0
>
> hmm, would oprfer to see this as a simple integer.
Ok.

......

>> +     /* If there was only one node left */
>> +     if (t == xfer)
>> +             ch->xfer_head = NULL;
>> +     else if (ch->xfer_head == xfer)
>> +             ch->xfer_head = t;
>> +
>> +     list_del(&xfer->node);
>> +}
>
> couldn't xfer_head be infered from the transfer list?
Not if we are to support S3C2410_DMAF_CIRCULAR.

thanks