Hi,

On Sun, Sep 13, 2015 at 03:42:41PM +0200, Gerhard Bertelsmann wrote:

quoted

quoted

drivers/net/can/Kconfig                            |  10 +
drivers/net/can/Makefile                           |   1 +
drivers/net/can/sunxi_can.c                        | 879
+++++++++++++++++++++
3 files changed, 890 insertions(+)

diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index e8c96b8..439f67c 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig

@@ -129,6 +129,16 @@ config CAN_RCAR

	  To compile this driver as a module, choose M here: the module will
	  be called rcar_can.

+config CAN_SUNXI
+	tristate "Allwinner SUNXI CAN controller"

It would be better if we could mention that it's a driver for the
sun4i family and derived.

I'm using it on Bpi with A20. AFAIK a SUN7I. Anyway I'm not aware
of all Allwinner SoCs. I will change it to any text you want.

CAN_SUN4I and Allwinner A10 CAN controller will do just fine.

Now that I'm thinking of it, can you also enable this driver in the
sunxi and multi_v7 defconfigs please (in separate patches)?

quoted

We have no guarantee that there won't be any IP on other sunxi SoCs
incompatible with this one that would need a new driver.

Right. IMHO A10 and A20 are working - maybe Fxx(?).

I was more thinking of the A31, A23, A80 and alike, but yeah, it might
be true for the F20 too.

quoted

quoted

+static int sunxican_open(struct net_device *dev)
+{
+	int err;
+
+	/* common open */
+	err = open_candev(dev);
+	if (err)
+		return err;
+
+	/* register interrupt handler */
+	err = request_irq(dev->irq, sunxi_can_interrupt, IRQF_SHARED,
+			  dev->name, dev);
+	if (err) {
+		netdev_err(dev, "request_irq err: %d\n", err);
+		close_candev(dev);
+		return -EAGAIN;
+	}
+
+	sunxi_can_start(dev);
+
+	can_led_event(dev, CAN_LED_EVENT_OPEN);
+
+	netif_start_queue(dev);
+
+	return 0;
+}
+
+static int sunxican_close(struct net_device *dev)
+{
+	struct sunxican_priv *priv = netdev_priv(dev);
+
+	netif_stop_queue(dev);
+	set_reset_mode(dev);
+	clk_disable_unprepare(priv->clk);

Are you sure you have the right use count on that clock? You're
calling sunxi_can_start from several places, but you call
clk_disable_unprepare only from here.

I will check this (again).

quoted

Even if it does start, it is really confusing. Please move the
clk_prepare_enable in the open function.

That was my first approach. IMHO it's useful to enable/disable
according to the state of the CAN interface to save power.

Yes, it makes sense to have the clock enabled/disabled in open/close.

I was just saying that these calls should be balanced, and having the
clk_prepare_enable in the sunxi_can_start doesn't help that.

quoted

quoted

+static int __maybe_unused sunxi_can_suspend(struct device *device)
+{
+	struct net_device *dev = dev_get_drvdata(device);
+	struct sunxican_priv *priv = netdev_priv(dev);
+	u32 mode;
+	int err;
+
+	if (netif_running(dev)) {
+		netif_stop_queue(dev);
+		netif_device_detach(dev);
+	}
+
+	err = set_reset_mode(dev);
+	if (err)
+		return err;
+
+	mode = readl(priv->base + SUNXI_REG_MSEL_ADDR);
+	writel(mode | SUNXI_MSEL_SLEEP_MODE, priv->base +
SUNXI_REG_MSEL_ADDR);
+
+	priv->can.state = CAN_STATE_SLEEPING;
+
+	clk_disable(priv->clk);
+	return 0;
+}
+
+static int __maybe_unused sunxi_can_resume(struct device *device)
+{
+	struct net_device *dev = dev_get_drvdata(device);
+	struct sunxican_priv *priv = netdev_priv(dev);
+	u32 mode;
+	int err;
+
+	err = clk_enable(priv->clk);
+	if (err) {
+		netdev_err(dev, "clk_enable() failed, error %d\n", err);
+		return err;
+	}
+
+	err = set_reset_mode(dev);
+	if (err)
+		return err;
+
+	mode = readl(priv->base + SUNXI_REG_MSEL_ADDR);
+	writel(mode & ~(SUNXI_MSEL_SLEEP_MODE),
+	       priv->base + SUNXI_REG_MSEL_ADDR);
+	err = set_normal_mode(dev);
+	if (err)
+		return err;
+
+	priv->can.state = CAN_STATE_ERROR_ACTIVE;
+	if (netif_running(dev)) {
+		netif_device_attach(dev);
+		netif_start_queue(dev);
+	}
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(sunxi_can_pm_ops, sunxi_can_suspend,
sunxi_can_resume);

How did you test those hooks? There's no suspend support yet.

I didn't test them.

Then remove them.

quoted

quoted

+static struct platform_driver sunxi_can_driver = {
+	.driver = {
+		.name = DRV_NAME,
+		.pm = &sunxi_can_pm_ops,
+		.of_match_table = sunxican_of_match,
+	},
+	.probe = sunxican_probe,
+	.remove = sunxican_remove,
+};
+
+module_platform_driver(sunxi_can_driver);
+
+MODULE_AUTHOR("Peter Chen [off-list ref]");
+MODULE_AUTHOR("Gerhard Bertelsmann [off-list ref]");
+MODULE_LICENSE("Dual BSD/GPL");
+MODULE_DESCRIPTION(DRV_NAME "CAN driver for Allwinner SoCs (A10/A20)");
+MODULE_VERSION(DRV_MODULE_VERSION);

Is this going to be useful?

The module version is already redundant with the kernel version, and
there's a good chance it will never reach 1.0.

Will remove the version number.

General remark:

Somebody asked me to add linux-sunxi dev list. Seems to be not a
good idea before the driver was ready. Seems to be hard to fulfill
the differentness.

I will remove linux-sunxi for the next patch set. I will try to get
linux-can people satisfied and then I'm done with it. I'm doing this
in my spare time, but it seems to get an endless story. I'm not
offended in any kind, my spare time is just limited.

The driver is working so far and the patches also apply to 4.3.

Cc'ing linux-sunxi is great, but it's an orthogonal issue.

Whenever you post a patch, you're supposed to Cc all the maintainers
listed for the MAINTAINERS file, and the get_maintainers.pl script
helps you with that.

If you use that script on your driver, you'll get:

./scripts/get_maintainer.pl -f drivers/net/can/sunxi_can.c
Wolfgang Grandegger [off-list ref] (maintainer:CAN NETWORK DRIVERS)
Marc Kleine-Budde [off-list ref] (maintainer:CAN NETWORK DRIVERS)
Maxime Ripard [off-list ref] (maintainer:ARM/Allwinner A1X SoC support)
linux-can-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list:CAN NETWORK DRIVERS)
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list:NETWORKING DRIVERS)
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org (moderated list:ARM/Allwinner A1X SoC support)
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list)

All these people are supposed to be reviewing your driver, and while
Marc will have a look at how your driver interact with the CAN
framework, I'll have a look at how your driver interact with the rest
of the code we did for the Allwinner SoCs. We look at different
aspects, we have different kind of comments.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com