Re: [PATCH v2 2/4] mtd: atmel_nand: add Nand Flash Controller (NFC) support

[Josh Wu <josh.wu@atmel.com>]

Hi, JC

On 5/27/2013 6:26 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:

[snip]

>>> yes but you need to update the compatible or check the IP support it too
>>> as has-nfc will mean I have the nfs on the hardware but not enable it
>>>
>>> the SoC dtsi will describe you have it but the board will decide to enable it or
>>> not
>> I can add some code to check whether is NFC supported by read SMC IP
>> version. But I'm afraid
>> such code may not work for AVR arch.
>>
>> Another way is just add a 'enable-nfc' compatible string. Which is
>> for board to enable the NFC feature.
>>
>> I'm prefer to the later one. What do you think?
> I do not like no 2
>
> I like this
>
> 		nand0: nand@60000000 {
> 			compatible = "atmel,at91rm9200-nand";
> 			#address-cells = <1>;
> 			#size-cells = <1>;
> 			reg = < 0x60000000 0x01000000   /* EBI CS3 */
> 				0xffffc070 0x00000490   /* SMC PMECC regs */
> 				0xffffc500 0x00000100   /* SMC PMECC Error Location regs */
> 				0x00100000 0x00100000   /* ROM code */
> 				>;
> 			interrupts = <5 IRQ_TYPE_LEVEL_HIGH 6>;
> 			atmel,nand-addr-offset = <21>;
> 			atmel,nand-cmd-offset = <22>;
> 			pinctrl-names = "default";
> 			pinctrl-0 = <&pinctrl_nand0_ale_cle>;
> 			atmel,pmecc-lookup-table-offset = <0x10000 0x18000>;
> 			status = "disabled";
>
> 			nfc@70000000 {
> 				compatible = "atmel,sama5d3-nfc";
> 				reg = < 0x70000000 0x10000000   /* NFC Command Registers */
> 					0xffffc000 0x00000070   /* NFC HSMC regs */
> 					0x00200000 0x00100000   /* NFC SRAM banks */
> 				>;
> 			}
> 		};
>
> so you can enable/disable it easly

Since I am not use such dts definition before, following is my guest 
implementation for above dts:
So I need define an additional platform driver for "atmel,sama5d3-nfc" 
with probe/remove function. right?
if yes, then there are two question are rise out:
1. which driver will be probed first? or it is depends? Since I think 
the "atmel,sama5d3-nfc" can be probe first is good for me.
2. how do these two drivers share data with others? I check at91 pinctrl 
as a example, and it use a static pointer array store gpio data, and 
pinctrl can access it. Seems that is enough for me.

>
>
>>>>   Examples:
>>>>   nand0: nand@40000000,0 {
>>>> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
>>>> index f747791..48d7ee6 100644
>>>> --- a/drivers/mtd/nand/atmel_nand.c
>>>> +++ b/drivers/mtd/nand/atmel_nand.c
>>>> @@ -18,6 +18,9 @@
>>>>    *  Add Programmable Multibit ECC support for various AT91 SoC
>>>>    *     © Copyright 2012 ATMEL, Hong Xu
>>>>    *
>>>> + *  Add Nand Flash Controller support for SAMA5 SoC
>>>> + *     © Copyright 2013 ATMEL, Josh Wu (josh.wu@atmel.com)
>>>> + *
>>>>    * This program is free software; you can redistribute it and/or modify
>>>>    * it under the terms of the GNU General Public License version 2 as
>>>>    * published by the Free Software Foundation.
>>>> @@ -37,9 +40,11 @@
>>>>   #include <linux/mtd/nand.h>
>>>>   #include <linux/mtd/partitions.h>
>>>> +#include <linux/delay.h>
>>>>   #include <linux/dmaengine.h>
>>>>   #include <linux/gpio.h>
>>>>   #include <linux/io.h>
>>>> +#include <linux/interrupt.h>
>>>>   #include <linux/platform_data/atmel.h>
>>>>   #include <linux/pinctrl/consumer.h>
>>>> @@ -58,6 +63,7 @@ module_param(on_flash_bbt, int, 0);
>>>>   	__raw_writel((value), add + ATMEL_ECC_##reg)
>>>>   #include "atmel_nand_ecc.h"	/* Hardware ECC registers */
>>>> +#include "atmel_nand_nfc.h"	/* Nand Flash Controller definition */
>>>>   /* oob layout for large page size
>>>>    * bad block info is on bytes 0 and 1
>>>> @@ -85,6 +91,13 @@ static struct nand_ecclayout atmel_oobinfo_small = {
>>>>   	},
>>>>   };
> ....
>>>> +				dev_err(&pdev->dev,
>>>> +					"can't request input direction rdy gpio %d\n",
>>>> +					host->board.rdy_pin);
>>>> +				goto err_ecc_ioremap;
>>>> +			}
>>>> +
>>>> +			nand_chip->dev_ready = atmel_nand_device_ready;
>>>>   		}
>>>> -		res = gpio_direction_output(host->board.enable_pin, 1);
>>>> -		if (res < 0) {
>>>> -			dev_err(&pdev->dev,
>>>> -				"can't request output direction enable gpio %d\n",
>>>> -				host->board.enable_pin);
>>>> -			goto err_ecc_ioremap;
>>>> +		if (gpio_is_valid(host->board.enable_pin)) {
>>>> +			res = gpio_request(host->board.enable_pin,
>>> we need to use devm_ here too
>>> and everywhere we can in this driver
>> yes, but seems you already sent one patch to use devm_ for this driver.
>> https://patchwork.kernel.org/patch/1589071/
>> It is not merged in at91 tree yet.
>>
>> So what do you think if I put that patch rebased and put it as one
>> of this NFC series patches?
> yes do so

ok. I will rebase the patch and including it with nfc patches.

Best Regards,
Josh Wu

>
> Best Regards,
> J.