Re: [RFC PATCH v2 2/4] net: ethernet: xilinx: Add gmii2rgmii converter support

Le 04/07/2016 13:47, Appana Durga Kedareswara Rao a écrit :

Hi Nicolas,

	Thanks for the review...

quoted

quoted

diff --git a/include/linux/xilinx_gmii2rgmii.h

b/include/linux/xilinx_gmii2rgmii.h
new file mode 100644
index 0000000..b328ee7

--- /dev/null
+++ b/include/linux/xilinx_gmii2rgmii.h

@@ -0,0 +1,24 @@

Here, header of the file seems needed.

Sure will fix in the next version...

quoted

quoted

+#ifndef _GMII2RGMII_H
+#define _GMII2RGMII_H
+
+#include <linux/of.h>
+#include <linux/phy.h>
+#include <linux/mii.h>
+
+#define XILINX_GMII2RGMII_FULLDPLX		BMCR_FULLDPLX
+#define XILINX_GMII2RGMII_SPEED1000		BMCR_SPEED1000
+#define XILINX_GMII2RGMII_SPEED100		BMCR_SPEED100
+#define XILINX_GMII2RGMII_REG_NUM			0x10
+
+struct gmii2rgmii {
+	struct net_device	*dev;
+	struct mii_bus		*mii_bus;
+	struct phy_device	*gmii2rgmii_phy_dev;
+	void			*platform_data;
+	int (*mdio_write)(struct mii_bus *bus, int mii_id, int reg,
+			  u16 val);
+	void (*fix_mac_speed)(struct gmii2rgmii *xphy, unsigned int speed);
+};
+
+extern int gmii2rgmii_phyprobe(struct gmii2rgmii *xphy); #endif

I see a compilation issue here:

You should provide a way to have this function even if the NET_VENDOR_XILINX
config option is not selected (test to compile with the sama5_defconfig and
you'll see).

Ok will fix in the next version...

quoted

What about making this function void in case of !XILINX?

This is one way to get rid of compilation error. Changes will be look like below

#ifdef CONFIG_NET_VENDOR_XILINX

You may need to have:
#if defined(CONFIG_NET_VENDOR_XILINX) && defined(CONFIG_XILINX_GMII2RGMII)

 extern int gmii2rgmii_phyprobe(struct gmii2rgmii *xphy);
#else
extern void gmii2rgmii_phyprobe(struct gmii2rgmii *xphy);

No need for the line above...

void gmii2rgmii_phyprobe(struct gmii2rgmii *xphy)
{
}

On one single line, like:

static inline void gmii2rgmii_phyprobe(struct gmii2rgmii *xphy) { }

#endif
For me the changes are looking odd...

For me, it's okay...

Other possible ways 
	1)  Put a config check around phyprobe api in the macb driver.
#ifdef CONFIG_XILINX_GMII2RGMII
                       gmii2rgmii_phyprobe(&bp->converter_phy);
#endif

Nope!

	2) Select NET_VENDOR_XILINX in the macb Kconfig 
@ -22,6 +22,7 @@ config MACB
        tristate "Cadence MACB/GEM support"
        depends on HAS_DMA
        select PHYLIB
+       select NET_VENDOR_XILINX

Please let me know which one you prefer will fix that and will post v3...

First one with my changes is the best. But maybe wait for more feedback...

Bye,
-- 
Nicolas Ferre