Re: [PATCH v5 2/2] Ethernet driver for the WIZnet W5100 chip

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- Attachment #1: Type: text/plain, Size: 2656 bytes --]

On Mon, Apr 02, 2012 at 03:40:18PM +0600, Mike Sinkovsky wrote:
> 01.04.2012 3:23, Mark Brown wrote:
> >On Fri, Mar 30, 2012 at 01:00:06PM +0600, Mike Sinkovsky wrote:

> >>+config WIZNET_W5100
> >>+	tristate "WIZnet W5100 Ethernet support"
> >>+	depends on ARM || BLACKFIN

> >What are the architecture dependencies here?

> Original driver from chip manufacturer was written for ARM, and now
> we use it on Blackfin.
> Completely untested on other arch's, but should work. Maybe.

Remove the dependency then.  If there's an issue people can fix it.

> >>+static irqreturn_t w5100_interrupt(int irq, void *ndev_instance)
> >>+{
> >>+	struct net_device *ndev = ndev_instance;
> >>+	struct w5100_priv *priv = netdev_priv(ndev);
> >>+
> >>+	int ir = w5100_read(priv, W5100_S0_IR);
> >>+	w5100_write(priv, W5100_S0_IR, ir);
> >>+
> >>+	if (ir&  S0_IR_RECV) {
> >>+		if (napi_schedule_prep(&priv->napi)) {
> >>+			w5100_write(priv, W5100_IMR, 0);
> >>+			mmiowb();
> >>+			__napi_schedule(&priv->napi);
> >>+		}
> >>+	}
> >>+
> >>+	if (ir&  S0_IR_SENDOK) {
> >>+		if (unlikely(netif_queue_stopped(ndev)))
> >>+			netif_wake_queue(ndev);
> >>+	}
> >>+
> >>+	return IRQ_HANDLED;

> >This unconditionally acknowledges the interrupt even if one wasn't
> >reported by the device.

> Hm? Don't get you.
> W5100_S0_IR register is R/W1C - writing back clears it.
> Or what do you mean?

If you read back and no interrupts are flagged (all bits in the IRQ
status register clear) you'll still return IRQ_HANDLED.

> >This is rather an abuse of the resource API and will run into trouble on
> >device tree based systems.  You should use platform data for non-DT
> >systems.

> Ok, will move it to struct wiznet_platform_data.
> But here is downside - this gpio is optional, and if board doesn't
> have it - it must be initialized as negative value, not just
> omitted.

Sure, this is all totally standard to the Linux GPIO framework.
Realistically treating 0 as an invalid GPIO should be fine.

> >>+		if (request_irq(priv->link_irq, w5100_detect_link,
> >>+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> >>+				link_name, priv->ndev)<  0)

> >Suggest using request_any_context_irq() to increase the range of
> >supported interrupt controllers.

> Could it be anything but hard irq?

It could be a threaded IRQ (if the interrupt controller can't be
accessed from hard IRQ context).

> >>+	platform_set_drvdata(pdev, NULL);
> >>+	dev_info(&pdev->dev, "probe failed (%d)\n", err);

> >This will be done for you by the driver core.

> You mean platform_set_drvdata() and dev_info()? Maybe.
> I'm sure platform_driver will not do free_netdev() for me.

The error logging.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]