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

Groups > linux.kernel > #1355540 > unrolled thread

Re: [PATCH V14 5/9] dma: qcom_hidma: implement lower level hardware interface

Started byVinod Koul <vinod.koul@intel.com>
First post2016-03-11 03:10 +0100
Last post2016-03-14 15:00 +0100
Articles 11 — 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 V14 5/9] dma: qcom_hidma: implement lower level hardware  interface Vinod Koul <vinod.koul@intel.com> - 2016-03-11 03:10 +0100
    Re: [PATCH V14 5/9] dma: qcom_hidma: implement lower level hardware  interface Okaya@codeaurora.org - 2016-03-11 04:10 +0100
    Re: [PATCH V14 5/9] dma: qcom_hidma: implement lower level hardware  interface Sinan Kaya <okaya@codeaurora.org> - 2016-03-11 17:10 +0100
      Re: [PATCH V14 5/9] dma: qcom_hidma: implement lower level hardware  interface Vinod Koul <vinod.koul@intel.com> - 2016-03-11 17:30 +0100
        Re: [PATCH V14 5/9] dma: qcom_hidma: implement lower level hardware  interface Sinan Kaya <okaya@codeaurora.org> - 2016-03-11 17:50 +0100
          Re: [PATCH V14 5/9] dma: qcom_hidma: implement lower level hardware  interface Sinan Kaya <okaya@codeaurora.org> - 2016-03-11 20:30 +0100
            Re: [PATCH V14 5/9] dma: qcom_hidma: implement lower level hardware  interface Sinan Kaya <okaya@codeaurora.org> - 2016-03-11 23:00 +0100
              Re: [PATCH V14 5/9] dma: qcom_hidma: implement lower level hardware  interface Vinod Koul <vinod.koul@intel.com> - 2016-03-13 17:00 +0100
                Re: [PATCH V14 5/9] dma: qcom_hidma: implement lower level hardware  interface Sinan Kaya <okaya@codeaurora.org> - 2016-03-14 15:00 +0100
            Re: [PATCH V14 5/9] dma: qcom_hidma: implement lower level hardware  interface Vinod Koul <vinod.koul@intel.com> - 2016-03-13 17:00 +0100
              Re: [PATCH V14 5/9] dma: qcom_hidma: implement lower level hardware  interface Sinan Kaya <okaya@codeaurora.org> - 2016-03-14 15:00 +0100

#1355540 — Re: [PATCH V14 5/9] dma: qcom_hidma: implement lower level hardware interface

FromVinod Koul <vinod.koul@intel.com>
Date2016-03-11 03:10 +0100
SubjectRe: [PATCH V14 5/9] dma: qcom_hidma: implement lower level hardware interface
Message-ID<rbkGC-7yM-9@gated-at.bofh.it>
On Thu, Feb 04, 2016 at 11:34:36PM -0500, Sinan Kaya wrote:

> +
> +#define EVRE_SIZE			16	/* each EVRE is 16 bytes */
> +
> +#define TRCA_CTRLSTS_OFFSET		0x000
> +#define TRCA_RING_LOW_OFFSET		0x008
> +#define TRCA_RING_HIGH_OFFSET		0x00C
> +#define TRCA_RING_LEN_OFFSET		0x010
> +#define TRCA_READ_PTR_OFFSET		0x018
> +#define TRCA_WRITE_PTR_OFFSET		0x020
> +#define TRCA_DOORBELL_OFFSET		0x400
> +
> +#define EVCA_CTRLSTS_OFFSET		0x000
> +#define EVCA_INTCTRL_OFFSET		0x004
> +#define EVCA_RING_LOW_OFFSET		0x008
> +#define EVCA_RING_HIGH_OFFSET		0x00C
> +#define EVCA_RING_LEN_OFFSET		0x010
> +#define EVCA_READ_PTR_OFFSET		0x018
> +#define EVCA_WRITE_PTR_OFFSET		0x020
> +#define EVCA_DOORBELL_OFFSET		0x400
> +
> +#define EVCA_IRQ_STAT_OFFSET		0x100
> +#define EVCA_IRQ_CLR_OFFSET		0x108
> +#define EVCA_IRQ_EN_OFFSET		0x110
> +
> +#define EVRE_CFG_IDX			0
> +#define EVRE_LEN_IDX			1
> +#define EVRE_DEST_LOW_IDX		2
> +#define EVRE_DEST_HI_IDX		3
> +
> +#define EVRE_ERRINFO_BIT_POS		24
> +#define EVRE_CODE_BIT_POS		28
> +
> +#define EVRE_ERRINFO_MASK		GENMASK(3, 0)
> +#define EVRE_CODE_MASK			GENMASK(3, 0)

These are rest here are not namespace properly...

> +static int hidma_ll_enable(struct hidma_lldev *lldev)
> +{
> +	u32 val;
> +	int ret;
> +
> +	val = readl(lldev->evca + EVCA_CTRLSTS_OFFSET);
> +	val &= ~(CH_CONTROL_MASK << 16);
> +	val |= CH_ENABLE << 16;
> +	writel(val, lldev->evca + EVCA_CTRLSTS_OFFSET);
> +
> +	ret = readl_poll_timeout(lldev->evca + EVCA_CTRLSTS_OFFSET, val,
> +				 (HIDMA_CH_STATE(val) == CH_ENABLED) ||
> +				 (HIDMA_CH_STATE(val) == CH_RUNNING), 1000,
> +				 10000);
> +	if (ret) {
> +		dev_err(lldev->dev, "event channel did not get enabled\n");
> +		return ret;
> +	}
> +
> +	val = readl(lldev->trca + TRCA_CTRLSTS_OFFSET);
> +	val &= ~(CH_CONTROL_MASK << 16);
> +	val |= CH_ENABLE << 16;
> +	writel(val, lldev->trca + TRCA_CTRLSTS_OFFSET);
> +
> +	ret = readl_poll_timeout(lldev->trca + TRCA_CTRLSTS_OFFSET, val,
> +				 (HIDMA_CH_STATE(val) == CH_ENABLED) ||
> +				 (HIDMA_CH_STATE(val) == CH_RUNNING), 1000,
> +				 10000);

first arg for readl_poll_timeout is accessor fn to do read, which doesnt
seem to be here... so what did i miss?


> +void hidma_ll_queue_request(struct hidma_lldev *lldev, u32 tre_ch)
> +{
> +	struct hidma_tre *tre;
> +	unsigned long flags;
> +
> +	tre = &lldev->trepool[tre_ch];
> +
> +	/* copy the TRE into its location in the TRE ring */
> +	spin_lock_irqsave(&lldev->lock, flags);
> +	tre->tre_index = lldev->tre_write_offset / TRE_SIZE;
> +	lldev->pending_tre_list[tre->tre_index] = tre;
> +	memcpy(lldev->tre_ring + lldev->tre_write_offset, &tre->tre_local[0],
> +	       TRE_SIZE);
> +	lldev->tx_status_list[tre->idx].err_code = 0;
> +	lldev->tx_status_list[tre->idx].err_info = 0;
> +	tre->queued = 1;
> +	lldev->pending_tre_count++;
> +	lldev->tre_write_offset = (lldev->tre_write_offset + TRE_SIZE)
> +	    % lldev->tre_ring_size;

These and above one should be right justfied per coding style

-- 
~Vinod

[toc] | [next] | [standalone]

#1355563

FromOkaya@codeaurora.org
Date2016-03-11 04:10 +0100
Message-ID<rblCG-8hC-17@gated-at.bofh.it>
In reply to#1355540
> On Thu, Feb 04, 2016 at 11:34:36PM -0500, Sinan Kaya wrote:
>
>> +
>> +#define EVRE_SIZE			16	/* each EVRE is 16 bytes */
>> +
>> +#define TRCA_CTRLSTS_OFFSET		0x000
>> +#define TRCA_RING_LOW_OFFSET		0x008
>> +#define TRCA_RING_HIGH_OFFSET		0x00C
>> +#define TRCA_RING_LEN_OFFSET		0x010
>> +#define TRCA_READ_PTR_OFFSET		0x018
>> +#define TRCA_WRITE_PTR_OFFSET		0x020
>> +#define TRCA_DOORBELL_OFFSET		0x400
>> +
>> +#define EVCA_CTRLSTS_OFFSET		0x000
>> +#define EVCA_INTCTRL_OFFSET		0x004
>> +#define EVCA_RING_LOW_OFFSET		0x008
>> +#define EVCA_RING_HIGH_OFFSET		0x00C
>> +#define EVCA_RING_LEN_OFFSET		0x010
>> +#define EVCA_READ_PTR_OFFSET		0x018
>> +#define EVCA_WRITE_PTR_OFFSET		0x020
>> +#define EVCA_DOORBELL_OFFSET		0x400
>> +
>> +#define EVCA_IRQ_STAT_OFFSET		0x100
>> +#define EVCA_IRQ_CLR_OFFSET		0x108
>> +#define EVCA_IRQ_EN_OFFSET		0x110
>> +
>> +#define EVRE_CFG_IDX			0
>> +#define EVRE_LEN_IDX			1
>> +#define EVRE_DEST_LOW_IDX		2
>> +#define EVRE_DEST_HI_IDX		3
>> +
>> +#define EVRE_ERRINFO_BIT_POS		24
>> +#define EVRE_CODE_BIT_POS		28
>> +
>> +#define EVRE_ERRINFO_MASK		GENMASK(3, 0)
>> +#define EVRE_CODE_MASK			GENMASK(3, 0)
>
> These are rest here are not namespace properly...
>
>> +static int hidma_ll_enable(struct hidma_lldev *lldev)
>> +{
>> +	u32 val;
>> +	int ret;
>> +
>> +	val = readl(lldev->evca + EVCA_CTRLSTS_OFFSET);
>> +	val &= ~(CH_CONTROL_MASK << 16);
>> +	val |= CH_ENABLE << 16;
>> +	writel(val, lldev->evca + EVCA_CTRLSTS_OFFSET);
>> +
>> +	ret = readl_poll_timeout(lldev->evca + EVCA_CTRLSTS_OFFSET, val,
>> +				 (HIDMA_CH_STATE(val) == CH_ENABLED) ||
>> +				 (HIDMA_CH_STATE(val) == CH_RUNNING), 1000,
>> +				 10000);
>> +	if (ret) {
>> +		dev_err(lldev->dev, "event channel did not get enabled\n");
>> +		return ret;
>> +	}
>> +
>> +	val = readl(lldev->trca + TRCA_CTRLSTS_OFFSET);
>> +	val &= ~(CH_CONTROL_MASK << 16);
>> +	val |= CH_ENABLE << 16;
>> +	writel(val, lldev->trca + TRCA_CTRLSTS_OFFSET);
>> +
>> +	ret = readl_poll_timeout(lldev->trca + TRCA_CTRLSTS_OFFSET, val,
>> +				 (HIDMA_CH_STATE(val) == CH_ENABLED) ||
>> +				 (HIDMA_CH_STATE(val) == CH_RUNNING), 1000,
>> +				 10000);
>
> first arg for readl_poll_timeout is accessor fn to do read, which doesnt
> seem to be here... so what did i miss?
>

First argument is the address to read.

#define readl_poll_timeout(addr, val, cond, delay_us, timeout_us) \
         readx_poll_timeout(readl, addr, val, cond, delay_us, timeout_us)


>
>> +void hidma_ll_queue_request(struct hidma_lldev *lldev, u32 tre_ch)
>> +{
>> +	struct hidma_tre *tre;
>> +	unsigned long flags;
>> +
>> +	tre = &lldev->trepool[tre_ch];
>> +
>> +	/* copy the TRE into its location in the TRE ring */
>> +	spin_lock_irqsave(&lldev->lock, flags);
>> +	tre->tre_index = lldev->tre_write_offset / TRE_SIZE;
>> +	lldev->pending_tre_list[tre->tre_index] = tre;
>> +	memcpy(lldev->tre_ring + lldev->tre_write_offset, &tre->tre_local[0],
>> +	       TRE_SIZE);
>> +	lldev->tx_status_list[tre->idx].err_code = 0;
>> +	lldev->tx_status_list[tre->idx].err_info = 0;
>> +	tre->queued = 1;
>> +	lldev->pending_tre_count++;
>> +	lldev->tre_write_offset = (lldev->tre_write_offset + TRE_SIZE)
>> +	    % lldev->tre_ring_size;
>
> These and above one should be right justfied per coding style
>
> --
> ~Vinod
>

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

#1356024

FromSinan Kaya <okaya@codeaurora.org>
Date2016-03-11 17:10 +0100
Message-ID<rbxNw-hy-13@gated-at.bofh.it>
In reply to#1355540
On 3/10/2016 9:06 PM, Vinod Koul wrote:
> On Thu, Feb 04, 2016 at 11:34:36PM -0500, Sinan Kaya wrote:
> 
>> +
>> +#define EVRE_SIZE			16	/* each EVRE is 16 bytes */
>> +
>> +#define TRCA_CTRLSTS_OFFSET		0x000
>> +#define TRCA_RING_LOW_OFFSET		0x008
>> +#define TRCA_RING_HIGH_OFFSET		0x00C
>> +#define TRCA_RING_LEN_OFFSET		0x010
>> +#define TRCA_READ_PTR_OFFSET		0x018
>> +#define TRCA_WRITE_PTR_OFFSET		0x020
>> +#define TRCA_DOORBELL_OFFSET		0x400
>> +
>> +#define EVCA_CTRLSTS_OFFSET		0x000
>> +#define EVCA_INTCTRL_OFFSET		0x004
>> +#define EVCA_RING_LOW_OFFSET		0x008
>> +#define EVCA_RING_HIGH_OFFSET		0x00C
>> +#define EVCA_RING_LEN_OFFSET		0x010
>> +#define EVCA_READ_PTR_OFFSET		0x018
>> +#define EVCA_WRITE_PTR_OFFSET		0x020
>> +#define EVCA_DOORBELL_OFFSET		0x400
>> +
>> +#define EVCA_IRQ_STAT_OFFSET		0x100
>> +#define EVCA_IRQ_CLR_OFFSET		0x108
>> +#define EVCA_IRQ_EN_OFFSET		0x110
>> +
>> +#define EVRE_CFG_IDX			0
>> +#define EVRE_LEN_IDX			1
>> +#define EVRE_DEST_LOW_IDX		2
>> +#define EVRE_DEST_HI_IDX		3
>> +
>> +#define EVRE_ERRINFO_BIT_POS		24
>> +#define EVRE_CODE_BIT_POS		28
>> +
>> +#define EVRE_ERRINFO_MASK		GENMASK(3, 0)
>> +#define EVRE_CODE_MASK			GENMASK(3, 0)
> 
> These are rest here are not namespace properly...

If I understood it right, you want me to prefix them with as HIDMA_xyz.
Correct?

> 
>> +static int hidma_ll_enable(struct hidma_lldev *lldev)
>> +{
>> +	u32 val;
>> +	int ret;
>> +
>> +	val = readl(lldev->evca + EVCA_CTRLSTS_OFFSET);
>> +	val &= ~(CH_CONTROL_MASK << 16);
>> +	val |= CH_ENABLE << 16;
>> +	writel(val, lldev->evca + EVCA_CTRLSTS_OFFSET);
>> +
>> +	ret = readl_poll_timeout(lldev->evca + EVCA_CTRLSTS_OFFSET, val,
>> +				 (HIDMA_CH_STATE(val) == CH_ENABLED) ||
>> +				 (HIDMA_CH_STATE(val) == CH_RUNNING), 1000,
>> +				 10000);
>> +	if (ret) {
>> +		dev_err(lldev->dev, "event channel did not get enabled\n");
>> +		return ret;
>> +	}
>> +
>> +	val = readl(lldev->trca + TRCA_CTRLSTS_OFFSET);
>> +	val &= ~(CH_CONTROL_MASK << 16);
>> +	val |= CH_ENABLE << 16;
>> +	writel(val, lldev->trca + TRCA_CTRLSTS_OFFSET);
>> +
>> +	ret = readl_poll_timeout(lldev->trca + TRCA_CTRLSTS_OFFSET, val,
>> +				 (HIDMA_CH_STATE(val) == CH_ENABLED) ||
>> +				 (HIDMA_CH_STATE(val) == CH_RUNNING), 1000,
>> +				 10000);
> 
> first arg for readl_poll_timeout is accessor fn to do read, which doesnt
> seem to be here... so what did i miss?
> 
> 
>> +void hidma_ll_queue_request(struct hidma_lldev *lldev, u32 tre_ch)
>> +{
>> +	struct hidma_tre *tre;
>> +	unsigned long flags;
>> +
>> +	tre = &lldev->trepool[tre_ch];
>> +
>> +	/* copy the TRE into its location in the TRE ring */
>> +	spin_lock_irqsave(&lldev->lock, flags);
>> +	tre->tre_index = lldev->tre_write_offset / TRE_SIZE;
>> +	lldev->pending_tre_list[tre->tre_index] = tre;
>> +	memcpy(lldev->tre_ring + lldev->tre_write_offset, &tre->tre_local[0],
>> +	       TRE_SIZE);
>> +	lldev->tx_status_list[tre->idx].err_code = 0;
>> +	lldev->tx_status_list[tre->idx].err_info = 0;
>> +	tre->queued = 1;
>> +	lldev->pending_tre_count++;

Is this the only one without alignment? I couldn't understand what you mean by 
above one?

>> +	lldev->tre_write_offset = (lldev->tre_write_offset + TRE_SIZE)
>> +	    % lldev->tre_ring_size;
> 
> These and above one should be right justfied per coding style
> 




-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

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

#1356034

FromVinod Koul <vinod.koul@intel.com>
Date2016-03-11 17:30 +0100
Message-ID<rby6S-sb-11@gated-at.bofh.it>
In reply to#1356024
On Fri, Mar 11, 2016 at 11:02:36AM -0500, Sinan Kaya wrote:
> > These are rest here are not namespace properly...
> 
> If I understood it right, you want me to prefix them with as HIDMA_xyz.
> Correct?

Yes..

> >> +	/* copy the TRE into its location in the TRE ring */
> >> +	spin_lock_irqsave(&lldev->lock, flags);
> >> +	tre->tre_index = lldev->tre_write_offset / TRE_SIZE;
> >> +	lldev->pending_tre_list[tre->tre_index] = tre;
> >> +	memcpy(lldev->tre_ring + lldev->tre_write_offset, &tre->tre_local[0],
> >> +	       TRE_SIZE);

This one

> >> +	lldev->tx_status_list[tre->idx].err_code = 0;
> >> +	lldev->tx_status_list[tre->idx].err_info = 0;
> >> +	tre->queued = 1;
> >> +	lldev->pending_tre_count++;
> 
> Is this the only one without alignment? I couldn't understand what you mean by 
> above one?

quoting Coding Style:

Statements longer than 80 columns will be broken into sensible chunks, unless
exceeding 80 columns significantly increases readability and does not hide
information. Descendants are always substantially shorter than the parent and
are placed substantially to the right.


> 
> >> +	lldev->tre_write_offset = (lldev->tre_write_offset + TRE_SIZE)
> >> +	    % lldev->tre_ring_size;
> > 
> > These and above one should be right justfied per coding style

-- 
~Vinod

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

#1356056

FromSinan Kaya <okaya@codeaurora.org>
Date2016-03-11 17:50 +0100
Message-ID<rbyqe-CG-31@gated-at.bofh.it>
In reply to#1356034
On 3/11/2016 11:32 AM, Vinod Koul wrote:
>>>> memcpy(lldev->tre_ring + lldev->tre_write_offset, &tre->tre_local[0],
>>>> > >> +	       TRE_SIZE);
> This one
> 
>>>> > >> +	lldev->tx_status_list[tre->idx].err_code = 0;
>>>> > >> +	lldev->tx_status_list[tre->idx].err_info = 0;
>>>> > >> +	tre->queued = 1;
>>>> > >> +	lldev->pending_tre_count++;
>> > 
>> > Is this the only one without alignment? I couldn't understand what you mean by 
>> > above one?
> quoting Coding Style:
> 
> Statements longer than 80 columns will be broken into sensible chunks, unless
> exceeding 80 columns significantly increases readability and does not hide
> information. 

> "Descendants are always substantially shorter than the parent and
> are placed substantially to the right."
> 
> 

Sorry for my poor English. I never got this rule. 

Which one is a "substantially" right? Can you give me an example?

I need to understand how you'd write this to satisfy the above rule.

like this:

memcpy(lldev->tre_ring + lldev->tre_write_offset, &tre->tre_local[0],
       TRE_SIZE);

or

memcpy(lldev->tre_ring + lldev->tre_write_offset,
       &tre->tre_local[0], TRE_SIZE);

or

memcpy(lldev->tre_ring + lldev->tre_write_offset, &tre->tre_local[0],
							   TRE_SIZE);

or

memcpy(lldev->tre_ring + lldev->tre_write_offset,
	&tre->tre_local[0], 
	TRE_SIZE);

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

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

#1356145

FromSinan Kaya <okaya@codeaurora.org>
Date2016-03-11 20:30 +0100
Message-ID<rbAV4-2r7-3@gated-at.bofh.it>
In reply to#1356056
On 3/11/2016 11:44 AM, Sinan Kaya wrote:
> On 3/11/2016 11:32 AM, Vinod Koul wrote:
>>>>> memcpy(lldev->tre_ring + lldev->tre_write_offset, &tre->tre_local[0],
>>>>>>>> +	       TRE_SIZE);
>> This one
>>
>>>>>>>> +	lldev->tx_status_list[tre->idx].err_code = 0;
>>>>>>>> +	lldev->tx_status_list[tre->idx].err_info = 0;
>>>>>>>> +	tre->queued = 1;
>>>>>>>> +	lldev->pending_tre_count++;
>>>>
>>>> Is this the only one without alignment? I couldn't understand what you mean by 
>>>> above one?
>> quoting Coding Style:
>>
>> Statements longer than 80 columns will be broken into sensible chunks, unless
>> exceeding 80 columns significantly increases readability and does not hide
>> information. 
> 
>> "Descendants are always substantially shorter than the parent and
>> are placed substantially to the right."
>>
>>
> 
> Sorry for my poor English. I never got this rule. 
> 
> Which one is a "substantially" right? Can you give me an example?
> 
> I need to understand how you'd write this to satisfy the above rule.
> 
> like this:
> 
> memcpy(lldev->tre_ring + lldev->tre_write_offset, &tre->tre_local[0],
>        TRE_SIZE);
> 
> or
> 
> memcpy(lldev->tre_ring + lldev->tre_write_offset,
>        &tre->tre_local[0], TRE_SIZE);
> 
> or
> 
> memcpy(lldev->tre_ring + lldev->tre_write_offset, &tre->tre_local[0],
> 							   TRE_SIZE);
> 
> or
> 
> memcpy(lldev->tre_ring + lldev->tre_write_offset,
> 	&tre->tre_local[0], 
> 	TRE_SIZE);
> 
so, I looked at other examples in drivers/dma/dw/core.c file...

I'm seeing two different patterns in the code. One pattern is to align
the next line to the first character of the first line like I did based
on the previous review comments.

	mem_width = min_t(unsigned int,
                          data_width, dwc_fast_ffs(mem | len));

The second example places an extra tab like this.

	list_add_tail(&desc->desc_node,
			&first->tx_list);


Based on this example: this is how I'm changing the second one

+       lldev->tre_write_offset = (lldev->tre_write_offset + HIDMA_TRE_SIZE)
+                                       % lldev->tre_ring_size;
+

I'm still not sure what you want to do with this:

Is this what you want to do ?
        memcpy(lldev->tre_ring + lldev->tre_write_offset, &tre->tre_local[0],
-              HIDMA_TRE_SIZE);
+               HIDMA_TRE_SIZE);

I also got flagged before that HIDMA_TRE_SIZE does not start from the first 
character.

I have done the renaming. This is all left for me to post a follow up.


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

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

#1356210

FromSinan Kaya <okaya@codeaurora.org>
Date2016-03-11 23:00 +0100
Message-ID<rbDge-48r-17@gated-at.bofh.it>
In reply to#1356145
Vinod,

On 3/11/2016 2:29 PM, Sinan Kaya wrote:
> I have done the renaming. This is all left for me to post a follow up.

I just posted v15 (forgot to put v15 on 4/4).

http://www.spinics.net/lists/arm-kernel/msg490282.html

I wanted to get v15 out before the end of the week with the macro renames. 
Let me know which lines need corrections.

There is also a single line change for the object hierarchy patch. The code 
was looking up DMA attributes (OF:dma-coherent) from the wrong place.

Thanks and have a good weekend,
Sinan

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

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

#1356695

FromVinod Koul <vinod.koul@intel.com>
Date2016-03-13 17:00 +0100
Message-ID<rcgAW-7Is-9@gated-at.bofh.it>
In reply to#1356210
On Fri, Mar 11, 2016 at 04:59:23PM -0500, Sinan Kaya wrote:
> Vinod,
> 
> On 3/11/2016 2:29 PM, Sinan Kaya wrote:
> > I have done the renaming. This is all left for me to post a follow up.
> 
> I just posted v15 (forgot to put v15 on 4/4).

Don't do that manually!

git formt-patch --subject-prefix is for that

> 
> http://www.spinics.net/lists/arm-kernel/msg490282.html
> 
> I wanted to get v15 out before the end of the week with the macro renames. 
> Let me know which lines need corrections.
> 
> There is also a single line change for the object hierarchy patch. The code 
> was looking up DMA attributes (OF:dma-coherent) from the wrong place.

-- 
~Vinod

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

#1357251

FromSinan Kaya <okaya@codeaurora.org>
Date2016-03-14 15:00 +0100
Message-ID<rcBco-4sZ-37@gated-at.bofh.it>
In reply to#1356695
On 3/13/2016 12:00 PM, Vinod Koul wrote:
> On Fri, Mar 11, 2016 at 04:59:23PM -0500, Sinan Kaya wrote:
>> Vinod,
>>
>> On 3/11/2016 2:29 PM, Sinan Kaya wrote:
>>> I have done the renaming. This is all left for me to post a follow up.
>>
>> I just posted v15 (forgot to put v15 on 4/4).
> 
> Don't do that manually!
> 
> git formt-patch --subject-prefix is for that
> 

Thanks for the hint. I was using the --annotate option and adding version numbers.

>>
>> http://www.spinics.net/lists/arm-kernel/msg490282.html
>>
>> I wanted to get v15 out before the end of the week with the macro renames. 
>> Let me know which lines need corrections.
>>
>> There is also a single line change for the object hierarchy patch. The code 
>> was looking up DMA attributes (OF:dma-coherent) from the wrong place.
> 


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

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

#1356693

FromVinod Koul <vinod.koul@intel.com>
Date2016-03-13 17:00 +0100
Message-ID<rcgAV-7Is-1@gated-at.bofh.it>
In reply to#1356145
On Fri, Mar 11, 2016 at 02:29:41PM -0500, Sinan Kaya wrote:
> On 3/11/2016 11:44 AM, Sinan Kaya wrote:
> > On 3/11/2016 11:32 AM, Vinod Koul wrote:
> >>>>> memcpy(lldev->tre_ring + lldev->tre_write_offset, &tre->tre_local[0],
> >>>>>>>> +	       TRE_SIZE);
> >> This one

I would write this as:

memcpy(lldev->tre_ring + lldev->tre_write_offset,
		&tre->tre_local[0], TRE_SIZE);

To make it look more readable

> >>
> >>>>>>>> +	lldev->tx_status_list[tre->idx].err_code = 0;
> >>>>>>>> +	lldev->tx_status_list[tre->idx].err_info = 0;
> >>>>>>>> +	tre->queued = 1;
> >>>>>>>> +	lldev->pending_tre_count++;
> >>>>
> >>>> Is this the only one without alignment? I couldn't understand what you mean by 
> >>>> above one?
> >> quoting Coding Style:
> >>
> >> Statements longer than 80 columns will be broken into sensible chunks, unless
> >> exceeding 80 columns significantly increases readability and does not hide
> >> information. 
> > 
> >> "Descendants are always substantially shorter than the parent and
> >> are placed substantially to the right."
> >>
> >>
> > 
> > Sorry for my poor English. I never got this rule. 
> > 
> > Which one is a "substantially" right? Can you give me an example?
> > 
> > I need to understand how you'd write this to satisfy the above rule.
> > 
> > like this:
> > 
> > memcpy(lldev->tre_ring + lldev->tre_write_offset, &tre->tre_local[0],
> >        TRE_SIZE);

No

> > 
> > or
> > 
> > memcpy(lldev->tre_ring + lldev->tre_write_offset,
> >        &tre->tre_local[0], TRE_SIZE);

Better or above

> > 
> > or
> > 
> > memcpy(lldev->tre_ring + lldev->tre_write_offset, &tre->tre_local[0],
> > 							   TRE_SIZE);

This doesnt look very readable IMHO, mostly try to use common sense and if
it looks good and easy to read then you might have nailed it :)


> > 
> > or
> > 
> > memcpy(lldev->tre_ring + lldev->tre_write_offset,
> > 	&tre->tre_local[0], 
> > 	TRE_SIZE);
> > 
> so, I looked at other examples in drivers/dma/dw/core.c file...
> 
> I'm seeing two different patterns in the code. One pattern is to align
> the next line to the first character of the first line like I did based
> on the previous review comments.
> 
> 	mem_width = min_t(unsigned int,
>                           data_width, dwc_fast_ffs(mem | len));
> 
> The second example places an extra tab like this.
> 
> 	list_add_tail(&desc->desc_node,
> 			&first->tx_list);
> 
> 
> Based on this example: this is how I'm changing the second one
> 
> +       lldev->tre_write_offset = (lldev->tre_write_offset + HIDMA_TRE_SIZE)
> +                                       % lldev->tre_ring_size;
> +
> 
> I'm still not sure what you want to do with this:
> 
> Is this what you want to do ?
>         memcpy(lldev->tre_ring + lldev->tre_write_offset, &tre->tre_local[0],
> -              HIDMA_TRE_SIZE);
> +               HIDMA_TRE_SIZE);
> 
> I also got flagged before that HIDMA_TRE_SIZE does not start from the first 
> character.
> 
> I have done the renaming. This is all left for me to post a follow up.

OK

-- 
~Vinod

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

#1357248

FromSinan Kaya <okaya@codeaurora.org>
Date2016-03-14 15:00 +0100
Message-ID<rcBcn-4sZ-27@gated-at.bofh.it>
In reply to#1356693
Hi Vinod,

On 3/13/2016 11:59 AM, Vinod Koul wrote:
> I would write this as:
> 
> memcpy(lldev->tre_ring + lldev->tre_write_offset,
> 		&tre->tre_local[0], TRE_SIZE);
> 
> To make it look more readable

Got it.

> 
>>>> > >>
>>>>>>>>>> > >>>>>>>> +	lldev->tx_status_list[tre->idx].err_code = 0;
>>>>>>>>>> > >>>>>>>> +	lldev->tx_status_list[tre->idx].err_info = 0;
>>>>>>>>>> > >>>>>>>> +	tre->queued = 1;
>>>>>>>>>> > >>>>>>>> +	lldev->pending_tre_count++;
>>>>>> > >>>>
>>>>>> > >>>> Is this the only one without alignment? I couldn't understand what you mean by 
>>>>>> > >>>> above one?
>>>> > >> quoting Coding Style:
>>>> > >>
>>>> > >> Statements longer than 80 columns will be broken into sensible chunks, unless
>>>> > >> exceeding 80 columns significantly increases readability and does not hide
>>>> > >> information. 
>>> > > 
>>>> > >> "Descendants are always substantially shorter than the parent and
>>>> > >> are placed substantially to the right."
>>>> > >>
>>>> > >>
>>> > > 
>>> > > Sorry for my poor English. I never got this rule. 
>>> > > 
>>> > > Which one is a "substantially" right? Can you give me an example?
>>> > > 
>>> > > I need to understand how you'd write this to satisfy the above rule.
>>> > > 
>>> > > like this:
>>> > > 
>>> > > memcpy(lldev->tre_ring + lldev->tre_write_offset, &tre->tre_local[0],
>>> > >        TRE_SIZE);
> No
> 
>>> > > 
>>> > > or
>>> > > 
>>> > > memcpy(lldev->tre_ring + lldev->tre_write_offset,
>>> > >        &tre->tre_local[0], TRE_SIZE);
> Better or above
> 

I'll do this to be consistent with the rest of the code. 

Please let me know when you want to have the updated version. I don't want to post v16
while you are reviewing the rest of the code.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web