Subject: Re: [PATCH 4/4] cpsw: add switchdev support

From: Andrew Lunn <[email-masked]>
Date: Thu, 24 May 2018 15:12:29 +0200

> @@ -2626,7 +2750,11 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
>  	data->mac_control = prop;
>  
>  	if (of_property_read_bool(node, "dual_emac"))
> -		data->dual_emac = 1;
> +		data->switch_mode = CPSW_DUAL_EMAC;
> +
> +	/* switchdev overrides DTS */
> +	if (IS_ENABLED(CONFIG_TI_CPSW_SWITCHDEV))
> +		data->switch_mode = CPSW_SWITCHDEV;

Device tree is supposed to describe the hardware. Using that hardware
in different ways is not something you should describe in DT.

There are also a lot of IS_ENABLED() here, which i don't like. It is a
lot better than #ifdef, but we should try to do better. It would be
good to split this cleanly into three parts. A generic library, which
does not care about DUAL_MAC or SWITCHDEV. A driver which implements
legacy DUAL MAC etc. And a driver which implements SWITCHDEV. We can
then give this new switchdev driver a different compatible. It i still
encoding in device tree how to use the hardware, but it is more
implicit, rather than explicit.

	  Andrew