Re: [PATCH v3 4/4] net: mtip: The L2 switch driver for imx287

[Andrew Lunn <andrew@lunn.ch>]

> +static void read_atable(struct switch_enet_private *fep, int index,
> +			unsigned long *read_lo, unsigned long *read_hi)
> +{
> +	unsigned long atable_base = (unsigned long)fep->hwentry;
> +
> +	*read_lo = readl((const void *)atable_base + (index << 3));
> +	*read_hi = readl((const void *)atable_base + (index << 3) + 4);
> +}
> +
> +static void write_atable(struct switch_enet_private *fep, int index,
> +			 unsigned long write_lo, unsigned long write_hi)
> +{
> +	unsigned long atable_base = (unsigned long)fep->hwentry;
> +
> +	writel(write_lo, (void *)atable_base + (index << 3));
> +	writel(write_hi, (void *)atable_base + (index << 3) + 4);
> +}

It would be nice to have the mtip_ prefix on all functions.

> +static int mtip_open(struct net_device *dev)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +	struct switch_enet_private *fep = priv->fep;
> +	int ret, port_idx = priv->portnum - 1;
> +
> +	if (fep->usage_count == 0) {
> +		clk_enable(fep->clk_ipg);
> +		netif_napi_add(dev, &fep->napi, mtip_rx_napi);
> +
> +		ret = mtip_alloc_buffers(dev);
> +		if (ret)
> +			return ret;

nitpick: You might want to turn the clock off before returning the
error.

> +	}
> +
> +	fep->link[port_idx] = 0;
> +
> +	/* Probe and connect to PHY when open the interface, if already
> +	 * NOT done in the switch driver probe (or when the device is
> +	 * re-opened).
> +	 */
> +	ret = mtip_mii_probe(dev);
> +	if (ret) {
> +		mtip_free_buffers(dev);

I've not checked. Does this do the opposite of netif_napi_add()?

> +static void mtip_set_multicast_list(struct net_device *dev)
> +{
> +	unsigned int i, bit, data, crc;
> +
> +	if (dev->flags & IFF_PROMISC) {
> +		dev_info(&dev->dev, "%s: IFF_PROMISC\n", __func__);

You can save one level of indentation with a return here.

> +	} else {
> +		if (dev->flags & IFF_ALLMULTI) {
> +			dev_info(&dev->dev, "%s: IFF_ALLMULTI\n", __func__);

and other level here.

> +		} else {
> +			struct netdev_hw_addr *ha;
> +			u_char *addrs;
> +
> +			netdev_for_each_mc_addr(ha, dev) {
> +				addrs = ha->addr;
> +				/* Only support group multicast for now */
> +				if (!(*addrs & 1))
> +					continue;

You could pull there CRC caluclation out into a helper. You might also
want to search the tree and see if it exists somewhere else.

> +
> +				/* calculate crc32 value of mac address */
> +				crc = 0xffffffff;
> +
> +				for (i = 0; i < 6; i++) {

Is 6 the lengh of a MAC address? There is a #define for that.

> +					data = addrs[i];
> +					for (bit = 0; bit < 8;
> +					     bit++, data >>= 1) {
> +						crc = (crc >> 1) ^
> +						(((crc ^ data) & 1) ?
> +						CRC32_POLY : 0);
> +					}
> +				}
> +			}
> +		}
> +	}
> +}
> +

> +struct switch_enet_private *mtip_netdev_get_priv(const struct net_device *ndev)
> +{
> +	if (ndev->netdev_ops == &mtip_netdev_ops)
> +		return netdev_priv(ndev);
> +
> +	return NULL;
> +}
> +

> +static int __init mtip_switch_dma_init(struct switch_enet_private *fep)
> +{
> +	struct cbd_t *bdp, *cbd_base;
> +	int ret, i;
> +
> +	/* Check mask of the streaming and coherent API */
> +	ret = dma_set_mask_and_coherent(&fep->pdev->dev, DMA_BIT_MASK(32));
> +	if (ret < 0) {
> +		dev_warn(&fep->pdev->dev, "No suitable DMA available\n");

Can you recover from this? Or should it be dev_err()?

More later...

	Andrew