| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 05 Jan 2011 21:57:16 +0100
From: Wolfgang Grandegger <wg@...ndegger.com>
To: Kurt Van Dijck <kurt.van.dijck@....be>
CC: netdev@...r.kernel.org, socketcan-core@...ts.berlios.de
Subject: Re: [PATCH net-next-2.6 v2 1/2] can: add driver for Softing card
Hi Kurt,
here comes my review... First some general remarks. As Mark already
pointed out, there are still some coding style issues:
- Please use the following style for multi line comments:
/*
* Comment
*/
- Please separate the function header from the body by one empty line.
- Please avoid alignment of expressions and structure members.
- Please use a consistent style for function declaration and
continuation lines.
More comments inline...
On 12/23/2010 10:43 On 01/04/2011 04:07 PM, Kurt Van Dijck wrote:
> This patch adds a driver for the platform:softing device.
> This will create (up to) 2 CAN network devices from 1
> platform:softing device
>
> Signed-off-by: Kurt Van Dijck <kurt.van.dijck@....be>
>
> ---
> drivers/net/can/Kconfig | 2 +
> drivers/net/can/Makefile | 1 +
> drivers/net/can/softing/Kconfig | 16 +
> drivers/net/can/softing/Makefile | 5 +
> drivers/net/can/softing/softing.h | 193 ++++++
> drivers/net/can/softing/softing_fw.c | 648 ++++++++++++++++++++
> drivers/net/can/softing/softing_main.c | 903 ++++++++++++++++++++++++++++
> drivers/net/can/softing/softing_platform.h | 38 ++
> 8 files changed, 1806 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index d5a9db6..986195e 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -117,6 +117,8 @@ source "drivers/net/can/sja1000/Kconfig"
>
> source "drivers/net/can/usb/Kconfig"
>
> +source "drivers/net/can/softing/Kconfig"
> +
> 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 07ca159..53c82a7 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_CAN_DEV) += can-dev.o
> can-dev-y := dev.o
>
> obj-y += usb/
> +obj-y += softing/
Please use "obj-$(CONFIG_CAN_SOFTING)" here.
> obj-$(CONFIG_CAN_SJA1000) += sja1000/
> obj-$(CONFIG_CAN_MSCAN) += mscan/
> diff --git a/drivers/net/can/softing/Kconfig b/drivers/net/can/softing/Kconfig
> new file mode 100644
> index 0000000..072f337
> --- /dev/null
> +++ b/drivers/net/can/softing/Kconfig
> @@ -0,0 +1,16 @@
> +config CAN_SOFTING
> + tristate "Softing Gmbh CAN generic support"
> + depends on CAN_DEV
> + ---help---
> + Support for CAN cards from Softing Gmbh & some cards
> + from Vector Gmbh.
> + Softing Gmbh CAN cards come with 1 or 2 physical busses.
> + Those cards typically use Dual Port RAM to communicate
> + with the host CPU. The interface is then identical for PCI
> + and PCMCIA cards. This driver operates on a platform device,
> + which has been created by softing_cs or softing_pci driver.
> + Warning:
> + The API of the card does not allow fine control per bus, but
> + controls the 2 busses on the card together.
> + As such, some actions (start/stop/busoff recovery) on 1 bus
> + must bring down the other bus too temporarily.
> diff --git a/drivers/net/can/softing/Makefile b/drivers/net/can/softing/Makefile
> new file mode 100644
> index 0000000..7878b7b
> --- /dev/null
> +++ b/drivers/net/can/softing/Makefile
> @@ -0,0 +1,5 @@
> +
> +softing-y := softing_main.o softing_fw.o
> +obj-$(CONFIG_CAN_SOFTING) += softing.o
> +
> +ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/softing/softing.h b/drivers/net/can/softing/softing.h
> new file mode 100644
> index 0000000..72d3adb
> --- /dev/null
> +++ b/drivers/net/can/softing/softing.h
> @@ -0,0 +1,193 @@
> +/*
> + * softing common interfaces
> + *
> + * by Kurt Van Dijck, 06-2008
> + */
> +
> +#include <linux/atomic.h>
> +#include <linux/netdevice.h>
> +#include <linux/ktime.h>
> +#include <linux/mutex.h>
> +#include <linux/spinlock.h>
> +#include <linux/can.h>
> +#include <linux/can/dev.h>
> +
> +#include "softing_platform.h"
> +
> +#ifndef CAN_CTRLMODE_BERR_REPORTING
> +#define CAN_CTRLMODE_BERR_REPORTING 0
> +#endif
Hm, do you really need that definition?
> +struct softing;
> +
> +struct softing_priv {
> + struct can_priv can; /* must be the first member! */
> + struct net_device *netdev;
> + struct softing *card;
> + struct {
> + int pending;
> + /* variables wich hold the circular buffer */
> + int echo_put;
> + int echo_get;
> + } tx;
> + struct can_bittiming_const btr_const;
> + int index;
> + u8 output;
> + u16 chip;
> +};
> +#define netdev2softing(netdev) ((struct softing_priv *)netdev_priv(netdev))
> +
> +struct softing {
> + const struct softing_platform_data *pdat;
> + struct platform_device *pdev;
> + struct net_device *net[2];
> + spinlock_t spin; /* protect this structure & DPRAM access */
> + ktime_t ts_ref;
> + ktime_t ts_overflow; /* timestamp overflow value, in ktime */
> +
> + struct {
> + /* indication of firmware status */
> + int up;
> + /* protection of the 'up' variable */
> + struct mutex lock;
> + } fw;
> + struct {
> + int nr;
> + int requested;
> + int svc_count;
> + unsigned int dpram_position;
> + } irq;
> + struct {
> + int pending;
> + int last_bus;
> + /*
> + * keep the bus that last tx'd a message,
> + * in order to let every netdev queue resume
> + */
> + } tx;
> + __iomem uint8_t *dpram;
> + unsigned long dpram_phys;
> + unsigned long dpram_size;
> + struct {
> + u32 serial, fw, hw, lic;
> + u16 chip[2];
> + u32 freq;
> + } id;
> +};
> +
> +extern int softing_default_output(struct net_device *netdev);
> +
> +extern ktime_t softing_raw2ktime(struct softing *card, u32 raw);
> +
> +extern int softing_fct_cmd(struct softing *card, int16_t cmd, uint16_t vector,
> + const char *msg);
> +
> +extern int softing_bootloader_command(struct softing *card, int16_t cmd,
> + const char *msg);
> +
> +/* Load firmware after reset */
> +extern int softing_load_fw(const char *file, struct softing *card,
> + __iomem uint8_t *virt, unsigned int size, int offset);
> +
> +/* Load final application firmware after bootloader */
> +extern int softing_load_app_fw(const char *file, struct softing *card);
> +
> +/*
> + * enable or disable irq
> + * only called with fw.lock locked
> + */
> +extern int softing_enable_irq(struct softing *card, int enable);
> +
> +/* start/stop 1 bus on card */
> +extern int softing_startstop(struct net_device *netdev, int up);
> +
> +/* netif_rx() */
> +extern int softing_netdev_rx(struct net_device *netdev,
> + const struct can_frame *msg, ktime_t ktime);
> +
> +/* SOFTING DPRAM mappings */
> +#define DPRAM_RX 0x0000
> + #define DPRAM_RX_SIZE 32
> + #define DPRAM_RX_CNT 16
> +#define DPRAM_RX_RD 0x0201 /* uint8_t */
> +#define DPRAM_RX_WR 0x0205 /* uint8_t */
> +#define DPRAM_RX_LOST 0x0207 /* uint8_t */
> +
> +#define DPRAM_FCT_PARAM 0x0300 /* int16_t [20] */
> +#define DPRAM_FCT_RESULT 0x0328 /* int16_t */
> +#define DPRAM_FCT_HOST 0x032b /* uint16_t */
> +
> +#define DPRAM_INFO_BUSSTATE 0x0331 /* uint16_t */
> +#define DPRAM_INFO_BUSSTATE2 0x0335 /* uint16_t */
> +#define DPRAM_INFO_ERRSTATE 0x0339 /* uint16_t */
> +#define DPRAM_INFO_ERRSTATE2 0x033d /* uint16_t */
> +#define DPRAM_RESET 0x0341 /* uint16_t */
> +#define DPRAM_CLR_RECV_FIFO 0x0345 /* uint16_t */
> +#define DPRAM_RESET_TIME 0x034d /* uint16_t */
> +#define DPRAM_TIME 0x0350 /* uint64_t */
> +#define DPRAM_WR_START 0x0358 /* uint8_t */
> +#define DPRAM_WR_END 0x0359 /* uint8_t */
> +#define DPRAM_RESET_RX_FIFO 0x0361 /* uint16_t */
> +#define DPRAM_RESET_TX_FIFO 0x0364 /* uint8_t */
> +#define DPRAM_READ_FIFO_LEVEL 0x0365 /* uint8_t */
> +#define DPRAM_RX_FIFO_LEVEL 0x0366 /* uint16_t */
> +#define DPRAM_TX_FIFO_LEVEL 0x0366 /* uint16_t */
> +
> +#define DPRAM_TX 0x0400 /* uint16_t */
> + #define DPRAM_TX_SIZE 16
> + #define DPRAM_TX_CNT 32
> +#define DPRAM_TX_RD 0x0601 /* uint8_t */
> +#define DPRAM_TX_WR 0x0605 /* uint8_t */
> +
> +#define DPRAM_COMMAND 0x07e0 /* uint16_t */
> +#define DPRAM_RECEIPT 0x07f0 /* uint16_t */
> +#define DPRAM_IRQ_TOHOST 0x07fe /* uint8_t */
> +#define DPRAM_IRQ_TOCARD 0x07ff /* uint8_t */
> +
> +#define DPRAM_V2_RESET 0x0e00 /* uint8_t */
> +#define DPRAM_V2_IRQ_TOHOST 0x0e02 /* uint8_t */
> +
> +#define TXMAX (DPRAM_TX_CNT - 1)
> +
> +/* DPRAM return codes */
> +#define RES_NONE 0
> +#define RES_OK 1
> +#define RES_NOK 2
> +#define RES_UNKNOWN 3
> +/* DPRAM flags */
> +#define CMD_TX 0x01
> +#define CMD_ACK 0x02
> +#define CMD_XTD 0x04
> +#define CMD_RTR 0x08
> +#define CMD_ERR 0x10
> +#define CMD_BUS2 0x80
> +
> +/*
> + * some inline DPRAM acces function
> + * to prevent extra dependency between softing & softingcs
> + */
> +/* reset DPRAM */
> +static inline void softing_set_reset_dpram(struct softing *card)
> +{
> + if (card->pdat->generation >= 2) {
> + uint8_t tmp;
> + spin_lock_bh(&card->spin);
> + tmp = ioread8(&card->dpram[DPRAM_V2_RESET]);
> + tmp &= ~1;
> + iowrite8(tmp, &card->dpram[DPRAM_V2_RESET]);
> + spin_unlock_bh(&card->spin);
> + }
> +}
> +
> +static inline void softing_clr_reset_dpram(struct softing *card)
> +{
> + if (card->pdat->generation >= 2) {
> + uint8_t tmp;
Empty line please.
> + spin_lock_bh(&card->spin);
> + tmp = ioread8(&card->dpram[DPRAM_V2_RESET]);
> + tmp |= 1;
Could be done in one line or even without tmp.
> + iowrite8(tmp, &card->dpram[DPRAM_V2_RESET]);
> + spin_unlock_bh(&card->spin);
> + }
> +}
> +
> diff --git a/drivers/net/can/softing/softing_fw.c b/drivers/net/can/softing/softing_fw.c
> new file mode 100644
> index 0000000..03ed853
> --- /dev/null
> +++ b/drivers/net/can/softing/softing_fw.c
> @@ -0,0 +1,648 @@
> +/*
> +* drivers/net/can/softing/softing_fw.c
> +*
> +* Copyright (C) 2008-2010
> +*
> +* - Kurt Van Dijck, EIA Electronics
> +*
> +* This program is free software; you can redistribute it and/or modify
> +* it under the terms of the version 2 of the GNU General Public License
> +* as published by the Free Software Foundation
> +*
> +* This program is distributed in the hope that it will be useful,
> +* but WITHOUT ANY WARRANTY; without even the implied warranty of
> +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +* GNU General Public License for more details.
> +*
> +* You should have received a copy of the GNU General Public License
> +* along with this program; if not, write to the Free Software
> +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> +*/
> +
> +#include <linux/firmware.h>
> +#include <linux/sched.h>
> +#include <asm/div64.h>
> +
> +#include "softing.h"
> +
> +int softing_fct_cmd(struct softing *card, int16_t cmd, uint16_t vector,
> + const char *msg)
> +{
> + int ret;
> + unsigned long stamp;
> +
> + if (vector == RES_OK)
> + vector = RES_NONE;
> + iowrite16(cmd, &card->dpram[DPRAM_FCT_PARAM]);
> + iowrite8(vector >> 8, &card->dpram[DPRAM_FCT_HOST + 1]);
> + iowrite8(vector, &card->dpram[DPRAM_FCT_HOST]);
> +
> + /* be sure to flush this to the card */
> + wmb();
> + stamp = jiffies + 1 * HZ;
> + /* wait for card */
> + do {
> + /* DPRAM_FCT_HOST is _not_ aligned */
> + ret = ioread8(&card->dpram[DPRAM_FCT_HOST]) +
> + (ioread8(&card->dpram[DPRAM_FCT_HOST + 1]) << 8);
> + /* don't have any cached variables */
> + rmb();
> + if (ret == RES_OK) {
> + /* don't read return-value now */
> + ret = ioread16(&card->dpram[DPRAM_FCT_RESULT]);
> + if (ret)
> + dev_alert(&card->pdev->dev,
> + "%s returned %u\n", msg, ret);
> + return 0;
Why do you not return an error here?
> + }
> + if (time_after(jiffies, stamp))
> + break;
> + /* process context => relax */
> + usleep_range(500, 10000);
> + } while (!signal_pending(current));
> +
> + if (ret == RES_NONE) {
> + dev_alert(&card->pdev->dev,
> + "%s, no response from card on %u/0x%02x\n",
> + msg, cmd, vector);
> + return 1;
> + } else {
> + dev_alert(&card->pdev->dev,
> + "%s, bad response from card on %u/0x%02x, 0x%04x\n",
> + msg, cmd, vector, ret);
> + /* make sure to return something not 0 */
> + return ret ?: 1;
What does it return if ret > 0?
> + }
> +}
> +
> +int softing_bootloader_command(struct softing *card, int16_t cmd,
> + const char *msg)
> +{
> + int ret;
> + unsigned long stamp;
Empty line please.
> + iowrite16(RES_NONE, &card->dpram[DPRAM_RECEIPT]);
> + iowrite16(cmd, &card->dpram[DPRAM_COMMAND]);
> + /* be sure to flush this to the card */
> + wmb();
> + stamp = jiffies + 3 * HZ;
> + /* wait for card */
> + do {
> + ret = ioread16(&card->dpram[DPRAM_RECEIPT]);
> + /* don't have any cached variables */
> + rmb();
> + if (ret == RES_OK)
> + return 0;
> + if (time_after(jiffies, stamp))
> + break;
> + /* process context => relax */
> + usleep_range(500, 10000);
> + } while (!signal_pending(current));
> +
> + switch (ret) {
> + case RES_NONE:
> + dev_alert(&card->pdev->dev, "%s: no response from card\n", msg);
> + break;
> + case RES_NOK:
> + dev_alert(&card->pdev->dev, "%s: response from card nok\n",
> + msg);
> + break;
> + case RES_UNKNOWN:
> + dev_alert(&card->pdev->dev, "%s: command 0x%04x unknown\n",
> + msg, cmd);
> + break;
> + default:
> + dev_alert(&card->pdev->dev, "%s: bad response from card: %i\n",
> + msg, ret);
> + break;
> + }
> + return ret ?: 1;
Ditto.
> +}
> +
> +static int fw_parse(const uint8_t **pmem, uint16_t *ptype, uint32_t *paddr,
> + uint16_t *plen, const uint8_t **pdat)
> +{
> + uint16_t checksum[2];
> + const uint8_t *mem;
> + const uint8_t *end;
> +
> + mem = *pmem;
> + *ptype = le16_to_cpup((void *)&mem[0]);
> + *paddr = le32_to_cpup((void *)&mem[2]);
> + *plen = le16_to_cpup((void *)&mem[6]);
> + *pdat = &mem[8];
You often handle arrays of specific data. Couldn't those be described
better by structs also avoiding ugly casts??
> + /* verify checksum */
> + end = &mem[8 + *plen];
> + checksum[0] = le16_to_cpup((void *)end);
> + for (checksum[1] = 0; mem < end; ++mem)
> + checksum[1] += *mem;
> + if (checksum[0] != checksum[1])
> + return -EINVAL;
> + /* increment */
> + *pmem += 10 + *plen;
> + return 0;
> +}
> +
> +int softing_load_fw(const char *file, struct softing *card,
> + __iomem uint8_t *dpram, unsigned int size, int offset)
> +{
> + const struct firmware *fw;
> + int ret, ok = 0;
> + const uint8_t *mem, *end, *dat;
> + uint16_t type, len;
> + uint32_t addr;
> + uint8_t buf[1024];
Please avoid allocating large arrays on the stack.
> +
> + ret = request_firmware(&fw, file, &card->pdev->dev);
> + if (ret) {
> + dev_alert(&card->pdev->dev, "request_firmware(%s) got %i\n",
> + file, ret);
> + return ret;
> + }
> + dev_dbg(&card->pdev->dev, "%s, firmware(%s) got %u bytes"
> + ", offset %c0x%04x\n",
> + card->pdat->name, file, (unsigned int)fw->size,
> + (offset >= 0) ? '+' : '-', (unsigned int)abs(offset));
> + /* parse the firmware */
> + mem = fw->data;
> + end = &mem[fw->size];
> + /* look for header record */
> + ret = fw_parse(&mem, &type, &addr, &len, &dat);
> + if (ret < 0)
> + goto fw_end;
> + if (type != 0xffff) {
> + dev_alert(&card->pdev->dev, "firware starts with type 0x%04x\n",
> + type);
> + goto fw_end;
> + }
> + if (strncmp("Structured Binary Format, Softing GmbH" , dat, len)) {
> + dev_info(&card->pdev->dev, "firware string '%.*s'\n", len, dat);
> + goto fw_end;
> + }
> + ok |= 1;
> + /* ok, we had a header */
> + while (mem < end) {
> + ret = fw_parse(&mem, &type, &addr, &len, &dat);
> + if (ret)
> + break;
> + if (type == 3) {
> + /* start address */
> + ok |= 2;
> + continue;
> + } else if (type == 1) {
> + /* eof */
> + ok |= 4;
> + goto fw_end;
> + } else if (type != 0) {
> + dev_alert(&card->pdev->dev,
> + "unknown record type 0x%04x\n", type);
> + break;
> + }
You still use a lot of magic constants. Giving them a name would make
the code more readable.
> + if ((addr + len + offset) > size) {
> + dev_alert(&card->pdev->dev,
> + "firmware out of range (0x%08x / 0x%08x)\n",
> + (addr + len + offset), size);
> + goto fw_end;
> + }
> + memcpy_toio(&dpram[addr + offset], dat, len);
> + /* be sure to flush caches from IO space */
> + mb();
> + if (len > sizeof(buf)) {
> + dev_info(&card->pdev->dev,
> + "record too big for verify (%u)\n", len);
> + continue;
> + }
> + /* verify record data */
> + memcpy_fromio(buf, &dpram[addr + offset], len);
> + if (!memcmp(buf, dat, len))
> + /* is ok */
> + continue;
> + dev_alert(&card->pdev->dev, "0x%08x:0x%03x at 0x%u failed\n",
> + addr, len, addr + offset);
> + goto fw_end;
> + }
> +fw_end:
> + release_firmware(fw);
> + if (0x5 == (ok & 0x5))
> + /* got eof & start */
> + return 0;
Ditto.
> + dev_info(&card->pdev->dev, "firmware %s failed\n", file);
> + return ret ?: -EINVAL;
> +}
> +
> +int softing_load_app_fw(const char *file, struct softing *card)
> +{
> + const struct firmware *fw;
> + const uint8_t *mem, *end, *dat;
> + int ret, ok = 0, j;
> + uint16_t type, len;
> + uint32_t addr, start_addr = 0;
> + unsigned int sum, rx_sum;
> +
> + ret = request_firmware(&fw, file, &card->pdev->dev);
> + if (ret) {
> + dev_alert(&card->pdev->dev, "request_firmware(%s) got %i\n",
> + file, ret);
> + return ret;
> + }
> + dev_dbg(&card->pdev->dev, "firmware(%s) got %lu bytes\n",
> + file, (unsigned long)fw->size);
> + /* parse the firmware */
> + mem = fw->data;
> + end = &mem[fw->size];
> + /* look for header record */
> + ret = fw_parse(&mem, &type, &addr, &len, &dat);
> + if (ret)
> + goto fw_end;
> + if (type != 0xffff) {
> + dev_alert(&card->pdev->dev, "firware starts with type 0x%04x\n",
Typo?
> + type);
> + goto fw_end;
> + }
> + if (strncmp("Structured Binary Format, Softing GmbH", dat, len)) {
> + dev_alert(&card->pdev->dev, "firware string '%.*s' fault\n",
> + len, dat);
> + goto fw_end;
> + }
> + ok |= 1;
> + /* ok, we had a header */
> + while (mem < end) {
> + ret = fw_parse(&mem, &type, &addr, &len, &dat);
> + if (ret)
> + break;
> +
> + if (type == 3) {
> + /* start address */
> + start_addr = addr;
> + ok |= 2;
> + continue;
> + } else if (type == 1) {
> + /* eof */
> + ok |= 4;
> + goto fw_end;
> + } else if (type != 0) {
> + dev_alert(&card->pdev->dev,
> + "unknown record type 0x%04x\n", type);
> + break;
> + }
> +
> + /* regualar data */
> + for (sum = 0, j = 0; j < len; ++j)
> + sum += dat[j];
> + /* work in 16bit (target) */
> + sum &= 0xffff;
> +
> + memcpy_toio(&card->dpram[card->pdat->app.offs], dat, len);
> + iowrite32(card->pdat->app.offs + card->pdat->app.addr,
> + &card->dpram[DPRAM_COMMAND + 2]);
> + iowrite32(addr, &card->dpram[DPRAM_COMMAND + 6]);
> + iowrite16(len, &card->dpram[DPRAM_COMMAND + 10]);
> + iowrite8(1, &card->dpram[DPRAM_COMMAND + 12]);
See my comment about using arrays above.
> + if (softing_bootloader_command(card, 1, "loading app."))
> + goto fw_end;
> + /* verify checksum */
> + rx_sum = ioread16(&card->dpram[DPRAM_RECEIPT + 2]);
> + if (rx_sum != sum) {
> + dev_alert(&card->pdev->dev, "SRAM seems to be damaged"
> + ", wanted 0x%04x, got 0x%04x\n", sum, rx_sum);
> + goto fw_end;
> + }
> + }
> +fw_end:
> + release_firmware(fw);
> + if (ok != 7)
> + goto fw_failed;
> + /* got start, start_addr, & eof */
> + iowrite32(start_addr, &card->dpram[DPRAM_COMMAND + 2]);
> + iowrite8(1, &card->dpram[DPRAM_COMMAND + 6]);
> + if (softing_bootloader_command(card, 3, "start app."))
> + goto fw_failed;
> + dev_info(&card->pdev->dev, "firmware %s up\n", file);
> + return 0;
> +fw_failed:
> + dev_info(&card->pdev->dev, "firmware %s failed\n", file);
> + return ret ?: -EINVAL;
> +}
> +
> +static int softing_reset_chip(struct softing *card)
> +{
> + do {
> + /* reset chip */
> + iowrite8(0, &card->dpram[DPRAM_RESET_RX_FIFO]);
> + iowrite8(0, &card->dpram[DPRAM_RESET_RX_FIFO+1]);
> + iowrite8(1, &card->dpram[DPRAM_RESET]);
> + iowrite8(0, &card->dpram[DPRAM_RESET+1]);
> +
> + if (!softing_fct_cmd(card, 0, 0, "reset_chip"))
> + break;
> + if (signal_pending(current))
> + goto failed;
> + /* sync */
> + if (softing_fct_cmd(card, 99, 0x55, "sync-a"))
> + goto failed;
> + if (softing_fct_cmd(card, 99, 0xaa, "sync-a"))
> + goto failed;
> + } while (1);
> + card->tx.pending = 0;
> + return 0;
> +failed:
> + return -EIO;
> +}
> +
> +static void softing_initialize_timestamp(struct softing *card)
> +{
> + uint64_t ovf;
> +
> + card->ts_ref = ktime_get();
> +
> + /* 16MHz is the reference */
> + ovf = 0x100000000ULL * 16;
> + do_div(ovf, card->pdat->freq ?: 16);
> +
> + card->ts_overflow = ktime_add_us(ktime_set(0, 0), ovf);
> +}
> +
> +ktime_t softing_raw2ktime(struct softing *card, u32 raw)
> +{
> + uint64_t rawl;
> + ktime_t now, real_offset;
> + ktime_t target;
> + ktime_t tmp;
> +
> + now = ktime_get();
> + real_offset = ktime_sub(ktime_get_real(), now);
> +
> + /* find nsec from card */
> + rawl = raw * 16;
> + do_div(rawl, card->pdat->freq ?: 16);
> + target = ktime_add_us(card->ts_ref, rawl);
> + /* test for overflows */
> + tmp = ktime_add(target, card->ts_overflow);
> + while (unlikely(ktime_to_ns(tmp) > ktime_to_ns(now))) {
> + card->ts_ref = ktime_add(card->ts_ref, card->ts_overflow);
> + target = tmp;
> + tmp = ktime_add(target, card->ts_overflow);
> + }
> + return ktime_add(target, real_offset);
> +}
> +
> +static inline int softing_error_reporting(struct net_device *netdev)
> +{
> + struct softing_priv *priv = netdev_priv(netdev);
> +
> + return (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
> + ? 1 : 0;
> +}
> +
> +int softing_startstop(struct net_device *dev, int up)
> +{
> + int ret;
> + struct softing *card;
> + struct softing_priv *priv;
> + struct net_device *netdev;
> + int mask_start;
> + int j, error_reporting;
> + struct can_frame msg;
> + const struct can_bittiming *bt;
> +
> + priv = netdev_priv(dev);
> + card = priv->card;
> +
> + if (!card->fw.up)
> + return -EIO;
> +
> + ret = mutex_lock_interruptible(&card->fw.lock);
> + if (ret)
> + return ret;
> +
> + mask_start = 0;
> + if (dev && up)
> + /* prepare to start this bus as well */
> + mask_start |= (1 << priv->index);
> + /* bring netdevs down */
> + for (j = 0; j < ARRAY_SIZE(card->net); ++j) {
> + netdev = card->net[j];
> + if (!netdev)
> + continue;
> + priv = netdev_priv(netdev);
> +
> + if (dev != netdev)
> + netif_stop_queue(netdev);
> +
> + if (netif_running(netdev)) {
> + if (dev != netdev)
> + mask_start |= (1 << j);
> + priv->tx.pending = 0;
> + priv->tx.echo_put = 0;
> + priv->tx.echo_get = 0;
> + /* this bus' may just have called open_candev()
Please use
/*
* Comment
*/
> + * which is rather stupid to call close_candev()
> + * already
> + * but we may come here from busoff recovery too
> + * in which case the echo_skb _needs_ flushing too.
> + * just be sure to call open_candev() again
> + */
> + close_candev(netdev);
> + }
> + priv->can.state = CAN_STATE_STOPPED;
> + }
> + card->tx.pending = 0;
> +
> + softing_enable_irq(card, 0);
> + ret = softing_reset_chip(card);
> + if (ret)
> + goto failed;
> + if (!mask_start)
> + /* no busses to be brought up */
> + goto card_done;
> +
> + if ((mask_start & 1) && (mask_start & 2)
> + && (softing_error_reporting(card->net[0])
> + != softing_error_reporting(card->net[1]))) {
> + dev_alert(&card->pdev->dev,
> + "err_reporting flag differs for busses\n");
> + goto invalid;
> + }
> + error_reporting = 0;
> + if (mask_start & 1) {
> + netdev = card->net[0];
> + priv = netdev_priv(netdev);
> + error_reporting += softing_error_reporting(netdev);
> + /* init chip 1 */
> + bt = &priv->can.bittiming;
> + iowrite16(bt->brp, &card->dpram[DPRAM_FCT_PARAM + 2]);
> + iowrite16(bt->sjw, &card->dpram[DPRAM_FCT_PARAM + 4]);
> + iowrite16(bt->phase_seg1 + bt->prop_seg,
> + &card->dpram[DPRAM_FCT_PARAM + 6]);
> + iowrite16(bt->phase_seg2, &card->dpram[DPRAM_FCT_PARAM + 8]);
> + iowrite16((priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES) ? 1 : 0,
> + &card->dpram[DPRAM_FCT_PARAM + 10]);
> + if (softing_fct_cmd(card, 1, 0, "initialize_chip[0]"))
> + goto failed;
> + /* set mode */
> + iowrite16(0, &card->dpram[DPRAM_FCT_PARAM + 2]);
> + iowrite16(0, &card->dpram[DPRAM_FCT_PARAM + 4]);
> + if (softing_fct_cmd(card, 3, 0, "set_mode[0]"))
> + goto failed;
> + /* set filter */
> + /* 11bit id & mask */
> + iowrite16(0x0000, &card->dpram[DPRAM_FCT_PARAM + 2]);
> + iowrite16(0x07ff, &card->dpram[DPRAM_FCT_PARAM + 4]);
> + /* 29bit id.lo & mask.lo & id.hi & mask.hi */
> + iowrite16(0x0000, &card->dpram[DPRAM_FCT_PARAM + 6]);
> + iowrite16(0xffff, &card->dpram[DPRAM_FCT_PARAM + 8]);
> + iowrite16(0x0000, &card->dpram[DPRAM_FCT_PARAM + 10]);
> + iowrite16(0x1fff, &card->dpram[DPRAM_FCT_PARAM + 12]);
> + if (softing_fct_cmd(card, 7, 0, "set_filter[0]"))
> + goto failed;
> + /* set output control */
> + iowrite16(priv->output, &card->dpram[DPRAM_FCT_PARAM + 2]);
> + if (softing_fct_cmd(card, 5, 0, "set_output[0]"))
> + goto failed;
> + }
> + if (mask_start & 2) {
Magic constants?
> + netdev = card->net[1];
> + priv = netdev_priv(netdev);
> + error_reporting += softing_error_reporting(netdev);
> + /* init chip2 */
> + bt = &priv->can.bittiming;
> + iowrite16(bt->brp, &card->dpram[DPRAM_FCT_PARAM + 2]);
> + iowrite16(bt->sjw, &card->dpram[DPRAM_FCT_PARAM + 4]);
> + iowrite16(bt->phase_seg1 + bt->prop_seg,
> + &card->dpram[DPRAM_FCT_PARAM + 6]);
> + iowrite16(bt->phase_seg2, &card->dpram[DPRAM_FCT_PARAM + 8]);
> + iowrite16((priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES) ? 1 : 0,
> + &card->dpram[DPRAM_FCT_PARAM + 10]);
> + if (softing_fct_cmd(card, 2, 0, "initialize_chip[1]"))
> + goto failed;
> + /* set mode2 */
> + iowrite16(0, &card->dpram[DPRAM_FCT_PARAM + 2]);
> + iowrite16(0, &card->dpram[DPRAM_FCT_PARAM + 4]);
> + if (softing_fct_cmd(card, 4, 0, "set_mode[1]"))
> + goto failed;
> + /* set filter2 */
> + /* 11bit id & mask */
> + iowrite16(0x0000, &card->dpram[DPRAM_FCT_PARAM + 2]);
> + iowrite16(0x07ff, &card->dpram[DPRAM_FCT_PARAM + 4]);
> + /* 29bit id.lo & mask.lo & id.hi & mask.hi */
> + iowrite16(0x0000, &card->dpram[DPRAM_FCT_PARAM + 6]);
> + iowrite16(0xffff, &card->dpram[DPRAM_FCT_PARAM + 8]);
> + iowrite16(0x0000, &card->dpram[DPRAM_FCT_PARAM + 10]);
> + iowrite16(0x1fff, &card->dpram[DPRAM_FCT_PARAM + 12]);
> + if (softing_fct_cmd(card, 8, 0, "set_filter[1]"))
> + goto failed;
> + /* set output control2 */
> + iowrite16(priv->output, &card->dpram[DPRAM_FCT_PARAM + 2]);
> + if (softing_fct_cmd(card, 6, 0, "set_output[1]"))
> + goto failed;
> + }
> + /* enable_error_frame */
> + /*
> + if (error_reporting) {
> + if (softing_fct_cmd(card, 51, 0, "enable_error_frame"))
> + goto failed;
> + }
> + */
Please remove dead code!
> + /* initialize interface */
> + iowrite16(1, &card->dpram[DPRAM_FCT_PARAM + 2]);
> + iowrite16(1, &card->dpram[DPRAM_FCT_PARAM + 4]);
> + iowrite16(1, &card->dpram[DPRAM_FCT_PARAM + 6]);
> + iowrite16(1, &card->dpram[DPRAM_FCT_PARAM + 8]);
> + iowrite16(1, &card->dpram[DPRAM_FCT_PARAM + 10]);
> + iowrite16(1, &card->dpram[DPRAM_FCT_PARAM + 12]);
> + iowrite16(1, &card->dpram[DPRAM_FCT_PARAM + 14]);
> + iowrite16(1, &card->dpram[DPRAM_FCT_PARAM + 16]);
> + iowrite16(1, &card->dpram[DPRAM_FCT_PARAM + 18]);
> + iowrite16(1, &card->dpram[DPRAM_FCT_PARAM + 20]);
Could be coded more efficiently with a for loop.
> + if (softing_fct_cmd(card, 17, 0, "initialize_interface"))
> + goto failed;
> + /* enable_fifo */
> + if (softing_fct_cmd(card, 36, 0, "enable_fifo"))
> + goto failed;
> + /* enable fifo tx ack */
> + if (softing_fct_cmd(card, 13, 0, "fifo_tx_ack[0]"))
> + goto failed;
> + /* enable fifo tx ack2 */
> + if (softing_fct_cmd(card, 14, 0, "fifo_tx_ack[1]"))
> + goto failed;
> + /* enable timestamps */
> + /* is default, no code found */
> + /* start_chip */
> + if (softing_fct_cmd(card, 11, 0, "start_chip"))
> + goto failed;
> + iowrite8(0, &card->dpram[DPRAM_INFO_BUSSTATE]);
> + iowrite8(0, &card->dpram[DPRAM_INFO_BUSSTATE2]);
> + dev_info(&card->pdev->dev, "%s up\n", __func__);
> + if (card->pdat->generation < 2) {
> + iowrite8(0, &card->dpram[DPRAM_V2_IRQ_TOHOST]);
> + /* flush the DPRAM caches */
> + wmb();
> + }
> +
> + softing_initialize_timestamp(card);
> +
> + /*
> + * do socketcan notifications/status changes
> + * from here, no errors should occur, or the failed: part
> + * must be reviewed
> + */
> + memset(&msg, 0, sizeof(msg));
> + msg.can_id = CAN_ERR_FLAG | CAN_ERR_RESTARTED;
> + msg.can_dlc = CAN_ERR_DLC;
> + for (j = 0; j < ARRAY_SIZE(card->net); ++j) {
> + if (!(mask_start & (1 << j)))
> + continue;
> + netdev = card->net[j];
> + if (!netdev)
> + continue;
> + priv = netdev_priv(netdev);
> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> + open_candev(netdev);
> + if (dev != netdev) {
> + /* notify other busses on the restart */
> + softing_netdev_rx(netdev, &msg, ktime_set(0, 0));
> + ++priv->can.can_stats.restarts;
> + }
> + netif_wake_queue(netdev);
> + }
> +
> + /* enable interrupts */
> + ret = softing_enable_irq(card, 1);
> + if (ret)
> + goto failed;
> +card_done:
> + mutex_unlock(&card->fw.lock);
> + return 0;
> +failed:
> + dev_alert(&card->pdev->dev, "firmware failed, going idle\n");
> +invalid:
> + softing_enable_irq(card, 0);
> + softing_reset_chip(card);
> + mutex_unlock(&card->fw.lock);
> + /* bring all other interfaces down */
> + for (j = 0; j < ARRAY_SIZE(card->net); ++j) {
> + netdev = card->net[j];
> + if (!netdev)
> + continue;
> + dev_close(netdev);
> + }
> + return -EIO;
> +}
> +
> +int softing_default_output(struct net_device *netdev)
> +{
> + struct softing_priv *priv = netdev_priv(netdev);
> + struct softing *card = priv->card;
> +
> + switch (priv->chip) {
> + case 1000:
> + if (card->pdat->generation < 2)
> + return 0xfb;
> + return 0xfa;
> + case 5:
> + return 0x60;
> + default:
> + return 0x40;
> + }
> +}
Again, some magic constants.
> diff --git a/drivers/net/can/softing/softing_main.c b/drivers/net/can/softing/softing_main.c
> new file mode 100644
> index 0000000..4f74075
> --- /dev/null
> +++ b/drivers/net/can/softing/softing_main.c
> @@ -0,0 +1,903 @@
> +/*
> +* drivers/net/can/softing/softing_main.c
> +*
> +* Copyright (C) 2008-2010
> +*
> +* - Kurt Van Dijck, EIA Electronics
> +*
> +* This program is free software; you can redistribute it and/or modify
> +* it under the terms of the version 2 of the GNU General Public License
> +* as published by the Free Software Foundation
> +*
> +* This program is distributed in the hope that it will be useful,
> +* but WITHOUT ANY WARRANTY; without even the implied warranty of
> +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +* GNU General Public License for more details.
> +*
> +* You should have received a copy of the GNU General Public License
> +* along with this program; if not, write to the Free Software
> +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> +*/
> +
> +#include <linux/version.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +
> +#include "softing.h"
> +
> +#define TX_ECHO_SKB_MAX (TXMAX/2)
> +
> +/*
> + * test is a specific CAN netdev
> + * is online (ie. up 'n running, not sleeping, not busoff
> + */
> +static inline int canif_is_active(struct net_device *netdev)
> +{
> + struct can_priv *can = netdev_priv(netdev);
Empty line, please.
> + if (!netif_running(netdev))
> + return 0;
> + return (can->state <= CAN_STATE_ERROR_PASSIVE);
> +}
> +
> +/* trigger the tx queue-ing */
> +static netdev_tx_t
> +softing_netdev_start_xmit(struct sk_buff *skb, struct net_device *dev)
See general comments.
> +{
> + struct softing_priv *priv = netdev_priv(dev);
> + struct softing *card = priv->card;
> + int ret;
> + uint8_t *ptr;
> + uint8_t fifo_wr, fifo_rd;
> + struct can_frame *cf = (struct can_frame *)skb->data;
> + uint8_t buf[DPRAM_TX_SIZE];
> +
> + if (can_dropped_invalid_skb(dev, skb))
> + return NETDEV_TX_OK;
> +
> + spin_lock(&card->spin);
> +
> + ret = NETDEV_TX_BUSY;
> + if (!card->fw.up)
> + goto xmit_done;
> + if (card->tx.pending >= TXMAX)
> + goto xmit_done;
> + if (priv->tx.pending >= TX_ECHO_SKB_MAX)
> + goto xmit_done;
What about using "||"?
> + fifo_wr = ioread8(&card->dpram[DPRAM_TX_WR]);
> + fifo_rd = ioread8(&card->dpram[DPRAM_TX_RD]);
> + if (fifo_wr == fifo_rd)
> + /* fifo full */
> + goto xmit_done;
> + memset(buf, 0, sizeof(buf));
> + ptr = buf;
> + *ptr = CMD_TX;
> + if (cf->can_id & CAN_RTR_FLAG)
> + *ptr |= CMD_RTR;
> + if (cf->can_id & CAN_EFF_FLAG)
> + *ptr |= CMD_XTD;
> + if (priv->index)
> + *ptr |= CMD_BUS2;
> + ++ptr;
> + *ptr++ = cf->can_dlc;
> + *ptr++ = (cf->can_id >> 0);
> + *ptr++ = (cf->can_id >> 8);
> + if (cf->can_id & CAN_EFF_FLAG) {
> + *ptr++ = (cf->can_id >> 16);
> + *ptr++ = (cf->can_id >> 24);
> + } else {
> + /* increment 1, not 2 as you might think */
> + ptr += 1;
> + }
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + memcpy(ptr, &cf->data[0], cf->can_dlc);
> + memcpy_toio(&card->dpram[DPRAM_TX + DPRAM_TX_SIZE * fifo_wr],
> + buf, DPRAM_TX_SIZE);
> + if (++fifo_wr >= DPRAM_TX_CNT)
> + fifo_wr = 0;
> + iowrite8(fifo_wr, &card->dpram[DPRAM_TX_WR]);
> + card->tx.last_bus = priv->index;
> + ++card->tx.pending;
> + ++priv->tx.pending;
> + can_put_echo_skb(skb, dev, priv->tx.echo_put);
> + ++priv->tx.echo_put;
> + if (priv->tx.echo_put >= TX_ECHO_SKB_MAX)
> + priv->tx.echo_put = 0;
> + /* can_put_echo_skb() saves the skb, safe to return TX_OK */
> + ret = NETDEV_TX_OK;
> +xmit_done:
> + spin_unlock(&card->spin);
> + if (card->tx.pending >= TXMAX) {
> + int j;
Empty line please.
> + for (j = 0; j < ARRAY_SIZE(card->net); ++j) {
> + if (card->net[j])
> + netif_stop_queue(card->net[j]);
> + }
> + }
> + if (ret != NETDEV_TX_OK)
> + netif_stop_queue(dev);
> +
> + return ret;
> +}
> +
> +/*
> + * shortcut for skb delivery
> + */
> +int softing_netdev_rx(struct net_device *netdev,
> + const struct can_frame *msg, ktime_t ktime)
> +{
> + struct sk_buff *skb;
> + struct can_frame *cf;
> + int ret;
> +
> + skb = alloc_can_skb(netdev, &cf);
> + if (!skb)
> + return -ENOMEM;
> + memcpy(cf, msg, sizeof(*msg));
> + skb->tstamp = ktime;
> + ret = netif_rx(skb);
> + if (ret == NET_RX_DROP)
> + ++netdev->stats.rx_dropped;
Hm, I wonder if all Socket-CAN drivers should handle the return value of
netif_rx that way?
> + return ret;
> +}
> +
> +/*
> + * softing_handle_1
> + * pop 1 entry from the DPRAM queue, and process
> + */
> +static int softing_handle_1(struct softing *card)
> +{
> + int j;
> + struct net_device *netdev;
> + struct softing_priv *priv;
> + ktime_t ktime;
> + struct can_frame msg;
> +
> + int lost_msg;
> + uint8_t fifo_rd, fifo_wr;
> + unsigned int cnt = 0;
> + uint8_t *ptr;
> + u32 tmp;
> + u8 cmd;
> + uint8_t buf[DPRAM_RX_SIZE];
> +
> + memset(&msg, 0, sizeof(msg));
> + /* test for lost msgs */
> + lost_msg = ioread8(&card->dpram[DPRAM_RX_LOST]);
> + if (lost_msg) {
> + /* reset condition */
> + iowrite8(0, &card->dpram[DPRAM_RX_LOST]);
> + /* prepare msg */
> + msg.can_id = CAN_ERR_FLAG | CAN_ERR_CRTL;
> + msg.can_dlc = CAN_ERR_DLC;
> + msg.data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> + /*
> + * service to all busses, we don't know which it was applicable
> + * but only service busses that are online
> + */
> + for (j = 0; j < ARRAY_SIZE(card->net); ++j) {
> + netdev = card->net[j];
> + if (!netdev)
> + continue;
> + if (!canif_is_active(netdev))
> + /* a dead bus has no overflows */
> + continue;
> + ++netdev->stats.rx_over_errors;
> + softing_netdev_rx(netdev, &msg, ktime_set(0, 0));
> + }
> + /* prepare for other use */
> + memset(&msg, 0, sizeof(msg));
> + ++cnt;
> + }
> +
> + fifo_rd = ioread8(&card->dpram[DPRAM_RX_RD]);
> + fifo_wr = ioread8(&card->dpram[DPRAM_RX_WR]);
> +
> + if (++fifo_rd >= DPRAM_RX_CNT)
> + fifo_rd = 0;
> + if (fifo_wr == fifo_rd)
> + return cnt;
> +
> + memcpy_fromio(buf, &card->dpram[DPRAM_RX + DPRAM_RX_SIZE*fifo_rd],
> + DPRAM_RX_SIZE);
> + mb();
> + /* trigger dual port RAM */
> + iowrite8(fifo_rd, &card->dpram[DPRAM_RX_RD]);
> +
> + ptr = buf;
> + cmd = *ptr++;
> + if (cmd == 0xff) {
> + /* not quite usefull, probably the card has got out */
> + dev_alert(&card->pdev->dev, "got cmd 0x%02x,"
> + " I suspect the card is lost\n", cmd);
> + return 0;
> + }
> + netdev = card->net[0];
> + if (cmd & CMD_BUS2)
> + netdev = card->net[1];
> + priv = netdev_priv(netdev);
> +
> + if (cmd & CMD_ERR) {
> + u8 can_state;
> + u8 state;
> + state = *ptr++;
> +
> + msg.can_id = CAN_ERR_FLAG;
> + msg.can_dlc = CAN_ERR_DLC;
> +
> + if (state & 0x80) {
Again some magic constants!
> + can_state = CAN_STATE_BUS_OFF;
> + msg.can_id |= CAN_ERR_BUSOFF;
> + state = 2;
Ditto.
> + } else if (state & 0x60) {
> + can_state = CAN_STATE_ERROR_PASSIVE;
> + msg.can_id |= CAN_ERR_BUSERROR;
A state change is not a bus error! You should use:
msg.can_id |= CAN_ERR_CRTL;
> + msg.data[1] = CAN_ERR_CRTL_TX_PASSIVE;
> + state = 1;
> + } else {
> + can_state = CAN_STATE_ERROR_ACTIVE;
> + state = 0;
> + msg.can_id |= CAN_ERR_BUSERROR;
Ditto.
> + }
> + /* update DPRAM */
> + iowrite8(state, &card->dpram[priv->index ?
> + DPRAM_INFO_BUSSTATE2 : DPRAM_INFO_BUSSTATE]);
> + /* timestamp */
> + tmp = le32_to_cpup((void *)ptr);
> + ptr += 4;
> + ktime = softing_raw2ktime(card, tmp);
> +
> + ++priv->can.can_stats.bus_error;
Ditto.
> + ++netdev->stats.rx_errors;
> + /* update internal status */
> + if (can_state != priv->can.state) {
> + priv->can.state = can_state;
> + if (can_state == CAN_STATE_ERROR_PASSIVE)
> + ++priv->can.can_stats.error_passive;
> + if (can_state == CAN_STATE_BUS_OFF) {
else if?
> + /* this calls can_close_cleanup() */
> + can_bus_off(netdev);
> + netif_stop_queue(netdev);
> + }
> + /* trigger socketcan */
> + softing_netdev_rx(netdev, &msg, ktime);
> + }
> +
> + } else {
> + if (cmd & CMD_RTR)
> + msg.can_id |= CAN_RTR_FLAG;
> + /* acknowledge, was tx msg
> + * no real tx flag to set
> + if (cmd & CMD_ACK) {
> + }
> + */
> + msg.can_dlc = get_can_dlc(*ptr++);
> + if (cmd & CMD_XTD) {
> + msg.can_id |= CAN_EFF_FLAG;
> + msg.can_id |= le32_to_cpup((void *)ptr);
> + ptr += 4;
> + } else {
> + msg.can_id |= le16_to_cpup((void *)ptr);
> + ptr += 2;
> + }
> + /* timestamp */
> + tmp = le32_to_cpup((void *)ptr);
> + ptr += 4;
> + ktime = softing_raw2ktime(card, tmp);
> + if (!(msg.can_id & CAN_RTR_FLAG))
> + memcpy(&msg.data[0], ptr, 8);
> + ptr += 8;
> + /* update socket */
> + if (cmd & CMD_ACK) {
> + struct sk_buff *skb;
> + skb = priv->can.echo_skb[priv->tx.echo_get];
> + if (skb)
> + skb->tstamp = ktime;
> + can_get_echo_skb(netdev, priv->tx.echo_get);
> + ++priv->tx.echo_get;
> + if (priv->tx.echo_get >= TX_ECHO_SKB_MAX)
> + priv->tx.echo_get = 0;
> + if (priv->tx.pending)
> + --priv->tx.pending;
> + if (card->tx.pending)
> + --card->tx.pending;
> + ++netdev->stats.tx_packets;
> + netdev->stats.tx_bytes += msg.can_dlc;
> + } else {
> + ++netdev->stats.rx_packets;
> + netdev->stats.rx_bytes += msg.can_dlc;
> + softing_netdev_rx(netdev, &msg, ktime);
> + }
> + }
> + ++cnt;
> + return cnt;
> +}
> +
> +/*
> + * real interrupt handler
> + */
> +static irqreturn_t softing_irq_thread(int irq, void *dev_id)
> +{
> + struct softing *card = (struct softing *)dev_id;
> + struct net_device *netdev;
> + struct softing_priv *priv;
> + int j, offset, work_done;
> +
> + work_done = 0;
> + spin_lock_bh(&card->spin);
> + while (softing_handle_1(card) > 0) {
> + ++card->irq.svc_count;
> + ++work_done;
> + }
> + spin_unlock_bh(&card->spin);
> + /* resume tx queue's */
> + offset = card->tx.last_bus;
> + for (j = 0; j < ARRAY_SIZE(card->net); ++j) {
> + if (card->tx.pending >= TXMAX)
> + break;
> + netdev = card->net[(j + offset + 1) % card->pdat->nbus];
> + if (!netdev)
> + continue;
> + priv = netdev_priv(netdev);
> + if (!canif_is_active(netdev))
> + /* it makes no sense to wake dead busses */
> + continue;
> + if (priv->tx.pending >= TX_ECHO_SKB_MAX)
> + continue;
> + ++work_done;
> + netif_wake_queue(netdev);
> + }
> + return work_done ? IRQ_HANDLED : IRQ_NONE;
> +}
> +
> +/*
> + * interrupt routines:
> + * schedule the 'real interrupt handler'
> + */
> +static
> +irqreturn_t softing_irq_v2(int irq, void *dev_id)
> +{
> + struct softing *card = (struct softing *)dev_id;
> + uint8_t ir;
> +
> + ir = ioread8(&card->dpram[DPRAM_V2_IRQ_TOHOST]);
> + iowrite8(0, &card->dpram[DPRAM_V2_IRQ_TOHOST]);
> + return (1 == ir) ? IRQ_WAKE_THREAD : IRQ_NONE;
> +}
> +
> +static
> +irqreturn_t softing_irq_v1(int irq, void *dev_id)
See general comments.
> +{
> + struct softing *card = (struct softing *)dev_id;
> + uint8_t ir;
> +
> + ir = ioread8(&card->dpram[DPRAM_IRQ_TOHOST]);
> + iowrite8(0, &card->dpram[DPRAM_IRQ_TOHOST]);
> + return ir ? IRQ_WAKE_THREAD : IRQ_NONE;
> +}
> +
> +/*
> + * netdev/candev inter-operability
> + */
> +static int softing_netdev_open(struct net_device *ndev)
> +{
> + int ret;
> +
> + /* check or determine and set bittime */
> + ret = open_candev(ndev);
> + if (ret)
> + goto failed;
> + ret = softing_startstop(ndev, 1);
> + if (ret)
> + goto failed;
> + return 0;
> +failed:
> + return ret;
Do you really need that label?
> +}
> +
> +static int softing_netdev_stop(struct net_device *ndev)
> +{
> + int ret;
> +
> + netif_stop_queue(ndev);
> +
> + /* softing cycle does close_candev() */
> + ret = softing_startstop(ndev, 0);
> + return ret;
> +}
> +
> +static int softing_candev_set_mode(struct net_device *ndev, enum can_mode mode)
> +{
> + int ret;
> +
> + switch (mode) {
> + case CAN_MODE_START:
> + /* softing_startstop does close_candev() */
> + ret = softing_startstop(ndev, 1);
> + return ret;
> + case CAN_MODE_STOP:
> + case CAN_MODE_SLEEP:
> + return -EOPNOTSUPP;
> + }
> + return 0;
> +}
> +
> +/*
> + * Softing device management helpers
> + */
> +int softing_enable_irq(struct softing *card, int enable)
> +{
> + int ret;
Empty line, please.
> + if (!enable) {
> + if (card->irq.requested && card->irq.nr) {
> + free_irq(card->irq.nr, card);
> + card->irq.requested = 0;
> + }
> + return 0;
> + }
> + if (!card->irq.requested && (card->irq.nr)) {
> + ret = request_threaded_irq(card->irq.nr,
> + (card->pdat->generation >= 2)
> + ? softing_irq_v2 : softing_irq_v1,
> + softing_irq_thread, IRQF_SHARED,
> + dev_name(&card->pdev->dev), card);
> + if (ret) {
> + dev_alert(&card->pdev->dev,
> + "request_threaded_irq(%u) failed\n",
> + card->irq.nr);
> + return ret;
> + }
> + card->irq.requested = 1;
> + }
> + return 0;
> +}
> +
> +static void softing_card_shutdown(struct softing *card)
> +{
> + int fw_up = 0;
Empty line, please.
> + dev_dbg(&card->pdev->dev, "%s()\n", __func__);
Please reduce the debugging output to a few useful messages (for the
final user).
> + if (mutex_lock_interruptible(&card->fw.lock))
> + /* return -ERESTARTSYS*/;
What is the "if" then good for? Do you want to handle the return code?
> + fw_up = card->fw.up;
> + card->fw.up = 0;
> +
> + if (card->irq.requested && card->irq.nr) {
> + free_irq(card->irq.nr, card);
> + card->irq.requested = 0;
> + }
> + if (fw_up) {
> + if (card->pdat->enable_irq)
> + card->pdat->enable_irq(card->pdev, 0);
> + softing_set_reset_dpram(card);
> + if (card->pdat->reset)
> + card->pdat->reset(card->pdev, 1);
> + }
> + mutex_unlock(&card->fw.lock);
> +}
> +
> +static int softing_card_boot(struct softing *card)
> +{
> + int j;
> + static const uint8_t stream[] = {
> + 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, };
> + unsigned char back[sizeof(stream)];
Empty line please.
> + dev_dbg(&card->pdev->dev, "%s()\n", __func__);
See comment above.
> + if (mutex_lock_interruptible(&card->fw.lock))
> + return -ERESTARTSYS;
> + if (card->fw.up) {
> + mutex_unlock(&card->fw.lock);
> + return 0;
> + }
> + /* reset board */
> + if (card->pdat->enable_irq)
> + card->pdat->enable_irq(card->pdev, 1);
> + /* boot card */
> + softing_set_reset_dpram(card);
> + if (card->pdat->reset)
> + card->pdat->reset(card->pdev, 1);
> + for (j = 0; (j + sizeof(stream)) < card->dpram_size;
> + j += sizeof(stream)) {
> +
> + memcpy_toio(&card->dpram[j], stream, sizeof(stream));
> + /* flush IO cache */
> + mb();
> + memcpy_fromio(back, &card->dpram[j], sizeof(stream));
> +
> + if (!memcmp(back, stream, sizeof(stream)))
> + continue;
> + /* memory is not equal */
> + dev_alert(&card->pdev->dev, "dpram failed at 0x%04x\n", j);
> + goto open_failed;
> + }
> + wmb();
> + /* load boot firmware */
> + if (softing_load_fw(card->pdat->boot.fw, card, card->dpram,
> + card->dpram_size,
> + card->pdat->boot.offs -
> + card->pdat->boot.addr))
> + goto open_failed;
> + /* load loader firmware */
> + if (softing_load_fw(card->pdat->load.fw, card, card->dpram,
> + card->dpram_size,
> + card->pdat->load.offs -
> + card->pdat->load.addr))
> + goto open_failed;
> +
> + if (card->pdat->reset)
> + card->pdat->reset(card->pdev, 0);
> + softing_clr_reset_dpram(card);
> + if (softing_bootloader_command(card, 0, "card boot"))
> + goto open_failed;
> + if (softing_load_app_fw(card->pdat->app.fw, card))
> + goto open_failed;
> + /* reset chip */
> + iowrite8(0, &card->dpram[DPRAM_RESET_RX_FIFO]);
> + iowrite8(0, &card->dpram[DPRAM_RESET_RX_FIFO+1]);
> + iowrite8(1, &card->dpram[DPRAM_RESET]);
> + iowrite8(0, &card->dpram[DPRAM_RESET+1]);
> + /* sync */
> + if (softing_fct_cmd(card, 99, 0x55, "sync-a"))
> + goto open_failed;
> + if (softing_fct_cmd(card, 99, 0xaa, "sync-a"))
> + goto open_failed;
> + /* reset chip */
> + if (softing_fct_cmd(card, 0, 0, "reset_chip"))
> + goto open_failed;
> + /* get_serial */
> + if (softing_fct_cmd(card, 43, 0, "get_serial_number"))
> + goto open_failed;
> + card->id.serial = ioread32(&card->dpram[DPRAM_FCT_PARAM]);
> + /* get_version */
> + if (softing_fct_cmd(card, 12, 0, "get_version"))
> + goto open_failed;
> + card->id.fw = ioread16(&card->dpram[DPRAM_FCT_PARAM + 2]);
> + card->id.hw = ioread16(&card->dpram[DPRAM_FCT_PARAM + 4]);
> + card->id.lic = ioread16(&card->dpram[DPRAM_FCT_PARAM + 6]);
> + card->id.chip[0] = ioread16(&card->dpram[DPRAM_FCT_PARAM + 8]);
> + card->id.chip[1] = ioread16(&card->dpram[DPRAM_FCT_PARAM + 10]);
> +
> + dev_info(&card->pdev->dev, "card booted, type %s, "
> + "serial %u, fw %u, hw %u, lic %u, chip (%u,%u)\n",
> + card->pdat->name, card->id.serial, card->id.fw, card->id.hw,
> + card->id.lic, card->id.chip[0], card->id.chip[1]);
> +
> + card->fw.up = 1;
> + mutex_unlock(&card->fw.lock);
> + return 0;
> +open_failed:
> + card->fw.up = 0;
> + if (card->pdat->enable_irq)
> + card->pdat->enable_irq(card->pdev, 0);
> + softing_set_reset_dpram(card);
> + if (card->pdat->reset)
> + card->pdat->reset(card->pdev, 1);
> + mutex_unlock(&card->fw.lock);
> + return -EIO;
> +}
> +
> +/*
> + * netdev sysfs
> + */
> +static ssize_t show_channel(struct device *dev
> + , struct device_attribute *attr, char *buf)
See general comments. Here and for the following function declarations.
> +{
> + struct net_device *ndev = to_net_dev(dev);
> + struct softing_priv *priv = netdev2softing(ndev);
> + return sprintf(buf, "%i\n", priv->index);
> +}
> +
> +static ssize_t show_chip(struct device *dev
> + , struct device_attribute *attr, char *buf)
> +{
> + struct net_device *ndev = to_net_dev(dev);
> + struct softing_priv *priv = netdev2softing(ndev);
> + return sprintf(buf, "%i\n", priv->chip);
> +}
> +
> +static ssize_t show_output(struct device *dev
> + , struct device_attribute *attr, char *buf)
> +{
> + struct net_device *ndev = to_net_dev(dev);
> + struct softing_priv *priv = netdev2softing(ndev);
> + return sprintf(buf, "0x%02x\n", priv->output);
> +}
> +
> +static ssize_t store_output(struct device *dev
> + , struct device_attribute *attr
> + , const char *buf, size_t count)
> +{
> + struct net_device *ndev = to_net_dev(dev);
> + struct softing_priv *priv = netdev2softing(ndev);
> + struct softing *card = priv->card;
> + unsigned long val;
> + int ret;
> +
> + ret = strict_strtoul(buf, 0, &val);
> + if (ret < 0)
> + return ret;
> + val &= 0xFF;
> +
> + ret = mutex_lock_interruptible(&card->fw.lock);
> + if (ret)
> + return -ERESTARTSYS;
> + if (netif_running(ndev)) {
> + mutex_unlock(&card->fw.lock);
> + return -EBUSY;
> + }
> + priv->output = val;
> + mutex_unlock(&card->fw.lock);
> + return count;
> +}
> +
> +static const DEVICE_ATTR(channel, S_IRUGO, show_channel, NULL);
> +static const DEVICE_ATTR(chip, S_IRUGO, show_chip, NULL);
> +static const DEVICE_ATTR(output, S_IRUGO | S_IWUSR, show_output, store_output);
> +
> +static const struct attribute *const netdev_sysfs_attrs[] = {
> + &dev_attr_channel.attr,
> + &dev_attr_chip.attr,
> + &dev_attr_output.attr,
> + NULL,
> +};
> +static const struct attribute_group netdev_sysfs_group = {
> + .name = NULL,
> + .attrs = (struct attribute **)netdev_sysfs_attrs,
> +};
> +
> +static const struct net_device_ops softing_netdev_ops = {
> + .ndo_open = softing_netdev_open,
> + .ndo_stop = softing_netdev_stop,
> + .ndo_start_xmit = softing_netdev_start_xmit,
> +};
> +
> +static const struct can_bittiming_const softing_btr_const = {
> + .tseg1_min = 1,
> + .tseg1_max = 16,
> + .tseg2_min = 1,
> + .tseg2_max = 8,
> + .sjw_max = 4, /* overruled */
> + .brp_min = 1,
> + .brp_max = 32, /* overruled */
> + .brp_inc = 1,
> +};
> +
> +
> +static struct net_device *softing_netdev_create(
> + struct softing *card, u16 chip_id)
> +{
> + struct net_device *netdev;
> + struct softing_priv *priv;
> +
> + netdev = alloc_candev(sizeof(*priv), TX_ECHO_SKB_MAX);
> + if (!netdev) {
> + dev_alert(&card->pdev->dev, "alloc_candev failed\n");
> + return NULL;
> + }
> + priv = netdev_priv(netdev);
> + priv->netdev = netdev;
> + priv->card = card;
> + memcpy(&priv->btr_const, &softing_btr_const, sizeof(priv->btr_const));
> + priv->btr_const.brp_max = card->pdat->max_brp;
> + priv->btr_const.sjw_max = card->pdat->max_sjw;
> + priv->can.bittiming_const = &priv->btr_const;
> + priv->can.clock.freq = 8000000;
Another magic constant.
> + priv->chip = chip_id;
> + priv->output = softing_default_output(netdev);
> + SET_NETDEV_DEV(netdev, &card->pdev->dev);
> +
> + netdev->flags |= IFF_ECHO;
> + netdev->netdev_ops = &softing_netdev_ops;
> + priv->can.do_set_mode = softing_candev_set_mode;
See general comments.
> + priv->can.ctrlmode_supported =
> + CAN_CTRLMODE_3_SAMPLES;/* | CAN_CTRLMODE_BERR_REPORTING */;
Hm, any chance to support CAN_CTRLMODE_BERR_REPORTING? If not, please
remove the comment.
> + return netdev;
> +}
> +
> +static int softing_netdev_register(struct net_device *netdev)
> +{
> + int ret;
> +
> + /*
> + * provide bus-specific sysfs attributes _during_ the uevent
> + */
> + netdev->sysfs_groups[0] = &netdev_sysfs_group;
> + ret = register_candev(netdev);
> + if (ret) {
> + dev_alert(&netdev->dev, "register failed\n");
> + return ret;
> + }
> + return 0;
> +}
> +
> +static void softing_netdev_cleanup(struct net_device *netdev)
> +{
> + unregister_candev(netdev);
> + free_candev(netdev);
> +}
> +
> +/*
> + * sysfs for Platform device
> + */
> +#define DEV_ATTR_RO(name, member) \
> +static ssize_t show_##name(struct device *dev, \
> + struct device_attribute *attr, char *buf) \
> +{ \
> + struct softing *card = platform_get_drvdata(to_platform_device(dev)); \
> + return sprintf(buf, "%u\n", card->member); \
> +} \
> +static DEVICE_ATTR(name, 0444, show_##name, NULL)
> +
> +DEV_ATTR_RO(serial , id.serial);
> +DEV_ATTR_RO(firmware , id.fw);
> +DEV_ATTR_RO(hardware , id.hw);
> +DEV_ATTR_RO(license , id.lic);
> +DEV_ATTR_RO(freq , id.freq);
> +DEV_ATTR_RO(txpending , tx.pending);
> +
> +static struct attribute *softing_pdev_attrs[] = {
> + &dev_attr_serial.attr,
> + &dev_attr_firmware.attr,
> + &dev_attr_hardware.attr,
> + &dev_attr_license.attr,
> + &dev_attr_freq.attr,
> + &dev_attr_txpending.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group softing_pdev_group = {
> + .attrs = softing_pdev_attrs,
> +};
> +
> +/*
> + * platform driver
> + */
> +static int softing_pdev_remove(struct platform_device *pdev)
> +{
> + struct softing *card = platform_get_drvdata(pdev);
> + int j;
> +
> + /* first, disable card*/
> + softing_card_shutdown(card);
> +
> + for (j = 0; j < ARRAY_SIZE(card->net); ++j) {
> + if (!card->net[j])
> + continue;
> + softing_netdev_cleanup(card->net[j]);
> + card->net[j] = NULL;
> + }
> + sysfs_remove_group(&pdev->dev.kobj, &softing_pdev_group);
> +
> + iounmap(card->dpram);
> + kfree(card);
> + return 0;
> +}
> +
> +static int softing_pdev_probe(struct platform_device *pdev)
> +{
> + const struct softing_platform_data *pdat = pdev->dev.platform_data;
> + struct softing *card;
> + struct net_device *netdev;
> + struct softing_priv *priv;
> + struct resource *pres;
> + int ret;
> + int j;
> +
> + if (!pdat) {
> + dev_warn(&pdev->dev, "no platform data\n");
> + return -EINVAL;
> + }
> + if (pdat->nbus > ARRAY_SIZE(card->net)) {
> + dev_warn(&pdev->dev, "%u nets??\n", pdat->nbus);
> + return -EINVAL;
> + }
> +
> + card = kzalloc(sizeof(*card), GFP_KERNEL);
> + if (!card)
> + return -ENOMEM;
> + card->pdat = pdat;
> + card->pdev = pdev;
> + platform_set_drvdata(pdev, card);
> + /* try_module_get(THIS_MODULE); */
> + mutex_init(&card->fw.lock);
> + spin_lock_init(&card->spin);
> +
> + ret = -EINVAL;
> + pres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!pres)
> + goto platform_resource_failed;;
> + card->dpram_phys = pres->start;
> + card->dpram_size = pres->end - pres->start + 1;
> + card->dpram = ioremap_nocache(card->dpram_phys, card->dpram_size);
> + if (!card->dpram) {
> + dev_alert(&card->pdev->dev, "dpram ioremap failed\n");
> + goto ioremap_failed;
> + }
> +
> + pres = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + if (pres)
> + card->irq.nr = pres->start;
> +
> + /* reset card */
> + ret = -EIO;
> + if (softing_card_boot(card)) {
> + dev_alert(&pdev->dev, "failed to boot\n");
> + goto boot_failed;
> + }
> +
> + /* only now, the chip's are known */
> + card->id.freq = card->pdat->freq * 1000000UL;
It would be more flexible to specific the frequency in Hz!? Or use a
more logical member name, frey_mhz, at least.
> +
> + ret = sysfs_create_group(&pdev->dev.kobj, &softing_pdev_group);
> + if (ret < 0) {
> + dev_alert(&card->pdev->dev, "sysfs failed\n");
> + goto sysfs_failed;
> + }
> +
> + ret = -ENOMEM;
> + for (j = 0; j < ARRAY_SIZE(card->net); ++j) {
> + card->net[j] = netdev =
> + softing_netdev_create(card, card->id.chip[j]);
> + if (!netdev) {
> + dev_alert(&pdev->dev, "failed to make can[%i]", j);
> + goto netdev_failed;
> + }
> + priv = netdev_priv(card->net[j]);
> + priv->index = j;
> + ret = softing_netdev_register(netdev);
> + if (ret) {
> + free_candev(netdev);
> + card->net[j] = NULL;
> + dev_alert(&card->pdev->dev,
> + "failed to register can[%i]\n", j);
> + goto netdev_failed;
> + }
> + }
> + dev_info(&card->pdev->dev, "card initialised\n");
> + return 0;
> +
> +netdev_failed:
> + for (j = 0; j < ARRAY_SIZE(card->net); ++j) {
> + if (!card->net[j])
> + continue;
> + softing_netdev_cleanup(card->net[j]);
> + }
> + sysfs_remove_group(&pdev->dev.kobj, &softing_pdev_group);
> +sysfs_failed:
> + softing_card_shutdown(card);
> +boot_failed:
> + iounmap(card->dpram);
> +ioremap_failed:
> +platform_resource_failed:
> + kfree(card);
> + return ret;
> +}
> +
> +static struct platform_driver softing_driver = {
> + .driver = {
> + .name = "softing",
> + .owner = THIS_MODULE,
> + },
> + .probe = softing_pdev_probe,
> + .remove = softing_pdev_remove,
> +};
I'm missing the use of __devinit and friends.
> +MODULE_ALIAS("platform:softing");
> +
> +static int __init softing_start(void)
> +{
> + return platform_driver_register(&softing_driver);
> +}
> +
> +static void __exit softing_stop(void)
> +{
> + platform_driver_unregister(&softing_driver);
> +}
> +
> +module_init(softing_start);
> +module_exit(softing_stop);
> +
> +MODULE_DESCRIPTION("Softing DPRAM CAN driver");
> +MODULE_AUTHOR("Kurt Van Dijck <kurt.van.dijck@....be>");
> +MODULE_LICENSE("GPL");
GPL v2 ?
> diff --git a/drivers/net/can/softing/softing_platform.h b/drivers/net/can/softing/softing_platform.h
> new file mode 100644
> index 0000000..678df36
> --- /dev/null
> +++ b/drivers/net/can/softing/softing_platform.h
> @@ -0,0 +1,38 @@
> +
> +#include <linux/platform_device.h>
> +
> +#ifndef _SOFTING_DEVICE_H_
> +#define _SOFTING_DEVICE_H_
> +
> +/* softing firmware directory prefix */
> +#define fw_dir "softing-4.6/"
> +
> +struct softing_platform_data {
> + unsigned int manf;
> + unsigned int prod;
> + /* generation
> + * 1st with NEC or SJA1000
> + * 8bit, exclusive interrupt, ...
> + * 2nd only SJA1000
> + * 16bit, shared interrupt
> + */
Please the usual multiline comment style.
> + int generation;
> + int nbus; /* # busses on device */
> + unsigned int freq; /* crystal in MHz */
> + unsigned int max_brp;
> + unsigned int max_sjw;
> + unsigned long dpram_size;
> + char name[32];
> + struct {
> + unsigned long offs;
> + unsigned long addr;
> + const char *fw;
> + } boot, load, app;
> + /* reset() function, bring pdev in or out of reset, depending on
> + value */
> + int (*reset)(struct platform_device *pdev, int value);
> + int (*enable_irq)(struct platform_device *pdev, int value);
> +};
> +
> +#endif
> +
Wolfgang.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists