Subject: Re: [PATCH 01/16] c_can_platform: add FlexCard D-CAN support

From: Marc Kleine-Budde <[email-masked]>
Date: Mon, 09 Sep 2013 10:22:39 +0200

On 09/09/2013 09:24 AM, Benedikt Spranger wrote:
> FlexCard supports up to 8 D-CAN devices with a shared DMA-capable receive
> function. Add support for FlexCard D-CAN.
> 
> Signed-off-by: Benedikt Spranger <[email-masked]>

What are the prerequisites for this series? This doesn't compile against
current net-next/master (e7d33bb lockref: add ability to mark lockrefs
"dead").

More Comments inline.
Marc

> ---
>  drivers/net/can/c_can/c_can.h          |   1 +
>  drivers/net/can/c_can/c_can_platform.c | 149 ++++++++++++++++++++++++++++++++-
>  2 files changed, 148 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index d2e1c21..d2e2d20 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -148,6 +148,7 @@ enum c_can_dev_id {
>  	BOSCH_C_CAN_PLATFORM,
>  	BOSCH_C_CAN,
>  	BOSCH_D_CAN,
> +	BOSCH_D_CAN_FLEXCARD,
>  };
>  
>  /* c_can private data structure */
> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> index c6f838d..43a3e3f 100644
> --- a/drivers/net/can/c_can/c_can_platform.c
> +++ b/drivers/net/can/c_can/c_can_platform.c
> @@ -32,6 +32,7 @@
>  #include <linux/clk.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/flexcard.h>
>  
>  #include <linux/can/dev.h>
>  
> @@ -81,6 +82,112 @@ static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable)
>  	writel(val, priv->raminit_ctrlreg);
>  }
>  
> +static int c_can_rx_pkt(void *p, void *data, size_t len)
> +{

Why are the first two arguments a void *, why do we have len, that's
never used? Has the length already been checked, is it guaranteed >=
sizeof(struct fc_packet_buf)?

> +	struct net_device *dev = p;
> +	struct net_device_stats *stats = &dev->stats;
> +	struct fc_packet_buf *pb = data;
> +	union fc_packet_types *pt = &pb->packet;
> +	struct can_frame *frame;
> +	struct sk_buff *skb;
> +	u32 flags, id, state, type;
> +
> +	switch (le32_to_cpu(pb->header.type)) {
> +	case fc_packet_type_can:
> +		skb = alloc_can_skb(dev, &frame);
> +		if (!skb) {
> +			stats->rx_dropped++;
> +			return -ENOMEM;
> +		}
> +
> +		id = le32_to_cpu(pt->can_packet.id);
> +		flags = le32_to_cpu(pt->can_packet.flags);
> +
> +		if (id & BIT(29))
> +			frame->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG;
> +		else
> +			frame->can_id = id & CAN_SFF_MASK;
> +
> +		if (flags & BIT(13))
> +			frame->can_id |= CAN_RTR_FLAG;
> +		frame->can_dlc = (flags >> 8) & 0xf;

please use get_can_dlc((flags >> 8) & 0xf)

> +		memcpy(frame->data, pt->can_packet.data, frame->can_dlc);

Please don't copy data if you receive an RTR frame. What about the
endianess of the data?

> +		netif_receive_skb(skb);
> +
> +		stats->rx_packets++;
> +		stats->rx_bytes += frame->can_dlc;
> +		break;
> +
> +	case fc_packet_type_can_error:
> +		stats->rx_errors++;
> +
> +		skb = alloc_can_err_skb(dev, &frame);
> +		if (!skb)
> +			return -ENOMEM;
> +
> +		type = le32_to_cpu(pt->can_error_packet.type);
> +		state = le32_to_cpu(pt->can_error_packet.state);
> +
> +		switch (state) {
> +		case fc_can_state_warning:
> +			frame->data[1] |= CAN_ERR_CRTL_RX_WARNING;
> +			frame->data[1] |= CAN_ERR_CRTL_TX_WARNING;
> +			frame->can_id |= CAN_ERR_CRTL;
> +			break;
> +		case fc_can_state_error_passive:
> +			frame->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
> +			frame->data[1] |= CAN_ERR_CRTL_TX_PASSIVE;
> +			frame->can_id |= CAN_ERR_CRTL;
> +			break;
> +		case fc_can_state_bus_off:
> +			frame->can_id |= CAN_ERR_BUSOFF;
> +			break;
> +		}
> +
> +		switch (type) {
> +		case fc_can_error_stuff:
> +			frame->data[2] |= CAN_ERR_PROT_STUFF;
> +			frame->can_id |= CAN_ERR_PROT;
> +			break;
> +		case fc_can_error_form:
> +			frame->data[2] |= CAN_ERR_PROT_FORM;
> +			frame->can_id |= CAN_ERR_PROT;
> +			break;
> +		case fc_can_error_acknowledge:
> +			frame->can_id |= CAN_ERR_ACK;
> +			break;
> +		case fc_can_error_bit1:
> +			frame->data[2] |= CAN_ERR_PROT_BIT1;
> +			frame->can_id |= CAN_ERR_PROT;
> +			break;
> +		case fc_can_error_bit0:
> +			frame->data[2] |= CAN_ERR_PROT_BIT0;
> +			frame->can_id |= CAN_ERR_PROT;
> +			break;
> +		case fc_can_error_crc:
> +			frame->data[3] |= CAN_ERR_PROT_LOC_CRC_SEQ;
> +			frame->can_id |= CAN_ERR_PROT;
> +			break;
> +		case fc_can_error_parity:
> +			frame->data[1] |= CAN_ERR_PROT_UNSPEC;
> +			frame->can_id |= CAN_ERR_CRTL;
> +			break;
> +		}
> +		frame->data[5] = pt->can_error_packet.rx_error_counter;
> +		frame->data[6] = pt->can_error_packet.tx_error_counter;
> +
> +		stats->rx_bytes += frame->can_dlc;
> +		stats->rx_packets++;
> +
> +		netif_receive_skb(skb);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static struct platform_device_id c_can_id_table[] = {
>  	[BOSCH_C_CAN_PLATFORM] = {
>  		.name = KBUILD_MODNAME,
> @@ -93,6 +200,10 @@ static struct platform_device_id c_can_id_table[] = {
>  	[BOSCH_D_CAN] = {
>  		.name = "d_can",
>  		.driver_data = BOSCH_D_CAN,
> +	},
> +	[BOSCH_D_CAN_FLEXCARD] = {
> +		.name = "flexcard-d_can",
> +		.driver_data = BOSCH_D_CAN_FLEXCARD,
>  	}, {
>  	}
>  };
> @@ -108,7 +219,7 @@ MODULE_DEVICE_TABLE(of, c_can_of_table);
>  static int c_can_plat_probe(struct platform_device *pdev)
>  {
>  	int ret;
> -	void __iomem *addr;
> +	void __iomem *addr, *reset_reg;
>  	struct net_device *dev;
>  	struct c_can_priv *priv;
>  	const struct of_device_id *match;
> @@ -167,6 +278,8 @@ static int c_can_plat_probe(struct platform_device *pdev)
>  	}
>  
>  	priv = netdev_priv(dev);
> +	priv->can.clock.freq = clk_get_rate(clk);
> +
>  	switch (id->driver_data) {
>  	case BOSCH_C_CAN:
>  		priv->regs = reg_map_c_can;
> @@ -199,7 +312,26 @@ static int c_can_plat_probe(struct platform_device *pdev)
>  			dev_info(&pdev->dev, "control memory is not used for raminit\n");
>  		else
>  			priv->raminit = c_can_hw_raminit;
> +

Please remove

>  		break;
> +	case BOSCH_D_CAN_FLEXCARD:
> +		priv->regs = reg_map_d_can;
> +		priv->can.ctrlmode_supported |= CAN_CTRLMODE_3_SAMPLES;
> +		priv->read_reg = c_can_plat_read_reg_aligned_to_16bit;
> +		priv->write_reg = c_can_plat_write_reg_aligned_to_16bit;
> +		priv->instance = pdev->id;
> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +		reset_reg = ioremap(res->start, resource_size(res));
> +		if (!reset_reg || priv->instance < 0) {

priv->instance in unsinged

> +			dev_info(&pdev->dev, "Can't reset device.\n");
> +		} else {
> +			writel(1 << (4 + priv->instance), addr);
> +			udelay(100);
> +		}
> +		iounmap(reset_reg);
> +		priv->can.clock.freq = 16000000;
> +
>  	default:
>  		ret = -EINVAL;
>  		goto exit_free_device;
> @@ -208,7 +340,6 @@ static int c_can_plat_probe(struct platform_device *pdev)
>  	dev->irq = irq;
>  	priv->base = addr;
>  	priv->device = &pdev->dev;
> -	priv->can.clock.freq = clk_get_rate(clk);
>  	priv->priv = clk;
>  	priv->type = id->driver_data;
>  
> @@ -222,10 +353,22 @@ static int c_can_plat_probe(struct platform_device *pdev)
>  		goto exit_free_device;
>  	}
>  
> +	if (id->driver_data == BOSCH_D_CAN_FLEXCARD) {
> +		ret = fc_register_rx_pkt(FC_CANIF_OFFSET + pdev->id,
> +				dev, c_can_rx_pkt);
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +					"registering RX-CB failed %d\n", ret);
> +			goto exit_unregister_device;
> +		}
> +	}
> +
>  	dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
>  		 KBUILD_MODNAME, priv->base, dev->irq);
>  	return 0;
>  
> +exit_unregister_device:
> +	unregister_c_can_dev(dev);
>  exit_free_device:
>  	free_c_can_dev(dev);
>  exit_iounmap:
> @@ -246,6 +389,8 @@ static int c_can_plat_remove(struct platform_device *pdev)
>  	struct c_can_priv *priv = netdev_priv(dev);
>  	struct resource *mem;
>  
> +	if (priv->type == BOSCH_D_CAN_FLEXCARD)
> +		fc_unregister_rx_pkt(FC_CANIF_OFFSET + priv->instance);
>  	unregister_c_can_dev(dev);
>  
>  	free_c_can_dev(dev);
> 

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |