On Thu, Nov 12, 2009 at 8:36 PM, Wolfgang Grandegger <[email protected]>
wrote:
> 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.
echo_skb is used in can_put_echo_skb() and can_get_echo_skb(), example:
void can_get_echo_skb(struct net_device *dev, int idx)
{
struct can_priv *priv = netdev_priv(dev);
if (priv->echo_skb[idx]) {
netif_rx(priv->echo_skb[idx]);
priv->echo_skb[idx] = NULL;
}
}
EXPORT_SYMBOL_GPL(can_get_echo_skb);
>
>> + 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
