Re: [PATCH 2/2] xilinx_dma: Add reset support

[Ramiro Oliveira]

Hi Laurent,

Thank you for your feedback.

On 12/14/2016 8:16 PM, Laurent Pinchart wrote:
> Hi Ramiro,
> 
> Thank you for the patch.
> 
> On Wednesday 14 Dec 2016 17:18:24 Ramiro Oliveira wrote:
>> Add a DT property to control an optional external reset line
>>
>> Signed-off-by: Ramiro Oliveira <roliveir@xxxxxxxxxxxx>
>> ---
>>  drivers/dma/xilinx/xilinx_dma.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/dma/xilinx/xilinx_dma.c
>> b/drivers/dma/xilinx/xilinx_dma.c index 5c9f11b..b845224 100644
>> --- a/drivers/dma/xilinx/xilinx_dma.c
>> +++ b/drivers/dma/xilinx/xilinx_dma.c
>> @@ -46,6 +46,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/clk.h>
>>  #include <linux/io-64-nonatomic-lo-hi.h>
>> +#include <linux/reset.h>
> 
> I had neatly sorted the header alphabetically until someone added clk.h and 
> io-64-nonatomic-lo-hi.h :-( Could you please move reset.h just before slab.h ?
> 

Sure. Actually I was tempted to reorder it, but I decided not to do it. I'll do
it now

>>
>>  #include "../dmaengine.h"
>>
>> @@ -409,6 +410,7 @@ struct xilinx_dma_device {
>>  	struct clk *rxs_clk;
>>  	u32 nr_channels;
>>  	u32 chan_id;
>> +	struct reset_control *rst;
>>  };
>>
>>  /* Macros */
>> @@ -2543,6 +2545,27 @@ static int xilinx_dma_probe(struct platform_device
>> *pdev) if (IS_ERR(xdev->regs))
>>  		return PTR_ERR(xdev->regs);
>>
>> +	xdev->rst = devm_reset_control_get_optional(&pdev->dev, "reset");
> 
> devm_reset_control_get_optional() is deprecated as explained in linux/reset.h, 
> you should use devm_reset_control_get_optional_exclusive() or 
> devm_reset_control_get_optional_shared() instead, as applicable.
> 
> This being said, I'm wondering why the optional versions of those functions 
> exist, as they do exactly the same as the non-optional versions. The API feels 
> wrong, it should have been modelled like the GPIO API. Feel free to fix it if 
> you want :-) But that's out of scope for this patch.
> 

I missed the comment stating that devm_reset_control_get_optional() was deprecated.

I could fix it. Your sugestion is modelling these functions like the GPIO API?

>> +	if (IS_ERR(xdev->rst)) {
>> +		err = PTR_ERR(xdev->rst);
>> +		switch (err) {
>> +		case -ENOENT:
> 
> If you drop the name as proposed in the review of patch 1/2 you don't have to 
> check for -ENOENT.
> 

I'll do that.

>> +		case -ENOTSUPP:
>> +			xdev->rst = NULL;
>> +		break;
> 
> Wrong indentation.
> 
> You need to handle -EPROBE_DEFER and defer probing of the xilinx_dma device.
> 
>> +		default:
>> +			dev_err(xdev->dev, "error getting reset %d\n", err);
>> +		return err;
> 
> Wrong indentation.
> 
>> +		}
>> +	} else {
>> +		err = reset_control_deassert(xdev->rst);
>> +		if (err) {
>> +			dev_err(xdev->dev, "failed to deassert reset: %d\n",
>> +					err);
> 
> Wrong indentation.
> 
>> +			return err;
>> +		}
>> +	}
>> +
>>  	/* Retrieve the DMA engine properties from the device tree */
>>  	xdev->has_sg = of_property_read_bool(node, "xlnx,include-sg");
>>  	if (xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA)
>