Re: [PATCH v2 net-next 3/6] net: bcmasp: Add support for ASP2.0 Ethernet controller

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Le 26/04/2023 à 20:54, Justin Chen a écrit :
Add support for the Broadcom ASP 2.0 Ethernet controller which is first
introduced with 72165. This controller features two distinct Ethernet
ports that can be independently operated.

This patch supports:

- Wake-on-LAN using magic packets
- basic ethtool operations (link, counters, message level)
- MAC destination address filtering (promiscuous, ALL_MULTI, etc.)

Signed-off-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
Signed-off-by: Justin Chen <justinpopo6@xxxxxxxxx>
---

[...]

+void bcmasp_disable_all_filters(struct bcmasp_intf *intf)
+{
+	struct bcmasp_priv *priv = intf->parent;
+	unsigned int i;

Hi,

Nit: Some loop index are unsigned int, but most are int.
This could be done consistantly.

+
+	/* Disable all filters held by this port */
+	for (i = ASP_RX_FILT_MDA_RES_COUNT(intf); i < NUM_MDA_FILTERS; i++) {
+		if (priv->mda_filters[i].en &&
+		    priv->mda_filters[i].port == intf->port)
+			bcmasp_en_mda_filter(intf, 0, i);
+	}
+}

[...]

+static int bcmasp_probe(struct platform_device *pdev)
+{
+	struct device_node *ports_node, *intf_node;
+	const struct bcmasp_plat_data *pdata;
+	struct device *dev = &pdev->dev;
+	int ret, i, count = 0, port;
+	struct bcmasp_priv *priv;
+	struct bcmasp_intf *intf;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->irq = platform_get_irq(pdev, 0);
+	if (priv->irq <= 0) {
+		dev_err(dev, "invalid interrupt\n");
+		return -EINVAL;
+	}
+
+	priv->clk = devm_clk_get_optional_enabled(dev, "sw_asp");
+	if (IS_ERR(priv->clk)) {
+		dev_err(dev, "failed to request clock\n");
+		return PTR_ERR(priv->clk);
+	}
+
+	/* Base from parent node */
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base)) {
+		dev_err(dev, "failed to iomap\n");
+		return PTR_ERR(priv->base);
+	}
+
+	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
+	if (ret)
+		ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));

I don't think that this fallback is needed.
See [1].

More over, using dev_err_probe() would slighly simplify the probe function. (saves a few LoC, logs the error code in a human reading format)

[1]: https://lore.kernel.org/lkml/86bf852e-4220-52d4-259d-3455bc24def1@xxxxxxxxxx/T/#m022abc0051ede3ba1feeb06cefd59e2a8a5c7864

+	if (ret) {
+		dev_err(&pdev->dev, "unable to set DMA mask: %d\n", ret);
+		return ret;
+	}
+

[...]

+static int __maybe_unused bcmasp_suspend(struct device *d)
+{
+	struct bcmasp_priv *priv = dev_get_drvdata(d);
+	struct bcmasp_intf *intf;
+	unsigned int i;

Same

+	int ret = 0;

no need to initialize, but it is mostmy a matter of taste.

+
+	for (i = 0; i < priv->intf_count; i++) {
+		intf = priv->intfs[i];
+		if (!intf)
+			continue;
+
+		ret = bcmasp_interface_suspend(intf);
+		if (ret)
+			break;
+	}
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		return ret;
+
+	/* Whether Wake-on-LAN is enabled or not, we can always disable
+	 * the shared TX clock
+	 */
+	bcmasp_core_clock_set(priv, 0, ASP_CTRL_CLOCK_CTRL_ASP_TX_DISABLE);
+
+	bcmasp_core_clock_select(priv, true);
+
+	clk_disable_unprepare(priv->clk);
+
+	return ret;
+}
+
+static int __maybe_unused bcmasp_resume(struct device *d)
+{
+	struct bcmasp_priv *priv = dev_get_drvdata(d);
+	struct bcmasp_intf *intf;
+	unsigned int i;

same

+	int ret = 0;

no need to initialize, but it is mostmy a matter of taste.

Just my 2c,
CJ





[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux