Re: [net-next v2 2/3] net: ethernet: adi: Add ADIN1110 support

[Andrew Lunn]

> +static int adin1110_read_reg(struct adin1110_priv *priv, u16 reg, u32 *val)
> +{
> +	struct spi_transfer t[2] = {0};
> +	__le16 __reg = cpu_to_le16(reg);
> +	u32 header_len = ADIN1110_RD_HEADER_LEN;
> +	u32 read_len = ADIN1110_REG_LEN;
> +	int ret;

Reverse Christmass tree. Here and everywhere.

I would also drop the __ from __reg. Maybe call it le_reg?

> +static int adin1110_read_fifo(struct adin1110_port_priv *port_priv)
> +{

...

> +	frame_size_no_fcs = frame_size - ADIN1110_FRAME_HEADER_LEN - ADIN1110_FEC_LEN;
> +
> +	rxb = netdev_alloc_skb(port_priv->netdev, frame_size_no_fcs);
> +	if (!rxb)
> +		return -ENOMEM;
> +
> +	memset(priv->data, 0, round_len + ADIN1110_RD_HEADER_LEN);
> +
> +	priv->data[0] = ADIN1110_CD | FIELD_GET(GENMASK(12, 8), __reg);
> +	priv->data[1] = FIELD_GET(GENMASK(7, 0), __reg);
> +
> +	if (priv->append_crc) {
> +		priv->data[2] = adin1110_crc_data(&priv->data[0], 2);
> +		header_len++;
> +	}
> +
> +	t[0].tx_buf = &priv->data[0];
> +	t[0].len = header_len;
> +
> +	t[1].rx_buf = &priv->data[header_len];
> +	t[1].len = round_len;
> +
> +	ret = spi_sync_transfer(priv->spidev, t, 2);
> +	if (ret) {
> +		kfree_skb(rxb);
> +		return ret;
> +	}
> +
> +	/* Already forwarded to the other port if it did not match any MAC Addresses. */
> +	if (priv->forwarding)
> +		rxb->offload_fwd_mark = 1;
> +
> +	skb_put(rxb, frame_size_no_fcs);
> +	skb_copy_to_linear_data(rxb, &priv->data[header_len + ADIN1110_FRAME_HEADER_LEN],
> +				frame_size_no_fcs);

It looks like you could receive the frame direction into the skb. You
can consider the header_len + ADIN1110_FRAME_HEADER_LEN as just
another protocol header on the frame, which you remove using the
normal skb_ API, before passing the frame to the stack. That will save
you this memcpy.

> +
> +	rxb->protocol = eth_type_trans(rxb, port_priv->netdev);
> +
> +	netif_rx(rxb);
> +
> +	port_priv->rx_bytes += frame_size - ADIN1110_FRAME_HEADER_LEN;
> +	port_priv->rx_packets++;
> +
> +	return 0;
> +}
> +

> +/* ADIN1110 MAC-PHY contains an ADIN1100 PHY.
> + * ADIN2111 MAC-PHY contains two ADIN1100 PHYs.
> + * By registering a new MDIO bus we allow the PAL to discover
> + * the encapsulated PHY and probe the ADIN1100 driver.
> + */
> +static int adin1110_register_mdiobus(struct adin1110_priv *priv, struct device *dev)
> +{
> +	struct mii_bus *mii_bus;
> +	int ret;
> +
> +	mii_bus = devm_mdiobus_alloc(dev);
> +	if (!mii_bus)
> +		return -ENOMEM;
> +
> +	snprintf(priv->mii_bus_name, MII_BUS_ID_SIZE, "%s-%u",
> +		 priv->cfg->name, priv->spidev->chip_select);
> +
> +	mii_bus->name = priv->mii_bus_name;
> +	mii_bus->read = adin1110_mdio_read;
> +	mii_bus->write = adin1110_mdio_write;
> +	mii_bus->priv = priv;
> +	mii_bus->parent = dev;
> +	mii_bus->phy_mask = ~((u32)GENMASK(2, 0));
> +	mii_bus->probe_capabilities = MDIOBUS_C22_C45;

Your read/write functions return -EOPNOTSUPP on C45. So this is wrong.

> +static irqreturn_t adin1110_irq(int irq, void *p)
> +{
> +	struct adin1110_priv *priv = p;
> +	u32 status1;
> +	u32 val;
> +	int ret;
> +	int i;
> +
> +	mutex_lock(&priv->lock);

The MDIO bus operations are using the same lock. MDIO can be quite
slow. Do you really need mutual exclusion between MDIO and interrupts?
What exactly is this lock protecting?

  Andrew