On 2015-08-25 13:16, Brian Norris wrote:

A few more comments.

On Mon, Aug 03, 2015 at 11:27:26AM +0200, Stefan Agner wrote:

quoted

diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
new file mode 100644
index 0000000..5c8dfe8
--- /dev/null
+++ b/drivers/mtd/nand/vf610_nfc.c

@@ -0,0 +1,645 @@

...

quoted

+/*
+ * This function supports Vybrid only (MPC5125 would have full RB and four CS)
+ */
+static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
+{
+#ifdef CONFIG_SOC_VF610

Why the #ifdef? I don't see anything compile-time specific to SOC_VF610.

If this is trying to handle the comment above ("This function supports
Vybrid only (MPC5125 would have full RB and four CS)") then that's the
wrong way of doing it, as you need to support multiplatform kernels.
You'll need to have a way to differentiate the different platform
support at runtime, not compile time.

Yes it is trying to handle the comment above. Well, the other two
platforms I am aware of are also different architectures... (PowerPC and
ColdFire). I think we won't have a multi-architecture kernel anytime
soon, hence I think removing the code at compile time is the right thing
todo.

However, probably CONFIG_SOC_VF610 is the wrong symbol then, I could
just use CONFIG_ARM and add a comment that this might be different on
another other ARM SoC than VF610.

Just checked CodingStyle, and I see that IS_ENABLED is the preferred way
for conditional compiling.

So my suggestion:

static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
{
	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
	u32 tmp = vf610_nfc_read(nfc, NFC_ROW_ADDR);

	if (!IS_ENABLED(CONFIG_ARM))
		return;

	/*
	 * This code is only tested on the ARM platform VF610
	 * PowerPC based MPC5125 would have full RB and four CS
	 */
....

With that the compiler should be able to remove this (currently) ARM
VF610 specific code on the other supported architectures...

What do you think?

quoted

+	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
+	u32 tmp = vf610_nfc_read(nfc, NFC_ROW_ADDR);
+
+	tmp &= ~(ROW_ADDR_CHIP_SEL_RB_MASK | ROW_ADDR_CHIP_SEL_MASK);
+	tmp |= 1 << ROW_ADDR_CHIP_SEL_RB_SHIFT;
+
+	if (chip == 0)
+		tmp |= 1 << ROW_ADDR_CHIP_SEL_SHIFT;
+	else if (chip == 1)
+		tmp |= 2 << ROW_ADDR_CHIP_SEL_SHIFT;

	else ... ?

Maybe you can write this as a formulaic pattern (e.g.:

	tmp |= (chip + 1) << ROW_ADDR_CHIP_SEL_SHIFT;

) and just do the "max # of chips" checks on a per-platform basis in the
probe(). Then I'm guessing this same function can apply to both
platforms. (I'm not looking at HW datasheets for this, BTW, just
guessing based on the context here.)

It seems that MCP5125 is different than VF610. MCP5125 has 4 chip
selects and 4 R/B signals, whereas VF610 has only 2 chip selects and
just 1 R/B signals...

But wait...I see that you call nand_scan_ident() with a max of 1 chip.
So you won't ever see the chip > 0 case, right?

So does this driver support multiple flash attached or not? Looks like
you're assuming you'll only be using chip-select 0. (This is fine for
now, but at least your code should acknowledge this. Perhaps a comment
at the top under "limitations.")

Ok, will add that information under limitations.

quoted

+
+	vf610_nfc_write(nfc, NFC_ROW_ADDR, tmp);
+#endif
+}

...

quoted

+static int vf610_nfc_probe(struct platform_device *pdev)
+{

...

quoted

+	/* first scan to find the device and get the page size */
+	if (nand_scan_ident(mtd, 1, NULL)) {
+		err = -ENXIO;
+		goto error;
+	}

--
Stefan