Hi Viresh,
Thanks for reviewing the patch.
On Wed, Sep 28, 2011 at 1:15 PM, Viresh Kumar <viresh.kumar@xxxxxx> wrote:
> On 9/28/2011 11:20 AM, Alim Akhtar wrote:
>> Signed-off-by: Alim Akhtar <alim.akhtar@xxxxxxxxxxx>
>> ---
>> drivers/dma/amba-pl08x.c | 135 ++++++++++++++++++++++++++++++++++++++--------
>> 1 files changed, 112 insertions(+), 23 deletions(-)
>>
>
> It would be good if you can add pick some part from cover-letter and put it in
> commit log too.
>
Ok, I will write more comments in the commit log.
>> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
>> index cd8df7f..501540f 100644
>> --- a/drivers/dma/amba-pl08x.c
>> +++ b/drivers/dma/amba-pl08x.c
>> @@ -110,6 +129,11 @@ struct pl08x_lli {
>> u32 dst;
>> u32 lli;
>> u32 cctl;
>> + /*
>> + * Samsung pl080 DMAC has one exrta control register
>> + * which is used to hold the transfer_size
>> + */
>> + u32 cctl1;
>
> Will you write transfer_size in cctl also? What is the purpose of cctl1?
>
The main difference between Primecell PL080 and samsung variant is in
LLI control register bit [0:11] is reserved in case of samsung pl080
and one extra register is add to hold the transfer size at offset
0x10. The purpose of cctl1 is store the transfer_size.
>> @@ -215,11 +255,23 @@ static void pl08x_start_txd(struct pl08x_dma_chan *plchan,
>> cpu_relax();
>>
>> /* Do not access config register until channel shows as inactive */
>> - val = readl(phychan->base + PL080_CH_CONFIG);
>> - while ((val & PL080_CONFIG_ACTIVE) || (val & PL080_CONFIG_ENABLE))
>> + if (pl08x->vd->is_pl080_s3c) {
>> + val = readl(phychan->base + PL080S_CH_CONFIG);
>> + while ((val & PL080_CONFIG_ACTIVE) ||
>> + (val & PL080_CONFIG_ENABLE))
>> + val = readl(phychan->base + PL080S_CH_CONFIG);
>> +
>> + writel(val | PL080_CONFIG_ENABLE,
>> + phychan->base + PL080S_CH_CONFIG);
>> + } else {
>> val = readl(phychan->base + PL080_CH_CONFIG);
>> + while ((val & PL080_CONFIG_ACTIVE) ||
>> + (val & PL080_CONFIG_ENABLE))
>> + val = readl(phychan->base + PL080_CH_CONFIG);
>>
>> - writel(val | PL080_CONFIG_ENABLE, phychan->base + PL080_CH_CONFIG);
>> + writel(val | PL080_CONFIG_ENABLE,
>> + phychan->base + PL080_CH_CONFIG);
>> + }
>
> You have similar stuff in most the the changes in this patch, because offset of
> config register changes for s3c, you placed in if,else block.
>
> If you check these changes again, there is a lot of code duplication in this if,else
> blocks. The only different thing in if,else is the offset of CH_CONFIG register.
>
> It would be much better if you can do following:
>
> u32 ch_cfg_off;
>
> if (pl08x->vd->is_pl080_s3c)
> ch_cfg_off = PL080S_CH_CONFIG;
> else
> ch_cfg_off = PL080_CH_CONFIG;
>
> Now, this offset can be used in existing code, without much code duplication.
>
I will use suggestion and remove the code duplications.
>> @@ -569,6 +641,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>> u32 cctl, early_bytes = 0;
>> size_t max_bytes_per_lli, total_bytes = 0;
>> struct pl08x_lli *llis_va;
>> + size_t lli_len = 0, target_len, tsize, odd_bytes;
>>
>> txd->llis_va = dma_pool_alloc(pl08x->pool, GFP_NOWAIT, &txd->llis_bus);
>> if (!txd->llis_va) {
>> @@ -700,7 +773,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>> * width left
>> */
>> while (bd.remainder > (mbus->buswidth - 1)) {
>> - size_t lli_len, tsize, width;
>> + size_t width;
>>
>
> why do we need above two changes. odd_bytes and target_len are still unused.
>
sorry, I will remove the used variables.
>> /*
>> * If enough left try to send max possible,
>> @@ -759,6 +832,9 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,
>> llis_va[num_llis - 1].lli = 0;
>> /* The final LLI element shall also fire an interrupt. */
>> llis_va[num_llis - 1].cctl |= PL080_CONTROL_TC_IRQ_EN;
>> + /* Keep the TransferSize seperate to fill samsung specific register */
>> + if (pl08x->vd->is_pl080_s3c)
>> + llis_va[num_llis - 1].cctl1 |= lli_len;
>
> I couldn't get this completely. Why do you keep only length of
> last lli in cctl1. what about all other llis. Also why |=
> and not directly cctl1 = lli_len
>
I will write more explanation about it.
> --
> viresh
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
