[ PATCH V2 0/7] add the GPMI controller driver for IMX23/IMX28

[Huang Shijie]

Hi:
> Hi,
>
> Huang Shijie writes:
>> Hi:
>>> Hi,
>>>
>>> Huang Shijie writes:
>>>> The general-purpose media interface(GPMI) controller is a flexible interface
>>>> to up to several NAND flashs.
>>>>
>>>> The Bose Ray-Choudhury Hocquenghem(BCH) module is a hardware ECC accelerator.
>>>>
>>>> With the help of BCH, the GPMI controller can choose to do the hardware ECC or
>>>> not.
>>>>
>>>> This driver is based the Shawn Guo's DMA patches for IMX23/IMX28,
>>>> please refer to :
>>>> 	http://git.infradead.org/users/vkoul/slave-dma.git/commit/a580b8c5429a624d120cd603e1498bf676e2b4da
>>>>
>>>> v1 -->   v2:
>>>> 	[0] merge the common files into the gpmi-nfc-main.c
>>>> 	[1] change the code to get the clock.
>>>> 	[2] remove the timing in the nand_device_info{}
>>>> 	[3] fix DMA errors
>>>> 	[4] add the nand_device_info.[ch] to generic code
>>>> 	[5] use the chip->onfi_version for the ONFI nand
>>>> 	[6] useless init
>>>> 	[7] others
>>>>
>>>> Huang Shijie (7):
>>>>     ARM: add GPMI support for imx23/imx28
>>>>     dmaengine: change the flags of request_irq()
>>>>     MTD : add the database for the NANDs
>>>>     MTD : add the common code for GPMI controller driver
>>>>     MTD : add GPMI support for imx23
>>>>     MTD : add GPMI support for imx28
>>>>     MTD : add GPMI driver in the config and Makefile
>>>>
>>>>    arch/arm/mach-mxs/Kconfig                       |    2 +
>>>>    arch/arm/mach-mxs/clock-mx23.c                  |    3 +
>>>>    arch/arm/mach-mxs/clock-mx28.c                  |    3 +
>>>>    arch/arm/mach-mxs/devices-mx23.h                |    3 +
>>>>    arch/arm/mach-mxs/devices-mx28.h                |    3 +
>>>>    arch/arm/mach-mxs/devices/Kconfig               |    3 +
>>>>    arch/arm/mach-mxs/devices/Makefile              |    1 +
>>>>    arch/arm/mach-mxs/devices/platform-gpmi.c       |  140 ++
>>>>    arch/arm/mach-mxs/include/mach/devices-common.h |    4 +
>>>>    arch/arm/mach-mxs/include/mach/gpmi-nfc.h       |   62 +
>>>>    arch/arm/mach-mxs/mach-mx23evk.c                |   37 +
>>>>    arch/arm/mach-mxs/mach-mx28evk.c                |   37 +
>>>>    drivers/dma/mxs-dma.c                           |    2 +-
>>>>    drivers/mtd/nand/Kconfig                        |   10 +
>>>>    drivers/mtd/nand/Makefile                       |    1 +
>>>>    drivers/mtd/nand/gpmi-nfc/Makefile              |    7 +
>>>>    drivers/mtd/nand/gpmi-nfc/bch-regs-imx23.h      |  342 ++++
>>>>    drivers/mtd/nand/gpmi-nfc/bch-regs-imx28.h      |  342 ++++
>>>>
>>> Those two files are identical except for the file name included in the
>>> comment.
>>> If a new SoC with differences in the register layout pops up, that
>>> should be handled by using namespace prefixes for the definitions
>>> rather than by adding new files.
>>>
>> I ever thought to merge the headers into one file, such as
>> "bch-regs-imx23_imx28.h".
>> But what about the imx508? Do i have to split the header in future.
>> Or add a separate file such as "bch-regs-imx508.h"?
>>
> If it is so much different from the others, that might be necessary.
> Otherwise you could use the same approach as proposed for i.MX23 and
> i.MX28.
>
ok. thanks.
>>>>    drivers/mtd/nand/gpmi-nfc/gpmi-regs-imx23.h     |  381 ++++
>>>>    drivers/mtd/nand/gpmi-nfc/gpmi-regs-imx28.h     |  370 ++++
>>>>
>>> These files differ only in a few definitions. They should be combined
>>> into one file and the differences sorted out using appropriate
>>> namespace prefixes like:
>>> |#define MX23_BP_GPMI_CTRL0_LOCK_CS	22
>>> |#define MX23_BM_GPMI_CTRL0_LOCK_CS	(1<<   MX23_BP_GPMI_CTRL0_LOCK_CS)
>>> |#define BP_GPMI_CTRL0_CS		20
>>> |#define MX23_BM_GPMI_CTRL0_CS		(3<<   BP_GPMI_CTRL0_CS)
>>> |#define MX28_BM_GPMI_CTRL0_CS		(7<<   BP_GPMI_CTRL0_CS)
>>>
>>> The code could either use if clauses checking the platform_id to
>>> decide which definition to use, or use hooked functions and initialize
>>> the function pointers depending on the platform_id.
>>>
>> The same issue with the imx508.
>>
>>> Also, I prefer notations like '(1<<   22)' over '0x00400000' for bit
>>> masks, because that makes it easier to match the definition with the
>>> bit numbers given in the documentation.
>>>
>>> Further the 'RSVD' entries should be removed from the register
>>> definitions (even if they are generated from the XML).
>>>
>> thanks. I will modify it in next version.
>>>>    drivers/mtd/nand/gpmi-nfc/hal-imx23.c           |  555 +++++
>>>>    drivers/mtd/nand/gpmi-nfc/hal-imx28.c           |  503 +++++
>>>>
>>> These files are more or less identical. The code should be moved to
>>> the main file and the differences sorted out using the platform_ids.
>>>
>> In actually, the two files are identical.
>>
>> It's a big headache to me.
>>
>> Do i have to sacrifice the logic clearness for reducing some code to
>> make the less code lines?
>>
> It's not (only) code size, but maintainability. If you have duplicated
> code it's harder to keep it in sync when something has to be changed.
> You will not maintain the code forever, so it's important that others
> can understand the code easily.
>
ok.

But I am sure that I will _SUPPORT_ the driver for several years. :)
>> If I merge the code to the main file, the main file will become bigger.
>>
> But much easier to handle. You can find all functions in one file and
> do not have to search several files when looking for a specific
> function.
>
sorry.

I do not think it is a good solution to package all the code in a
file. Every one thinks in different ways.

>> And I have to add the imx508 code in the main file too in future.
>>
>> What's more, the GPMI will be the only NAND controller for the IMX cpu,
>> even in the imx6x,
>> What should i do about the imx6x? still add it in the main file?
>>
> Why not? If it's so much different that this is not feasible, there is
> no point in merging it into the current driver anyway. In this case it
> would probably be better to create a completely new driver for it.
>
If i create a new driver, there will be more duplicated code. Because the
new driver can share all the code except that it should have its own 
hal-imxXX.c file.

>> An acceptable solution is merge:
>> [1] merge the imx23/imx28 into one separate file, such as hal-imx23-imx28.c
>> [2] add the hal-imx508.c or hal-imx6x.c in future.
>>
>> What about the solution? I really reluctant to merge them in the main file.
>>
> I would keep as many functions identical as possible. If there are
> simple differences like those between i.MX23 and i.MX28 they should be
> handled by if clauses inside the affected functions or by hooking
> individual functions depending on the platform_id.
> You should have a look at the i.MX CSPI driver (drivers/spi/spi_imx.c)
> which handles a variety of chips from i.MX1 thru i.MX51 in this way.
>
The i.MX cspi driver is just a small driver. GPMI driver is much bigger 
then it.
But still thanks.

> Lothar Waßmann

Best Regards
Huang Shijie