Hi,
On Sun, Sep 13, 2015 at 03:42:41PM +0200, Gerhard Bertelsmann wrote:
>> 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)?
>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.
>>+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).
>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.
>>+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.
>>+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 <[email protected]>");
>>+MODULE_AUTHOR("Gerhard Bertelsmann <[email protected]>");
>>+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 <[email protected]> (maintainer:CAN NETWORK
DRIVERS)
Marc Kleine-Budde <[email protected]> (maintainer:CAN NETWORK DRIVERS)
Maxime Ripard <[email protected]>
(maintainer:ARM/Allwinner A1X SoC support)
[email protected] (open list:CAN NETWORK DRIVERS)
[email protected] (open list:NETWORKING DRIVERS)
[email protected] (moderated list:ARM/Allwinner A1X
SoC support)
[email protected] (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
