Hi Andy,
Thank you very much for the review comments and your time...
On 20/4/2020 6:28 am, Andy Shevchenko wrote:
On Fri, Apr 17, 2020 at 04:21:47PM +0800, Ramuthevar,Vadivel MuruganX wrote:
From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx>
This patch adds the new IP of Nand Flash Controller(NFC) support
on Intel's Lightning Mountain(LGM) SoC.
DMA is used for burst data transfer operation, also DMA HW supports
aligned 32bit memory address and aligned data access by default.
DMA burst of 8 supported. Data register used to support the read/write
operation from/to device.
NAND controller driver implements ->exec_op() to replace legacy hooks,
these specific call-back method to execute NAND operations.
I guess untested version slipped into mailing list...
See below why.
Sorry, This is original patch only , header files are mis-aligned so
looks like un-tested patch.
...
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-direction.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/resource.h>
+#include <linux/sched.h>
+#include <linux/types.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/rawnand.h>
+#include <linux/mtd/nand_ecc.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
Do you need this?
Noted, will check and drop if it is notnecessary.
+#include <linux/mtd/partitions.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <mtd/mtd-abi.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mtd/nand.h>
Basically, do you need all of them?
And maybe keep them in order?
Sure, will update.
...
+static int lgm_dma_init(struct device *dev, struct lgm_nand_host *lgm_host)
+{
+ int ret;
+
+ /* Prepare for TX DMA: */
+ lgm_host->dma_tx = dma_request_chan(dev, "tx");
+ if (IS_ERR(lgm_host->dma_tx)) {
+ ret = PTR_ERR(lgm_host->dma_tx);
+ dev_err(dev, "can't get the TX DMA channel, error %d!\n", ret);
+ goto err;
+ }
+
+ /* Prepare for RX: */
+ lgm_host->dma_rx = dma_request_chan(dev, "rx");
+ if (IS_ERR(lgm_host->dma_rx)) {
+ ret = PTR_ERR(lgm_host->dma_rx);
+ dev_err(dev, "can't get the RX DMA channel, error %d\n", ret);
I suspect this error path hasn't been tested. I don't see where tx channel
freeing is happening.
Good catch, Thanks!, will update
+ goto err;
+ }
+
+ return 0;
+err:
+ return ret;
Redundant label.
Noted.
+}
...
+ res = devm_platform_ioremap_resource_byname(pdev, lgm_host->cs_name);
+ lgm_host->nandaddr_va = res;
+ nandaddr_pa = res->start;
+ if (IS_ERR(lgm_host->nandaddr_va))
+ return PTR_ERR(lgm_host->nandaddr_va);
I'm wonderig what is this. How is it even compile?
Agreed!, need a correction, but it's compiled.
Regards
Vadivel
