Hi,

On 3/4/2016 9:17 PM, Arnd Bergmann wrote:

On Friday 04 March 2016 17:22:19 Joao Pinto wrote:

quoted

This patch adds a glue pci driver for the Synopsys G210 Test Chip.

Signed-off-by: Joao Pinto <jpinto-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>

Mostly ok, just a few suggestions:

quoted

+
+/* Test Chip type expected values */
+#define TC_G210_20BIT 20
+#define TC_G210_40BIT 40
+#define TC_G210_DEFAULTBIT 40
+
+static int tc_type = TC_G210_DEFAULTBIT;
+module_param(tc_type, int, 0);
+MODULE_PARM_DESC(tc_type, "Test Chip Type (20 = 20-bit, 40 = 40-bit)");

What is the effect of setting the wrong one here? I was thinking
it would be best to have the default be 'invalid' and then return
an error from the probe() function when you neither value is
set.

You're right. Maybe an TC_G210_20BIT_INV by default and also check if the
inserted value is TC_G210_20BIT or TC_G210_40BIT. if not, then abort the probe.

quoted

+
+	/* Check Test Chip type and set the specific setup routine */
+	if (tc_type == TC_G210_20BIT) {
+		tc_dwc_g210_pci_hba_vops.custom_phy_initialization =
+						tc_dwc_g210_config_20_bit;
+	} else if (tc_type == TC_G210_40BIT) {
+		tc_dwc_g210_pci_hba_vops.custom_phy_initialization =
+						tc_dwc_g210_config_40_bit;
+	}

As for the platform driver, I would define two separate structures here,
and then mark the operations as 'const'.

quoted

+static const struct pci_device_id tc_dwc_g210_pci_tbl[] = {
+	{ PCI_VENDOR_ID_SYNOPSYS, 0xB101, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
+	{ PCI_VENDOR_ID_SYNOPSYS, 0xB102, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
+	{ }	/* terminate list */
+};

Is there any difference between these two IDs?

The Synopsys can be identified by one of these 2 IDs. There is no particular reason.

	Arnd

Joao
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html