From: Marc Kleine-Budde <mkl@pengutronix.de>
To: Benedikt Spranger <b.spranger@linutronix.de>
Cc: netdev@vger.kernel.org,
Alexander Frank <Alexander.Frank@eberspaecher.com>,
Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
Holger Dengler <dengler@linutronix.de>
Subject: Re: [PATCH 01/16] c_can_platform: add FlexCard D-CAN support
Date: Mon, 09 Sep 2013 10:22:39 +0200 [thread overview]
Message-ID: <522D854F.2030207@pengutronix.de> (raw)
In-Reply-To: <1378711513-2548-2-git-send-email-b.spranger@linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 8463 bytes --]
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 <b.spranger@linutronix.de>
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 |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]
next prev parent reply other threads:[~2013-09-09 8:22 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-09-09 7:24 [PATCH 00/16] Support for Eberspächer Flexcard DCAN function Benedikt Spranger
2013-09-09 7:24 ` [PATCH 01/16] c_can_platform: add FlexCard D-CAN support Benedikt Spranger
2013-09-09 8:22 ` Marc Kleine-Budde
2013-09-09 8:22 ` Marc Kleine-Budde [this message]
2013-09-09 7:24 ` [PATCH 02/16] c_can: add generic D-CAN RAM initialization support Benedikt Spranger
2013-09-09 8:34 ` Marc Kleine-Budde
2013-09-09 7:25 ` [PATCH 03/16] c_can: simplify arbitration register handling Benedikt Spranger
2013-09-09 9:16 ` Marc Kleine-Budde
2013-09-09 9:16 ` Marc Kleine-Budde
2013-09-09 7:25 ` [PATCH 04/16] c_can: fix receive buffer configuration Benedikt Spranger
2013-09-09 10:51 ` Marc Kleine-Budde
2013-09-09 7:25 ` [PATCH 05/16] c_can: use 32 bit access for D_CAN Benedikt Spranger
2013-09-09 9:37 ` Marc Kleine-Budde
2013-09-09 7:25 ` [PATCH 06/16] c_can: consider set bittiming may fail Benedikt Spranger
2013-09-09 9:39 ` Marc Kleine-Budde
2013-09-09 9:39 ` Marc Kleine-Budde
2013-09-09 7:25 ` [PATCH 07/16] c_can: reconfigre message objects after leaving init state Benedikt Spranger
2013-09-09 7:25 ` [PATCH 08/16] c_can: Add FlexCard CAN TX fifo support Benedikt Spranger
2013-09-09 9:47 ` Marc Kleine-Budde
2013-09-09 7:25 ` [PATCH 09/16] c_can: expicit 32bit access on D_CAN to message buffer data register Benedikt Spranger
2013-09-09 11:20 ` Marc Kleine-Budde
2013-09-09 7:25 ` [PATCH 10/16] c_can: add 16bit align 32bit access functions Benedikt Spranger
2013-09-09 9:57 ` Marc Kleine-Budde
2013-09-09 7:25 ` [PATCH 11/16] c_can: stop netqueue if hardware is busy Benedikt Spranger
2013-09-09 10:05 ` Marc Kleine-Budde
2013-09-09 10:21 ` Marc Kleine-Budde
2013-09-09 7:25 ` [PATCH 12/16] c_can: Add flag to disable automatic retransmission of CAN frames Benedikt Spranger
2013-09-09 10:21 ` Marc Kleine-Budde
2013-09-09 10:34 ` Marc Kleine-Budde
2013-09-09 7:25 ` [PATCH 13/16] c_can: flexcard: add ioctl to reset FIFO message object Benedikt Spranger
2013-09-09 10:24 ` Marc Kleine-Budde
2013-09-09 7:25 ` [PATCH 14/16] flexcard: can: CAN local loopback using SKB pflags Benedikt Spranger
2013-09-09 7:25 ` [PATCH 15/16] flexcard: can: Configure CAN loopback packages (TXACK) Benedikt Spranger
2013-09-09 10:29 ` Marc Kleine-Budde
2013-09-09 7:25 ` [PATCH 16/16] c_can: fix TX packet accounting Benedikt Spranger
2013-09-09 10:31 ` Marc Kleine-Budde
2013-09-09 7:54 ` [PATCH 00/16] Support for Eberspächer Flexcard DCAN function Marc Kleine-Budde
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=522D854F.2030207@pengutronix.de \
--to=mkl@pengutronix.de \
--cc=Alexander.Frank@eberspaecher.com \
--cc=b.spranger@linutronix.de \
--cc=bigeasy@linutronix.de \
--cc=dengler@linutronix.de \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.