Hi Oliver, > this weekend i spent some (more) time to make the CPC-Card PCMCIA driver > running with the latest 3.1rc4 kernel.
Cool :)
Signed-offs are missing for a proper submission, of course.
> diff -u -r -N a/drivers/net/can/sja1000/ems_pcmcia.c
> b/drivers/net/can/sja1000/ems_pcmcia.c
> --- a/drivers/net/can/sja1000/ems_pcmcia.c 1970-01-01 01:00:00.000000000
> +0100
> +++ b/drivers/net/can/sja1000/ems_pcmcia.c 2011-09-05 14:07:39.772637681
> +0200
> @@ -0,0 +1,375 @@
> +/*
> + * Copyright (C) 2008 Sebastian Haas <h...@ems-wuensche.com>
> + * Copyright (C) 2010 Markus Plessing <pless...@ems-wuensche.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the version 2 of the GNU General Public License
> + * as published by the Free Software Foundation
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software Foundation,
> + * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> + */
I'd suggest to drop the address. Only troubles when it changes.
> +struct ems_pcmcia_card {
> + int channels;
> +
> + struct pcmcia_device *pcmcia_dev;
> + struct net_device *net_dev[EMS_PCMCIA_MAX_CHAN];
> +
> + void __iomem *base_addr;
Minor: I'd remove the empty lines here.
> +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;
You ack the IRQ if there is no card?
> +static inline int ems_pcmcia_check_chan(struct sja1000_priv *priv)
> +{
> + unsigned char res;
> +
> + /* Make sure SJA1000 is in reset mode */
> + ems_pcmcia_write_reg(priv, REG_MOD, 1);
> +
> + ems_pcmcia_write_reg(priv, REG_CDR, CDR_PELICAN);
No newline?
> +
> + /* read reset-values */
> + res = ems_pcmcia_read_reg(priv, REG_CDR);
> +
> + if (res == CDR_PELICAN)
> + return 1;
ditto?
> +
> + return 0;
> +}
> +
> +static void ems_pcmcia_del_card(struct pcmcia_device *pdev)
> +{
> + struct ems_pcmcia_card *card = pdev->priv;
> + struct net_device *dev;
> + int i = 0;
> +
> + if (!card)
> + return;
> +
> + free_irq(pdev->irq, card);
> +
> + for (i = 0; i < card->channels; i++) {
> + dev = card->net_dev[i];
> +
> + if (!dev)
> + continue;
Ditto?
> +
> + printk(KERN_INFO "%s: removing %s on channel #%d\n",
> + DRV_NAME, dev->name, i);
dev_info
> + unregister_sja1000dev(dev);
> + free_sja1000dev(dev);
> + }
> +
> + writeb(EMS_CMD_UMAP, card->base_addr);
> +
> + if (card->base_addr)
> + iounmap(card->base_addr);
If is always true
> +
> + 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 == NULL) {
> + printk(KERN_ERR "%s: unable to allocate memory\n", DRV_NAME);
dev_err
> + return -ENOMEM;
> + }
> +
> + pdev->priv = card;
> +
> + card->channels = 0;
> +
> + card->base_addr = ioremap(base, EMS_PCMCIA_MEM_SIZE);
> + if (card->base_addr == NULL) {
> + 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;
> + }
> +
> + ems_pcmcia_card_reset(card);
> +
> + /* 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++) {
> + dev = alloc_sja1000dev(0);
> + if (dev == NULL) {
> + 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
Last two lines can be squashed?
> + + (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_info (and ditto for all other printk)
> + 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);
> + } else {
> + free_sja1000dev(dev);
> + }
> + }
> +
> + err = request_irq(dev->irq, &ems_pcmcia_interrupt, IRQF_SHARED,
> + DRV_NAME, (void *)card);
No need to cast.
...
> + /* Allocate a memory window */
> + dev->resource[2]->flags =
> + WIN_DATA_WIDTH_8|WIN_MEMORY_TYPE_CM|WIN_ENABLE;
Spaces around operator. Didn't checkpatch warn?
> +static int __init ems_pcmcia_init(void)
> +{
> + return pcmcia_register_driver(&ems_pcmcia_driver);
> +}
> +
> +static void __exit ems_pcmcia_exit(void)
> +{
> + pcmcia_unregister_driver(&ems_pcmcia_driver);
> +}
> +
> +module_init(ems_pcmcia_init);
> +module_exit(ems_pcmcia_exit);
Latest trend is to put these directly below the functions.
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
signature.asc
Description: Digital signature
_______________________________________________ Socketcan-core mailing list Socketcan-core@lists.berlios.de https://lists.berlios.de/mailman/listinfo/socketcan-core
