Re: Add support for PEAK PCMCIA PCAN-PC card

[Grosjean Stephane <s.grosjean@peak-system.com>]

Hi Wolfgang,


Le 10/01/2012 14:41, Wolfgang Grandegger a écrit :
> Hi Stephane,
>
> please send a patch using a proper subject line and commit message. Also
> add a CC to the pcmcia linux mailing list. Thanks.

Ok I'll do that for v2.

> +
> +/* PEAK-System PCMCIA driver name */
> +#define DRV_NAME "peak_pccard"
> I find the mix of "peak_pcmcia" and "peak_pccard" and "pcan_pccard" for
> the same hardware confusing. Please use just one name and prefix.

Ok, decision was made: peak_pcmcia, once for all!
Closed.

>> +
>> +/* PEAK-System PCMCIA driver version */
>> +#define DRV_VERSION_MAJOR	0
>> +#define DRV_VERSION_MINOR	2
>> +#define DRV_VERSION_SUBMINOR	0
>> +
>> +#define DRV_VERSION_STRING __stringify(DRV_VERSION_MAJOR)"."\
>> +	__stringify(DRV_VERSION_MINOR)"."\
>> +	__stringify(DRV_VERSION_SUBMINOR)
> As for the USB driver, I would remove this redundant versioning stuff.
> Also version 0.2.0 does not really sound like like a stable piece of
> software.

Removed from both drivers.
Closed.

> +
> +#define DRV_CHAN_MAX 2
> +
> +#define DRV_CAN_CLOCK (16000000 / 2)
> Hm, the prefix DRV_ here and below does not really make sense. But well,
> it's a minor issue.

PCC_
Closed.

>> +static int pcan_write_eeprom(struct pcan_pccard *card, u16 addr, u8 v)
>> +{
>> +	u8 status;
>> +	int err, i;
>> +
>> +	/* write instruction */
>> +	pcan_write_reg(card, DRV_SPI_INR, 0x06);
> Macro definitons would be nice.

TODO

>> +	/* wait until write enable */
>> +	for (i = 0; i<  DRV_WRITE_MAX_LOOP; i++) {
>> +		/* RDSR */
> This comment does not tell me anything.

Ok, what about these one?

/* write command reading the status register */
...
/* get the status register and check write enable bit */

then;

/* write command reading the status register */
...
/* get the status register and check write in progress bit */


> +
> +write_eeprom_err:
> +	dev_info(&card->dev->dev,
> +		"writing 0x%x in eeprom @0x%x failed (err %d)\n",
> +		v, addr, err);
> Info or error. Can this happen? If yes, how often?

It's an error and I never saw it. Moreover, writing in eeprom is (only) 
done to power up/down the can connectors (so first when driver is 
loaded, second when it is unloaded).
Moved the dev_info() into dev_err().

>> +	return err;
>> +}
>> +
>> +/*
>> + * control the leds
>> + * @led_mask: should be an or made of DRV_LED(x)
>> + * @state: DRV_LED_ON, DRV_LED_OFF, DRV_LED_SLOW or DRV_LED_FAST
>> + */
> Please remove the docbook tags.

Ok.

>> +/*
>> + * interrupt service routine
>> + */
>> +static irqreturn_t pcan_isr(int irq, void *dev_id)
>> +{
>> +	struct pcan_pccard *card = dev_id;
>> +	struct net_device *netdev;
>> +	irqreturn_t retval = IRQ_NONE;
>> +	int i, again;
>> +
>> +	do {
>> +		again = 0;
>> +
>> +		/* Check interrupt for each channel */
>> +		for (i = 0; i<  card->chan_count; i++) {
>> +			netdev = card->channel[i].netdev;
>> +			if (!netdev)
>> +				continue;
>> +
>> +			if (sja1000_interrupt(irq, netdev) == IRQ_HANDLED)
>> +				again = 1;
>> +		}
>> +
>> +		/* At least one channel handled the interrupt */
>> +		if (again)
>> +			retval = IRQ_HANDLED;
>> +
>> +	} while (again);
> I wonder if it makes sense to limit the loop count.

Had a look to sja1000_interrupt(): you're right, could enter in an 
infinite loop if one of the sja1000 failed into always sending interrupt 
signals...
On my side, I wonder why entering such a do { } while (again) loop?

>> +
>> +	dev_info(&dev->dev, "new PEAK-System pcmcia card detected:\n");
>> +
>> +	dev_info(&dev->dev, "%s %s\n",
>> +		dev->prod_id[0] ? dev->prod_id[0] : "PEAK-System",
>> +		dev->prod_id[1] ? dev->prod_id[1] : "PCAN-PC Card");
> One line (dev_info) should be enough. Also "dev->dev" is not really
> readable. "dev->p[cmcia_]dev" would be much better.

Since this is the 2nd time this request comes, considering its priority 
as increased: thus, changed into a single dev_info();
dev changed into pdev: Ok.


>> +	card = kzalloc(sizeof(struct pcan_pccard), GFP_KERNEL);
>> +	if (!card) {
>> +		dev_err(&dev->dev, "kzalloc() failure\n");
> Please be more specific, e.g.: "couldn't allocated card memory".

As you want, you're (one of) the chief(s) ;-)
Closed.

>> +
>> +	/* sja1000 api uses iomem */
>> +	card->ioport_addr = ioport_map(dev->resource[0]->start,
>> +					resource_size(dev->resource[0]));
>> +	if (!card->ioport_addr) {
>> +		err = -ENOMEM;
>> +		goto probe_err_3;
>> +	}
>> +
>> +	/* display firware version */
>> +	dev_info(&dev->dev, "firmware %d.%d\n",
>> +		pcan_read_reg(card, DRV_FW_MAJOR),
>> +		pcan_read_reg(card, DRV_FW_MINOR));
>> +
>> +	/* detect available channels */
>> +	pcan_add_channels(card);
>> +	if (!card->chan_count)
>> +		goto probe_err_3;
>> +
>> +	/* init the timer which controls the leds */
>> +	init_timer(&card->led_timer);
>> +	card->led_timer.function = pcan_led_timer;
>> +	card->led_timer.data = (unsigned long)card;
>> +
>> +	/* request the given irq */
>> +	err = request_irq(dev->irq,&pcan_isr, IRQF_SHARED, DRV_NAME, card);
>> +	if (err)
>> +		goto probe_err_4;
>> +
>> +	/* power on the connectors */
>> +	pcan_set_can_power(card, 1);
>> +
>> +	return 0;
>> +
>> +probe_err_4:
>> +	/* unregister can devices from network */
>> +	pcan_free_channels(card);
>> +
>> +probe_err_3:
>> +	kfree(card);
>> +	dev->priv = NULL;
>> +
>> +probe_err_2:
>> +	pcmcia_disable_device(dev);
>> +
>> +probe_err_1:
>> +	return err;
> I'm missing ioport_unmap()!

Yes you're perfectly right: in many cases, it will be right called by 
pcan_free_channels() but not before going to "probe_err_3", when no 
channel are detected. So changed code above into:

         /* detect available channels */
         pcan_add_channels(card);
         if (!card->chan_count) {
             ioport_unmap(card->ioport_addr);
             goto probe_err_3;
         }

> Thanks for your contribution.
>
> Wolfgang.

You're welcome, thanks for your time.

Stéphane