From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wolfgang Grandegger Subject: Re: [PATCH net-next v2 2/4] can: cc770: add legacy ISA bus driver for the CC770 and AN82527 Date: Tue, 10 Jan 2012 17:23:59 +0100 Message-ID: <4F0C661F.6030508@grandegger.com> References: <4F0C31F1.20908@essax.com> <4F0C4EA8.7060002@grandegger.com> <4F0C63C6.5080305@essax.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: Received: from ngcobalt02.manitu.net ([217.11.48.102]:55016 "EHLO ngcobalt02.manitu.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756246Ab2AJQYS (ORCPT ); Tue, 10 Jan 2012 11:24:18 -0500 In-Reply-To: <4F0C63C6.5080305@essax.com> Sender: linux-can-owner@vger.kernel.org List-ID: To: info@essax.com Cc: David Laight , Oliver Hartkopp , henrik@proconx.com, netdev@vger.kernel.org, linux-can@vger.kernel.org, socketcan-users@lists.berlios.de, IreneV , Stanislav Yelenskiy , oe@port.de, henrik@focus-sw.com 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.