Subject: Re: [PATCH net-next v2 2/4] can: cc770: add legacy ISA bus driver for the CC770 and AN82527

From: Wolfgang Grandegger <[email-masked]>
Date: Tue, 10 Jan 2012 17:23:59 +0100

Hi Wolfgang,

On 01/10/2012 05:13 PM, Wolfgang Zarre wrote:
> Hello Wolfgang,
> 
>> On 01/10/2012 01:41 PM, Wolfgang Zarre wrote:
>>> Hello David,
>>>>
>>>>> cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv,
>>>>>                             int reg, u8 val)
>>>>>     {
>>>>>         unsigned long base = (unsigned long)priv->reg_base;
>>>>> +    unsigned long flags;
>>>>>
>>>>> +    spin_lock_irqsave(&outb_lock, flags);
>>>>>         outb(reg, base);
>>>>>         outb(val, base + 1);
>>>>> +    spin_unlock_irqrestore(&outb_lock, flags);
>>>>
>>>> Is there a 'read_reg_indirect' function??
>>>
>>> Yes, there is.
>>>
>>>> If so it also needs to use the same mutex.
>>>
>>> Actually, I don't think that we have a problem with mutex
>>> beside that it's using just one inb() statement but having
>>> for sure with an interrupt between both outb() statements which
>>> seems to be critical for the cc770.
>>
>> But the indirect read function also sets the address register before
>> reading the data using inb(). This sequence should also not be
>> interrupted and therefore we need to synchronize. For the indirect
>> access of the SJA1000 we also need to add spinlocks. Wonder why nobody
>> complained so far.
> 
> So, if I understand correct that means that inb() can be interrupted
> between
> setting the address and reading. If this is the case then yes, we need
> spinlock if this is not the case then IMHO we wouldn't need or am I wrong?

I think we speak about different things. inb() cannot be interrupted but
outb() followed by inb(). For indirect accesses we need something like:

/* Spinlock for cc770_isa_port_write_reg_indirect */
static DEFINE_SPINLOCK(cc770_isa_port_lock);

static u8 cc770_isa_port_read_reg_indirect(const struct cc770_priv *priv,
                                             int reg)
{
        unsigned long base = (unsigned long)priv->reg_base;
	unsigned long flags;
	u8 val;

	spin_lock_irqsave(&cc770_isa_port_lock, flags);
        outb(reg, base);
        val = inb(base + 1);
	spin_unlock_irqrestore(&cc770_isa_port_lock, flags);    

	return val;
}

static void cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv,
                                                int reg, u8 val)
{
        unsigned long base = (unsigned long)priv->reg_base;
	unsigned long flags;

	spin_lock_irqsave(&cc770_isa_port_lock, flags);
        outb(reg, base);
        outb(val, base + 1);
	spin_unlock_irqrestore(&cc770_isa_port_lock, flags);    
}

Hope we are in synch now.

Wolfgang.