Re: [MeeGo-Dev][PATCH v2] Topcliff: Update PCH_CAN driver to 2.6.35

[Daniel Baluta]

On Mon, Sep 13, 2010 at 3:22 PM, Masayuki Ohtak
<masa-korg@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> +#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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/