Re: [PATCH v2 1/3] mtd: rawnand: cadence: support deferred prob when DMA is not ready

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: "Rabara, Niravkumar L" <niravkumar.l.rabara@intel.com>
Cc: Richard Weinberger <richard@nod.at>,
	 Vignesh Raghavendra <vigneshr@ti.com>,
	 "linux@treblig.org" <linux@treblig.org>,
	 Shen Lichuan <shenlichuan@vivo.com>,
	 Jinjie Ruan <ruanjinjie@huawei.com>,
	"u.kleine-koenig@baylibre.com" <u.kleine-koenig@baylibre.com>,
	"linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>
Subject: Re: [PATCH v2 1/3] mtd: rawnand: cadence: support deferred prob when DMA is not ready
Date: Wed, 29 Jan 2025 11:02:49 +0100	[thread overview]
Message-ID: <874j1i0wfq.fsf@bootlin.com> (raw)
In-Reply-To: <BL3PR11MB653276DFD3339ADAADC70CCFA2EE2@BL3PR11MB6532.namprd11.prod.outlook.com> (Niravkumar L. Rabara's message of "Wed, 29 Jan 2025 09:17:29 +0000")

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/