Re: [PATCH v3] can/sja1000: add support for PEAK-System PCMCIA card

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 14.02.2012 11:16, Stephane Grosjean wrote:

> 
> Le 14/02/2012 10:59, Oliver Hartkopp a écrit :
> 
>> @@ -492,6 +492,9 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>       while ((isrc = priv->read_reg(priv, REG_IR))&&  (n<  SJA1000_MAX_IRQ)) {
>>           n++;
>>           status = priv->read_reg(priv, REG_SR);
>> +        /* check for absent controller due to hw unplug */
>> +        if (status == 0xFF&&  !sja1000_probe_chip(dev))
>> +            break;
>>
>>           if (isrc&  IRQ_WUI)
>>               netdev_warn(dev, "wakeup interrupt\n");
>> @@ -508,6 +511,9 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>               while (status&  SR_RBS) {
>>                   sja1000_rx(dev);
>>                   status = priv->read_reg(priv, REG_SR);
>> +                /* check for absent controller */
>> +                if (status == 0xFF&&  !sja1000_probe_chip(dev))
>> +                    break;
>>               }
>>           }
>>           if (isrc&  (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {
>>
>> The big advantage is, that we do not need another expensive register read nor
>> another loop counter variable in the case everything is working fine.
>>
>> Only when the _already_ read status variable becomes 0xFF we check in the
>> mode register if the device is still present.
>>
>> And finally there's a very little chance left to create rubbish data.
> 
> 
> Quite perfect, except that we will push a "rubbish" error frame before
> exiting, and we will return IRQ_HANDLED while I would prefer IRQ_NONE...
> But does it matter?


An alternative would be to replace the 'break' statements above with

	return IRQ_NONE;

like

		/* check for absent controller due to hw unplug */
		if (status == 0xFF && !sja1000_probe_chip(dev))
			return IRQ_NONE;

Don't know if this is 'good style' - but the while statement allows it ;-)

> 
> Moreover, regarding to this patch, should the peak_pcmcia custom isr always
> test for the card presence? I mean, this costs at least 2xtwo ioread8 for each
> hw irq!


I think the earlier we know the better.

But indeed both drivers (PEAK/EMS PCMCIA) do no need priv->pre_irq ...

So if we add the 'return IRQ_NONE' to the above patch the private check for
present hardware becomes obsolete as the private ISRs terminate properly when
they get an IRQ_NONE.

Right?

Regards,
Oliver