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

[Wolfgang Grandegger <wg@grandegger.com>]

Hi Oliver,

we start paving common code to partially cure pcmcia related problems,
that's exactly what I do not like.

On 02/14/2012 05:41 PM, Oliver Hartkopp wrote:
> 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)) {

In case of unplug, already the above read may return 0xff.

>>>           n++;
>>>           status = priv->read_reg(priv, REG_SR);
>>> +        /* check for absent controller due to hw unplug */
>>> +        if (status == 0xFF&&  !sja1000_probe_chip(dev))
>>> +            break;

Once...

>>>           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;

... twice... What about the other accesses?

>>>               }
>>>           }
>>>           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?

Well, it's far away from being perfect?

> 
> 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?

If it's clear that the interrupt came from that device, we should return
IRQ_HANDLED otherwise it's treated as spurious. I would accept *one* "if
(status == 0xFF ..." check in a good place.

Wolfgang.