Re: [PATCH net-next v3 12/14] onsemi: s2500: Add driver support for TS2500 MAC-PHY

[Andrew Lunn]

> +config S2500_MACPHY
> +	tristate "S2500 support"
> +	depends on SPI
> +	select NCN26000_PHY
> +	select OA_TC6
> +	help
> +	  Support for the onsemi TS2500 MACPHY Ethernet chip.
> +          It works under the framework that conform to OPEN Alliance
> +          10BASE-T1x Serial Interface specification.

tab vs spaces.

> +#define DRV_VERSION			"1.0.3.1"

Not used, and probably meaningless.

> +/* Definitions for MMS defined in Table 6 Open Alliance TC6 standard
> + * that not present in oa_tc6.h
> + */
> +#define S2500_OA_TC6_MACPHY_MMS0	0
> +#define S2500_OA_TC6_MAC_MMS1		1
> +#define S2500_OA_TC6_VS1_MMS12		12

Maybe these should be added to oa_tc6.h, if they are part of the
standard.

> +#define S2500_MMS_MII			(S2500_OA_TC6_MACPHY_MMS0 << 16)
> +#define S2500_MMS_MAC			(S2500_OA_TC6_MAC_MMS1 << 16)
> +#define S2500_MMS_PCS			(OA_TC6_PHY_C45_PCS_MMS2 << 16)
> +#define S2500_MMS_PMDPMA		(OA_TC6_PHY_C45_PMA_PMD_MMS3 << 16)
> +#define S2500_MMS_VS1			(S2500_OA_TC6_VS1_MMS12 << 16)
> +#define S2500_MMS_VS2			(OA_TC6_PHY_C45_VS_PLCA_MMS4 << 16)

This kind of suggests we should rethink the API for reading/writing
registers. It should be possible to pass MMS and register as seperate
values, and let the core combine them.

> +struct s2500_info;
> +
> +struct s2500_info {

???

> +/* Initializes the net device MAC address by reading the UID stored

Should that be OUI, not UID?

> +	/* Convert the OID in host byte order */
> +	for (i = 2; i >= 0; --i) {
> +		addr[i] = 0;
> +		for (j = 0; j < 8; ++j) {
> +			addr[i] |= (val & 1) << (7 - j);
> +			val >>= 1;
> +		}
> +	}

That seems pretty complex. What exactly is it doing?

	Andrew