On Thu, Sep 07, 2017 at 06:00:04PM +0800, Baolin Wang wrote:
> +static void sprd_dma_chn_update(struct sprd_dma_chn *schan, u32 reg,
> + u32 mask, u32 val)
right justfied pls
> +static void sprd_dma_clear_int(struct sprd_dma_chn *schan)
> +{
> + u32 mask = SPRD_DMA_INT_MASK << SPRD_DMA_INT_CLR_OFFSET;
> + u32 val = SPRD_DMA_INT_MASK << SPRD_DMA_INT_CLR_OFFSET;
both seems same..?
> +
> + sprd_dma_chn_update(schan, SPRD_DMA_CHN_INTC, mask, val);
do you need local values here, we can just call sprd_dma_chn_update() with
the mask and values!
Also looking at thus shoulnt SPRD_DMA_INT_MASK bits be defined for the bits
we have in spec, if so why are we shifting then, perhpas u should redo the
clear routine to pass mask, shift, bits..?
Same comment for this and others
> +static enum dma_int_type sprd_dma_get_int_type(struct sprd_dma_chn *schan)
> +{
> + u32 intc_sts = readl(schan->chn_base + SPRD_DMA_CHN_INTC) &
> + SPRD_DMA_CHN_INT_STS;
right justfied
> +static enum dma_request_mode sprd_dma_get_req_type(struct sprd_dma_chn
> *schan)
> +{
> + u32 frag_reg = readl(schan->chn_base + SPRD_DMA_CHN_FRG_LEN);
> + u32 req_type = (frag_reg >> SPRD_DMA_REQ_MODE_OFFSET) &
> + SPRD_DMA_REQ_MODE_MASK;
> +
> + switch (req_type) {
> + case 0:
> + return SPRD_DMA_FRAG_REQ;
which is 0
> +
> + case 1:
> + return SPRD_DMA_BLK_REQ;
and 1 and so on so why the coonversion?
you can do:
switch (req_type) {
case 0:
case 1:
case 2:
case 3:
return req_type;
default:
return SPRD_DMA_FRAG_REQ;
> +
> + case 2:
> + return SPRD_DMA_TRANS_REQ;
> +
> + case 3:
> + return SPRD_DMA_LIST_REQ;
> +
> + default:
> + return SPRD_DMA_FRAG_REQ;
> + }
> +}
> + if ((src_step != 0 && des_step != 0) || (src_step | des_step) == 0) {
> + fix_en = 0;
> + } else {
> + fix_en = 1;
> + if (src_step)
> + fix_mode = 1;
> + else
> + fix_mode = 0;
> + }
> +
> + hw->frg_len = datawidth << SPRD_DMA_SRC_DATAWIDTH_OFFSET |
> + datawidth << SPRD_DMA_DES_DATAWIDTH_OFFSET |
> + req_mode << SPRD_DMA_REQ_MODE_OFFSET |
> + fix_mode << SPRD_DMA_FIX_SEL_OFFSET |
> + fix_en << SPRD_DMA_FIX_SEL_EN |
> + (fragment_len & SPRD_DMA_FRG_LEN_MASK);
> + hw->blk_len = block_len & SPRD_DMA_BLK_LEN_MASK;
> +
> + hw->intc = SPRD_DMA_CFG_ERR_INT_EN;
empty line here please
> + switch (irq_mode) {
> + case SPRD_DMA_NO_INT:
> + break;
no handling?
> + case SPRD_DMA_FRAG_INT:
> + hw->intc |= SPRD_DMA_FRAG_INT_EN;
> + break;
empty line after break helps readablity
> + case SPRD_DMA_BLK_INT:
> + hw->intc |= SPRD_DMA_BLK_INT_EN;
> + break;
> + case SPRD_DMA_BLK_FRAG_INT:
> + hw->intc |= SPRD_DMA_BLK_INT_EN | SPRD_DMA_FRAG_INT_EN;
> + break;
> + case SPRD_DMA_TRANS_INT:
> + hw->intc |= SPRD_DMA_TRANS_INT_EN;
> + break;
> + case SPRD_DMA_TRANS_FRAG_INT:
> + hw->intc |= SPRD_DMA_TRANS_INT_EN | SPRD_DMA_FRAG_INT_EN;
> + break;
> + case SPRD_DMA_TRANS_BLK_INT:
> + hw->intc |= SPRD_DMA_TRANS_INT_EN | SPRD_DMA_BLK_INT_EN;
> + break;
> + case SPRD_DMA_LIST_INT:
> + hw->intc |= SPRD_DMA_LIST_INT_EN;
> + break;
> + case SPRD_DMA_CFGERR_INT:
> + hw->intc |= SPRD_DMA_CFG_ERR_INT_EN;
> + break;
> + default:
> + dev_err(sdev->dma_dev.dev, "invalid irq mode\n");
> + return -EINVAL;
[snip]
> +struct dma_async_tx_descriptor *sprd_dma_prep_dma_memcpy(struct dma_chan
> *chan,
> + dma_addr_t dest,
> + dma_addr_t src,
> + size_t len,
> + unsigned long flags)
> +{
> + struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> + struct sprd_dma_desc *sdesc;
> + int ret;
> +
> + sdesc = kzalloc(sizeof(struct sprd_dma_desc), GFP_NOWAIT);
sizeof(*sdesc) pls
> + ret = dma_async_device_register(&sdev->dma_dev);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "register dma device failed:%d\n", ret);
> + goto err_register;
> + }
> +
> + sprd_dma_info.dma_cap = sdev->dma_dev.cap_mask;
> + ret = of_dma_controller_register(np, of_dma_simple_xlate,
> + &sprd_dma_info);
> + if (ret)
> + goto err_of_register;
> +
> + pm_runtime_put_sync(&pdev->dev);
why put_sync, i though you didnt want these?
--
~Vinod
