Hello,
>> > --- a/drivers/mtd/nand/raw/cadence-nand-controller.c
>> > +++ b/drivers/mtd/nand/raw/cadence-nand-controller.c
>> > @@ -2908,7 +2908,7 @@ static int cadence_nand_init(struct cdns_nand_ctrl
>> *cdns_ctrl)
>> > if (!cdns_ctrl->dmac) {
>> > dev_err(cdns_ctrl->dev,
>> > "Unable to get a DMA channel\n");
>> > - ret = -EBUSY;
>> > + ret = -EPROBE_DEFER;
>>
>> Does it work if there is no DMA channel provided? The bindings do not mention
>> DMA channels as mandatory.
>>
>
> The way Cadence NAND controller driver is written in such a way that it uses
> has_dma=1 as hardcoded value, indicating that slave DMA interface is connected
> to DMA engine. However, it does not utilize the dedicated DMA channel information
> from the device tree.
This is not ok.
> Driver works without external DMA interface i.e. has_dma=0.
> However current driver does not have a mechanism to configure it from
> device tree.
What? Why are you requesting a DMA channel from a dmaengine in this case?
Please make the distinction between the OS implementation (the driver)
and the DT binding which describe the HW and only the HW.
>> Also, wouldn't it be more pleasant to use another helper from the DMA core
>> that returns a proper return code? So we now which one among -EBUSY, -
>> ENODEV or -EPROBE_DEFER we get?
>>
>
> Agree.
> I will change to "dma_request_chan_by_mask" instead of "dma_request_channel "
> so it can return a proper error code.
>
> cdns_ctrl->dmac = dma_request_chan_by_mask(&mask);
> if (IS_ERR(cdns_ctrl->dmac)) {
> ret = PTR_ERR(cdns_ctrl->dmac);
> if (ret != -EPROBE_DEFER)
> dev_err(cdns_ctrl->dev,
> "Failed to get a DMA channel:%d\n",ret);
> goto disable_irq;
> }
>
> Is this reasonable?
It is better, but maybe you can use dev_err_probe() instead to include
the EPROBE_DEFER error handling.
Thanks,
Miquèl
