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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <577A572F.1050004@atmel.com>
Date:	Mon, 4 Jul 2016 14:31:43 +0200
From:	Nicolas Ferre <nicolas.ferre@...el.com>
To:	Appana Durga Kedareswara Rao <appana.durga.rao@...inx.com>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	"mark.rutland@....com" <mark.rutland@....com>,
	Michal Simek <michals@...inx.com>,
	Soren Brinkmann <sorenb@...inx.com>,
	Punnaiah Choudary Kalluri <punnaia@...inx.com>,
	"f.fainelli@...il.com" <f.fainelli@...il.com>,
	"andrew@...n.ch" <andrew@...n.ch>,
	Anirudha Sarangi <anirudh@...inx.com>,
	Harini Katakam <harinik@...inx.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: 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...
> 
>>> 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...
> 
>>
>>> +#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...
> 
>>
>> 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