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
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
signature.asc
Description: Digital signature
