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

[Peter Ujfalusi]

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