Re: [PATCH 06/17] mtd: rawnand: cafe: Factor out the controller initialization logic

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: linux-mtd@lists.infradead.org,
	Tudor Ambarus <tudor.ambarus@microchip.com>,
	David Woodhouse <dwmw2@infradead.org>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Richard Weinberger <richard@nod.at>
Subject: Re: [PATCH 06/17] mtd: rawnand: cafe: Factor out the controller initialization logic
Date: Mon, 27 Apr 2020 21:45:35 +0200	[thread overview]
Message-ID: <20200427214535.7d424c8f@xps13> (raw)
In-Reply-To: <20200427082028.394719-7-boris.brezillon@collabora.com>

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Apr
2020 10:20:16 +0200:

> The same code is present in the probe and resume path. Let's create
> an init helper to factor that out.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Nice catch! A nitpick below.

> ---
>  drivers/mtd/nand/raw/cafe_nand.c | 189 +++++++++++--------------------
>  1 file changed, 67 insertions(+), 122 deletions(-)
> 

[...]

> @@ -980,74 +988,11 @@ MODULE_DEVICE_TABLE(pci, cafe_nand_tbl);
>  
>  static int cafe_nand_resume(struct pci_dev *pdev)
>  {
> -	uint32_t ctrl;
>  	struct mtd_info *mtd = pci_get_drvdata(pdev);
>  	struct nand_chip *chip = mtd_to_nand(mtd);
>  	struct cafe_priv *cafe = nand_get_controller_data(chip);
>  
> -       /* Start off by resetting the NAND controller completely */
> -	cafe_writel(cafe, CAFE_GLOBAL_RESET_NAND, GLOBAL_RESET);
> -	cafe_writel(cafe, 0, GLOBAL_RESET);
> -	cafe_writel(cafe, 0xffffffff, NAND_IRQ_MASK);
> -
> -	/* Restore timing configuration */
> -	cafe_writel(cafe, timing[0], NAND_TIMING1);
> -	cafe_writel(cafe, timing[1], NAND_TIMING2);
> -	cafe_writel(cafe, timing[2], NAND_TIMING3);
> -
> -        /* Disable master reset, enable NAND clock */
> -	ctrl = cafe_readl(cafe, GLOBAL_CTRL);
> -	ctrl &= ~(CAFE_GLOBAL_SW_RESET_SET |
> -		  CAFE_GLOBAL_SW_RESET_CLEAR |
> -		  CAFE_GLOBAL_MASTER_RESET_SET |
> -		  CAFE_GLOBAL_MASTER_RESET_CLEAR |
> -		  CAFE_GLOBAL_NAND_CLK_ENABLE);
> -	ctrl |= CAFE_GLOBAL_NAND_CLK_ENABLE |
> -		CAFE_GLOBAL_SDH_CLK_ENABLE |
> -		CAFE_GLOBAL_CCIC_CLK_ENABLE;
> -	cafe_writel(cafe,
> -		    ctrl |
> -		    CAFE_GLOBAL_MASTER_RESET_SET |
> -		    CAFE_GLOBAL_SW_RESET_SET,
> -		    GLOBAL_CTRL);
> -	cafe_writel(cafe,
> -		    ctrl |
> -		    CAFE_GLOBAL_MASTER_RESET_CLEAR |
> -		    CAFE_GLOBAL_SW_RESET_CLEAR,
> -		    GLOBAL_CTRL);
> -
> -	cafe_writel(cafe, 0, NAND_DMA_CTRL);
> -
> -	cafe_writel(cafe,
> -		    CAFE_GLOBAL_NAND_CLK_ENABLE |
> -		    CAFE_GLOBAL_SDH_CLK_ENABLE |
> -		    CAFE_GLOBAL_CCIC_CLK_ENABLE |
> -		    CAFE_GLOBAL_MASTER_RESET_SET |
> -		    CAFE_GLOBAL_SW_RESET_CLEAR,
> -		    GLOBAL_CTRL);
> -	cafe_writel(cafe,
> -		    CAFE_GLOBAL_NAND_CLK_ENABLE |
> -		    CAFE_GLOBAL_SDH_CLK_ENABLE |
> -		    CAFE_GLOBAL_CCIC_CLK_ENABLE |
> -		    CAFE_GLOBAL_MASTER_RESET_CLEAR |
> -		    CAFE_GLOBAL_SW_RESET_CLEAR,
> -		    GLOBAL_CTRL);
> -
> -	/* Set up DMA address */
> -	cafe_writel(cafe, cafe->dmaaddr & 0xffffffff, NAND_DMA_ADDR0);
> -	if (sizeof(cafe->dmaaddr) > 4)
> -	/* Shift in two parts to shut the compiler up */
> -		cafe_writel(cafe, (cafe->dmaaddr >> 16) >> 16, NAND_DMA_ADDR1);
> -	else
> -		cafe_writel(cafe, 0, NAND_DMA_ADDR1);
> -
> -	/* Enable NAND IRQ in global IRQ mask register */
> -	cafe_writel(cafe,
> -		    CAFE_GLOBAL_IRQ_PCI_ERROR |
> -		    CAFE_GLOBAL_IRQ_CCIC |
> -		    CAFE_GLOBAL_IRQ_SDH |
> -		    CAFE_GLOBAL_IRQ_NAND,
> -		    GLOBAL_IRQ_MASK);
> +	cafe_nand_init(cafe);

A new line would be nice here :p

>  	return 0;
>  }
>  

Thanks,
Miquèl

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