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

Groups > linux.kernel > #1387117 > unrolled thread

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

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 V17 1/3] dmaengine: qcom_hidma: implement lower level  hardware interface Vinod Koul <vinod.koul@intel.com> - 2016-04-26 05:30 +0200
    Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level  hardware interface Sinan Kaya <okaya@codeaurora.org> - 2016-04-26 17:10 +0200
      Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level  hardware interface Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-04-26 17:20 +0200
        Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level  hardware interface Sinan Kaya <okaya@codeaurora.org> - 2016-04-26 17:30 +0200
      Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level  hardware interface Vinod Koul <vinod.koul@intel.com> - 2016-04-26 18:20 +0200
        Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level  hardware interface Sinan Kaya <okaya@codeaurora.org> - 2016-04-28 21:40 +0200
          Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level  hardware interface Sinan Kaya <okaya@codeaurora.org> - 2016-05-01 06:40 +0200

#1387117 — Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

On Mon, Apr 11, 2016 at 10:21:11AM -0400, Sinan Kaya wrote:

> + * HIDMA is not aware of IOMMU presence since it follows the DMA API. All
> + * IOMMU latency will be built into the data movement time. By the time
> + * interrupt happens, IOMMU lookups + data movement has already taken place.

Do you mean dmaengine API or dma mapping API here? Where is you IOMMU
located wrt to dma controller?

> + *
> + * While the first read in a typical PCI endpoint ISR flushes all outstanding
> + * requests traditionally to the destination, this concept does not apply
> + * here for this HW.
> + */
> +static void hidma_ll_int_handler_internal(struct hidma_lldev *lldev)
> +{
> +	u32 status;
> +	u32 enable;
> +	u32 cause;
> +
> +	/*
> +	 * Fine tuned for this HW...
> +	 *
> +	 * This ISR has been designed for this particular hardware. Relaxed
> +	 * read and write accessors are used for performance reasons due to
> +	 * interrupt delivery guarantees. Do not copy this code blindly and
> +	 * expect that to work.
> +	 */
> +	status = readl_relaxed(lldev->evca + HIDMA_EVCA_IRQ_STAT_REG);
> +	enable = readl_relaxed(lldev->evca + HIDMA_EVCA_IRQ_EN_REG);
> +	cause = status & enable;
> +
> +	while (cause) {
> +		if ((cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS)) ||
> +		    (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) ||
> +		    (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS)) ||
> +		    (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS)) ||
> +		    (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))) {

Switch please

> +			u8 err_code = HIDMA_EVRE_STATUS_ERROR;
> +			u8 err_info = 0xFF;
> +
> +			/* Clear out pending interrupts */
> +			writel(cause, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
> +
> +			dev_err(lldev->dev, "error 0x%x, resetting...\n",
> +				cause);
> +
> +			hidma_cleanup_pending_tre(lldev, err_info, err_code);
> +
> +			/* reset the channel for recovery */
> +			if (hidma_ll_setup(lldev)) {

should this be done in ISR?

> +int hidma_ll_resume(struct hidma_lldev *lldev)
> +{
> +	return hidma_ll_enable(lldev);
> +}

why do we need this empty function, use hidma_ll_enable.

> +bool hidma_ll_isenabled(struct hidma_lldev *lldev)
> +{
> +	u32 val;
> +
> +	val = readl(lldev->trca + HIDMA_TRCA_CTRLSTS_REG);
> +	lldev->trch_state = HIDMA_CH_STATE(val);
> +	val = readl(lldev->evca + HIDMA_EVCA_CTRLSTS_REG);
> +	lldev->evch_state = HIDMA_CH_STATE(val);
> +
> +	/* both channels have to be enabled before calling this function */
> +	if (((lldev->trch_state == HIDMA_CH_ENABLED) ||
> +	     (lldev->trch_state == HIDMA_CH_RUNNING)) &&
> +	    ((lldev->evch_state == HIDMA_CH_ENABLED) ||
> +	     (lldev->evch_state == HIDMA_CH_RUNNING)))
> +		return true;

hmmm this looks hard to read, why not do:

is_chan_enabled(state)
{
	switch (state) {
	case HIDMA_CH_ENABLED:
	case HIDMA_CH_RUNNING:
		return true;
	default:
		return false;
}

and then :

	if (is_chan_enabled(lldev->trch_state) &&
			is_chan_enabled(lldev->evch_state))


> +void hidma_ll_start(struct hidma_lldev *lldev)
> +{
> +	hidma_ll_hw_start(lldev);
> +}

Another dummy :(

> +/*
> + * Note that even though we stop this channel
> + * if there is a pending transaction in flight
> + * it will complete and follow the callback.
> + * This request will prevent further requests
> + * to be made.

Why the odd formating?

> +int hidma_ll_uninit(struct hidma_lldev *lldev)
> +{
> +	int rc = 0;
> +	u32 val;
> +
> +	if (!lldev)
> +		return -ENODEV;
> +
> +	if (lldev->initialized) {
> +		u32 required_bytes;
> +
> +		lldev->initialized = 0;
> +
> +		required_bytes = sizeof(struct hidma_tre) * lldev->nr_tres;
> +		tasklet_kill(&lldev->task);
> +		memset(lldev->trepool, 0, required_bytes);
> +		lldev->trepool = NULL;
> +		lldev->pending_tre_count = 0;
> +		lldev->tre_write_offset = 0;
> +
> +		rc = hidma_ll_reset(lldev);
> +
> +		/*
> +		 * Clear all pending interrupts again.
> +		 * Otherwise, we observe reset complete interrupts.
> +		 */
> +		val = readl(lldev->evca + HIDMA_EVCA_IRQ_STAT_REG);
> +		writel(val, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
> +		hidma_ll_enable_irq(lldev, 0);

uninit enables irq?

-- 
~Vinod

[toc] | [next] | [standalone]

#1387600

On 4/25/2016 11:28 PM, Vinod Koul wrote:
> On Mon, Apr 11, 2016 at 10:21:11AM -0400, Sinan Kaya wrote:
> 
>> + * HIDMA is not aware of IOMMU presence since it follows the DMA API. All
>> + * IOMMU latency will be built into the data movement time. By the time
>> + * interrupt happens, IOMMU lookups + data movement has already taken place.
> 
> Do you mean dmaengine API or dma mapping API here? Where is you IOMMU
> located wrt to dma controller?

I mean DMA API in this context. The code works on dma_addr_t. That means the caller
takes care of DMA mapping before coming to this code. This code is not involved in
any of the DMA mapping calls or any other IOMMU related operations. All of this 
is abstracted from this code.

The IOMMU is right in front of this device. That's why, all IOMMU operations are 
inclusive into the data transfers. 

> 
>> + *
>> + * While the first read in a typical PCI endpoint ISR flushes all outstanding
>> + * requests traditionally to the destination, this concept does not apply
>> + * here for this HW.
>> + */
>> +static void hidma_ll_int_handler_internal(struct hidma_lldev *lldev)
>> +{
>> +	u32 status;
>> +	u32 enable;
>> +	u32 cause;
>> +
>> +	/*
>> +	 * Fine tuned for this HW...
>> +	 *
>> +	 * This ISR has been designed for this particular hardware. Relaxed
>> +	 * read and write accessors are used for performance reasons due to
>> +	 * interrupt delivery guarantees. Do not copy this code blindly and
>> +	 * expect that to work.
>> +	 */
>> +	status = readl_relaxed(lldev->evca + HIDMA_EVCA_IRQ_STAT_REG);
>> +	enable = readl_relaxed(lldev->evca + HIDMA_EVCA_IRQ_EN_REG);
>> +	cause = status & enable;
>> +
>> +	while (cause) {
>> +		if ((cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS)) ||
>> +		    (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) ||
>> +		    (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS)) ||
>> +		    (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS)) ||
>> +		    (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))) {
> 
> Switch please

Cause is a combined status register. Let's say it contains 0x41. I need to check
if bit 0 or bit 6 is set in this value for each case condition. The value is not 0x40 
or 0x1. 

I created macro like this instead.

+#define HIDMA_IS_ERR_INTERRUPT(cause)                          \
+       (cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS))   || \
+       (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) || \
+       (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS))       || \
+       (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS))    || \
+       (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))

and replaced the if statement as follows

if (HIDMA_IS_ERR_INTERRUPT(cause)) {

> 
>> +			u8 err_code = HIDMA_EVRE_STATUS_ERROR;
>> +			u8 err_info = 0xFF;
>> +
>> +			/* Clear out pending interrupts */
>> +			writel(cause, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
>> +
>> +			dev_err(lldev->dev, "error 0x%x, resetting...\n",
>> +				cause);
>> +
>> +			hidma_cleanup_pending_tre(lldev, err_info, err_code);
>> +
>> +			/* reset the channel for recovery */
>> +			if (hidma_ll_setup(lldev)) {
> 
> should this be done in ISR?

I created a new tasklet called rst_task and posted the code there. 

 /*
+ * Abort all transactions and perform a reset.
+ */
+static void hidma_ll_abort(unsigned long arg)
+{
+       u8 err_code = HIDMA_EVRE_STATUS_ERROR;
+       u8 err_info = 0xFF;
+
+       dev_err(lldev->dev, "error 0x%x, resetting...\n",
+               cause);
+
+       hidma_cleanup_pending_tre(lldev, err_info, err_code);
+
+       /* reset the channel for recovery */
+       if (hidma_ll_setup(lldev)) {
+               dev_err(lldev->dev,
+                       "channel reinitialize failed after error\n");
+               return;
+       }
+       hidma_ll_control_irq(lldev, ENABLE_IRQS);
+}

+               if (HIDMA_IS_ERR_INTERRUPT(cause)) {
                        /* Clear out pending interrupts */
                        writel(cause, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);

-                       dev_err(lldev->dev, "error 0x%x, resetting...\n",
-                               cause);
-
-                       hidma_cleanup_pending_tre(lldev, err_info, err_code);
-
-                       /* reset the channel for recovery */
-                       if (hidma_ll_setup(lldev)) {
-                               dev_err(lldev->dev,
-                                       "channel reinitialize failed after error\n");
-                               return;
-                       }
-                       hidma_ll_enable_irq(lldev, ENABLE_IRQS);
+                       tasklet_schedule(&lldev->rst_task);
 



> 
>> +int hidma_ll_resume(struct hidma_lldev *lldev)
>> +{
>> +	return hidma_ll_enable(lldev);
>> +}
> 
> why do we need this empty function, use hidma_ll_enable.

hidma_ll_enable is a common function that gets called from multiple places. 
hidma_ll_resume and hidma_ll_pause is used by the OS interface for pausing
and resuming the DMA channel. 

> 
>> +bool hidma_ll_isenabled(struct hidma_lldev *lldev)
>> +{
>> +	u32 val;
>> +
>> +	val = readl(lldev->trca + HIDMA_TRCA_CTRLSTS_REG);
>> +	lldev->trch_state = HIDMA_CH_STATE(val);
>> +	val = readl(lldev->evca + HIDMA_EVCA_CTRLSTS_REG);
>> +	lldev->evch_state = HIDMA_CH_STATE(val);
>> +
>> +	/* both channels have to be enabled before calling this function */
>> +	if (((lldev->trch_state == HIDMA_CH_ENABLED) ||
>> +	     (lldev->trch_state == HIDMA_CH_RUNNING)) &&
>> +	    ((lldev->evch_state == HIDMA_CH_ENABLED) ||
>> +	     (lldev->evch_state == HIDMA_CH_RUNNING)))
>> +		return true;
> 
> hmmm this looks hard to read, why not do:
> 
> is_chan_enabled(state)
> {
> 	switch (state) {
> 	case HIDMA_CH_ENABLED:
> 	case HIDMA_CH_RUNNING:
> 		return true;
> 	default:
> 		return false;
> }
> 
> and then :
> 
> 	if (is_chan_enabled(lldev->trch_state) &&
> 			is_chan_enabled(lldev->evch_state))
> 
Sure, makes sense.

> 
>> +void hidma_ll_start(struct hidma_lldev *lldev)
>> +{
>> +	hidma_ll_hw_start(lldev);
>> +}
> 
> Another dummy :(

renamed hidma_ll_hw_start to hidma_ll_start and removed this function.

> 
>> +/*
>> + * Note that even though we stop this channel
>> + * if there is a pending transaction in flight
>> + * it will complete and follow the callback.
>> + * This request will prevent further requests
>> + * to be made.
> 
> Why the odd formating?

aligned to 75 characters.

> 
>> +int hidma_ll_uninit(struct hidma_lldev *lldev)
>> +{
>> +	int rc = 0;
>> +	u32 val;
>> +
>> +	if (!lldev)
>> +		return -ENODEV;
>> +
>> +	if (lldev->initialized) {
>> +		u32 required_bytes;
>> +
>> +		lldev->initialized = 0;
>> +
>> +		required_bytes = sizeof(struct hidma_tre) * lldev->nr_tres;
>> +		tasklet_kill(&lldev->task);
>> +		memset(lldev->trepool, 0, required_bytes);
>> +		lldev->trepool = NULL;
>> +		lldev->pending_tre_count = 0;
>> +		lldev->tre_write_offset = 0;
>> +
>> +		rc = hidma_ll_reset(lldev);
>> +
>> +		/*
>> +		 * Clear all pending interrupts again.
>> +		 * Otherwise, we observe reset complete interrupts.
>> +		 */
>> +		val = readl(lldev->evca + HIDMA_EVCA_IRQ_STAT_REG);
>> +		writel(val, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
>> +		hidma_ll_enable_irq(lldev, 0);
> 
> uninit enables irq?
> 
renamed to hidma_ll_control_irq. Second argument is the bits to enable. If 0, 
interrupts are disabled.

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

#1387614

On Tue, Apr 26, 2016 at 6:04 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> On 4/25/2016 11:28 PM, Vinod Koul wrote:
>> On Mon, Apr 11, 2016 at 10:21:11AM -0400, Sinan Kaya wrote:

>>> +    while (cause) {
>>> +            if ((cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS)) ||
>>> +                (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) ||
>>> +                (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS)) ||
>>> +                (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS)) ||
>>> +                (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))) {
>>
>> Switch please
>
> Cause is a combined status register. Let's say it contains 0x41. I need to check
> if bit 0 or bit 6 is set in this value for each case condition. The value is not 0x40
> or 0x1.
>
> I created macro like this instead.
>
> +#define HIDMA_IS_ERR_INTERRUPT(cause)                          \
> +       (cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS))   || \
> +       (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) || \
> +       (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS))       || \
> +       (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS))    || \
> +       (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))

This looks overheaded.

#define HIDMA_XXX (BIT(a) | BIT (b) ... BIT(n))

>
> and replaced the if statement as follows
>
> if (HIDMA_IS_ERR_INTERRUPT(cause)) {

if (cause & HIDMA_XXX) {

-- 
With Best Regards,
Andy Shevchenko

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

#1387621

On 4/26/2016 11:10 AM, Andy Shevchenko wrote:
> On Tue, Apr 26, 2016 at 6:04 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>> On 4/25/2016 11:28 PM, Vinod Koul wrote:
>>> On Mon, Apr 11, 2016 at 10:21:11AM -0400, Sinan Kaya wrote:
> 
>>>> +    while (cause) {
>>>> +            if ((cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS)) ||
>>>> +                (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) ||
>>>> +                (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS)) ||
>>>> +                (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS)) ||
>>>> +                (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))) {
>>>
>>> Switch please
>>
>> Cause is a combined status register. Let's say it contains 0x41. I need to check
>> if bit 0 or bit 6 is set in this value for each case condition. The value is not 0x40
>> or 0x1.
>>
>> I created macro like this instead.
>>
>> +#define HIDMA_IS_ERR_INTERRUPT(cause)                          \
>> +       (cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS))   || \
>> +       (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) || \
>> +       (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS))       || \
>> +       (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS))    || \
>> +       (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))
> 
> This looks overheaded.
> 
> #define HIDMA_XXX (BIT(a) | BIT (b) ... BIT(n))
> 
>>
>> and replaced the if statement as follows
>>
>> if (HIDMA_IS_ERR_INTERRUPT(cause)) {
> 
> if (cause & HIDMA_XXX) {
> 

This is even better.

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

#1387662

On Tue, Apr 26, 2016 at 11:04:55AM -0400, Sinan Kaya wrote:
> On 4/25/2016 11:28 PM, Vinod Koul wrote:
> >> +
> >> +			/* reset the channel for recovery */
> >> +			if (hidma_ll_setup(lldev)) {
> > 
> > should this be done in ISR?
> 
> I created a new tasklet called rst_task and posted the code there. 

sounds better

> 
>  /*
> + * Abort all transactions and perform a reset.
> + */
> +static void hidma_ll_abort(unsigned long arg)
> +{
> +       u8 err_code = HIDMA_EVRE_STATUS_ERROR;
> +       u8 err_info = 0xFF;
> +
> +       dev_err(lldev->dev, "error 0x%x, resetting...\n",
> +               cause);

right justify this and others as well please

> >> +int hidma_ll_resume(struct hidma_lldev *lldev)
> >> +{
> >> +	return hidma_ll_enable(lldev);
> >> +}
> > 
> > why do we need this empty function, use hidma_ll_enable.
> 
> hidma_ll_enable is a common function that gets called from multiple places. 
> hidma_ll_resume and hidma_ll_pause is used by the OS interface for pausing
> and resuming the DMA channel. 

is there a reason why we can't have the code in resume and that being called
internally as well as externally?

> >> +/*
> >> + * Note that even though we stop this channel
> >> + * if there is a pending transaction in flight
> >> + * it will complete and follow the callback.
> >> + * This request will prevent further requests
> >> + * to be made.
> > 
> > Why the odd formating?
> 
> aligned to 75 characters.

This seems to be 50 chars!

-- 
~Vinod

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

#1390478

On 4/26/2016 12:24 PM, Vinod Koul wrote:
>> +
>> > +       dev_err(lldev->dev, "error 0x%x, resetting...\n",
>> > +               cause);
> right justify this and others as well please
> 

Can you please point me to other lines that need to be fixed please? It looks good
to me though it doesn't to you. I want to make sure that I'm touching the right ones.
You seem to prefer a tab following an open parenthesis in your code. Do you want me to
add a tab for every single multi-line such that it comes this for instance?


	ret = readl_poll_timeout(lldev->trca + HIDMA_TRCA_CTRLSTS_REG, val,
				 	HIDMA_CH_STATE(val) == HIDMA_CH_DISABLED, 
					1000, 10000); 

instead of

	ret = readl_poll_timeout(lldev->trca + HIDMA_TRCA_CTRLSTS_REG, val,
				 HIDMA_CH_STATE(val) == HIDMA_CH_DISABLED, 1000,
				 10000); 


>>>> > >> +int hidma_ll_resume(struct hidma_lldev *lldev)
>>>> > >> +{
>>>> > >> +	return hidma_ll_enable(lldev);
>>>> > >> +}
>>> > > 
>>> > > why do we need this empty function, use hidma_ll_enable.
>> > 
>> > hidma_ll_enable is a common function that gets called from multiple places. 
>> > hidma_ll_resume and hidma_ll_pause is used by the OS interface for pausing
>> > and resuming the DMA channel. 
> is there a reason why we can't have the code in resume and that being called
> internally as well as externally?
> 

I'll have the pause code in OS layer call hidma_ll_enable and get rid of pause
function here.

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

#1391737

On 4/28/2016 3:30 PM, Sinan Kaya wrote:
> On 4/26/2016 12:24 PM, Vinod Koul wrote:
>>> +
>>>> +       dev_err(lldev->dev, "error 0x%x, resetting...\n",
>>>> +               cause);
>> right justify this and others as well please
>>
> 
> Can you please point me to other lines that need to be fixed please? It looks good
> to me though it doesn't to you. I want to make sure that I'm touching the right ones.
> You seem to prefer a tab following an open parenthesis in your code. Do you want me to
> add a tab for every single multi-line such that it comes this for instance?
> 
> 
> 	ret = readl_poll_timeout(lldev->trca + HIDMA_TRCA_CTRLSTS_REG, val,
> 				 	HIDMA_CH_STATE(val) == HIDMA_CH_DISABLED, 
> 					1000, 10000); 
> 
> instead of
> 
> 	ret = readl_poll_timeout(lldev->trca + HIDMA_TRCA_CTRLSTS_REG, val,
> 				 HIDMA_CH_STATE(val) == HIDMA_CH_DISABLED, 1000,
> 				 10000); 
> 
> 
>>>>>>>> +int hidma_ll_resume(struct hidma_lldev *lldev)
>>>>>>>> +{
>>>>>>>> +	return hidma_ll_enable(lldev);
>>>>>>>> +}
>>>>>>
>>>>>> why do we need this empty function, use hidma_ll_enable.
>>>>
>>>> hidma_ll_enable is a common function that gets called from multiple places. 
>>>> hidma_ll_resume and hidma_ll_pause is used by the OS interface for pausing
>>>> and resuming the DMA channel. 
>> is there a reason why we can't have the code in resume and that being called
>> internally as well as externally?
>>
> 
> I'll have the pause code in OS layer call hidma_ll_enable and get rid of pause
> function here.
> 


I posted v18 along with all the style corrections and other things you asked me.
Please let me know if there is anything outstanding.



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