>> More inline...
>>
>> Ira W. Snyder wrote:
>>> The Janz VMOD-ICAN3 is a MODULbus daughterboard which fits onto any
>>> MODULbus carrier board. It is an intelligent CAN controller with a
>>> microcontroller and associated firmware.
>>>
>>> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
>>> Cc:
socketcan-core@lists.berlios.de
>>> Cc:
netdev@vger.kernel.org
>>> ---
>>> drivers/net/can/Kconfig | 10 +
>>> drivers/net/can/Makefile | 1 +
>>> drivers/net/can/janz-ican3.c | 1659 ++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 1670 insertions(+), 0 deletions(-)
>>> create mode 100644 drivers/net/can/janz-ican3.c
>>>
>>> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
>>> index 05b7517..2c5227c 100644
>>> --- a/drivers/net/can/Kconfig
>>> +++ b/drivers/net/can/Kconfig
>>> @@ -63,6 +63,16 @@ config CAN_BFIN
>>> To compile this driver as a module, choose M here: the
>>> module will be called bfin_can.
>>>
>>> +config CAN_JANZ_ICAN3
>>> + tristate "Janz VMOD-ICAN3 Intelligent CAN controller"
>>> + depends on CAN_DEV && MFD_JANZ_CMODIO
>>> + ---help---
>>> + Driver for Janz VMOD-ICAN3 Intelligent CAN controller module, which
>>> + connects to a MODULbus carrier board.
>>> +
>>> + This driver can also be built as a module. If so, the module will be
>>> + called janz-ican3.ko.
>>> +
>>> source "drivers/net/can/mscan/Kconfig"
>>>
>>> source "drivers/net/can/sja1000/Kconfig"
>>> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
>>> index 7a702f2..9047cd0 100644
>>> --- a/drivers/net/can/Makefile
>>> +++ b/drivers/net/can/Makefile
>>> @@ -15,5 +15,6 @@ obj-$(CONFIG_CAN_AT91) += at91_can.o
>>> obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o
>>> obj-$(CONFIG_CAN_MCP251X) += mcp251x.o
>>> obj-$(CONFIG_CAN_BFIN) += bfin_can.o
>>> +obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.o
>>>
>>> ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
>>> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
>>> new file mode 100644
>>> index 0000000..94d4995
>>> --- /dev/null
>>> +++ b/drivers/net/can/janz-ican3.c
>> [snip]
>> +struct ican3_dev {
>>> +
>>> + /* must be the first member */
>>> + struct can_priv can;
>>> +
>>> + /* CAN network device */
>>> + struct net_device *ndev;
>>> + struct napi_struct napi;
>>> +
>>> + /* Device for printing */
>>> + struct device *dev;
>>> +
>>> + /* module number */
>>> + unsigned int num;
>>> +
>>> + /* base address of registers and IRQ */
>>> + struct janz_cmodio_onboard_regs __iomem *ctrl;
>>> + struct ican3_dpm_control *dpmctrl;
>>> + void __iomem *dpm;
>>> + int irq;
>>> +
>>> + /* old and new style host interface */
>>> + unsigned int iftype;
>>> + spinlock_t lock;
>> Please describe what the lock is used for.
>>
>>> + /* new host interface */
>>> + unsigned int rx_int;
>>> + unsigned int rx_num;
>>> + unsigned int tx_num;
>>> +
>>> + /* fast host interface */
>>> + unsigned int fastrx_start;
>>> + unsigned int fastrx_int;
>>> + unsigned int fastrx_num;
>>> + unsigned int fasttx_start;
>>> + unsigned int fasttx_num;
>>> +
>>> + /* first free DPM page */
>>> + unsigned int free_page;
>>> +};
>> [snip]
>>> +static void ican3_to_can_frame(struct ican3_dev *mod,
>>> + struct ican3_fast_desc *desc,
>>> + struct can_frame *cf)
>>> +{
>>> + if ((desc->command & ICAN3_CAN_TYPE_MASK) == ICAN3_CAN_TYPE_SFF) {
>>> + dev_dbg(mod->dev, "%s: old frame format\n", __func__);
>> This prints a debug message for every message which is not really
>> useful for the user. Please remove.
>>
>>> + if (desc->data[1] & ICAN3_SFF_RTR)
>>> + cf->can_id |= CAN_RTR_FLAG;
>>> +
>>> + cf->can_id |= desc->data[0] << 3;
>>> + cf->can_id |= (desc->data[1] & 0xe0) >> 5;
>>> + cf->can_dlc = desc->data[1] & ICAN3_CAN_DLC_MASK;
>>> + memcpy(cf->data, &desc->data[2], sizeof(cf->data));
>>> + } else {
>>> + dev_dbg(mod->dev, "%s: new frame format\n", __func__);
>> Ditto.
>>
>>> + cf->can_dlc = desc->data[0] & ICAN3_CAN_DLC_MASK;
>>> + if (desc->data[0] & ICAN3_EFF_RTR)
>>> + cf->can_id |= CAN_RTR_FLAG;
>>> +
>>> + if (desc->data[0] & ICAN3_EFF) {
>>> + cf->can_id |= CAN_EFF_FLAG;
>>> + cf->can_id |= desc->data[2] << 21; /* 28-21 */
>>> + cf->can_id |= desc->data[3] << 13; /* 20-13 */
>>> + cf->can_id |= desc->data[4] << 5; /* 12-5 */
>>> + cf->can_id |= (desc->data[5] & 0xf8) >> 3;
>>> + } else {
>>> + cf->can_id |= desc->data[2] << 3; /* 10-3 */
>>> + cf->can_id |= desc->data[3] >> 5; /* 2-0 */
>>> + }
>>> +
>>> + memcpy(cf->data, &desc->data[6], sizeof(cf->data));
>>> + }
>>> +}
>>> +
>>> +static void can_frame_to_ican3(struct ican3_dev *mod,
>>> + struct can_frame *cf,
>>> + struct ican3_fast_desc *desc)
>>> +{
>>> + /* clear out any stale data in the descriptor */
>>> + memset(desc->data, 0, sizeof(desc->data));
>>> +
>>> + /* we always use the extended format, with the ECHO flag set */
>>> + desc->command = ICAN3_CAN_TYPE_EFF;
>>> + desc->data[0] |= cf->can_dlc;
>>> + desc->data[1] |= ICAN3_ECHO;
>>> +
>>> + if (cf->can_id & CAN_RTR_FLAG)
>>> + desc->data[0] |= ICAN3_EFF_RTR;
>>> +
>>> + /* pack the id into the correct places */
>>> + if (cf->can_id & CAN_EFF_FLAG) {
>>> + dev_dbg(mod->dev, "%s: extended frame\n", __func__);
>> Ditto.
>>
>>> + desc->data[0] |= ICAN3_EFF;
>>> + desc->data[2] = (cf->can_id & 0x1fe00000) >> 21; /* 28-21 */
>>> + desc->data[3] = (cf->can_id & 0x001fe000) >> 13; /* 20-13 */
>>> + desc->data[4] = (cf->can_id & 0x00001fe0) >> 5; /* 12-5 */
>>> + desc->data[5] = (cf->can_id & 0x0000001f) << 3; /* 4-0 */
>>> + } else {
>>> + dev_dbg(mod->dev, "%s: standard frame\n", __func__);
>> Ditto.
>>
>>> + desc->data[2] = (cf->can_id & 0x7F8) >> 3; /* bits 10-3 */
>>> + desc->data[3] = (cf->can_id & 0x007) << 5; /* bits 2-0 */
>>> + }
>>> +
>>> + /* copy the data bits into the descriptor */
>>> + memcpy(&desc->data[6], cf->data, sizeof(cf->data));
>>> +}
>> [snip]
>>> +/*
>>> + * Handle CAN Event Indication Messages from the firmware
>>> + *
>>> + * The ICAN3 firmware provides the values of some SJA1000 registers when it
>>> + * generates this message. The code below is largely copied from the
>>> + * drivers/net/can/sja1000/sja1000.c file, and adapted as necessary
>>> + */
>>> +static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
>>> +{
>>> + struct net_device *dev = mod->ndev;
>>> + struct net_device_stats *stats = &dev->stats;
>>> + enum can_state state = mod->can.state;
>>> + struct can_frame *cf;
>>> + struct sk_buff *skb;
>>> + u8 status, isrc;
>>> +
>>> + /* we can only handle the SJA1000 part */
>>> + if (msg->data[1] != CEVTIND_CHIP_SJA1000) {
>>> + dev_err(mod->dev, "unable to handle errors on non-SJA1000\n");
>>> + return -ENODEV;
>>> + }
>>> +
>>> + /* check the message length for sanity */
>>> + if (msg->len < 6) {
>>> + dev_err(mod->dev, "error message too short\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + skb = alloc_can_err_skb(dev, &cf);
>>> + if (skb == NULL)
>>> + return -ENOMEM;
>>> +
>>> + isrc = msg->data[0];
>>> + status = msg->data[3];
>>> +
>>> + /* data overrun interrupt */
>>> + if (isrc == CEVTIND_DOI || isrc == CEVTIND_LOST) {
>> Here and for the other errors below a dev_dbg() would be useful. Please
>> check sja1000.c.
>>
>>> + cf->can_id |= CAN_ERR_CRTL;
>>> + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
>>> + stats->rx_over_errors++;
>>> + stats->rx_errors++;
>>> + dev_info(mod->dev, "%s: overflow frame generated\n", __func__);
>> s/dev_info/dev_dbg/ ?
>>
>
> Whoops, a leftover from debugging. Will change to dev_dbg().
>
>>> + }
>>> +
>>> + /* error warning interrupt */
>>> + if (isrc == CEVTIND_EI) {
>>> + if (status & SR_BS) {
>>> + state = CAN_STATE_BUS_OFF;
>>> + cf->can_id |= CAN_ERR_BUSOFF;
>>> + can_bus_off(dev);
>>> + } else if (status & SR_ES) {
>>> + state = CAN_STATE_ERROR_WARNING;
>>> + } else {
>>> + state = CAN_STATE_ERROR_ACTIVE;
>>> + }
>>> + }
>>> +
>>> + /* bus error interrupt */
>>> + if (isrc == CEVTIND_BEI) {
>>> + u8 ecc = msg->data[2];
>> Add an empty line, please.
>>
>>> + mod->can.can_stats.bus_error++;
>>> + stats->rx_errors++;
>>> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>>> +
>>> + switch (ecc & ECC_MASK) {
>>> + case ECC_BIT:
>>> + cf->data[2] |= CAN_ERR_PROT_BIT;
>>> + break;
>>> + case ECC_FORM:
>>> + cf->data[2] |= CAN_ERR_PROT_FORM;
>>> + break;
>>> + case ECC_STUFF:
>>> + cf->data[2] |= CAN_ERR_PROT_STUFF;
>>> + break;
>>> + default:
>>> + cf->data[2] |= CAN_ERR_PROT_UNSPEC;
>>> + cf->data[3] = ecc & ECC_SEG;
>>> + break;
>>> + }
>>> +
>>> + if ((ecc & ECC_DIR) == 0)
>>> + cf->data[2] |= CAN_ERR_PROT_TX;
>>> + }
>>> +
>>> + if (state != mod->can.state && (state == CAN_STATE_ERROR_WARNING ||
>>> + state == CAN_STATE_ERROR_PASSIVE)) {
>>> + u8 rxerr = msg->data[4];
>>> + u8 txerr = msg->data[5];
>> Ditto.
>>
>>> + cf->can_id |= CAN_ERR_CRTL;
>>> + if (state == CAN_STATE_ERROR_WARNING) {
>>> + mod->can.can_stats.error_warning++;
>>> + cf->data[1] = (txerr > rxerr) ?
>>> + CAN_ERR_CRTL_TX_WARNING :
>>> + CAN_ERR_CRTL_RX_WARNING;
>>> + } else {
>>> + mod->can.can_stats.error_passive++;
>>> + cf->data[1] = (txerr > rxerr) ?
>>> + CAN_ERR_CRTL_TX_PASSIVE :
>>> + CAN_ERR_CRTL_RX_PASSIVE;
>>> + }
>>> + }
>>> +
>>> + mod->can.state = state;
>>> + stats->rx_errors++;
>>> + stats->rx_bytes += cf->can_dlc;
>>> + netif_rx(skb);
>>> + return 0;
>>> +}
>> [snip]
>>> +static irqreturn_t ican3_irq(int irq, void *dev_id)
>>> +{
>>> + struct ican3_dev *mod = dev_id;
>>> + u8 stat;
>>> +
>>> + /*
>>> + * The interrupt status register on this device reports interrupts
>>> + * as zeroes instead of using ones like most other devices
>>> + */
>>> + stat = ioread8(&mod->ctrl->int_disable) & (1 << mod->num);
>>> + if (stat == (1 << mod->num))
>>> + return IRQ_NONE;
>>> +
>>> + dev_dbg(mod->dev, "IRQ: module %d\n", mod->num);
>> Please remove this dev_dbg() as well.
>>
>> [snip]
>>> +/*
>>> + * Startup an ICAN module, bringing it into fast mode
>>> + */
>>> +static int __devinit ican3_startup_module(struct ican3_dev *mod)
>>> +{
>>> + int ret;
>>> +
>>> + ret = ican3_reset_module(mod);
>>> + if (ret) {
>>> + dev_err(mod->dev, "unable to reset module\n");
>>> + return ret;
>>> + }
>>> +
>>> + /* re-enable interrupts so we can send messages */
>>> + iowrite8(1 << mod->num, &mod->ctrl->int_enable);
>>> +
>>> + ret = ican3_msg_connect(mod);
>>> + if (ret) {
>>> + dev_err(mod->dev, "unable to connect to module\n");
>>> + return ret;
>>> + }
>>> +
>>> + ican3_init_new_host_interface(mod);
>>> + ret = ican3_msg_newhostif(mod);
>>> + if (ret) {
>>> + dev_err(mod->dev, "unable to switch to new-style interface\n");
>>> + return ret;
>>> + }
>>> +
>>> + ret = ican3_set_termination(mod, true);
>>> + if (ret) {
>>> + dev_err(mod->dev, "unable to enable termination\n");
>>> + return ret;
>>> + }
>>
>> Could you please allow the user to disable termination, e.g. via module parameter
>> "bus_termination=0" in case he uses a cable with built-in termination.
>>
>
> What would you think about a sysfs node instead, so it could be changed
> at runtime, on a per-daughtercard basis? Do you think enabling bus
> termination is a good default? IMO, it is a pretty safe default for most
> users.
