Hi Andre, thanks for this version. Code is much more readable IMHO. Just a few minor things:
> +/* Probe for a TSCAN1 device with JP1:JP2 jumper setting ID */
> +static int __devinit tscan1_match(struct device *dev, unsigned id)
> +{
> + int pld_base, sja1000_base, irq, i;
> + struct net_device *netdev;
> + struct sja1000_priv *priv;
> +
> + /* Request PLD I/O register region */
I'd think this comment explains the obvious, but maybe that's just me :)
> + pld_base = TSCAN1_PLD_ADDRESS + id * TSCAN1_PLD_SIZE;
> + if (!request_region(pld_base, TSCAN1_PLD_SIZE, "tscan1"))
> + return -EBUSY;
> +
> + /* Check magic values in PLD board identifier registers */
ditto
> + if (inb(pld_base + TSCAN1_ID1) != TSCAN1_ID1_VALUE ||
> + inb(pld_base + TSCAN1_ID2) != TSCAN1_ID2_VALUE)
> + goto out_release;
> +
> + /* Read JP4:JP5 jumper settings to find out the selected IRQ */
ditto
> + switch (inb(pld_base + TSCAN1_JUMPERS) & (TSCAN1_JP4 | TSCAN1_JP5)) {
> + case TSCAN1_JP4:
> + irq = 6;
> + break;
> + case TSCAN1_JP5:
> + irq = 7;
> + break;
> + case TSCAN1_JP4 | TSCAN1_JP5:
> + irq = 5;
> + break;
> + default:
> + goto out_release;
Maybe an errror message here? Do you really want to quit with -ENODEV?
> + }
> +
> + netdev = alloc_sja1000dev(0);
> + if (!netdev)
> + goto out_release;
That should really be -ENOMEM.
> +
> + dev_set_drvdata(dev, netdev);
> + SET_NETDEV_DEV(netdev, dev);
> +
> + netdev->base_addr = pld_base;
> + netdev->irq = irq;
> +
> + priv = netdev_priv(netdev);
> + priv->read_reg = tscan1_read;
> + priv->write_reg = tscan1_write;
> + priv->can.clock.freq = TSCAN1_SJA1000_XTAL / 2;
> + priv->cdr = CDR_CBP | CDR_CLK_OFF;
> + priv->ocr = OCR_TX0_PUSHPULL;
> +
> + /* Select the first SJA1000 I/O address that is free and that works */
> + for (i = 0; i < ARRAY_SIZE(tscan1_sja1000_addresses); i++) {
> + sja1000_base = tscan1_sja1000_addresses[i];
> + if (!request_region(sja1000_base, TSCAN1_SJA1000_SIZE,
> + "tscan1"))
> + continue;
> +
> + /* Set the SJA1000 I/O base address and enable it */
> + outb(TSCAN1_MODE_ENABLE | i, pld_base + TSCAN1_MODE);
> +
> + /* Probe and register SJA1000 */
> + priv->reg_base = (void __iomem *)sja1000_base;
> + if (!register_sja1000dev(netdev)) {
Maybe here a comment like: /* Success. Turn off the LED and return */ to
make it really obvious that this is the success-path?
> + outb(0, pld_base + TSCAN1_LED); /* turn the LED off */
> + netdev_info(netdev, "TS-CAN1 at 0x%x 0x%x irq %d\n",
> + pld_base, sja1000_base, irq);
> + return 0;
> + }
> +
> + /* The probe for an SJA1000 at this address failed: release */
Maybe incorporate some of the explanation you gave in your mail? Like:
"... might have failed because of BIOS-settings. Release and try next
region" ?
> + outb(0, pld_base + TSCAN1_MODE);
> + release_region(sja1000_base, TSCAN1_SJA1000_SIZE);
> + }
> +
> + dev_set_drvdata(dev, NULL);
> + free_sja1000dev(netdev);
> +out_release:
> + release_region(pld_base, TSCAN1_PLD_SIZE);
> + return -ENODEV;
> +}
> +
> +static int __devexit tscan1_remove(struct device *dev, unsigned id
> /*unused*/)
> +{
> + struct net_device *netdev;
> + struct sja1000_priv *priv;
> + int pld_base, sja1000_base;
> +
> + netdev = dev_get_drvdata(dev);
> + unregister_sja1000dev(netdev);
> + dev_set_drvdata(dev, NULL);
> +
> + priv = netdev_priv(netdev);
> + pld_base = netdev->base_addr;
> + sja1000_base = (int)priv->reg_base;
> +
> + /* Release SJA1000 I/O space */
obvious comment?
> + outb(0, pld_base + TSCAN1_MODE);
> + release_region(sja1000_base, TSCAN1_SJA1000_SIZE);
> +
> + /* Release PLD I/O space */
ditto
> + release_region(pld_base, TSCAN1_PLD_SIZE);
> +
> + /* Free net device */
ditto
> + free_sja1000dev(netdev);
> +
> + return 0;
> +}
> +
> +static struct isa_driver tscan1_isa_driver = {
> + .match = tscan1_match,
> + .remove = __devexit_p(tscan1_remove),
> + .driver = {
> + .name = "tscan1",
> + },
> +};
> +
> +static int __init tscan1_init(void)
> +{
> + return isa_register_driver(&tscan1_isa_driver, TSCAN1_MAXDEV);
> +}
> +module_init(tscan1_init);
> +
> +static void __exit tscan1_exit(void)
> +{
> + isa_unregister_driver(&tscan1_isa_driver);
> +}
> +module_exit(tscan1_exit);
> --
> 1.6.5
>
> --
> Andre B. Oliveira
> http://clientes.netvisao.pt/anbadeol/
> _______________________________________________
> Socketcan-core mailing list
> [email protected]
> https://lists.berlios.de/mailman/listinfo/socketcan-core
I also think you can send the next version to netdev.
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
signature.asc
Description: Digital signature
_______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
