netdev - Re: [MeeGo-Dev][PATCH v2] Topcliff: Update PCH

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AANLkTikLLmH-L_KZ8YOBgw6gxCpxPqSxr43pZF1Y8_-x@mail.gmail.com>
Date:	Mon, 13 Sep 2010 21:47:59 +0300
From:	Daniel Baluta <daniel.baluta@...il.com>
To:	Masayuki Ohtak <masa-korg@....okisemi.com>
Cc:	Wolfgang Grandegger <wg@...ndegger.com>,
	"David S. Miller" <davem@...emloft.net>,
	Wolfram Sang <w.sang@...gutronix.de>,
	Christian Pellegrin <chripell@...e.org>,
	Barry Song <21cnbao@...il.com>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	socketcan-core@...ts.berlios.de, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, meego-dev@...go.com,
	"Andrew\"" <andrew.chih.howe.khor@...el.com>,
	"Wang Qi\"" <qi.wang@...el.com>, gregkh@...e.de,
	"Wang Yong Y\"" <yong.y.wang@...el.com>,
	Tomoya MORINAGA <morinaga526@....okisemi.com>,
	arjan@...ux.intel.com
Subject: Re: [MeeGo-Dev][PATCH v2] Topcliff: Update PCH_CAN driver to 2.6.35

On Mon, Sep 13, 2010 at 3:22 PM, Masayuki Ohtak
<masa-korg@....okisemi.com> wrote:
> CAN driver of Topcliff PCH
>
> Topcliff PCH is the platform controller hub that is going to be used in
> Intel's upcoming general embedded platform. All IO peripherals in
> Topcliff PCH are actually devices sitting on AMBA bus.
> Topcliff PCH has CAN I/F. This driver enables CAN function.
>
> Signed-off-by: Masayuki Ohtake <masa-korg@....okisemi.com>
> +#define MAX_CAN_DEVICES              1
Where is this used?

> +#define MAX_BITRATE            0x3e8
> +#define NUM_NODES              2000    /* Maximum number of
> +                                                Software FIFO nodes. */
I think you don't need NUM_NODES.
> +/**
> + * struct pch_can_msg - CAN message structure
> + * @ide:       Standard/extended msg
> + * @id:                11 or 29 bit msg id
> + * @dlc:       Size of data
> + * @data:      Message pay load
> + * @rtr:       RTR message
> + */
> +struct pch_can_msg {
> +       unsigned short ide;
> +       unsigned int id;
> +       unsigned short dlc;
> +       unsigned char data[PCH_CAN_MSG_DATA_LEN];
> +       unsigned short rtr;
> +};

I think you should use can_frame, and try to get rid of pch_can hardware
abstraction.


> +struct can_hw {
> +       void __iomem *io_base;
> +};

Why do you need can_hw?

> +static int pch_can_get_rx_enable(struct can_hw *can, u32 buff_num, u32 *enable,
> +                                struct net_device *ndev)
Here ndev is used only for debugging purposes. It doesn't seem a natural
approach.

> +static irqreturn_t pch_can_handler(int irq, void *dev_id)
> +{
> +       irqreturn_t retval = IRQ_NONE;
> +       u32 int_stat;
> +       struct pch_can_os *can_os;
> +       struct net_device *ndev = (struct net_device *)dev_id;
> +       struct pch_can_priv *priv = netdev_priv(ndev);
> +       dev_dbg(&can_os->ndev->dev, "%s -> Invoked.\n", __func__);
> +
> +       can_os = &priv->pch_can_os_p;
> +       int_stat = pch_can_int_pending(can_os->can);

Where are the interrupt status bit(s) cleared?

> +       dev_dbg(&can_os->ndev->dev,
> +         "%s -> pch_can_int_pending returned value: %x\n", __func__, int_stat);
> +

As someone stated before I think you are trying to port an
old fashion CAN character device based driver.

Although, having the old source code, should be helpful
I think you should read the existing CAN drivers (drivers/net/can)
and start from scratch trying to follow the ideas implemented there.

I also have a character driver very similar with yours
(ICP CAN for EP80579) and I struggle for some time
to get a working version with Socket CAN interface.

I don't have any 'official' position towards Linux Socket CAN
so please consider my comments as being advisory.

thanks,
Daniel.
--
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