Hello, as mentioned, for kernel inclusion you need to prepare patches for David Millers "net-next-2.6" GIT tree, which you can get as shown below:
$ git clone \ git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next-2.6.git The patch should then be sent to the netdev mailing list with CC to the Socketcan-core and other related mailing lists. Furthermore, your patch should be checked with checkpatch.pl to find the obvious coding style violations. Many lines are too long. More details about the coding style requirements can be found in the kernel "Documentation/CodingStyle" file. Other general comment: - please use "static inline functions" in favor of macro definitions. - the "static inline functions" should be short (just a few lines) and the name should be in lowercase. - please use #defines or enums for constants. - please check if structures for the register layout results in better code, as discussed. - please remove extensive debugging output mainly useful for development. - in the kernel, please use u8 in favor of uint8_t, and the same for the other types. More comments inline... Barry Song wrote: > Signed-off-by: Barry Song <[email protected]> > Signed-off-by: Heinz-Jürgen Oertel <[email protected]> Hm, the german "umlaut" makes trouble here. > --- > drivers/net/can/Kconfig | 10 + > drivers/net/can/Makefile | 1 + > drivers/net/can/bfin-can.c | 674 > ++++++++++++++++++++++++++++++++++++++++++++ > drivers/net/can/bfin-can.h | 162 +++++++++++ > 4 files changed, 847 insertions(+), 0 deletions(-) > create mode 100644 drivers/net/can/bfin-can.c > create mode 100644 drivers/net/can/bfin-can.h Some times ago we agreed to use underscores for driver names, therefore s/bfin-can/bfin_can/. > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig > index df32c10..be305c5 100644 > --- a/drivers/net/can/Kconfig > +++ b/drivers/net/can/Kconfig > @@ -95,6 +95,16 @@ config CAN_AT91 > ---help--- > This is a driver for the SoC CAN controller in Atmel's AT91SAM9263. > > +config CAN_BFIN > + depends on CAN_DEV && (BF534 || BF536 || BF537 || BF538 || BF539 || > BF54x) > + tristate "Analog Devices Blackfin on-chip CAN" > + ---help--- > + Driver for the Analog Devices Blackfin on-chip CAN controllers > + > + To compile this driver as a module, choose M here: the > + module will be called bfin-can. > + > + > config CAN_DEBUG_DEVICES > bool "CAN devices debugging messages" > depends on CAN > diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile > index 0dea627..72618b0 100644 > --- a/drivers/net/can/Makefile > +++ b/drivers/net/can/Makefile > @@ -11,5 +11,6 @@ obj-y += usb/ > > obj-$(CONFIG_CAN_SJA1000) += sja1000/ > obj-$(CONFIG_CAN_AT91) += at91_can.o > +obj-$(CONFIG_CAN_BFIN) += bfin-can.o > > ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG > diff --git a/drivers/net/can/bfin-can.c b/drivers/net/can/bfin-can.c > new file mode 100644 > index 0000000..c2ad42e > --- /dev/null > +++ b/drivers/net/can/bfin-can.c > @@ -0,0 +1,674 @@ > +/* > + * Blackfin On-Chip CAN Driver > + * > + * Copyright 2004-2009 Analog Devices Inc. > + * > + * Enter bugs at http://blackfin.uclinux.org/ > + * > + * Licensed under the GPL-2 or later. Please replace the above line with the usual GPL vX bla bla. > + */ > + > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/types.h> > +#include <linux/interrupt.h> > +#include <linux/errno.h> > +#include <linux/netdevice.h> > +#include <linux/skbuff.h> > +#include <linux/platform_device.h> > + > +#include <linux/can.h> > +#include <linux/can/dev.h> > +#include <linux/can/error.h> > + > +#include <asm/portmux.h> > +#include "bfin-can.h" > + > +#define DRV_NAME "bfin_can" The driver name should match the file name. > + > +static struct can_bittiming_const bfin_can_bittiming_const = { > + .name = DRV_NAME, > + .tseg1_min = 1, > + .tseg1_max = 16, > + .tseg2_min = 1, > + .tseg2_max = 8, > + .sjw_max = 4, > + /* Although the BRP field can be set to any value, it is recommended > + * that the value be greater than or equal to 4, as restrictions > + * apply to the bit timing configuration when BRP is less than 4. > + */ > + .brp_min = 4, > + .brp_max = 1024, > + .brp_inc = 1, > +}; > + > +static int bfin_can_set_bittiming(struct net_device *dev) > +{ > + struct bfin_can_priv *priv = netdev_priv(dev); > + struct can_bittiming *bt = &priv->can.bittiming; > + u16 clk, timing; > + > + clk = bt->brp - 1; Do you need an extra variable here? > + timing = ((bt->sjw - 1) << 8) | (bt->prop_seg + bt->phase_seg1 - 1) | > + ((bt->phase_seg2 - 1) << 4); Consider using #defines for the constants, here and in many other places. > + /* > + * If the SAM bit is set, the input signal is oversampled three times > at the SCLK rate. > + */ > + if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES) > + timing |= SAM; > + > + CAN_WRITE_CTRL(priv, OFFSET_CLOCK, clk); > + CAN_WRITE_CTRL(priv, OFFSET_TIMING, timing); > + > + dev_info(dev->dev.parent, > + "setting can bitrate:%d brp:%d prop_seg:%d > phase_seg1:%d phase_seg2:%d\n", > + bt->bitrate, bt->brp, bt->prop_seg, bt->phase_seg1, > bt->phase_seg2); Please remove as "ip -d link show canX" will list the values. Instead add dev_info(dev->dev.parent, "setting clk=%d timing=%#x\n", ....); > + return 0; > +} > + > +static void bfin_can_set_reset_mode(struct net_device *dev) > +{ > + struct bfin_can_priv *priv = netdev_priv(dev); > + int i; > + > + dev_dbg(dev->dev.parent, "%s enter\n", __func__); Please remove this and similar lines below. > + /* disable interrupts */ > + CAN_WRITE_CTRL(priv, OFFSET_MBIM1, 0); > + CAN_WRITE_CTRL(priv, OFFSET_MBIM2, 0); > + CAN_WRITE_CTRL(priv, OFFSET_GIM, 0); > + > + /* reset can and enter configuration mode */ > + CAN_WRITE_CTRL(priv, OFFSET_CONTROL, SRS | CCR); > + SSYNC(); I'm just curious. Why do you need a "SSYNC". And where is SRS or CCR defined? > + CAN_WRITE_CTRL(priv, OFFSET_CONTROL, CCR); > + SSYNC(); > + while (!(CAN_READ_CTRL(priv, OFFSET_CONTROL) & CCA)) > + continue; Please implement a timeout with a defined delay per loop, e.g. udelay(10). Does it take long? > + /* > + * All mailbox configurations are marked as inactive > + * by writing to CAN Mailbox Configuration Registers 1 and 2 > + * For all bits: 0 - Mailbox disabled, 1 - Mailbox enabled > + */ > + CAN_WRITE_CTRL(priv, OFFSET_MC1, 0); > + CAN_WRITE_CTRL(priv, OFFSET_MC2, 0); > + > + /* Set Mailbox Direction */ > + CAN_WRITE_CTRL(priv, OFFSET_MD1, 0xFFFF); /* mailbox 1-16 are RX */ > + CAN_WRITE_CTRL(priv, OFFSET_MD2, 0); /* mailbox 17-32 are TX */ > + > + /* RECEIVE_STD_CHL */ > + for (i = 0; i < 2; i++) { > + CAN_WRITE_OID(priv, RECEIVE_STD_CHL + i, 0); > + CAN_WRITE_ID0(priv, RECEIVE_STD_CHL, 0); > + CAN_WRITE_DLC(priv, RECEIVE_STD_CHL + i, 0); > + CAN_WRITE_AMH(priv, RECEIVE_STD_CHL + i, 0x1FFF); > + CAN_WRITE_AML(priv, RECEIVE_STD_CHL + i, 0xFFFF); > + } > + > + /* RECEIVE_EXT_CHL */ > + for (i = 0; i < 2; i++) { > + CAN_WRITE_XOID(priv, RECEIVE_EXT_CHL + i, 0); > + CAN_WRITE_DLC(priv, RECEIVE_EXT_CHL + i, 0); > + CAN_WRITE_AMH(priv, RECEIVE_EXT_CHL + i, 0x1FFF); > + CAN_WRITE_AML(priv, RECEIVE_EXT_CHL + i, 0xFFFF); > + } > + > + CAN_WRITE_CTRL(priv, OFFSET_MC2, 1 << (TRANSMIT_CHL - 16)); > + CAN_WRITE_CTRL(priv, OFFSET_MC1, (1 << RECEIVE_STD_CHL) + (1 << > RECEIVE_EXT_CHL)); > + SSYNC(); > + > + priv->can.state = CAN_STATE_STOPPED; > +} > + > +static void bfin_can_set_normal_mode(struct net_device *dev) > +{ > + struct bfin_can_priv *priv = netdev_priv(dev); > + > + dev_dbg(dev->dev.parent, "%s enter\n", __func__); > + > + /* leave configuration mode */ > + CAN_WRITE_CTRL(priv, OFFSET_CONTROL, CAN_READ_CTRL(priv, > OFFSET_CONTROL) & ~CCR); > + while (CAN_READ_CTRL(priv, OFFSET_STATUS) & CCA) > + continue; Ditto, see above. > + /* clear _All_ tx and rx interrupts */ > + CAN_WRITE_CTRL(priv, OFFSET_MBTIF1, 0xFFFF); > + CAN_WRITE_CTRL(priv, OFFSET_MBTIF2, 0xFFFF); > + CAN_WRITE_CTRL(priv, OFFSET_MBRIF1, 0xFFFF); > + CAN_WRITE_CTRL(priv, OFFSET_MBRIF2, 0xFFFF); > + /* clear global interrupt status register */ > + CAN_WRITE_CTRL(priv, OFFSET_GIS, 0x7FF); /* overwrites with '1' */ > + > + /* Initialize Interrupts Please use: /* * Initialize Interrupts > + * - set bits in the mailbox interrupt mask register > + * - global interrupt mask > + */ > + > + CAN_WRITE_CTRL(priv, OFFSET_MBIM1, (1 << RECEIVE_STD_CHL) + (1 << > RECEIVE_EXT_CHL)); > + CAN_WRITE_CTRL(priv, OFFSET_MBIM2, 1 << (TRANSMIT_CHL - 16)); In general, you could use the kernel's BIT() for the shift operations. > + CAN_WRITE_CTRL(priv, OFFSET_GIM, EPIM | BOIM | RMLIM); > + SSYNC(); > +} > + > + > +static void bfin_can_start(struct net_device *dev) > +{ > + struct bfin_can_priv *priv = netdev_priv(dev); > + > + dev_dbg(dev->dev.parent, "%s enter\n", __func__); > + > + /* leave reset mode */ > + if (priv->can.state != CAN_STATE_STOPPED) > + bfin_can_set_reset_mode(dev); > + > + /* leave reset mode */ > + bfin_can_set_normal_mode(dev); > +} > + > +static int bfin_can_set_mode(struct net_device *dev, enum can_mode mode) > +{ > + dev_dbg(dev->dev.parent, "%s enter\n", __func__); > + > + switch (mode) { > + case CAN_MODE_START: > + bfin_can_start(dev); > + if (netif_queue_stopped(dev)) > + netif_wake_queue(dev); > + break; > + > + default: > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > + > +/* > + * transmit a CAN message > + * message layout in the sk_buff should be like this: > + * xx xx xx xx ff ll 00 11 22 33 44 55 66 77 > + * [ can-id ] [flags] [len] [can data (up to 8 bytes] > + */ Please remove the lines above (even if they are as well in the SJA1000 driver). > +static int bfin_can_start_xmit(struct sk_buff *skb, struct net_device *dev) > +{ > + struct bfin_can_priv *priv = netdev_priv(dev); > + struct net_device_stats *stats = &dev->stats; > + struct can_frame *cf = (struct can_frame *)skb->data; > + uint8_t dlc; s/uint8_t/u8, see general comments. > + canid_t id; > + > + dev_dbg(dev->dev.parent, "%s enter\n", __func__); > + > + netif_stop_queue(dev); > + > + dlc = cf->can_dlc; > + id = cf->can_id; > + > + /* fill id and data length code */ > + if (id & CAN_EFF_FLAG) { > + if (id & CAN_RTR_FLAG) > + CAN_WRITE_XOID_RTR(priv, TRANSMIT_CHL, id); > + else > + CAN_WRITE_XOID(priv, TRANSMIT_CHL, id); > + } else { > + if (id & CAN_RTR_FLAG) > + CAN_WRITE_OID_RTR(priv, TRANSMIT_CHL, id); > + else > + CAN_WRITE_OID(priv, TRANSMIT_CHL, id); > + } > + > + BFIN_CAN_WRITE_MSG(priv, TRANSMIT_CHL, cf->data, dlc); Use a better name, e.g. bfin_can_write_data (or payload). > + CAN_WRITE_DLC(priv, TRANSMIT_CHL, dlc); > + > + stats->tx_bytes += dlc; Please increase that counter when TX is done. You should also increment stats->tx_packets there. > + dev->trans_start = jiffies; > + > + can_put_echo_skb(skb, dev, 0); > + > + /* set transmit request */ > + CAN_WRITE_CTRL(priv, OFFSET_TRS2, 1 << (TRANSMIT_CHL - 16)); > + return 0; > +} > + > +/* Our watchdog timed out. Called by the up layer */ > +static void bfin_can_timeout(struct net_device *dev) > +{ > + dev_dbg(dev->dev.parent, "%s enter\n", __func__); > + > + /* We can accept TX packets again */ > + dev->trans_start = jiffies; > + netif_wake_queue(dev); > +} Please remove the TX watchdog. We don't need it. > +static void bfin_can_rx(struct net_device *dev, uint16_t isrc) > +{ > + struct bfin_can_priv *priv = netdev_priv(dev); > + struct net_device_stats *stats = &dev->stats; > + struct can_frame *cf; > + struct sk_buff *skb; > + canid_t id; > + uint8_t dlc; > + int obj; > + > + dev_dbg(dev->dev.parent, "%s enter\n", __func__); > + > + skb = dev_alloc_skb(sizeof(struct can_frame)); Please use alloc_can_skb() making various lines below obsolete. > + if (skb == NULL) > + return; > + skb->dev = dev; > + skb->protocol = htons(ETH_P_CAN); > + > + /* get id and data length code */ > + if (isrc & (1 << RECEIVE_EXT_CHL)) { > + /* extended frame format (EFF) */ > + id = CAN_READ_XOID(priv, RECEIVE_EXT_CHL); > + id |= CAN_EFF_FLAG; > + obj = RECEIVE_EXT_CHL; > + } else { > + /* standard frame format (SFF) */ > + id = CAN_READ_OID(priv, RECEIVE_STD_CHL); > + obj = RECEIVE_STD_CHL; > + } > + if (CAN_READ_ID1(priv, obj) & RTR) > + id |= CAN_RTR_FLAG; > + dlc = CAN_READ_DLC(priv, obj); > + > + cf = (struct can_frame *)skb_put(skb, sizeof(*cf)); > + memset(cf, 0, sizeof(struct can_frame)); > + cf->can_id = id; > + cf->can_dlc = dlc; > + > + BFIN_CAN_READ_MSG(priv, obj, cf->data, dlc); See comment about the name above. > + netif_rx(skb); > + > + dev->last_rx = jiffies; Not needed any more. > + stats->rx_packets++; > + stats->rx_bytes += dlc; > +} > + > +static int bfin_can_err(struct net_device *dev, uint16_t isrc, uint16_t > status) > +{ > + struct bfin_can_priv *priv = netdev_priv(dev); > + struct net_device_stats *stats = &dev->stats; > + struct can_frame *cf; > + struct sk_buff *skb; > + enum can_state state = priv->can.state; > + > + dev_dbg(dev->dev.parent, "%s enter\n", __func__); > + Please use alloc_can_err_skb() making various lines below obsolete. > + skb = dev_alloc_skb(sizeof(struct can_frame)); > + if (skb == NULL) > + return -ENOMEM; > + skb->dev = dev; > + skb->protocol = htons(ETH_P_CAN); > + cf = (struct can_frame *)skb_put(skb, sizeof(*cf)); > + memset(cf, 0, sizeof(struct can_frame)); > + cf->can_id = CAN_ERR_FLAG; > + cf->can_dlc = CAN_ERR_DLC; > + > + if (isrc & RMLIS) { > + /* data overrun interrupt */ Redundant comment? > + dev_dbg(dev->dev.parent, "data overrun interrupt\n"); > + cf->can_id |= CAN_ERR_CRTL; > + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW; > + stats->rx_over_errors++; > + stats->rx_errors++; > + } > + if (isrc & BOIS) { > + dev_dbg(dev->dev.parent, "bus-off mode interrupt\n"); Please remove line above as in all other cases. > + state = CAN_STATE_BUS_OFF; > + cf->can_id |= CAN_ERR_BUSOFF; > + can_bus_off(dev); Does the BFIN CAN controller recover from bus-off automatically? > + } > + if (isrc & EPIS) { > + /* error passive interrupt */ Redundant comment? > + dev_dbg(dev->dev.parent, "error passive interrupt\n"); > + state = CAN_STATE_ERROR_PASSIVE; > + } > + if ((isrc & EWTIS) || (isrc & EWRIS)) { > + dev_dbg(dev->dev.parent, "Error Warning Transmit/Receive > Interrupt\n"); Message in lowercase as above? > + state = CAN_STATE_ERROR_WARNING; > + } > + > + if (state != priv->can.state && (state == CAN_STATE_ERROR_WARNING || > + state == CAN_STATE_ERROR_PASSIVE)) { > + uint16_t cec = CAN_READ_CTRL(priv, OFFSET_CEC); > + uint8_t rxerr = cec; > + uint8_t txerr = cec >> 8; Please add an empty line here. > + cf->can_id |= CAN_ERR_CRTL; > + if (state == CAN_STATE_ERROR_WARNING) { > + priv->can.can_stats.error_warning++; > + cf->data[1] = (txerr > rxerr) ? > + CAN_ERR_CRTL_TX_WARNING : > + CAN_ERR_CRTL_RX_WARNING; > + } else { > + priv->can.can_stats.error_passive++; > + cf->data[1] = (txerr > rxerr) ? > + CAN_ERR_CRTL_TX_PASSIVE : > + CAN_ERR_CRTL_RX_PASSIVE; > + } > + } > + > + if (status) { > + priv->can.can_stats.bus_error++; > + > + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; > + > + if (status & BEF) > + cf->data[2] |= CAN_ERR_PROT_BIT; > + else if (status & FER) > + cf->data[2] |= CAN_ERR_PROT_FORM; > + else if (status & SER) > + cf->data[2] |= CAN_ERR_PROT_STUFF; > + else > + cf->data[2] |= CAN_ERR_PROT_UNSPEC; > + } > + > + priv->can.state = state; > + > + netif_rx(skb); > + > + dev->last_rx = jiffies; Please remove. > + stats->rx_packets++; > + stats->rx_bytes += cf->can_dlc; > + > + return 0; > +} > + > +irqreturn_t bfin_can_interrupt(int irq, void *dev_id) > +{ > + struct net_device *dev = dev_id; > + struct bfin_can_priv *priv = netdev_priv(dev); > + struct net_device_stats *stats = &dev->stats; > + uint16_t status, isrc; > + > + dev_dbg(dev->dev.parent, "%s enter, irq num:%d\n", __func__, irq); > + > + if ((irq == priv->tx_irq) && CAN_READ_CTRL(priv, OFFSET_MBTIF2)) { > + /* transmission complete interrupt */ > + CAN_WRITE_CTRL(priv, OFFSET_MBTIF2, 0xFFFF); > + stats->tx_packets++; > + can_get_echo_skb(dev, 0); > + netif_wake_queue(dev); > + } else if ((irq == priv->rx_irq) && CAN_READ_CTRL(priv, OFFSET_MBRIF1)) > { This and the following "else if" cases will be checked, even if "irq == priv->tx_irq". A switch statement seems more appropriate to me. > + /* receive interrupt */ > + isrc = CAN_READ_CTRL(priv, OFFSET_MBRIF1); > + CAN_WRITE_CTRL(priv, OFFSET_MBRIF1, 0xFFFF); > + bfin_can_rx(dev, isrc); > + } else if ((irq == priv->err_irq) && CAN_READ_CTRL(priv, OFFSET_GIS)) { > + /* error interrupt */ > + isrc = CAN_READ_CTRL(priv, OFFSET_GIS); > + status = CAN_READ_CTRL(priv, OFFSET_ESR); > + CAN_WRITE_CTRL(priv, OFFSET_GIS, 0x7FF); > + bfin_can_err(dev, isrc, status); > + } else > + return IRQ_NONE; > + > + return IRQ_HANDLED; > +} > + > +static int bfin_can_open(struct net_device *dev) > +{ > + int err; > + > + /* set chip into reset mode */ > + bfin_can_set_reset_mode(dev); > + > + /* common open */ > + err = open_candev(dev); > + if (err) > + return err; > + > + /* init and start chi */ s/chi/chip/ ? > + bfin_can_start(dev); > + > + netif_start_queue(dev); > + > + return 0; > +} > + > +static int bfin_can_close(struct net_device *dev) > +{ > + netif_stop_queue(dev); > + bfin_can_set_reset_mode(dev); > + > + close_candev(dev); > + > + return 0; > +} > + > +struct net_device *alloc_bfin_candev(void) > +{ > + struct net_device *dev; > + struct bfin_can_priv *priv; > + > + dev = alloc_candev(sizeof(*priv)); > + if (!dev) > + return NULL; > + > + priv = netdev_priv(dev); > + > + priv->dev = dev; > + priv->can.bittiming_const = &bfin_can_bittiming_const; > + priv->can.do_set_bittiming = bfin_can_set_bittiming; > + priv->can.do_set_mode = bfin_can_set_mode; > + > + return dev; > +} > + > +void free_bfin_candev(struct net_device *dev) > +{ > + free_candev(dev); > +} Please re-think if the two helper functions are really useful. At least add "static" and "__devinit" or "__devexit" > +static const struct net_device_ops bfin_can_netdev_ops = { > + .ndo_open = bfin_can_open, > + .ndo_stop = bfin_can_close, > + .ndo_start_xmit = bfin_can_start_xmit, > + .ndo_tx_timeout = bfin_can_timeout, Please remove .ndo_tx_timeout. > +}; > + > +int register_bfin_candev(struct net_device *dev) > +{ > + dev->flags |= IFF_ECHO; /* we support local echo */ > + dev->netdev_ops = &bfin_can_netdev_ops; > + > + bfin_can_set_reset_mode(dev); > + > + return register_candev(dev); > +} > + > +void unregister_bfin_candev(struct net_device *dev) > +{ > + bfin_can_set_reset_mode(dev); > + unregister_candev(dev); > +} Please re-think if the two helper functions are really useful. At least add "static" and "__devinit" or "__devexit" > +static int __devinit bfin_can_probe(struct platform_device *pdev) > +{ > + int err; > + struct net_device *dev; > + struct bfin_can_priv *priv; > + struct resource *res_mem, *rx_irq, *tx_irq, *err_irq; > + unsigned short *pdata; > + > + pdata = pdev->dev.platform_data; > + if (!pdata) { > + dev_err(&pdev->dev, "No platform data provided!\n"); > + err = -ENODEV; > + goto exit; "return -ENODEV;" would be fine here? > + } > + > + res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + rx_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > + tx_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 1); > + err_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 2); > + if (!res_mem || !rx_irq || !tx_irq || !err_irq) { > + err = -ENODEV; > + goto exit; Ditto. > + } > + > + if (!request_mem_region(res_mem->start, resource_size(res_mem), > + dev_name(&pdev->dev))) { > + err = -EBUSY; > + goto exit; Ditto. > + } > + > + /* request peripheral pins */ > + err = peripheral_request_list(pdata, dev_name(&pdev->dev)); > + if (err) > + goto exit_mem_release; > + > + dev = alloc_bfin_candev(); > + if (!dev) { > + err = -ENOMEM; > + goto exit_peri_pin_free; > + } > + > + /* register interrupt handler */ > + err = request_irq(rx_irq->start, &bfin_can_interrupt, 0, > + "bfin-can-rx", (void *)dev); > + if (err) > + goto exit_candev_free; > + err = request_irq(tx_irq->start, &bfin_can_interrupt, 0, > + "bfin-can-tx", (void *)dev); > + if (err) > + goto exit_rx_irq_free; > + err = request_irq(err_irq->start, &bfin_can_interrupt, 0, > + "bfin-can-err", (void *)dev); > + if (err) > + goto exit_tx_irq_free; Usually the interrupts are requested in the open function. Any reason why it's done that early? > + > + priv = netdev_priv(dev); > + priv->membase = res_mem->start; > + priv->rx_irq = rx_irq->start; > + priv->tx_irq = tx_irq->start; > + priv->err_irq = err_irq->start; > + priv->pin_list = pdata; > + priv->can.clock.freq = get_sclk(); > + > + dev_set_drvdata(&pdev->dev, dev); > + SET_NETDEV_DEV(dev, &pdev->dev); > + > + err = register_bfin_candev(dev); > + if (err) { > + dev_err(&pdev->dev, "registering failed (err=%d)\n", err); > + goto exit_err_irq_free; > + } > + > + dev_info(&pdev->dev, "%s device registered (reg_base=%p, rx_irq=%d, > tx_irq=%d, err_irq=%d, sclk=%d)\n", > + DRV_NAME, (void *)priv->membase, priv->rx_irq, > priv->tx_irq, priv->err_irq, > + priv->can.clock.freq); > + return 0; > + > +exit_err_irq_free: > + free_irq(err_irq->start, dev); > +exit_tx_irq_free: > + free_irq(tx_irq->start, dev); > +exit_rx_irq_free: > + free_irq(rx_irq->start, dev); > +exit_candev_free: > + free_bfin_candev(dev); > +exit_peri_pin_free: > + peripheral_free_list(pdata); > +exit_mem_release: > + release_mem_region(res_mem->start, resource_size(res_mem)); > +exit: > + return err; > +} > + > +static int __devexit bfin_can_remove(struct platform_device *pdev) > +{ > + struct net_device *dev = dev_get_drvdata(&pdev->dev); > + struct bfin_can_priv *priv = netdev_priv(dev); > + struct resource *res; > + > + unregister_bfin_candev(dev); > + dev_set_drvdata(&pdev->dev, NULL); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + release_mem_region(res->start, resource_size(res)); > + > + free_irq(priv->rx_irq, dev); > + free_irq(priv->tx_irq, dev); > + free_irq(priv->err_irq, dev); > + peripheral_free_list(priv->pin_list); > + > + free_bfin_candev(dev); > + return 0; > +} > + > +#ifdef CONFIG_PM > +static int bfin_can_suspend(struct platform_device *pdev, pm_message_t mesg) > +{ > + struct net_device *dev = dev_get_drvdata(&pdev->dev); > + struct bfin_can_priv *priv = netdev_priv(dev); > + > + if (netif_running(dev)) { > + /* enter sleep mode */ > + CAN_WRITE_CTRL(priv, OFFSET_CONTROL, > + CAN_READ_CTRL(priv, OFFSET_CONTROL) | SMR); > + SSYNC(); > + while (!(CAN_READ_CTRL(priv, OFFSET_INTR) & SMACK)) > + continue; Timeout? > + } > + > + return 0; > +} > + > +static int bfin_can_resume(struct platform_device *pdev) > +{ > + struct net_device *dev = dev_get_drvdata(&pdev->dev); > + struct bfin_can_priv *priv = netdev_priv(dev); > + > + if (netif_running(dev)) { > + /* leave sleep mode */ > + CAN_WRITE_CTRL(priv, OFFSET_INTR, 0); > + SSYNC(); > + } > + > + return 0; > +} > +#else > +#define bfin_can_suspend NULL > +#define bfin_can_resume NULL > +#endif /* CONFIG_PM */ Does suspend/resume work properly? > +static struct platform_driver bfin_can_driver = { > + .probe = bfin_can_probe, > + .remove = __devexit_p(bfin_can_remove), > + .suspend = bfin_can_suspend, > + .resume = bfin_can_resume, > + .driver = { > + .name = DRV_NAME, > + .owner = THIS_MODULE, > + }, > +}; > + > +static int __init bfin_can_init(void) > +{ > + return platform_driver_register(&bfin_can_driver); > +} > + > +module_init(bfin_can_init); > + > +static void __exit bfin_can_exit(void) > +{ > + platform_driver_unregister(&bfin_can_driver); > +} > + > +module_exit(bfin_can_exit); > + > +MODULE_AUTHOR("Barry Song <[email protected]>"); > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("Blackfin on-chip CAN netdevice driver"); > + > diff --git a/drivers/net/can/bfin-can.h b/drivers/net/can/bfin-can.h > new file mode 100644 > index 0000000..ec74168 > --- /dev/null > +++ b/drivers/net/can/bfin-can.h If the header file and the .c gets smaller, please consider merging it into the .c file. > @@ -0,0 +1,162 @@ > +/* > + * Blackfin On-Chip CAN Driver > + * > + * Copyright 2004-2009 Analog Devices Inc. > + * > + * Enter bugs at http://blackfin.uclinux.org/ > + * > + * Licensed under the GPL-2 or later. See above. > + */ > + > +#ifndef __BLACKFIN_CAN_H > +#define __BLACKFIN_CAN_H > + > +#include <asm/io.h> > + > +/* > + * bfin can private data > + */ > +struct bfin_can_priv { > + struct can_priv can; /* must be the first member */ > + struct sk_buff *echo_skb; Seems not be be used anywhere? It's the same in sja1000.h! Will need to fix that. > + struct net_device *dev; > + u32 membase; Please use "void __iomem *membase" and proper accessor functions for I/O, e.g. iowrite16 or bfin_write16 and friends. > + int rx_irq; > + int tx_irq; > + int err_irq; > + unsigned short *pin_list; > +}; > + > +/* > + * registers offset > + */ > +#define OFFSET_MB_MASK 0x100 > +#define OFFSET_MASK_AML 0x0 > +#define OFFSET_MASK_AMH 0x4 > +#define OFFSET_MB_OBJ 0x200 > +#define OFFSET_OBJ_DATA 0x0 > +#define OFFSET_OBJ_DLC 0x10 > +#define OFFSET_OBJ_ID0 0x18 > +#define OFFSET_OBJ_ID1 0x1C > +#define OFFSET_CLOCK 0x80 > +#define OFFSET_TIMING 0x84 > +#define OFFSET_STATUS 0x8C > +#define OFFSET_CEC 0x90 > +#define OFFSET_GIS 0x94 > +#define OFFSET_GIM 0x98 > +#define OFFSET_CONTROL 0xA0 > +#define OFFSET_INTR 0xA4 > +#define OFFSET_ESR 0xB4 > +#define OFFSET_MBIM1 0x28 > +#define OFFSET_MBIM2 0x68 > +#define OFFSET_MC1 0x0 > +#define OFFSET_MC2 0x40 > +#define OFFSET_MD1 0x4 > +#define OFFSET_MD2 0x44 > +#define OFFSET_TRS2 0x48 > +#define OFFSET_MBTIF1 0x20 > +#define OFFSET_MBTIF2 0x60 > +#define OFFSET_MBRIF1 0x24 > +#define OFFSET_MBRIF2 0x64 > + > +#define can_membase(priv) \ > + ((priv)->membase) > +#define can_channel_membase(priv, channel) \ > + ((priv)->membase + OFFSET_MB_OBJ + ((channel) << 5)) > +#define can_mask_membase(priv, channel) \ > + ((priv)->membase + OFFSET_MB_MASK + ((channel) << 3)) > + > +/* > + * read/write CAN registers and messages > + */ > +#define CAN_WRITE_REG(val, addr) \ > + writew((val), (u16 *)(addr)) > + > +#define CAN_READ_REG(addr) \ > + readw((u16 *)(addr)) > + > +#define CAN_WRITE_CTRL(priv, off, val) \ > + CAN_WRITE_REG(val, can_membase((priv)) + (off)) > + > +#define CAN_READ_CTRL(priv, off) \ > + CAN_READ_REG(can_membase((priv)) + (off)) > + > +#define CAN_WRITE_AML(priv, channel, aml) \ > + (CAN_WRITE_REG((aml), can_mask_membase(priv, channel) + > OFFSET_MASK_AML)) > + > +#define CAN_WRITE_AMH(priv, channel, amh) \ > + (CAN_WRITE_REG((amh), can_mask_membase(priv, channel) + > OFFSET_MASK_AMH)) > + > +#define CAN_WRITE_DLC(priv, channel, length) \ > + (CAN_WRITE_REG((length), can_channel_membase(priv, channel) + > OFFSET_OBJ_DLC)) > + > +#define CAN_READ_DLC(priv, channel) \ > + (CAN_READ_REG(can_channel_membase((priv), (channel)) + OFFSET_OBJ_DLC)) > + > +#define CAN_READ_OID(priv, channel) \ > + ((CAN_READ_REG(can_channel_membase((priv), (channel)) + OFFSET_OBJ_ID1) > & 0x1ffc) >> 2) > + > +#define CAN_READ_XOID(priv, channel) \ > + (((CAN_READ_REG(can_channel_membase((priv), (channel)) + > OFFSET_OBJ_ID1) & 0x1fff) << 16) \ > + + ((CAN_READ_REG(can_channel_membase((priv), (channel)) + > OFFSET_OBJ_ID0)))) > + > +#define CAN_READ_ID1(priv, channel) \ > + (CAN_READ_REG(can_channel_membase((priv), (channel)) + OFFSET_OBJ_ID1)) > + > +#define CAN_WRITE_ID0(priv, channel, val) \ > + CAN_WRITE_REG((val), can_channel_membase((priv), (channel)) + > OFFSET_OBJ_ID0) > + > +#define CAN_WRITE_OID(priv, channel, id) \ > + CAN_WRITE_REG(((id) << 2) | AME, can_channel_membase((priv), (channel)) > + OFFSET_OBJ_ID1) > + > +#define CAN_WRITE_XOID(priv, channel, id) \ > + do { \ > + CAN_WRITE_REG((id), can_channel_membase((priv), (channel)) + > OFFSET_OBJ_ID0); \ > + CAN_WRITE_REG((((id) & 0x1FFF0000) >> 16) + IDE + AME, \ > + can_channel_membase((priv), (channel)) + > OFFSET_OBJ_ID1); \ > + } while (0) > + > +#define CAN_WRITE_OID_RTR(priv, channel, id) \ > + CAN_WRITE_REG(((id) << 2) | RTR | AME, can_channel_membase((priv), > (channel)) + OFFSET_OBJ_ID1) > + > +#define CAN_WRITE_XOID_RTR(priv, channel, id) \ > + do { \ > + CAN_WRITE_REG((id), can_channel_membase((priv), (channel)) + > OFFSET_OBJ_ID0); \ > + CAN_WRITE_REG((((id) & 0x1FFF0000) >> 16) + IDE + RTR + AME, \ > + can_channel_membase((priv), (channel)) + > OFFSET_OBJ_ID1); \ > + } while (0) > + > +inline void BFIN_CAN_WRITE_MSG(struct bfin_can_priv *priv, int channel, u8 > *data, int dlc) > +{ > + int i; > + u16 val; > + > + for (i = 0; i < 8; i += 2) { > + val = ((7 - i) < dlc ? (data[7 - i]) : 0) + > + ((6 - i) < dlc ? (data[6 - i] << 8) : 0); > + CAN_WRITE_REG(val, can_channel_membase((priv), (channel)) + > OFFSET_OBJ_DATA + (i << 1)); > + } > +} > + > +inline void BFIN_CAN_READ_MSG(struct bfin_can_priv *priv, int channel, u8 > *data, int dlc) > +{ > + int i; > + u16 val; > + > + for (i = 0; i < 8; i += 2) { > + val = CAN_READ_REG(can_channel_membase((priv), (channel)) + > OFFSET_OBJ_DATA + (i << 1)); > + data[7 - i] = (7 - i) < dlc ? val : 0; > + data[6 - i] = (6 - i) < dlc ? (val >> 8) : 0; > + } > +} See my general comments about using marco definition functions. The last two functions are definetely too large to be inlined. Also please use "static inline ...". Thanks for your contribution. Wolfgang. _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
