Re: [PATCH 2/2] net: dsa: yt921x: Add support for Motorcomm YT921x

[Andrew Lunn <andrew@lunn.ch>]

> +#undef dev_dbg
> +#define dev_dbg dev_info

Things like this should not be in a driver submitted to mainline.  It
is also just as easy to add #define DEBUG to the first line of the
file and recomplie to get _dbg() prints working.

> +
> +/******** hardware definitions ********/
> +
> +#define YT921X_SMI_SWITCHIDf		GENMASK(3, 2)
> +#define  YT921X_SMI_SWITCHIDv(x)		FIELD_PREP(YT921X_SMI_SWITCHIDf, (x))
> +#define YT921X_SMI_ADf			BIT(1)
> +#define  YT921X_SMI_ADDRv			0
> +#define  YT921X_SMI_DATAv			YT921X_SMI_ADf
> +#define YT921X_SMI_RWf			BIT(0)
> +#define  YT921X_SMI_WRITEv			0
> +#define  YT921X_SMI_READv			YT921X_SMI_RWf

More of these lower case postfixes.

Documentation/process/coding-style.rst says:

  Encoding the type of a function into the name (so-called Hungarian
  notation) is asinine - the compiler knows the types anyway and can
  check those, and it only confuses the programmer.

This looks like a form of Hungarian notation. Please don't do that.

> +struct yt921x_chip_info {
> +	const char *name;
> +	u16 id;
> +	u8 mode;
> +	u8 extmode;
> +	u16 intif_mask;
> +	u16 extif_mask;
> +};
> +
> +static const struct yt921x_chip_info yt921x_chip_infos[] = {
> +	{ "YT9215SC", 0x9002, 1, 0, GENMASK(4, 0),   BIT(8) | BIT(9), },
> +	{ "YT9215S",  0x9002, 2, 0, GENMASK(4, 0),   BIT(8),          },
> +	{ "YT9215RB", 0x9002, 3, 0, GENMASK(4, 0),   BIT(8) | BIT(9), },
> +	{ "YT9214NB", 0x9002, 3, 2, BIT(1) | BIT(3), BIT(8) | BIT(9), },
> +	{ "YT9213NB", 0x9002, 3, 3, BIT(1) | BIT(3), BIT(9),          },
> +	{ "YT9218N",  0x9001, 0, 0, GENMASK(7, 0),   0,               },
> +	{ "YT9218MB", 0x9001, 1, 0, GENMASK(7, 0),   BIT(8) | BIT(9), },

Please add a few more #defines. e.g. what does mode = 1 mean? A
#define with a good name can explain that.

#define EXTERNAL_IF_PORT_8 BIT(8)
#define EXTERNAL_IF_PORT_9 BIT(9)

etc.

> +static int
> +__yt921x_smi_read(struct yt921x_priv *priv, u32 reg, u32 *valp)

Try to avoid using __ symbols. They are supposed to be used by the
compiler.

> +static int yt921x_smi_read(struct yt921x_priv *priv, u32 reg, u32 *valp)
> +{
> +	int res;
> +
> +	res = yt921x_smi_acquire(priv);
> +	if (unlikely(res != 0))

SMI operations are not fast path. Don't use unlikely(). A typical DSA
driver never touches actual Ethernet frames, so there is no need for
this anywhere in this code.

> +static int yt921x_dsa_setup(struct dsa_switch *ds)
> +{
> +	/* Always register one mdio bus for the internal/default mdio bus. This
> +	 * maybe represented in the device tree, but is optional.
> +	 */
> +	child = of_get_child_by_name(np, "mdio");
> +	res = yt921x_mbus_int_init(priv, child);
> +	of_node_put(child);
> +	if (unlikely(res != 0))
> +		return res;
> +	ds->user_mii_bus = priv->mbus_int;
> +
> +	/* Walk the device tree, and see if there are any other nodes which say
> +	 * they are compatible with the external mdio bus.
> +	 */
> +	priv->mbus_ext = NULL;
> +	for_each_available_child_of_node(np, child) {
> +		if (!of_device_is_compatible(child,
> +					     "motorcomm,yt921x-mdio-external"))
> +			continue;
> +
> +		res = yt921x_mbus_ext_init(priv, child);
> +		if (unlikely(res != 0)) {
> +			of_node_put(child);
> +			return res;
> +		}
> +		break;

Please add a device tree binding. It needs to be the first patch of
the series, and you need to Cc: the device tree Maintainers.


    Andrew

---
pw-bot: cr