Re: [PATCH net-next v2 4/4] net: dsa: soce: Add basic support for SoC-e switch IP cores

[Andrew Lunn]

> +#define SOCE_MAX_NUM_PORTS 31
> +#define SOCE_MAX_MDIO_ADDR 32

Given what the binding says, that looks odd.

> +#define SOCE_MAX_MDIO_OUTPUTS SOCE_MAX_NUM_PORTS
> +
> +struct dsa_switch;
> +
> +struct soce_mdio_ops {
> +	int (*phy_read)(struct dsa_switch *ds, int mdio_output, int phy_addr,
> +			int regnum);
> +	int (*phy_write)(struct dsa_switch *ds, int mdio_output, int phy_addr,
> +			 int regnum, u16 val);
> +	int (*phy_read_c45)(struct dsa_switch *ds, int mdio_output, int phy_addr,
> +			    int devad, int regnum);
> +	int (*phy_write_c45)(struct dsa_switch *ds, int mdio_output, int phy_addr,
> +			     int devad, int regnum, u16 val);
> +};
> +
> +struct soce_dsa_local {
> +	void __iomem *base_addr;
> +	void __iomem *mdio_master_addr;
> +	/* Serializes all logical buses sharing the MDIO master. */
> +	struct mutex mdio_lock;

What is the MDIO master?

> +static const struct soce_mdio_ops soce_mdio_ops_c22_c45 = {
> +	.phy_read	= soce_mdio_23_02_read,
> +	.phy_write	= soce_mdio_23_02_write,
> +	.phy_read_c45	= soce_mdio_23_02_read_c45,
> +	.phy_write_c45	= soce_mdio_23_02_write_c45,
> +};

It seems like this is the only struct soce_mdio_ops. Does the IP
support different MDIO buses? Is this level of abstraction actually
needed?

> +static inline bool soce_mdio_output_valid(int mdio_output)
> +{
> +	return mdio_output >= 0 && mdio_output < SOCE_MAX_MDIO_OUTPUTS;
> +}

No inline functions in .c files. Let the compile decide.

> +static inline bool soce_mdio_addr_valid(int phy_addr)
> +{
> +	return phy_addr >= 0 && phy_addr < SOCE_MAX_MDIO_ADDR;
> +}
> +
> +static inline bool soce_mdio_c22_reg_valid(int regnum)
> +{
> +	return regnum >= 0 && regnum <= SOCE_MDIO_C22_REG_MAX;
> +}
> +
> +static inline bool soce_mdio_c45_params_valid(int devad, int regnum)
> +{
> +	return devad >= 0 && devad <= SOCE_MDIO_C45_DEVAD_MAX &&
> +	       regnum >= 0 && regnum <= SOCE_MDIO_C45_REG_MAX;
> +}

Do you see any other MDIO driver doing this sort of checking?

> +static int soce_mdio_read(struct mii_bus *bus, int addr, int reg)
> +{
> +	struct soce_mdio_bus *state = bus->priv;
> +	struct soce_dsa_local *local;
> +	struct soce_priv *priv;
> +	int ret;
> +
> +	priv = state->ds->priv;
> +	local = &priv->local;
> +
> +	if (!local->mdio_ops || !local->mdio_ops->phy_read)
> +		return -EOPNOTSUPP;
> +
> +	if (!soce_mdio_addr_valid(addr))
> +		return -EINVAL;
> +
> +	mutex_lock(&local->mdio_lock);
> +	ret = local->mdio_ops->phy_read(state->ds, state->mdio_output, addr,
> +					reg);
> +	mutex_unlock(&local->mdio_lock);

What is the lock protecting?

> +static int soce_register_mdio_bus(struct soce_priv *priv, struct device *dev,
> +				  struct device_node *mdio_np,
> +				  u32 mdio_output)
> +{
> +	struct soce_mdio_bus *state;
> +	struct mii_bus *bus;
> +
> +	bus = devm_mdiobus_alloc(dev);
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	state->ds = priv->ds;
> +	state->mdio_output = mdio_output;

Can you think of a better name than mdio_output. It seems to be the
bus number?

> +
> +	bus->priv = state;
> +	bus->name = "soce mdio";
> +	bus->read = soce_mdio_read;
> +	bus->write = soce_mdio_write;
> +	bus->read_c45 = soce_mdio_read_c45;
> +	bus->write_c45 = soce_mdio_write_c45;
> +	/* ds->dst can be NULL during probe, before dsa_register_switch() */
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mdio-%u", dev_name(dev),
> +		 mdio_output);
> +	bus->parent = dev;
> +
> +	return devm_of_mdiobus_register(dev, bus, mdio_np);
> +}
> +int soce_mdio_23_02_read_c45(struct dsa_switch *ds, int mdio_output,
> +			     int phy_addr, int devad, int regnum)
> +{
> +	void __iomem *ctrl, *params, *read_reg, *write_reg;
> +	struct soce_priv *priv = ds->priv;
> +	struct soce_dsa_local *local;
> +	u32 regvalue;
> +	int ret;
> +
> +	if (!soce_mdio_output_valid(mdio_output) ||
> +	    !soce_mdio_addr_valid(phy_addr) ||
> +	    !soce_mdio_c45_params_valid(devad, regnum))
> +		return -EINVAL;

Hasn't this already been checked once?

	Andrew