Hi Javier, >From: Javier Martinez Canillas [mailto:javier@xxxxxxxxxxxx] > >Hello Pekon, > >On Fri, May 16, 2014 at 9:07 PM, Pekon Gupta <pekon@xxxxxx> wrote: >> This patch enables 'wait-pin' monitoring in NAND driver if following properties >> are present under NAND DT node >> gpmc,wait-pin = <wait-pin number> >> gpmc,wait-on-read; >> gpmc,wait-on-write; >> As NAND generic framework uses common path nand_chip->dev_ready() for monitoring >> completion of Read and Write status, so wait-pin monitoring is enabled only when >> both 'gpmc,wait-on-read' and 'gpmc,wait-on-write' are specified. >> > >I see that the GPMC driver checks if "gpmc,wait-pin" property is >defined and only in that case tries to read both "gpmc,wait-on-read" >and "gpmc,wait-on-write" and prints a "read/write wait monitoring not >enabled!" warning if one of those is not defined. > >So my question is, it's a requirement and these 3 properties need to >always be defined together? If that is the case then I wonder why >there are 3 different properties and why not just defining >"gpmc,wait-pin" implies "gpmc,wait-on-{read,write}" ? > GPMC as a hardware engine allows selection of wait-pin independently for both Read and Write paths, but NAND generic framework uses single common function nand_chip->dev_ready() which is used for both Read and Write paths to poll wait-pin. So, in case of NAND both 'gpmc,wait-on-read' and 'gpmc,wait-on-write' should be simultaneously defined to enable wait-pin monitoring. But GPMC being generic hardware engine for NOR, OneNAND and other parallel interfaces like Camera, Ethernet the two separate bindings allow flexibility to use wait-pin monitoring for only one of the paths {Read | Write}. Therefore this check is added in gpmc_nand_init(), and not in gpmc_read_settings_dt() which is common for all type of GPMC devices (NAND, NOR, .. ) >Or if is valid to define "gpmc-wait-pin" without >"gpmc-wait-on-{read,write}" or only one those then why that scary >warning is printed? > >Whatever is the case I think that the GPMC DT binding documentation >(Documentation/devicetree/bindings/bus/ti-gpmc.txt) should be more >clear on this regard. > Agree. But I think this clarification fits better in NAND specific documentation Documentation/devicetree/bindings/mtd/gpmc-nand.txt >> Signed-off-by: Pekon Gupta <pekon@xxxxxx> >> --- >> arch/arm/mach-omap2/gpmc-nand.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c >> index 4349e82..7dc742d 100644 >> --- a/arch/arm/mach-omap2/gpmc-nand.c >> +++ b/arch/arm/mach-omap2/gpmc-nand.c >> @@ -123,11 +123,13 @@ int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data, >> } >> } >> >> - if (gpmc_nand_data->of_node) >> + if (gpmc_nand_data->of_node) { >> gpmc_read_settings_dt(gpmc_nand_data->of_node, &s); >> - else >> + if (s.wait_on_read && s.wait_on_write) > >You said that wait-on-pin monitoring is only enabled if both >"gpmc,wait-on-read" and "gpmc,wait-on-write" are specified. But in >that case I think we should clear the wait_pin on >gpmc_read_settings_dt() if either "gpmc,wait-on-read" or >"gpmc,wait-on-write" were not specified and check s.wait_pin instead. > >Or is this semantic only for NAND and other devices connected to the >GPMC behave differently wrt "gpmc,wait-pin"? > True, this is only for NAND. Other device drivers may use wait-pin either 'only for Read' or 'only for Write' access. However I doubt why anyone would do so, unless there is some design bug | constrain. >> + gpmc_nand_data->dev_ready = "true"; > >You should really use gpmc_nand_data->dev_ready = true here. The C99 >standard allows you to assign a string literal to a _Bool type and >will be evaluated to 1 but that is confusing and I haven't seen that >used in other part of the kernel. > Sorry, my bad. I over looked it. I'll fix it in next version along with Documentation update. With regards, pekon ��.n��������+%������w��{.n�����{�������ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
