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

Andrew Lunn
2018-05-24T13:12:31Z

> @@ -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