On 09/13/11 09:56, Marc Kleine-Budde wrote:
> On 09/12/2011 08:44 PM, Oliver Hartkopp wrote:
>> +static irqreturn_t ems_pcmcia_interrupt(int irq, void *dev_id)
>> +{
>> + struct ems_pcmcia_card *card = dev_id;
>> + struct net_device *dev;
>> + irqreturn_t retval = IRQ_NONE;
>> + int i, again;
>> +
>> + /* Card not present */
>> + if (readw(card->base_addr) != 0xAA55)
>> + return IRQ_HANDLED;
>
> this looks fishy. Is it possible, that there is no card here? Why return
> IRQ_HANDLED in this case?
>
This has been introduced to handle PCMCIA card ejects under heavy load.
Probably IRQ_NONE could be returned here also - but if we come to this point
everything is removed anyway.
>> +
>> + do {
>> + again = 0;
>> +
>> + /* Check interrupt for each channel */
>> + for (i = 0; i < EMS_PCMCIA_MAX_CHAN; i++) {
>
> If I understand the code correct, you can use card->channels...
>
>> + dev = card->net_dev[i];
>> + if (!dev)
>> + continue;
>
> ...and drop the if (!dev) here.
I don't know if all the initialization stuff in ems_pcmcia_add_card() and the
interrupt handler is completely irq-save.
Therefore i would keep this untouched - especially as EMS_PCMCIA_MAX_CHAN is 2
and not 20 ...
Maybe Markus or Sebastian would like to change it.
>
>> +
>> + if (sja1000_interrupt(irq, dev) == IRQ_HANDLED)
>> + again = 1;
>> + }
>> + /* At least one channel handled the interrupt */
>> + if (again)
>> + retval = IRQ_HANDLED;
>> +
>> + } while (again);
>> +
>> + return retval;
>> +}
>> +
>> +/*
>> + * Check if a CAN controller is present at the specified location
>> + * by trying to set 'em into the PeliCAN mode
>> + */
>> +static inline int ems_pcmcia_check_chan(struct sja1000_priv *priv)
>> +{
>> + /* Make sure SJA1000 is in reset mode */
>> + ems_pcmcia_write_reg(priv, REG_MOD, 1);
>> + ems_pcmcia_write_reg(priv, REG_CDR, CDR_PELICAN);
>> +
>> + /* read reset-values */
>> + if (ems_pcmcia_read_reg(priv, REG_CDR) == CDR_PELICAN)
>> + return 1;
>> +
>> + return 0;
>> +}
>> +
>> +static void ems_pcmcia_del_card(struct pcmcia_device *pdev)
>> +{car
>> + struct ems_pcmcia_card *card = pdev->priv;
>> + struct net_device *dev;
>> + int i;
>> +
>> + if (!card)
>> + return;
>
> can this happen?
Don't know.
>
>> +
>> + free_irq(pdev->irq, card);
>> +
>> + for (i = 0; i < card->channels; i++) {
>> + dev = card->net_dev[i];
>> + if (!dev)
>> + continue;
>
> can this happen?
See above comment about the card setup.
>
>> +
>> + printk(KERN_INFO "%s: removing %s on channel #%d\n",
>> + DRV_NAME, dev->name, i);
>
> here you have a pcmcia_device, so you can use:
> dev_info(&pdev->dev, "");
This looks a bit broken then:
can0: removing can0 on channel #1
Now it is
ems_pcmcia: removing can0 on channel #1
>
>> + unregister_sja1000dev(dev);
>> + free_sja1000dev(dev);
>> + }
>> +
>> + writeb(EMS_CMD_UMAP, card->base_addr);
>> + iounmap(card->base_addr);
>> + kfree(card);
>> +
>> + pdev->priv = NULL;
>> +}
>> +
>> +/*
>> + * Probe PCI device for EMS CAN signature and register each available
>> + * CAN channel to SJA1000 Socket-CAN subsystem.
>> + */
>> +static int __devinit ems_pcmcia_add_card(struct pcmcia_device *pdev,
>> + unsigned long base)
>> +{
>> + struct sja1000_priv *priv;
>> + struct net_device *dev;
>> + struct ems_pcmcia_card *card;
>> + int err, i;
>> +
>> + /* Allocating card structures to hold addresses, ... */
>> + card = kzalloc(sizeof(struct ems_pcmcia_card), GFP_KERNEL);
>> + if (!card) {
>> + printk(KERN_ERR "%s: unable to allocate memory\n", DRV_NAME);
>
> dev_err()
Is there a &pdev->dev ?? IMHO no.
>
>> + return -ENOMEM;
>> + }
>> +
>> + pdev->priv = card;
>> + card->channels = 0;
>> +
>> + card->base_addr = ioremap(base, EMS_PCMCIA_MEM_SIZE);
>> + if (!card->base_addr) {
>> + err = -ENOMEM;
>> + goto failure_cleanup;
>> + }
>> +
>> + /* Check for unique EMS CAN signature */
>> + if (readw(card->base_addr) != 0xAA55) {
>> + printk(KERN_ERR "%s: No EMS CPC Card hardware found.\n",
>> + DRV_NAME);
>
> dev_err();
>
>> + err = -ENODEV;
>> + goto failure_cleanup;
>> + }
>> +
>> + /* Request board reset */
>> + writeb(EMS_CMD_RESET, card->base_addr);
>> +
>> + /* Make sure CAN controllers are mapped into card's memory space */
>> + writeb(EMS_CMD_MAP, card->base_addr);
>> +
>> + /* Detect available channels */
>> + for (i = 0; i < EMS_PCMCIA_MAX_CHAN; i++) {
>
> i < ARRAY_SIZE(card->net_dev)
Does not look more readable to me.
>
>> + dev = alloc_sja1000dev(0);
>> + if (!dev) {
>> + err = -ENOMEM;
>> + goto failure_cleanup;
>> + }
>> +
>> + card->net_dev[i] = dev;
>> + priv = netdev_priv(dev);
>> + priv->priv = card;
>> + SET_NETDEV_DEV(dev, &pdev->dev);
>> +
>> + priv->irq_flags = IRQF_SHARED;
>> + dev->irq = pdev->irq;
>> + priv->reg_base = card->base_addr + EMS_PCMCIA_CAN_BASE_OFFSET +
>> + (i * EMS_PCMCIA_CAN_CTRL_SIZE);
>> +
>> + /* Check if channel is present */
>> + if (ems_pcmcia_check_chan(priv)) {
>> + priv->read_reg = ems_pcmcia_read_reg;
>> + priv->write_reg = ems_pcmcia_write_reg;
>> + priv->can.clock.freq = EMS_PCMCIA_CAN_CLOCK;
>> + priv->ocr = EMS_PCMCIA_OCR;
>> + priv->cdr = EMS_PCMCIA_CDR;
>> + priv->flags |= SJA1000_CUSTOM_IRQ_HANDLER;
>> +
>> + /* Register SJA1000 device */
>> + err = register_sja1000dev(dev);
>> + if (err) {
>> + printk(KERN_INFO "%s: registering device "
>> + "failed (err=%d)\n", DRV_NAME, err);
>
> dev_err (not info)
Yes. Will change in final post.
>
>> + free_sja1000dev(dev);
>> + goto failure_cleanup;
>> + }
>> +
>> + card->channels++;
>> +
>> + printk(KERN_INFO "%s: registered %s on channel "
>> + "#%d at 0x%p, irq %d\n", DRV_NAME, dev->name,
>> + i, priv->reg_base, dev->irq);
>
> dev_info
>
>> + } else
>> + free_sja1000dev(dev);
>> + }
>> +
>> + err = request_irq(dev->irq, &ems_pcmcia_interrupt, IRQF_SHARED,
>> + DRV_NAME, card);
>> + if (err) {
>> + printk(KERN_INFO "Registering device failed (err=%d)\n", err);
>
> dev_err (not info)
Will change in next post.
>
>> + goto failure_cleanup;
>> + }
>> +
>> + return 0;
>> +
>> +failure_cleanup:
>> + printk(KERN_ERR "Error: %d. Cleaning Up.\n", err);
>
> dev_err()
>
>> + ems_pcmcia_del_card(pdev);
>> +
>> + return err;
>> +}
>> +
>> +/*
>> + * Setup PCMCIA socket and probe for EMS CPC-CARD
>> + */
>> +static int __devinit ems_pcmcia_probe(struct pcmcia_device *dev)
>> +{
>> + int csval;
>> +
>> + /* General socket configuration */
>> + dev->config_flags |= CONF_ENABLE_IRQ;
>> + dev->config_index = 1;
>> + dev->config_regs = PRESENT_OPTION;
>> +
>> + /* The io structure describes IO port mapping */
>> + dev->resource[0]->end = 16;
>> + dev->resource[0]->flags |= IO_DATA_PATH_WIDTH_8;
>> + dev->resource[1]->end = 16;
>> + dev->resource[1]->flags |= IO_DATA_PATH_WIDTH_16;
>> + dev->io_lines = 5;
>> +
>> + /* Allocate a memory window */
>> + dev->resource[2]->flags =
>> + (WIN_DATA_WIDTH_8 | WIN_MEMORY_TYPE_CM | WIN_ENABLE);
>> + dev->resource[2]->start = dev->resource[2]->end = 0;
>> +
>> + csval = pcmcia_request_window(dev, dev->resource[2], 0);
>> + if (csval) {
>> + dev_err(&dev->dev, "pcmcia_request_window failed (err=%d)\n",
>> + csval);
>> + return 0;
>> + }
>> +
>> + csval = pcmcia_map_mem_page(dev, dev->resource[2], dev->config_base);
>> + if (csval) {
>> + dev_err(&dev->dev, "pcmcia_map_mem_page failed (err=%d)\n",
>> + csval);
>> + return 0;
>> + }
>> +
>> + csval = pcmcia_enable_device(dev);
>> + if (csval) {
>> + dev_err(&dev->dev, "pcmcia_enable_device failed (err=%d)\n",
>> + csval);
>> + return 0;
>> + }
>> +
>> + ems_pcmcia_add_card(dev, dev->resource[2]->start);
>> + return 0;
>> +}
>> +
>> +/*
>> + * Release claimed resources
>> + */
>> +static void ems_pcmcia_remove(struct pcmcia_device *dev)
>> +{
>> + ems_pcmcia_del_card(dev);
>> + pcmcia_disable_device(dev);
>> +}
>> +
>> +static struct pcmcia_driver ems_pcmcia_driver = {
>> + .name = DRV_NAME,
>> + .probe = ems_pcmcia_probe,
>> + .remove = ems_pcmcia_remove,
>> + .id_table = ems_pcmcia_tbl,
>> +};
>> +
>> +static int __init ems_pcmcia_init(void)
>> +{
>> + return pcmcia_register_driver(&ems_pcmcia_driver);
>> +}
>> +module_init(ems_pcmcia_init);
>> +
>> +static void __exit ems_pcmcia_exit(void)
>> +{
>> + pcmcia_unregister_driver(&ems_pcmcia_driver);
>> +}
>> +module_exit(ems_pcmcia_exit);
>>
>
> cheers, Marc
Finally i'm still unsure about the dev_??? stuff. Maybe it's best, we let the
Markus/Sebastian look over the code fist.
Thanks for the review!
Cheers, Oliver
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
