Sebastian,
Thank you. See my comments.
-barry
On Tue, Nov 10, 2009 at 2:18 PM, Sebastian Haas <[email protected]> wrote:
> Hi Barry,
>
> I'm sure Wolfgang will do a review, but let me also put some notes to your
> patch.
>
> 2 things right at the beginning. Please remove the debug outputs (e.g.
> entering specific functions). I would also recommend to put the contents of
> bfin-can.h into the bfin-can.c. You should rename bfin-can.c to bfin_can.c
> to match your drivers' name and the naming scheme of the other SocketCAN
> drivers.
>
> I found a lot of comments like:
> /*
> * ...
> */
> Please convert to /* ... */ where possible, these should save some lines.
>
> Please have a look at the other drivers how they handle register access. You
> hide a lot of access logic within macros.
>
> Sebastian
>
> Barry Song schrieb:
>>
>> Signed-off-by: Barry Song <[email protected]>
>> Signed-off-by: Heinz-JÃŒrgen Oertel <[email protected]>
>> ---
>> 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
>>
>> 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.
>> + */
>> +
>> +#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"
>> +
>> +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;
>> + timing = ((bt->sjw - 1) << 8) | (bt->prop_seg + bt->phase_seg1 -
>> 1) |
>> + ((bt->phase_seg2 - 1) << 4);
>> +
>> + /*
>> + * 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);
>> +
>> + 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__);
>> +
>> + /* 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();
>> + CAN_WRITE_CTRL(priv, OFFSET_CONTROL, CCR);
>> + SSYNC();
>> + while (!(CAN_READ_CTRL(priv, OFFSET_CONTROL) & CCA))
>> + continue;
>> +
>> + /*
>> + * 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;
>> +
>> + /* 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
>> + * - 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));
>> +
>> + 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]
>> + */
>> +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;
>> + 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);
>
> That is the only place where you use this inline function, save some lines
> by inlining it explicit and remove the inline function BFIN_...
I can't agree that. Whether encapsulating some codes to a funtion
doesn't depend on whether it is only called in one only place. People
include some codes to a function just because it can compete a
standalone work.
>
>> +
>> + CAN_WRITE_DLC(priv, TRANSMIT_CHL, dlc);
>> +
>> + stats->tx_bytes += dlc;
>> + 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);
>> +}
>> +
>> +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));
>
> Use alloc_can_skb() please.
Ok.
>
>> + if (skb == NULL)
>> + return;
>
>> + skb->dev = dev;
>> + skb->protocol = htons(ETH_P_CAN);
>
> No longer needed when using alloc_can_skb().
Ok.
>
>> +
>> + /* get id and data length code */
>> + if (isrc & (1 << RECEIVE_EXT_CHL)) {
>> + /* extended frame format (EFF) */
>> + id = CAN_READ_XOID(priv, RECEIVE_EXT_CHL);
>
> This is the only point where you use this macro, why dont you read directly
> and remove the macro.
same reason with BFIN_CAN_WRITE_MSG.
>
>> + id |= CAN_EFF_FLAG;
>> + obj = RECEIVE_EXT_CHL;
>> + } else {
>> + /* standard frame format (SFF) */
>> + id = CAN_READ_OID(priv, RECEIVE_STD_CHL);
>
> Dito.
>
>> + obj = RECEIVE_STD_CHL;
>> + }
>
> Empty line.
>
>> + 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));
>
> Not needed as well.
Ok.
>
>> + cf->can_id = id;
>> + cf->can_dlc = dlc;
>> +
>> + BFIN_CAN_READ_MSG(priv, obj, cf->data, dlc);
>
> That is the only place where you use this inline function, save some lines
> by inlining it explicit and remove the inline function BFIN_...
same reason with BFIN_CAN_WRITE_MSG.
>
>> + netif_rx(skb);
>> +
>> + dev->last_rx = jiffies;
>
> This is no longer needed as of 2.6.30 I think. See also next function.
Ok.
>
>> + 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__);
>> +
>> + skb = dev_alloc_skb(sizeof(struct can_frame));
>
> Use alloc_can_err_skb() please.
Ok.
>
>> + if (skb == NULL)
>> + return -ENOMEM;
>
>> + skb->dev = dev;
>> + skb->protocol = htons(ETH_P_CAN);
>
> Done in alloc_can_err_skb().
Ok.
>
>> + 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;
>
> Done in alloc_can_err_skb().
Ok.
>
>> + if (isrc & RMLIS) {
>> + /* data overrun interrupt */
>> + 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++;
>> + }
>
> Empty line please.
>
>> + if (isrc & BOIS) {
>> + dev_dbg(dev->dev.parent, "bus-off mode interrupt\n");
>> +
>> + state = CAN_STATE_BUS_OFF;
>> + cf->can_id |= CAN_ERR_BUSOFF;
>> + can_bus_off(dev);
>> + }
>
> Dito.
>
>> + if (isrc & EPIS) {
>> + /* error passive interrupt */
>> + dev_dbg(dev->dev.parent, "error passive interrupt\n");
>> + state = CAN_STATE_ERROR_PASSIVE;
>> + }
>
> Dito.
>
>> + if ((isrc & EWTIS) || (isrc & EWRIS)) {
>> + dev_dbg(dev->dev.parent, "Error Warning Transmit/Receive
>> Interrupt\n");
>> + 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;
>
> Dito.
>
>> + 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;
>
> No longer needed.
>
>> + 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)) {
>
> Is the additional paranthesis really necessary?
It's better to be added to detect possible errors in interrupt.
>
>> + /* 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)) {
>
> Dito.
>
>> + /* 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)) {
>
> Dito.
>
>> + /* 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 */
>> + 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));
>
> Why not sizeof(struct bfin_can_priv)? Reads better, right?
>
>> + 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);
>> +}
>
> If this is the only thing, this function will ever do remove it and call
> free_candev directly in your code.
>
>> +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,
>> +};
>> +
>> +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);
>> +}
>> +
>> +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;
>> + }
>> +
>> + 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;
>> + }
>> +
>> + if (!request_mem_region(res_mem->start, resource_size(res_mem),
>> + dev_name(&pdev->dev))) {
>> + err = -EBUSY;
>> + goto exit;
>> + }
>> +
>> + /* 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;
>> +
>> + 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;
>> + }
>> +
>> + 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 */
>> +
>> +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
>> @@ -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.
>> + */
>> +
>> +#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;
>> + struct net_device *dev;
>> + u32 membase;
>
> I would convert it to "u16 *" and remove the whole CAN_READ/WRITE_REG stuff.
>
>> + 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)
>> +
>
> Function are lower case.
>>
>> +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));
>> + }
>> +}
>> +
>
> Function are lower case.
Here some special functions which work in the same way with registers
reading/writing.
>>
>> +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;
>> + }
>> +}
>> +
>> +/*
>> + * transmit and receive channels
>> + */
>> +#define TRANSMIT_CHL 24
>> +#define RECEIVE_STD_CHL 0
>> +#define RECEIVE_EXT_CHL 4
>> +#define RECEIVE_RTR_CHL 8
>> +#define RECEIVE_EXT_RTR_CHL 12
>> +
>> +#endif /* __BLACKFIN_CAN_H */
>>
>>
>> ------------------------------------------------------------------------
>>
>> _______________________________________________
>> Socketcan-core mailing list
>> [email protected]
>> https://lists.berlios.de/mailman/listinfo/socketcan-core
>
> --
> EMS Dr. Thomas Wuensche e.K.
> Sonnenhang 3
> 85304 Ilmmuenster
> HRA Neuburg a.d. Donau, HR-Nr. 70.106
> Phone: +49-8441-490260
> Fax : +49-8441-81860
> http://www.ems-wuensche.com
>
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
