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.
[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
