Re: [PATCH] can: can_oc: Add driver for CAN_OC cores from Aeroflex Gaisler

[Marc Kleine-Budde <mkl@pengutronix.de>]

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

On 09/25/2012 07:52 AM, Andreas Larsson wrote:
> This driver is for the sja1000 compatible CAN_OC cores from Aeroflex
> Gaisler available in the GRLIB VHDL IP core library.

Why don't you describe a single sja1000 compatible core with an OF
device? The devices have indipendent address spaces and IRQs. With a
proper abstraction/desciption you would not need this driver.

> Multiple CAN controllers might be included in one platform device. The
> number of controllers is indicated by the "version" Open Firmware
> property of the device plus one.

Some general remarks:
- please use devm_* function, they do automatically cleanup if probe()
  fails or if the driver is unloaded. I've written the devm_* function
  names inline.

- Please avoid the double pointer void ** array:
  Create a "struct can_oc_priv" holding all your private data.
  You can work with an "open array" [1] to hold the pointers to the
  struct net_device.
  (I personally prefer an explizid length,
   not a NULL pointer terminated array.)

[1]  http://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html


> 
> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
> ---
>  drivers/net/can/sja1000/Kconfig  |    7 +
>  drivers/net/can/sja1000/Makefile |    1 +
>  drivers/net/can/sja1000/can_oc.c |  274 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 282 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/net/can/sja1000/can_oc.c
> 
> diff --git a/drivers/net/can/sja1000/Kconfig b/drivers/net/can/sja1000/Kconfig
> index 03df9a8..3573dc8 100644
> --- a/drivers/net/can/sja1000/Kconfig
> +++ b/drivers/net/can/sja1000/Kconfig
> @@ -105,4 +105,11 @@ config CAN_TSCAN1
>  	IRQ numbers are read from jumpers JP4 and JP5,
>  	SJA1000 IO base addresses are chosen heuristically (first that works).
>  
> +config CAN_CAN_OC
> +	tristate "Aeroflex Gaisler CAN_OC driver"
> +	depends on SPARC
> +	---help---
> +	  This driver is for CAN_OC cores from Aeroflex Gaisler
> +	  (http://www.gaisler.com).
> +
>  endif
> diff --git a/drivers/net/can/sja1000/Makefile b/drivers/net/can/sja1000/Makefile
> index b3d05cb..0186332 100644
> --- a/drivers/net/can/sja1000/Makefile
> +++ b/drivers/net/can/sja1000/Makefile
> @@ -13,5 +13,6 @@ obj-$(CONFIG_CAN_PEAK_PCMCIA) += peak_pcmcia.o
>  obj-$(CONFIG_CAN_PEAK_PCI) += peak_pci.o
>  obj-$(CONFIG_CAN_PLX_PCI) += plx_pci.o
>  obj-$(CONFIG_CAN_TSCAN1) += tscan1.o
> +obj-$(CONFIG_CAN_CAN_OC) += can_oc.o
>  
>  ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/sja1000/can_oc.c b/drivers/net/can/sja1000/can_oc.c
> new file mode 100644
> index 0000000..3c39c8e
> --- /dev/null
> +++ b/drivers/net/can/sja1000/can_oc.c
> @@ -0,0 +1,274 @@
> +/*
> + * Socket CAN driver for CAN_OC cores from Aeroflex Gaisler.
> + *
> + * 2012 (c) Aeroflex Gaisler AB
> + *
> + * General organization derived from sja1000_of_platform driver:
> + * - Copyright (C) 2008-2009 Wolfgang Grandegger <wg@grandegger.com>
> + *
> + * This driver supports CAN_OC CAN controllers available in the GRLIB
> + * VHDL IP core library.
> + *
> + * Full documentation of the CAN_OC core can be found here:
> + * http://www.gaisler.com/products/grlib/grip.pdf
> + *
> + * One device can contain several CAN_OC instantiations. The number of
> + * instantiations is indicated by the "version" Open Firmware property
> + * of the device plus one. The interrupt offset between such
> + * instantiations is 1 and the register base address offset between
> + * them is 0x100.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + *
> + * Contributors: Andreas Larsson <andreas@gaisler.com>
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/netdevice.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/can/dev.h>
> +
> +#include <linux/of_platform.h>
> +#include <asm/prom.h>
> +
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +
> +#include "sja1000.h"
> +
> +#define DRV_NAME "can_oc"
> +
> +MODULE_AUTHOR("Aeroflex Gaisler AB.");
> +MODULE_DESCRIPTION("Socket CAN driver for CAN_OC cores from Aeroflex Gaisler");
> +MODULE_LICENSE("GPL");
> +
> +#define CORE_OFFSET 0x100;

Please ad CAN_OC_ in front of that constant.

> +
> +static u8 can_oc_read_reg(const struct sja1000_priv *priv, int reg)
> +{
> +	return ioread8(priv->reg_base + reg);
> +}
> +
> +static void can_oc_write_reg(const struct sja1000_priv *priv,
> +				  int reg, u8 val)
> +{
> +	iowrite8(val, priv->reg_base + reg);
> +}
> +
> +/*
> + * Frees all devices in the NULL-terminated array and frees the
> + * array. Returns the base address of the register space
> + * (i.e. reg_base for core 0).
> + */
> +static void __iomem *can_oc_release_base_and_devices(void **base_and_devices)
> +{
> +	struct net_device *dev;
> +	struct sja1000_priv *priv;
> +	int i;
> +	void __iomem *base = (void __iomem *)base_and_devices[0];
> +	for (i = 0; base_and_devices[i+1]; i++) {
> +		if (IS_ERR(base_and_devices[i+1]))
> +			continue;
> +		dev = (struct net_device *)base_and_devices[i+1];
> +		priv = netdev_priv(dev);
> +		unregister_sja1000dev(dev);
> +		irq_dispose_mapping(dev->irq);
> +		free_sja1000dev(dev);
> +	}
> +	kfree(base_and_devices);
> +	return base;
> +}
> +
> +/*
> + * Sets up and registers core with the specified index returning the
> + * net_device in the out parameter odev
> + */
> +static int __devinit can_oc_core_probe(struct platform_device *ofdev,
> +					void __iomem *base, int index,
> +					struct net_device **odev)
> +{
> +	struct device_node *np = ofdev->dev.of_node;
> +	struct sja1000_priv *priv;
> +	const u32 *prop;
> +	int err, irq, prop_size;
> +	struct net_device *dev;
> +
> +	*odev = NULL;
> +	irq = irq_of_parse_and_map(np, index);
> +	if (irq == NO_IRQ) {
> +		dev_err(&ofdev->dev, "no irq found for core %d\n", index);
> +		return -ENODEV;
> +	}
> +
> +	dev = alloc_sja1000dev(0);
> +	if (!dev) {
> +		err = -ENOMEM;
> +		dev_err(&ofdev->dev,
> +			"unable to allocate memory for core %d\n", index);
> +		goto exit_dispose_irq;
> +	}
> +	dev->irq = irq;
> +
> +	priv = netdev_priv(dev);
> +	priv->reg_base = base + index * CORE_OFFSET;
> +	priv->irq_flags = IRQF_SHARED;
> +	priv->read_reg = can_oc_read_reg;
> +	priv->write_reg = can_oc_write_reg;
> +
> +	prop = of_get_property(np, "freq", &prop_size);
> +	if (prop && (prop_size ==  sizeof(u32))) {
> +		priv->can.clock.freq = *prop / 2;
> +	} else {
> +		err = -ENODEV;
> +		dev_err(&ofdev->dev, "unable to get \"freq\" property\n");
> +		goto exit_free_sja1000;
> +	}
> +
> +	dev_info(&ofdev->dev, "core %d: reg_base=0x%p irq=%d clock=%d\n",
> +		 index, priv->reg_base, dev->irq, priv->can.clock.freq);
> +	SET_NETDEV_DEV(dev, &ofdev->dev);
> +
> +	err = register_sja1000dev(dev);
> +	if (err) {
> +		dev_err(&ofdev->dev, "registering core %d failed (err=%d)\n",
> +			index, err);
> +		goto exit_free_sja1000;
> +	}
> +
> +	*odev = dev;
> +	return 0;
> +
> +exit_free_sja1000:
> +	free_sja1000dev(dev);
> +exit_dispose_irq:
> +	irq_dispose_mapping(irq);
> +
> +	return err;
> +}
> +
> +/* Initialize the CAN_OC Socket CAN */
> +static int __devinit can_oc_of_probe(struct platform_device *ofdev)
> +{
> +	struct device_node *np = ofdev->dev.of_node;
> +	struct resource res;
> +	const u32 *prop;
> +	int cores, i, err, res_size, prop_size;
> +	void __iomem *base;
> +	struct net_device *dev;
> +
> +	/* NULL-terminated array of pointers: base_and_devices[0] will
> +	 * contain the base address for the register address space for
> +	 * all the cores and base_and_devices[i+1] will contain a
> +	 * pointer to the net_device structure of core i */
> +	void **base_and_devices;

Please avoid this double pointer. base

> +
> +	const u32 *ampopts = of_get_property(np, "ampopts", NULL);

Please use of_property_read_u32() instead.

> +	if (ampopts) {
> +		dev_info(&ofdev->dev, "ampopts: 0x%08x\n", *ampopts);
> +		/* Ignore if used by another OS instance */
> +		if (*ampopts == 0)
> +			return -ENODEV;
> +	}
> +
> +	err = of_address_to_resource(np, 0, &res);
> +	if (err) {
> +		dev_err(&ofdev->dev, "invalid address\n");
> +		return err;
> +	}
> +
> +	res_size = resource_size(&res);
> +	if (!request_mem_region(res.start, res_size, DRV_NAME)) {
> +		dev_err(&ofdev->dev, "couldn't request %pR\n", &res);
> +		return -EBUSY;
> +	}
> +
> +	base = ioremap_nocache(res.start, res_size);
> +	if (!base) {
> +		dev_err(&ofdev->dev, "couldn't ioremap %pR\n", &res);
> +		err = -ENOMEM;
> +		goto exit_release_mem;
> +	}

Please use:

	platform_get_resource(pdev, IORESOURCE_MEM, 0);
	devm_request_and_ioremap(dev, res);

> +
> +	prop = of_get_property(np, "version", &prop_size);

of_property_read_u32()

> +	if (prop && (prop_size ==  sizeof(u32))) {
> +		cores = *prop + 1;
> +		dev_info(&ofdev->dev, "found %d cores\n", cores);
> +	} else {
> +		cores = 1; /* default */
> +		dev_err(&ofdev->dev,
> +			"Unable to determine number of cores - assuming one");
> +	}
> +
> +	base_and_devices = kzalloc((cores + 2) * sizeof(void *), GFP_KERNEL);

devm_kzalloc()

> +	if (!base_and_devices) {
> +		dev_err(&ofdev->dev, "couldn't allocate memory\n");
> +		err = -ENOMEM;
> +		goto exit_unmap_mem;
> +	}
> +	dev_set_drvdata(&ofdev->dev, base_and_devices);
> +
> +	base_and_devices[0] = base;
> +	for (i = 0; i < cores; i++) {
> +		if (!ampopts || (1 << i) & *ampopts) {
> +			err = can_oc_core_probe(ofdev, base, i, &dev);
> +			if (err)
> +				goto exit_release_base_and_devices;
> +			base_and_devices[i+1] = dev;
> +		} else {
> +			base_and_devices[i+1] = ERR_PTR(-ENXIO);

Why not just NULL, or rather nothing, as the array already initialized
with 0x0.

> +		}
> +	}
> +
> +	return 0;
> +
> +exit_release_base_and_devices:
> +	can_oc_release_base_and_devices(base_and_devices);
> +	dev_set_drvdata(&ofdev->dev, NULL);
> +exit_unmap_mem:
> +	iounmap(base);
> +exit_release_mem:
> +	release_mem_region(res.start, res_size);
> +
> +	return err;
> +}
> +
> +static int __devexit can_oc_of_remove(struct platform_device *ofdev)
> +{
> +	void **base_and_devices = dev_get_drvdata(&ofdev->dev);
> +	void __iomem *base;
> +	struct device_node *np = ofdev->dev.of_node;
> +	struct resource res;
> +
> +	base = can_oc_release_base_and_devices(base_and_devices);
> +	dev_set_drvdata(&ofdev->dev, NULL);
> +	iounmap(base);
> +
> +	of_address_to_resource(np, 0, &res);
> +	release_mem_region(res.start, resource_size(&res));
> +
> +	return 0;
> +}
> +
> +static struct of_device_id can_oc_of_match[] = {
> +	{.name = "GAISLER_CANAHB"},
> +	{.name = "01_019"},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, can_oc_of_match);
> +
> +static struct platform_driver can_oc_of_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +		.owner = THIS_MODULE,
> +		.of_match_table = can_oc_of_match,
> +	},
> +	.probe = can_oc_of_probe,
> +	.remove = __devexit_p(can_oc_of_remove),
> +};
> +
> +module_platform_driver(can_oc_of_driver);
> 

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]