Re: [PATCH v2 net-next] net: bcmgenet: enable driver to work without a device tree

[Florian Fainelli <f.fainelli@gmail.com>]

On 01/12/14 15:23, Florian Fainelli wrote:
> On 01/12/14 14:08, Petri Gynther wrote:
>> Broadcom 7xxx MIPS-based STB platforms do not use device trees.
>> Modify bcmgenet driver so that it can be used on those platforms.
> 
> Well, that statement is technically not true anymore thanks to Kevin's
> recent efforts, but this is not exactly what we have been
> advertising/supporting so far.
> 
> Looks mostly good to me, some minor nits below:
> 
>>
>> Signed-off-by: Petri Gynther <pgynther@google.com>
>> ---
> [snip]
>> +	if (dn) {
>> +		of_id = of_match_node(bcmgenet_match, dn);
>> +		if (!of_id)
>> +			return -EINVAL;
>> +	} else {
>> +		of_id = NULL;
>> +	}
> 
> You could probably get way with this else condition by assigning of_id
> to NULL by default.
> 
> [snip]
> 
>> +		if (pd->phy_addr >= 0 && pd->phy_addr < PHY_MAX_ADDR) {
>> +			phydev = mdio->phy_map[pd->phy_addr];
>> +		} else {
>> +			phydev = NULL;
>> +			for (i = 0; i < PHY_MAX_ADDR; i++) {
>> +				if (mdio->phy_map[i]) {
>> +					phydev = mdio->phy_map[i];
>> +					break;
>> +				}
>> +			}
> 
> phy_find_first() might provide a shorter version of this.
> 
>> +		}
>> +
>> +		if (!phydev) {
>> +			dev_err(kdev, "failed to register PHY device\n");
>> +			mdiobus_unregister(mdio);
>> +			return -ENODEV;
>> +		}
>> +	} else {
>> +		/*
>> +		 * MoCA port or no MDIO access.
>> +		 * Use 1000/FD fixed PHY to represent the link layer.
>> +		 */
>> +		struct fixed_phy_status fphy_status = {
>> +			.link = 1,
>> +			.duplex = DUPLEX_FULL,
>> +			.speed = SPEED_1000,
>> +			.pause = 0,
>> +			.asym_pause = 0,
>> +		};
>> +
>> +		phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
>> +		if (!phydev || IS_ERR(phydev)) {
>> +			dev_err(kdev, "failed to register fixed PHY device\n");
>> +			return -ENODEV;
>> +		}
> 
> This is typically done by platform code (not necessarily for good
> reasons though) but I cannot seen any problems with doing this here as well.

Well actually there could be one issue, we don't want to assume
1Gbits/sec here, we would want to be communicated a speed information
(typically ETH0_SPEED) in CFE so we can program the link parameters
accordingly, just in case someone interfaces this GENET instance with
e.g: a non-MDIO managed external 10/100 switch.

> 
> [snip]
> 
>> diff --git a/include/linux/platform_data/bcmgenet.h b/include/linux/platform_data/bcmgenet.h
>> new file mode 100644
>> index 0000000..3660133
>> --- /dev/null
>> +++ b/include/linux/platform_data/bcmgenet.h
>> @@ -0,0 +1,18 @@
>> +#ifndef __LINUX_PLATFORM_DATA_BCMGENET_H__
>> +#define __LINUX_PLATFORM_DATA_BCMGENET_H__
>> +
>> +#include <linux/compiler.h>
> 
> That include does not look necessary, you might want linux/types.h for
> "u8" though.
> 
>> +#include <linux/if_ether.h>
>> +
>> +struct bcmgenet_platform_data {
>> +	void __iomem *base_reg;
>> +	int           irq0;
>> +	int           irq1;
> 
> These 3 members are unused and should be communicated to the driver as
> resources, not side parameters, can you get rid of them?
> 
>> +	bool          mdio_enabled;
>> +	int           phy_type;
> 
> This is essentially phy_interface_t, so let's use that type directly here.
> 
>> +	int           phy_addr;
>> +	u8            macaddr[ETH_ALEN];
>> +	int           genet_version;
> 
> That is also an enum, if that helps, you could move it from
> drivers/net/ethernet/broadcom/bcmgenet.h there as well.
> --
> Florian
>