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

Groups > linux.kernel > #1494868 > unrolled thread

[PATCH v2 2/4] drivers: iio: ti_am335x_adc: add dma support

Started byMugunthan V N <mugunthanvnm@ti.com>
First post2016-10-03 15:10 +0200
Last post2016-10-05 11:00 +0200
Articles 7 — 2 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

  [PATCH v2 2/4] drivers: iio: ti_am335x_adc: add dma support Mugunthan V N <mugunthanvnm@ti.com> - 2016-10-03 15:10 +0200
    Re: [PATCH v2 2/4] drivers: iio: ti_am335x_adc: add dma support Peter Ujfalusi <peter.ujfalusi@ti.com> - 2016-10-04 10:40 +0200
      Re: [PATCH v2 2/4] drivers: iio: ti_am335x_adc: add dma support Mugunthan V N <mugunthanvnm@ti.com> - 2016-10-05 08:30 +0200
        Re: [PATCH v2 2/4] drivers: iio: ti_am335x_adc: add dma support Peter Ujfalusi <peter.ujfalusi@ti.com> - 2016-10-05 08:40 +0200
          Re: [PATCH v2 2/4] drivers: iio: ti_am335x_adc: add dma support Mugunthan V N <mugunthanvnm@ti.com> - 2016-10-05 10:20 +0200
            Re: [PATCH v2 2/4] drivers: iio: ti_am335x_adc: add dma support Peter Ujfalusi <peter.ujfalusi@ti.com> - 2016-10-05 10:50 +0200
              Re: [PATCH v2 2/4] drivers: iio: ti_am335x_adc: add dma support Mugunthan V N <mugunthanvnm@ti.com> - 2016-10-05 11:00 +0200

#1494868 — [PATCH v2 2/4] drivers: iio: ti_am335x_adc: add dma support

FromMugunthan V N <mugunthanvnm@ti.com>
Date2016-10-03 15:10 +0200
Subject[PATCH v2 2/4] drivers: iio: ti_am335x_adc: add dma support
Message-ID<sobah-5ks-1@gated-at.bofh.it>
This patch adds the required pieces to ti_am335x_adc driver for
DMA support

Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/iio/adc/ti_am335x_adc.c      | 145 ++++++++++++++++++++++++++++++++++-
 include/linux/mfd/ti_am335x_tscadc.h |   7 ++
 2 files changed, 149 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index c3cfacca..7e250be 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -30,10 +30,28 @@
 #include <linux/iio/buffer.h>
 #include <linux/iio/kfifo_buf.h>
 
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+
+#define DMA_BUFFER_SIZE		SZ_2K
+
+struct tiadc_dma {
+	struct dma_slave_config	conf;
+	struct dma_chan		*chan;
+	dma_addr_t		addr;
+	dma_cookie_t		cookie;
+	u8			*buf;
+	int			current_period;
+	int			period_size;
+	u8			fifo_thresh;
+};
+
 struct tiadc_device {
 	struct ti_tscadc_dev *mfd_tscadc;
+	struct tiadc_dma dma;
 	struct mutex fifo1_lock; /* to protect fifo access */
 	int channels;
+	int total_ch_enabled;
 	u8 channel_line[8];
 	u8 channel_step[8];
 	int buffer_en_ch_steps;
@@ -198,6 +216,67 @@ static irqreturn_t tiadc_worker_h(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
+static void tiadc_dma_rx_complete(void *param)
+{
+	struct iio_dev *indio_dev = param;
+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
+	struct tiadc_dma *dma = &adc_dev->dma;
+	u8 *data;
+	int i;
+
+	data = dma->buf + dma->current_period * dma->period_size;
+	dma->current_period = 1 - dma->current_period; /* swap the buffer ID */
+
+	for (i = 0; i < dma->period_size; i += indio_dev->scan_bytes) {
+		iio_push_to_buffers(indio_dev, data);
+		data += indio_dev->scan_bytes;
+	}
+}
+
+static int tiadc_start_dma(struct iio_dev *indio_dev)
+{
+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
+	struct tiadc_dma *dma = &adc_dev->dma;
+	struct dma_async_tx_descriptor *desc;
+
+	dma->current_period = 0; /* We start to fill period 0 */
+	/*
+	 * Make the fifo thresh as the multiple of total number of
+	 * channels enabled, so make sure that cyclic DMA period
+	 * length is also a multiple of total number of channels
+	 * enabled. This ensures that no invalid data is reported
+	 * to the stack via iio_push_to_buffers().
+	 */
+	dma->fifo_thresh = rounddown(FIFO1_THRESHOLD + 1,
+				     adc_dev->total_ch_enabled) - 1;
+	/* Make sure that period length is multiple of fifo thresh level */
+	dma->period_size = rounddown(DMA_BUFFER_SIZE / 2,
+				    (dma->fifo_thresh + 1) * sizeof(u16));
+
+	dma->conf.src_maxburst = dma->fifo_thresh + 1;
+	dmaengine_slave_config(dma->chan, &dma->conf);
+
+	desc = dmaengine_prep_dma_cyclic(dma->chan, dma->addr,
+					 dma->period_size * 2,
+					 dma->period_size, DMA_DEV_TO_MEM,
+					 DMA_PREP_INTERRUPT);
+	if (!desc)
+		return -EBUSY;
+
+	desc->callback = tiadc_dma_rx_complete;
+	desc->callback_param = indio_dev;
+
+	dma->cookie = dmaengine_submit(desc);
+
+	dma_async_issue_pending(dma->chan);
+
+	tiadc_writel(adc_dev, REG_FIFO1THR, dma->fifo_thresh);
+	tiadc_writel(adc_dev, REG_DMA1REQ, dma->fifo_thresh);
+	tiadc_writel(adc_dev, REG_DMAENABLE_SET, DMA_FIFO1);
+
+	return 0;
+}
+
 static int tiadc_buffer_preenable(struct iio_dev *indio_dev)
 {
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
@@ -218,20 +297,30 @@ static int tiadc_buffer_preenable(struct iio_dev *indio_dev)
 static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
 {
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
+	struct tiadc_dma *dma = &adc_dev->dma;
+	unsigned int irq_enable;
 	unsigned int enb = 0;
 	u8 bit;
 
 	tiadc_step_config(indio_dev);
-	for_each_set_bit(bit, indio_dev->active_scan_mask, adc_dev->channels)
+	for_each_set_bit(bit, indio_dev->active_scan_mask, adc_dev->channels) {
 		enb |= (get_adc_step_bit(adc_dev, bit) << 1);
+		adc_dev->total_ch_enabled++;
+	}
 	adc_dev->buffer_en_ch_steps = enb;
 
+	if (dma->chan)
+		tiadc_start_dma(indio_dev);
+
 	am335x_tsc_se_set_cache(adc_dev->mfd_tscadc, enb);
 
 	tiadc_writel(adc_dev,  REG_IRQSTATUS, IRQENB_FIFO1THRES
 				| IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
-	tiadc_writel(adc_dev,  REG_IRQENABLE, IRQENB_FIFO1THRES
-				| IRQENB_FIFO1OVRRUN);
+
+	irq_enable = IRQENB_FIFO1OVRRUN;
+	if (!dma->chan)
+		irq_enable |= IRQENB_FIFO1THRES;
+	tiadc_writel(adc_dev,  REG_IRQENABLE, irq_enable);
 
 	return 0;
 }
@@ -239,12 +328,18 @@ static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
 static int tiadc_buffer_predisable(struct iio_dev *indio_dev)
 {
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
+	struct tiadc_dma *dma = &adc_dev->dma;
 	int fifo1count, i, read;
 
 	tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
 				IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW));
 	am335x_tsc_se_clr(adc_dev->mfd_tscadc, adc_dev->buffer_en_ch_steps);
 	adc_dev->buffer_en_ch_steps = 0;
+	adc_dev->total_ch_enabled = 0;
+	if (dma->chan) {
+		tiadc_writel(adc_dev, REG_DMAENABLE_CLEAR, 0x2);
+		dmaengine_terminate_async(dma->chan);
+	}
 
 	/* Flush FIFO of leftover data in the time it takes to disable adc */
 	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
@@ -430,6 +525,38 @@ static const struct iio_info tiadc_info = {
 	.driver_module = THIS_MODULE,
 };
 
+static int tiadc_request_dma(struct platform_device *pdev,
+			     struct tiadc_device *adc_dev)
+{
+	struct tiadc_dma	*dma = &adc_dev->dma;
+	dma_cap_mask_t		mask;
+
+	/* Default slave configuration parameters */
+	dma->conf.direction = DMA_DEV_TO_MEM;
+	dma->conf.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
+	dma->conf.src_addr = adc_dev->mfd_tscadc->tscadc_phys_base + REG_FIFO1;
+
+	dma_cap_zero(mask);
+	dma_cap_set(DMA_CYCLIC, mask);
+
+	/* Get a channel for RX */
+	dma->chan = dma_request_chan(adc_dev->mfd_tscadc->dev, "fifo1");
+	if (!dma->chan)
+		return -ENODEV;
+
+	/* RX buffer */
+	dma->buf = dma_alloc_coherent(dma->chan->device->dev, DMA_BUFFER_SIZE,
+				      &dma->addr, GFP_KERNEL);
+	if (!dma->buf)
+		goto err;
+
+	return 0;
+err:
+	dma_release_channel(dma->chan);
+
+	return -ENOMEM;
+}
+
 static int tiadc_parse_dt(struct platform_device *pdev,
 			  struct tiadc_device *adc_dev)
 {
@@ -512,8 +639,14 @@ static int tiadc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, indio_dev);
 
+	err = tiadc_request_dma(pdev, adc_dev);
+	if (err && err != -ENODEV)
+		goto err_dma;
+
 	return 0;
 
+err_dma:
+	iio_device_unregister(indio_dev);
 err_buffer_unregister:
 	tiadc_iio_buffered_hardware_remove(indio_dev);
 err_free_channels:
@@ -525,8 +658,14 @@ static int tiadc_remove(struct platform_device *pdev)
 {
 	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
+	struct tiadc_dma *dma = &adc_dev->dma;
 	u32 step_en;
 
+	if (dma->chan) {
+		dma_free_coherent(dma->chan->device->dev, DMA_BUFFER_SIZE,
+				  dma->buf, dma->addr);
+		dma_release_channel(dma->chan);
+	}
 	iio_device_unregister(indio_dev);
 	tiadc_iio_buffered_hardware_remove(indio_dev);
 	tiadc_channels_remove(indio_dev);
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index e45a208..b9a53e0 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -23,6 +23,8 @@
 #define REG_IRQENABLE		0x02C
 #define REG_IRQCLR		0x030
 #define REG_IRQWAKEUP		0x034
+#define REG_DMAENABLE_SET	0x038
+#define REG_DMAENABLE_CLEAR	0x03c
 #define REG_CTRL		0x040
 #define REG_ADCFSM		0x044
 #define REG_CLKDIV		0x04C
@@ -36,6 +38,7 @@
 #define REG_FIFO0THR		0xE8
 #define REG_FIFO1CNT		0xF0
 #define REG_FIFO1THR		0xF4
+#define REG_DMA1REQ		0xF8
 #define REG_FIFO0		0x100
 #define REG_FIFO1		0x200
 
@@ -126,6 +129,10 @@
 #define FIFOREAD_DATA_MASK (0xfff << 0)
 #define FIFOREAD_CHNLID_MASK (0xf << 16)
 
+/* DMA ENABLE/CLEAR Register */
+#define DMA_FIFO0		BIT(0)
+#define DMA_FIFO1		BIT(1)
+
 /* Sequencer Status */
 #define SEQ_STATUS BIT(5)
 #define CHARGE_STEP		0x11
-- 
2.10.0.372.g6fe1b14

[toc] | [next] | [standalone]

#1495292

FromPeter Ujfalusi <peter.ujfalusi@ti.com>
Date2016-10-04 10:40 +0200
Message-ID<sotqy-cY-1@gated-at.bofh.it>
In reply to#1494868
On 10/03/16 16:03, Mugunthan V N wrote:
> +static int tiadc_request_dma(struct platform_device *pdev,
> +			     struct tiadc_device *adc_dev)
> +{
> +	struct tiadc_dma	*dma = &adc_dev->dma;
> +	dma_cap_mask_t		mask;
> +
> +	/* Default slave configuration parameters */
> +	dma->conf.direction = DMA_DEV_TO_MEM;
> +	dma->conf.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> +	dma->conf.src_addr = adc_dev->mfd_tscadc->tscadc_phys_base + REG_FIFO1;
> +
> +	dma_cap_zero(mask);
> +	dma_cap_set(DMA_CYCLIC, mask);
> +
> +	/* Get a channel for RX */
> +	dma->chan = dma_request_chan(adc_dev->mfd_tscadc->dev, "fifo1");
> +	if (!dma->chan)
> +		return -ENODEV;

dma_request_chan() ERR_PTR in case of failure, never NULL. You should reuse
the returned error code to support deferred probing.

> +
> +	/* RX buffer */
> +	dma->buf = dma_alloc_coherent(dma->chan->device->dev, DMA_BUFFER_SIZE,
> +				      &dma->addr, GFP_KERNEL);
> +	if (!dma->buf)
> +		goto err;
> +
> +	return 0;
> +err:
> +	dma_release_channel(dma->chan);
> +
> +	return -ENOMEM;
> +}
> +
>  static int tiadc_parse_dt(struct platform_device *pdev,
>  			  struct tiadc_device *adc_dev)
>  {
> @@ -512,8 +639,14 @@ static int tiadc_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, indio_dev);
>  
> +	err = tiadc_request_dma(pdev, adc_dev);
> +	if (err && err != -ENODEV)
> +		goto err_dma;

You should handle the deferred probing for DMA channel.

> +
>  	return 0;
>  
> +err_dma:
> +	iio_device_unregister(indio_dev);
>  err_buffer_unregister:
>  	tiadc_iio_buffered_hardware_remove(indio_dev);
>  err_free_channels:
> @@ -525,8 +658,14 @@ static int tiadc_remove(struct platform_device *pdev)
>  {
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>  	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> +	struct tiadc_dma *dma = &adc_dev->dma;
>  	u32 step_en;
>  
> +	if (dma->chan) {
> +		dma_free_coherent(dma->chan->device->dev, DMA_BUFFER_SIZE,
> +				  dma->buf, dma->addr);
> +		dma_release_channel(dma->chan);
> +	}
>  	iio_device_unregister(indio_dev);
>  	tiadc_iio_buffered_hardware_remove(indio_dev);
>  	tiadc_channels_remove(indio_dev);
> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index e45a208..b9a53e0 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -23,6 +23,8 @@
>  #define REG_IRQENABLE		0x02C
>  #define REG_IRQCLR		0x030
>  #define REG_IRQWAKEUP		0x034
> +#define REG_DMAENABLE_SET	0x038
> +#define REG_DMAENABLE_CLEAR	0x03c
>  #define REG_CTRL		0x040
>  #define REG_ADCFSM		0x044
>  #define REG_CLKDIV		0x04C
> @@ -36,6 +38,7 @@
>  #define REG_FIFO0THR		0xE8
>  #define REG_FIFO1CNT		0xF0
>  #define REG_FIFO1THR		0xF4
> +#define REG_DMA1REQ		0xF8
>  #define REG_FIFO0		0x100
>  #define REG_FIFO1		0x200
>  
> @@ -126,6 +129,10 @@
>  #define FIFOREAD_DATA_MASK (0xfff << 0)
>  #define FIFOREAD_CHNLID_MASK (0xf << 16)
>  
> +/* DMA ENABLE/CLEAR Register */
> +#define DMA_FIFO0		BIT(0)
> +#define DMA_FIFO1		BIT(1)
> +
>  /* Sequencer Status */
>  #define SEQ_STATUS BIT(5)
>  #define CHARGE_STEP		0x11
> 


-- 
Péter

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

#1495710

FromMugunthan V N <mugunthanvnm@ti.com>
Date2016-10-05 08:30 +0200
Message-ID<soNSh-5sp-1@gated-at.bofh.it>
In reply to#1495292
On Tuesday 04 October 2016 02:02 PM, Peter Ujfalusi wrote:
> On 10/03/16 16:03, Mugunthan V N wrote:
>> +static int tiadc_request_dma(struct platform_device *pdev,
>> +			     struct tiadc_device *adc_dev)
>> +{
>> +	struct tiadc_dma	*dma = &adc_dev->dma;
>> +	dma_cap_mask_t		mask;
>> +
>> +	/* Default slave configuration parameters */
>> +	dma->conf.direction = DMA_DEV_TO_MEM;
>> +	dma->conf.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
>> +	dma->conf.src_addr = adc_dev->mfd_tscadc->tscadc_phys_base + REG_FIFO1;
>> +
>> +	dma_cap_zero(mask);
>> +	dma_cap_set(DMA_CYCLIC, mask);
>> +
>> +	/* Get a channel for RX */
>> +	dma->chan = dma_request_chan(adc_dev->mfd_tscadc->dev, "fifo1");
>> +	if (!dma->chan)
>> +		return -ENODEV;
> 
> dma_request_chan() ERR_PTR in case of failure, never NULL. You should reuse
> the returned error code to support deferred probing.

Will fix this in v3.

> 
>> +
>> +	/* RX buffer */
>> +	dma->buf = dma_alloc_coherent(dma->chan->device->dev, DMA_BUFFER_SIZE,
>> +				      &dma->addr, GFP_KERNEL);
>> +	if (!dma->buf)
>> +		goto err;
>> +
>> +	return 0;
>> +err:
>> +	dma_release_channel(dma->chan);
>> +
>> +	return -ENOMEM;
>> +}
>> +
>>  static int tiadc_parse_dt(struct platform_device *pdev,
>>  			  struct tiadc_device *adc_dev)
>>  {
>> @@ -512,8 +639,14 @@ static int tiadc_probe(struct platform_device *pdev)
>>  
>>  	platform_set_drvdata(pdev, indio_dev);
>>  
>> +	err = tiadc_request_dma(pdev, adc_dev);
>> +	if (err && err != -ENODEV)
>> +		goto err_dma;
> 
> You should handle the deferred probing for DMA channel.

+	dma->chan = dma_request_chan(adc_dev->mfd_tscadc->dev, "fifo1");
+	if (IS_ERR(dma->chan)) {
+		int ret = PTR_ERR(dma->chan);
+
+		dma->chan = NULL;
+		return ret;
+	}

With this probe defer will be taken care and ADC will continue without
DMA when request channel returns -ENODEV.

Regards
Mugunthan V N

> 
>> +
>>  	return 0;
>>  
>> +err_dma:
>> +	iio_device_unregister(indio_dev);
>>  err_buffer_unregister:
>>  	tiadc_iio_buffered_hardware_remove(indio_dev);
>>  err_free_channels:
>> @@ -525,8 +658,14 @@ static int tiadc_remove(struct platform_device *pdev)
>>  {
>>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>  	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>> +	struct tiadc_dma *dma = &adc_dev->dma;
>>  	u32 step_en;
>>  
>> +	if (dma->chan) {
>> +		dma_free_coherent(dma->chan->device->dev, DMA_BUFFER_SIZE,
>> +				  dma->buf, dma->addr);
>> +		dma_release_channel(dma->chan);
>> +	}
>>  	iio_device_unregister(indio_dev);
>>  	tiadc_iio_buffered_hardware_remove(indio_dev);
>>  	tiadc_channels_remove(indio_dev);
>> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
>> index e45a208..b9a53e0 100644
>> --- a/include/linux/mfd/ti_am335x_tscadc.h
>> +++ b/include/linux/mfd/ti_am335x_tscadc.h
>> @@ -23,6 +23,8 @@
>>  #define REG_IRQENABLE		0x02C
>>  #define REG_IRQCLR		0x030
>>  #define REG_IRQWAKEUP		0x034
>> +#define REG_DMAENABLE_SET	0x038
>> +#define REG_DMAENABLE_CLEAR	0x03c
>>  #define REG_CTRL		0x040
>>  #define REG_ADCFSM		0x044
>>  #define REG_CLKDIV		0x04C
>> @@ -36,6 +38,7 @@
>>  #define REG_FIFO0THR		0xE8
>>  #define REG_FIFO1CNT		0xF0
>>  #define REG_FIFO1THR		0xF4
>> +#define REG_DMA1REQ		0xF8
>>  #define REG_FIFO0		0x100
>>  #define REG_FIFO1		0x200
>>  
>> @@ -126,6 +129,10 @@
>>  #define FIFOREAD_DATA_MASK (0xfff << 0)
>>  #define FIFOREAD_CHNLID_MASK (0xf << 16)
>>  
>> +/* DMA ENABLE/CLEAR Register */
>> +#define DMA_FIFO0		BIT(0)
>> +#define DMA_FIFO1		BIT(1)
>> +
>>  /* Sequencer Status */
>>  #define SEQ_STATUS BIT(5)
>>  #define CHARGE_STEP		0x11
>>
> 
>

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

#1495715

FromPeter Ujfalusi <peter.ujfalusi@ti.com>
Date2016-10-05 08:40 +0200
Message-ID<soO1X-5vN-1@gated-at.bofh.it>
In reply to#1495710
On 10/05/16 09:21, Mugunthan V N wrote:
> On Tuesday 04 October 2016 02:02 PM, Peter Ujfalusi wrote:
>> On 10/03/16 16:03, Mugunthan V N wrote:
>>> +static int tiadc_request_dma(struct platform_device *pdev,
>>> +			     struct tiadc_device *adc_dev)
>>> +{
>>> +	struct tiadc_dma	*dma = &adc_dev->dma;
>>> +	dma_cap_mask_t		mask;
>>> +
>>> +	/* Default slave configuration parameters */
>>> +	dma->conf.direction = DMA_DEV_TO_MEM;
>>> +	dma->conf.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
>>> +	dma->conf.src_addr = adc_dev->mfd_tscadc->tscadc_phys_base + REG_FIFO1;
>>> +
>>> +	dma_cap_zero(mask);
>>> +	dma_cap_set(DMA_CYCLIC, mask);
>>> +
>>> +	/* Get a channel for RX */
>>> +	dma->chan = dma_request_chan(adc_dev->mfd_tscadc->dev, "fifo1");
>>> +	if (!dma->chan)
>>> +		return -ENODEV;
>>
>> dma_request_chan() ERR_PTR in case of failure, never NULL. You should reuse
>> the returned error code to support deferred probing.
> 
> Will fix this in v3.
> 
>>
>>> +
>>> +	/* RX buffer */
>>> +	dma->buf = dma_alloc_coherent(dma->chan->device->dev, DMA_BUFFER_SIZE,
>>> +				      &dma->addr, GFP_KERNEL);
>>> +	if (!dma->buf)
>>> +		goto err;
>>> +
>>> +	return 0;
>>> +err:
>>> +	dma_release_channel(dma->chan);
>>> +
>>> +	return -ENOMEM;
>>> +}
>>> +
>>>  static int tiadc_parse_dt(struct platform_device *pdev,
>>>  			  struct tiadc_device *adc_dev)
>>>  {
>>> @@ -512,8 +639,14 @@ static int tiadc_probe(struct platform_device *pdev)
>>>  
>>>  	platform_set_drvdata(pdev, indio_dev);
>>>  
>>> +	err = tiadc_request_dma(pdev, adc_dev);
>>> +	if (err && err != -ENODEV)
>>> +		goto err_dma;
>>
>> You should handle the deferred probing for DMA channel.
> 
> +	dma->chan = dma_request_chan(adc_dev->mfd_tscadc->dev, "fifo1");
> +	if (IS_ERR(dma->chan)) {
> +		int ret = PTR_ERR(dma->chan);
> +
> +		dma->chan = NULL;
> +		return ret;

You don't need the 'ret' variable:
		return PTR_ERR(dma->chan);

> +	}
> 
> With this probe defer will be taken care and ADC will continue without
> DMA when request channel returns -ENODEV.

I would rather have explicit check for deferred probe:

err = tiadc_request_dma(pdev, adc_dev);
if (err && err == -EPROBE_DEFER)
	goto err_dma;


-- 
Péter

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

#1495751

FromMugunthan V N <mugunthanvnm@ti.com>
Date2016-10-05 10:20 +0200
Message-ID<soPAJ-6FR-3@gated-at.bofh.it>
In reply to#1495715
On Wednesday 05 October 2016 12:01 PM, Peter Ujfalusi wrote:
> On 10/05/16 09:21, Mugunthan V N wrote:
>> On Tuesday 04 October 2016 02:02 PM, Peter Ujfalusi wrote:
>>> On 10/03/16 16:03, Mugunthan V N wrote:
>>>> +static int tiadc_request_dma(struct platform_device *pdev,
>>>> +			     struct tiadc_device *adc_dev)
>>>> +{
>>>> +	struct tiadc_dma	*dma = &adc_dev->dma;
>>>> +	dma_cap_mask_t		mask;
>>>> +
>>>> +	/* Default slave configuration parameters */
>>>> +	dma->conf.direction = DMA_DEV_TO_MEM;
>>>> +	dma->conf.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
>>>> +	dma->conf.src_addr = adc_dev->mfd_tscadc->tscadc_phys_base + REG_FIFO1;
>>>> +
>>>> +	dma_cap_zero(mask);
>>>> +	dma_cap_set(DMA_CYCLIC, mask);
>>>> +
>>>> +	/* Get a channel for RX */
>>>> +	dma->chan = dma_request_chan(adc_dev->mfd_tscadc->dev, "fifo1");
>>>> +	if (!dma->chan)
>>>> +		return -ENODEV;
>>>
>>> dma_request_chan() ERR_PTR in case of failure, never NULL. You should reuse
>>> the returned error code to support deferred probing.
>>
>> Will fix this in v3.
>>
>>>
>>>> +
>>>> +	/* RX buffer */
>>>> +	dma->buf = dma_alloc_coherent(dma->chan->device->dev, DMA_BUFFER_SIZE,
>>>> +				      &dma->addr, GFP_KERNEL);
>>>> +	if (!dma->buf)
>>>> +		goto err;
>>>> +
>>>> +	return 0;
>>>> +err:
>>>> +	dma_release_channel(dma->chan);
>>>> +
>>>> +	return -ENOMEM;
>>>> +}
>>>> +
>>>>  static int tiadc_parse_dt(struct platform_device *pdev,
>>>>  			  struct tiadc_device *adc_dev)
>>>>  {
>>>> @@ -512,8 +639,14 @@ static int tiadc_probe(struct platform_device *pdev)
>>>>  
>>>>  	platform_set_drvdata(pdev, indio_dev);
>>>>  
>>>> +	err = tiadc_request_dma(pdev, adc_dev);
>>>> +	if (err && err != -ENODEV)
>>>> +		goto err_dma;
>>>
>>> You should handle the deferred probing for DMA channel.
>>
>> +	dma->chan = dma_request_chan(adc_dev->mfd_tscadc->dev, "fifo1");
>> +	if (IS_ERR(dma->chan)) {
>> +		int ret = PTR_ERR(dma->chan);
>> +
>> +		dma->chan = NULL;
>> +		return ret;
> 
> You don't need the 'ret' variable:
> 		return PTR_ERR(dma->chan);

So you mean change all *if (dma->chan)* to *if (IS_ERR(dma->chan))*?

> 
>> +	}
>>
>> With this probe defer will be taken care and ADC will continue without
>> DMA when request channel returns -ENODEV.
> 
> I would rather have explicit check for deferred probe:
> 
> err = tiadc_request_dma(pdev, adc_dev);
> if (err && err == -EPROBE_DEFER)
> 	goto err_dma;

But in this case any other failure other than -EPROBE_DEFER will be
masked and ADC will be in PIO mode?

Regards
Mugunthan V N

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

#1495760

FromPeter Ujfalusi <peter.ujfalusi@ti.com>
Date2016-10-05 10:50 +0200
Message-ID<soQ3L-6QO-15@gated-at.bofh.it>
In reply to#1495751
On 10/05/16 11:17, Mugunthan V N wrote:
> On Wednesday 05 October 2016 12:01 PM, Peter Ujfalusi wrote:
>> On 10/05/16 09:21, Mugunthan V N wrote:
>>> On Tuesday 04 October 2016 02:02 PM, Peter Ujfalusi wrote:
>>>> On 10/03/16 16:03, Mugunthan V N wrote:
>>>>> +static int tiadc_request_dma(struct platform_device *pdev,
>>>>> +			     struct tiadc_device *adc_dev)
>>>>> +{
>>>>> +	struct tiadc_dma	*dma = &adc_dev->dma;
>>>>> +	dma_cap_mask_t		mask;
>>>>> +
>>>>> +	/* Default slave configuration parameters */
>>>>> +	dma->conf.direction = DMA_DEV_TO_MEM;
>>>>> +	dma->conf.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
>>>>> +	dma->conf.src_addr = adc_dev->mfd_tscadc->tscadc_phys_base + REG_FIFO1;
>>>>> +
>>>>> +	dma_cap_zero(mask);
>>>>> +	dma_cap_set(DMA_CYCLIC, mask);
>>>>> +
>>>>> +	/* Get a channel for RX */
>>>>> +	dma->chan = dma_request_chan(adc_dev->mfd_tscadc->dev, "fifo1");
>>>>> +	if (!dma->chan)
>>>>> +		return -ENODEV;
>>>>
>>>> dma_request_chan() ERR_PTR in case of failure, never NULL. You should reuse
>>>> the returned error code to support deferred probing.
>>>
>>> Will fix this in v3.
>>>
>>>>
>>>>> +
>>>>> +	/* RX buffer */
>>>>> +	dma->buf = dma_alloc_coherent(dma->chan->device->dev, DMA_BUFFER_SIZE,
>>>>> +				      &dma->addr, GFP_KERNEL);
>>>>> +	if (!dma->buf)
>>>>> +		goto err;
>>>>> +
>>>>> +	return 0;
>>>>> +err:
>>>>> +	dma_release_channel(dma->chan);
>>>>> +
>>>>> +	return -ENOMEM;
>>>>> +}
>>>>> +
>>>>>  static int tiadc_parse_dt(struct platform_device *pdev,
>>>>>  			  struct tiadc_device *adc_dev)
>>>>>  {
>>>>> @@ -512,8 +639,14 @@ static int tiadc_probe(struct platform_device *pdev)
>>>>>  
>>>>>  	platform_set_drvdata(pdev, indio_dev);
>>>>>  
>>>>> +	err = tiadc_request_dma(pdev, adc_dev);
>>>>> +	if (err && err != -ENODEV)
>>>>> +		goto err_dma;
>>>>
>>>> You should handle the deferred probing for DMA channel.
>>>
>>> +	dma->chan = dma_request_chan(adc_dev->mfd_tscadc->dev, "fifo1");
>>> +	if (IS_ERR(dma->chan)) {
>>> +		int ret = PTR_ERR(dma->chan);
>>> +
>>> +		dma->chan = NULL;
>>> +		return ret;
>>
>> You don't need the 'ret' variable:
>> 		return PTR_ERR(dma->chan);
> 
> So you mean change all *if (dma->chan)* to *if (IS_ERR(dma->chan))*?

Oh, sorry. Your version was fine as you NULL the dma->chan before the return.

> 
>>
>>> +	}
>>>
>>> With this probe defer will be taken care and ADC will continue without
>>> DMA when request channel returns -ENODEV.
>>
>> I would rather have explicit check for deferred probe:
>>
>> err = tiadc_request_dma(pdev, adc_dev);
>> if (err && err == -EPROBE_DEFER)
>> 	goto err_dma;
> 
> But in this case any other failure other than -EPROBE_DEFER will be
> masked and ADC will be in PIO mode?

Yes, exactly. If we fail to get the DMA channel we should not use the DMA and
we only want to handle the deferred probing.

> 
> Regards
> Mugunthan V N
> 


-- 
Péter

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

#1495764

FromMugunthan V N <mugunthanvnm@ti.com>
Date2016-10-05 11:00 +0200
Message-ID<soQdr-6Ut-5@gated-at.bofh.it>
In reply to#1495760
On Wednesday 05 October 2016 02:16 PM, Peter Ujfalusi wrote:
> On 10/05/16 11:17, Mugunthan V N wrote:
>> On Wednesday 05 October 2016 12:01 PM, Peter Ujfalusi wrote:
>>> On 10/05/16 09:21, Mugunthan V N wrote:
>>>> On Tuesday 04 October 2016 02:02 PM, Peter Ujfalusi wrote:
>>>>> On 10/03/16 16:03, Mugunthan V N wrote:
>>>>>> +static int tiadc_request_dma(struct platform_device *pdev,
>>>>>> +			     struct tiadc_device *adc_dev)
>>>>>> +{
>>>>>> +	struct tiadc_dma	*dma = &adc_dev->dma;
>>>>>> +	dma_cap_mask_t		mask;
>>>>>> +
>>>>>> +	/* Default slave configuration parameters */
>>>>>> +	dma->conf.direction = DMA_DEV_TO_MEM;
>>>>>> +	dma->conf.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
>>>>>> +	dma->conf.src_addr = adc_dev->mfd_tscadc->tscadc_phys_base + REG_FIFO1;
>>>>>> +
>>>>>> +	dma_cap_zero(mask);
>>>>>> +	dma_cap_set(DMA_CYCLIC, mask);
>>>>>> +
>>>>>> +	/* Get a channel for RX */
>>>>>> +	dma->chan = dma_request_chan(adc_dev->mfd_tscadc->dev, "fifo1");
>>>>>> +	if (!dma->chan)
>>>>>> +		return -ENODEV;
>>>>>
>>>>> dma_request_chan() ERR_PTR in case of failure, never NULL. You should reuse
>>>>> the returned error code to support deferred probing.
>>>>
>>>> Will fix this in v3.
>>>>
>>>>>
>>>>>> +
>>>>>> +	/* RX buffer */
>>>>>> +	dma->buf = dma_alloc_coherent(dma->chan->device->dev, DMA_BUFFER_SIZE,
>>>>>> +				      &dma->addr, GFP_KERNEL);
>>>>>> +	if (!dma->buf)
>>>>>> +		goto err;
>>>>>> +
>>>>>> +	return 0;
>>>>>> +err:
>>>>>> +	dma_release_channel(dma->chan);
>>>>>> +
>>>>>> +	return -ENOMEM;
>>>>>> +}
>>>>>> +
>>>>>>  static int tiadc_parse_dt(struct platform_device *pdev,
>>>>>>  			  struct tiadc_device *adc_dev)
>>>>>>  {
>>>>>> @@ -512,8 +639,14 @@ static int tiadc_probe(struct platform_device *pdev)
>>>>>>  
>>>>>>  	platform_set_drvdata(pdev, indio_dev);
>>>>>>  
>>>>>> +	err = tiadc_request_dma(pdev, adc_dev);
>>>>>> +	if (err && err != -ENODEV)
>>>>>> +		goto err_dma;
>>>>>
>>>>> You should handle the deferred probing for DMA channel.
>>>>
>>>> +	dma->chan = dma_request_chan(adc_dev->mfd_tscadc->dev, "fifo1");
>>>> +	if (IS_ERR(dma->chan)) {
>>>> +		int ret = PTR_ERR(dma->chan);
>>>> +
>>>> +		dma->chan = NULL;
>>>> +		return ret;
>>>
>>> You don't need the 'ret' variable:
>>> 		return PTR_ERR(dma->chan);
>>
>> So you mean change all *if (dma->chan)* to *if (IS_ERR(dma->chan))*?
> 
> Oh, sorry. Your version was fine as you NULL the dma->chan before the return.
> 
>>
>>>
>>>> +	}
>>>>
>>>> With this probe defer will be taken care and ADC will continue without
>>>> DMA when request channel returns -ENODEV.
>>>
>>> I would rather have explicit check for deferred probe:
>>>
>>> err = tiadc_request_dma(pdev, adc_dev);
>>> if (err && err == -EPROBE_DEFER)
>>> 	goto err_dma;
>>
>> But in this case any other failure other than -EPROBE_DEFER will be
>> masked and ADC will be in PIO mode?
> 
> Yes, exactly. If we fail to get the DMA channel we should not use the DMA and
> we only want to handle the deferred probing.
> 

Okay, will update accordingly in next version.

Regards
Mugunthan V N

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web