On Fri, Nov 13, 2009 at 4:00 PM, Wolfgang Grandegger <[email protected]>
wrote:
> Barry Song wrote:
>> On Thu, Nov 12, 2009 at 8:36 PM, Wolfgang Grandegger <[email protected]>
>> wrote:
>>> Hello,
>>>
>>> as mentioned, for kernel inclusion you need to prepare patches for David
>>> Millers "net-next-2.6" GIT tree, which you can get as shown below:
>>>
>>> $ git clone \
>>> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next-2.6.git
>>>
>>> The patch should then be sent to the netdev mailing list with CC to the
>>> Socketcan-core and other related mailing lists.
>>>
>>> Furthermore, your patch should be checked with checkpatch.pl to find the
>>> obvious coding style violations. Many lines are too long. More details
>>> about the coding style requirements can be found in the kernel
>>> "Documentation/CodingStyle" file. Other general comment:
>>>
>>> - please use "static inline functions" in favor of macro definitions.
>>> - the "static inline functions" should be short (just a few lines) and
>>> the name should be in lowercase.
>>> - please use #defines or enums for constants.
>>> - please check if structures for the register layout results in better
>>> code, as discussed.
>>> - please remove extensive debugging output mainly useful for
>>> development.
>>> - in the kernel, please use u8 in favor of uint8_t, and the same for the
>>> other types.
>>>
>>> More comments inline...
>>>
>>> Barry Song wrote:
>
> [snip]
>
>>>> +static int bfin_can_set_bittiming(struct net_device *dev)
>>>> +{
>>>> + struct bfin_can_priv *priv = netdev_priv(dev);
>>>> + struct can_bittiming *bt = &priv->can.bittiming;
>>>> + u16 clk, timing;
>>>> +
>>>> + clk = bt->brp - 1;
>>> Do you need an extra variable here?
>>>
>>>> + timing = ((bt->sjw - 1) << 8) | (bt->prop_seg + bt->phase_seg1 - 1) |
>>>> + ((bt->phase_seg2 - 1) << 4);
>>> Consider using #defines for the constants, here and in many other places.
>>>
>>>> + /*
>>>> + * If the SAM bit is set, the input signal is oversampled three
>>>> times at the SCLK rate.
>>>> + */
>>>> + if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
>>>> + timing |= SAM;
>>>> +
>>>> + CAN_WRITE_CTRL(priv, OFFSET_CLOCK, clk);
>>>> + CAN_WRITE_CTRL(priv, OFFSET_TIMING, timing);
>>>> +
>>>> + dev_info(dev->dev.parent,
>>>> + "setting can bitrate:%d brp:%d prop_seg:%d
>>>> phase_seg1:%d phase_seg2:%d\n",
>>>> + bt->bitrate, bt->brp, bt->prop_seg, bt->phase_seg1,
>>>> bt->phase_seg2);
>>> Please remove as "ip -d link show canX" will list the values. Instead add
>> While I use "ip link set can0 type can bitrate 125000". I didn't see
>> these are printed by up level.
>
> But "ip -d link show can0" does list these values, right?
>
> [snip]
>>>> +/* Our watchdog timed out. Called by the up layer */
>>>> +static void bfin_can_timeout(struct net_device *dev)
>>>> +{
>>>> + dev_dbg(dev->dev.parent, "%s enter\n", __func__);
>>>> +
>>>> + /* We can accept TX packets again */
>>>> + dev->trans_start = jiffies;
>>>> + netif_wake_queue(dev);
>>>> +}
>>> Please remove the TX watchdog. We don't need it.
>> I am strange if hardware fails to send packets, how could we resume
>> the blocked TX queue if we don't call netif wakeup in the timeout
>> function?
>
> The hardware will retry until it succeeds or the device is stopped. Non
> of the other CAN drivers uses the TX timeout.
I am not insisting on adding timeout. Maybe I worry too much. I just
have a question about failure/restore mechanism for CAN driver. If the
transmit completion interrupt is lost due to some unknown failure, but
we block the incoming TX stream because the current tx is not
completed. Then is there any mechanism for the driver to reset and
resume to normal again?
I think Ethernet uses the timeout to handler this kind of issue.
>
> [snip]
>>>> +irqreturn_t bfin_can_interrupt(int irq, void *dev_id)
>>>> +{
>>>> + struct net_device *dev = dev_id;
>>>> + struct bfin_can_priv *priv = netdev_priv(dev);
>>>> + struct net_device_stats *stats = &dev->stats;
>>>> + uint16_t status, isrc;
>>>> +
>>>> + dev_dbg(dev->dev.parent, "%s enter, irq num:%d\n", __func__, irq);
>>>> +
>>>> + if ((irq == priv->tx_irq) && CAN_READ_CTRL(priv, OFFSET_MBTIF2)) {
>>>> + /* transmission complete interrupt */
>>>> + CAN_WRITE_CTRL(priv, OFFSET_MBTIF2, 0xFFFF);
>>>> + stats->tx_packets++;
>>>> + can_get_echo_skb(dev, 0);
>>>> + netif_wake_queue(dev);
>>>> + } else if ((irq == priv->rx_irq) && CAN_READ_CTRL(priv,
>>>> OFFSET_MBRIF1)) {
>>> This and the following "else if" cases will be checked, even if "irq ==
>>> priv->tx_irq". A switch statement seems more appropriate to me.
>> Switch will cause syntax errors for non-constant case.
>
> Of course, sorry for the noise. But my remark is still valid.
>
> [snip]
>>>> +static int __devinit bfin_can_probe(struct platform_device *pdev)
>>>> +{
>>>> + int err;
>>>> + struct net_device *dev;
>>>> + struct bfin_can_priv *priv;
>>>> + struct resource *res_mem, *rx_irq, *tx_irq, *err_irq;
>>>> + unsigned short *pdata;
>>>> +
>>>> + pdata = pdev->dev.platform_data;
>>>> + if (!pdata) {
>>>> + dev_err(&pdev->dev, "No platform data provided!\n");
>>>> + err = -ENODEV;
>>>> + goto exit;
>>> "return -ENODEV;" would be fine here?
>>>
>>>> + }
>>>> +
>>>> + res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> + rx_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>>> + tx_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 1);
>>>> + err_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 2);
>>>> + if (!res_mem || !rx_irq || !tx_irq || !err_irq) {
>>>> + err = -ENODEV;
>>>> + goto exit;
>>> Ditto.
>>>
>>>> + }
>>>> +
>>>> + if (!request_mem_region(res_mem->start, resource_size(res_mem),
>>>> + dev_name(&pdev->dev))) {
>>>> + err = -EBUSY;
>>>> + goto exit;
>>> Ditto.
>>>
>>>> + }
>>>> +
>>>> + /* request peripheral pins */
>>>> + err = peripheral_request_list(pdata, dev_name(&pdev->dev));
>>>> + if (err)
>>>> + goto exit_mem_release;
>>>> +
>>>> + dev = alloc_bfin_candev();
>>>> + if (!dev) {
>>>> + err = -ENOMEM;
>>>> + goto exit_peri_pin_free;
>>>> + }
>>>> +
>>>> + /* register interrupt handler */
>>>> + err = request_irq(rx_irq->start, &bfin_can_interrupt, 0,
>>>> + "bfin-can-rx", (void *)dev);
>>>> + if (err)
>>>> + goto exit_candev_free;
>>>> + err = request_irq(tx_irq->start, &bfin_can_interrupt, 0,
>>>> + "bfin-can-tx", (void *)dev);
>>>> + if (err)
>>>> + goto exit_rx_irq_free;
>>>> + err = request_irq(err_irq->start, &bfin_can_interrupt, 0,
>>>> + "bfin-can-err", (void *)dev);
>>>> + if (err)
>>>> + goto exit_tx_irq_free;
>>> Usually the interrupts are requested in the open function. Any reason
>>> why it's done that early?
>> Lazy-request is for sharing those resources with other devices. For
>> this case, irq number is only for CAN. request/release is maybe
>> overhead.
>
> If possible, it should be done in the open function.
>
> Wolfgang.
>
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
