[PATCH v9 4/6] ARM: davinci: Add a remoteproc driver implementation for OMAP-L13x DSP

[Sergei Shtylyov]

Hello.

On 09-04-2013 3:55, Tivy, Robert wrote:

>>>> +static int da8xx_rproc_probe(struct platform_device *pdev)
>>>> +{
>>>> +       struct device *dev = &pdev->dev;
>>>> +       struct da8xx_rproc *drproc;
>>>> +       struct rproc *rproc;
>>>> +       struct irq_data *irq_data;
>>>> +       struct resource *bootreg_res;
>>>> +       struct resource *chipsig_res;
>>>> +       struct clk *dsp_clk;
>>>> +       void __iomem *chipsig;
>>>> +       void __iomem *bootreg;
>>>> +       int irq;
>>>> +       int ret;
>>>> +
>> [...]
>>>> +       bootreg = devm_request_and_ioremap(dev, bootreg_res);
>>>> +       if (!bootreg) {
>>>> +               dev_err(dev, "unable to map boot register\n");
>>>> +               return -EADDRNOTAVAIL;
>>>> +       }
>>>> +
>>>> +       chipsig = devm_request_and_ioremap(dev, chipsig_res);

>>      I suggest that you use more modern (yes, already a newer interface
>> :-)
>> devm_ioremap_resource() instead -- it returns the error code (as a
>> pointer)
>> in case of error, and it certainly doesn't require you to print error
>> messages.

> Thanks, will do.

> I appreciate the notice of a more modern function, it's really tough to keep up with the flurry of activity to the kernel.

> Regarding this change, should the code use
> 	return PTR_ERR(bootreg);
> or
> 	return PTR_RET(bootreg);

    The former, to avoid duplicate IS_ERR() check.

> I ask because PTR_ERR() returns 'long' whereas PTR_RET() returns 'int' (and probe returns 'int'), but I see that the majority of existing code uses "return PTR_ERR()" in probe functions.

    But PTR_RET() uses PTR_ERR() internally anyway.

>>>> +       if (!chipsig) {
>>>> +               dev_err(dev, "unable to map CHIPSIG register\n");
>>>> +               return -EADDRNOTAVAIL;
>>>> +       }

WBR, Sergei