Hi Oliver,
looks quite good for me - more comments inline.
Am 14.09.2011 15:01, schrieb Oliver Hartkopp:
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.
This is right, the CPC-Card may be removed unsave and on heavy load
there may be unhandled interrupts. The IRQ_HANDLED is not mandatory
here, I think this refers to our experience in embedded development
where the interrupt flag has to be cleared (handled) even on error.
It's up to you what you think what's the best return value here.
+
+ 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...
Yes, card->channels should be used here (still the define in v3)...
+ 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.
I am also unsure. If it is irq-save, the if(!dev) can be dropped.
Therefore i would keep this untouched - especially as EMS_PCMCIA_MAX_CHAN is 2
and not 20 ...
To be more precise. The CPC-Card has never been sold with only one
channel. But there is a theoretical possibility for a faulty transeiver
(This seems the case for one internal card here) ... So in almost all
cases there will be 2 channels.
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.
As pdev->priv is set in ems_pcmcia_add_card it should not be NULL here
and ems_pcmcia_del_card is not be called if ems_pcmcia_add_card returns
with -ENOMEM. So, if my assumptions are right here, this will not happen.
+
+ 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
Yes, the driver name as output creator is much clearer than the name of
the net_device, I think.
+ 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.
No, if I understood this correct pdev->dev is initialized later and set
to the net_device allocated with alloc_sja1000dev(0) ...
+ 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.
Why calculating the size with sizeof when we can use the constant which
has been used to define the array size? Well, if the check
"__must_be_array", which is done by ARRAY_SIZE, is mandatory, ok.
+ 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);
pdev->dev is initialized here, isn't it?
+ 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.
From my point of view, this decision is up to the real kernel driver
developers out there ;-)
Thanks for the review!
Cheers, Oliver
Thanks for reworking :)
Best regards,
Markus
_______________________________________________
Socketcan-core mailing list
Socketcan-core@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/socketcan-core
