Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1738862 > unrolled thread

Re: [PATCH v3 2/2] dma: sprd: Add Spreadtrum DMA driver

Started byVinod Koul <vinod.koul@intel.com>
First post2017-09-25 11:50 +0200
Last post2017-09-26 02:20 +0200
Articles 4 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 2/2] dma: sprd: Add Spreadtrum DMA driver Vinod Koul <vinod.koul@intel.com> - 2017-09-25 11:50 +0200
    Re: [PATCH v3 2/2] dma: sprd: Add Spreadtrum DMA driver Baolin Wang <baolin.wang@linaro.org> - 2017-09-25 22:10 +0200
      Re: [PATCH v3 2/2] dma: sprd: Add Spreadtrum DMA driver Mark Brown <broonie@kernel.org> - 2017-09-25 23:20 +0200
        Re: [PATCH v3 2/2] dma: sprd: Add Spreadtrum DMA driver Baolin Wang <baolin.wang@linaro.org> - 2017-09-26 02:20 +0200

#1738862 — Re: [PATCH v3 2/2] dma: sprd: Add Spreadtrum DMA driver

FromVinod Koul <vinod.koul@intel.com>
Date2017-09-25 11:50 +0200
SubjectRe: [PATCH v3 2/2] dma: sprd: Add Spreadtrum DMA driver
Message-ID<utybw-4yR-3@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1739255

FromBaolin Wang <baolin.wang@linaro.org>
Date2017-09-25 22:10 +0200
Message-ID<utHRw-2MV-7@gated-at.bofh.it>
In reply to#1738862
Hi Vinod,

On 25 September 2017 at 17:47, Vinod Koul <vinod.koul@intel.com> wrote:
> 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

I have made these to right justified, but I do not know why it looks
like in this email. I checked the patch in patchwork[1], it already
right justified. But I will check again to make sure they are right
justified.

[1] https://patchwork.kernel.org/patch/9942025/

>
>> +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..?

Yes, I will save one line here in next version.

>
>> +
>> +     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!

Sine I want to keep this function to make things clear when users can
know what is going on from the function name. But if you think they
are redundant, I will remove these local values in next version.

>
> 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

OK.

>
>> +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

OK.

>
>> +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;

Make sense.

>
>> +
>> +     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

OK.

>
>> +     switch (irq_mode) {
>> +     case SPRD_DMA_NO_INT:
>> +             break;
>
> no handling?

Yes, we do not need to set any irq type enabled here.

>
>> +     case SPRD_DMA_FRAG_INT:
>> +             hw->intc |= SPRD_DMA_FRAG_INT_EN;
>> +             break;
>
> empty line after break helps readablity

OK. I will check the whole file.

>
>> +     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

OK.

>
>> +     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?

Sorry, I missing this and I will fix this in next version. Thanks for
your commnets.

-- 
Baolin.wang
Best Regards

[toc] | [prev] | [next] | [standalone]

#1739284

FromMark Brown <broonie@kernel.org>
Date2017-09-25 23:20 +0200
Message-ID<utIXf-3we-13@gated-at.bofh.it>
In reply to#1739255

[Multipart message — attachments visible in raw view] — view raw

On Tue, Sep 26, 2017 at 04:02:52AM +0800, Baolin Wang wrote:
> On 25 September 2017 at 17:47, Vinod Koul <vinod.koul@intel.com> wrote:
> > 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

> I have made these to right justified, but I do not know why it looks
> like in this email. I checked the patch in patchwork[1], it already
> right justified. But I will check again to make sure they are right
> justified.

This happens as a result of indenting with tabs - when things get quoted
or the characters get added at the start of the line you can get an
extra couple of characters on the line with no blank space but that's
less than a tab so the lines indented by tabs suddenly aren't lined up.
It'll be fine once the patch is applied.

[toc] | [prev] | [next] | [standalone]

#1739393

FromBaolin Wang <baolin.wang@linaro.org>
Date2017-09-26 02:20 +0200
Message-ID<utLLr-5oU-11@gated-at.bofh.it>
In reply to#1739284
On 26 September 2017 at 05:14, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Sep 26, 2017 at 04:02:52AM +0800, Baolin Wang wrote:
>> On 25 September 2017 at 17:47, Vinod Koul <vinod.koul@intel.com> wrote:
>> > 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
>
>> I have made these to right justified, but I do not know why it looks
>> like in this email. I checked the patch in patchwork[1], it already
>> right justified. But I will check again to make sure they are right
>> justified.
>
> This happens as a result of indenting with tabs - when things get quoted
> or the characters get added at the start of the line you can get an
> extra couple of characters on the line with no blank space but that's
> less than a tab so the lines indented by tabs suddenly aren't lined up.
> It'll be fine once the patch is applied.

Make sense. Thanks.

-- 
Baolin.wang
Best Regards

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web