From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753585AbbK3NTX (ORCPT ); Mon, 30 Nov 2015 08:19:23 -0500 Received: from mout.kundenserver.de ([212.227.126.133]:49722 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752088AbbK3NTW (ORCPT ); Mon, 30 Nov 2015 08:19:22 -0500 From: Arnd Bergmann To: Rongrong Zou Cc: gregkh@linuxfoundation.org, lixiancai@huawei.com, linux-kernel@vger.kernel.org, zourongrong@huawei.com, lijianhua@huawei.com, linuxarm@huawei.com, minyard@acm.org, lijianhua , Will Deacon , Catalin Marinas Subject: Re: [PATCH] Hisilicon LPC driver Date: Mon, 30 Nov 2015 14:19:06 +0100 Message-ID: <2285486.C9K5rVAjVJ@wuerfel> User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; ) In-Reply-To: <1448888837-6148-1-git-send-email-zourongrong@gmail.com> References: <1448888837-6148-1-git-send-email-zourongrong@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:ncaYYf/wFDH8UVxr+QkgF/8TdYnApTEhEu0fYsEtR0ebWOJI5yS 3jnWbvUJHPHPwhPrfK17wpfIcXX0wEh7Ehc96FzP5iQ9XhuibH/iAbfgJ/l/k86ee8bQjzf DFNcpvAqzcdGOLMFRaJ7lcV++QhcsqLqQ/qp5fsLCe0z+ee+xK6s/qtYInwOgFRsvehQXNz l4ghTYioSlJWPLnF1CKbQ== X-UI-Out-Filterresults: notjunk:1;V01:K0:2eTFq0ZMvXY=:rn87JOb97EKR/wspPGQJ+T ol6PgChKSVgwKDZO7NgxVnNQf4ciV9c+HBnsmgeLknjLP9bC/bHtvl0t20DhDCh8witewmadH QKC1u4YlAjeFQYfKAf3hndV3070Oh69aAo0w1o0DTrXAsu1uW5ZE2dmqhKnPr+k2VOmBoh/ew MGO4JwvFI9PEN53HB4reE1Zy73YgAUSoDtnOf8Fax28yf28BR+4K2drv/i5SmhxiTcbOnev6c LarzCnFHlQHNTC4zC+9KHrsRsDGzczIR8YUamzEeOHJbWKk+iq1iaia0pI3YzEkOoOhQmAAXQ bdTchu65VODZH82OtS4PvAqMPaW4gcbnpJa+eku3gnastti56fBT4esPFySdaNcQDl/B9VsEG H8s5EW1uogASJVbsjVNfvQvUsrBvf8282LUfsShgiwBdcx/x2mC0slZh3rjGrznQhjxOAvYFM 54YIfDxbV9zJTe1r0144KciNoJZvcOqHKRww4K/suEyBiMnGVA4jmF570K5eRYIClLlvGZwKB eFVG08mEGm9OHqeTlChaBssNfIEK3JNUHy7LT1M2Lm52ziXetjZwkh30BVp4KW0xBhPIhZJR0 bFyNl04jE+YlpdwcV1jjLVB+POMOH64Av+O1TZlTLIINJikli5ErUOMztU7GZe2h5Ey+pEGDN 4O6ZHF4kMsNHq5iCiW557SjhiSlarcqUG85+nryLL+4eQD8FRn9HuIFApdRZgB0TMGh1AwNIH cMXPyVD/0BGwFcMR Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Monday 30 November 2015 21:07:17 Rongrong Zou wrote: > This is the Low Pin Count driver for Hisilicon Hi1610 SoC. It is used > for LPC master accessing LPC slave device. > > We only implement I/O read and I/O write here, and the 2 interfaces are > exported for uart driver and ipmi_si driver. > > Signed-off-by: Rongrong Zou > Signed-off-by: lijianhua > --- > .../bindings/misc/hisilicon,low-pin-count.txt | 11 + > MAINTAINERS | 5 + > drivers/misc/Kconfig | 7 + > drivers/misc/Makefile | 1 + > drivers/misc/hisi_lpc.c | 292 +++++++++++++++++++++ > include/linux/hisi_lpc.h | 83 ++++++ > 6 files changed, 399 insertions(+) This should not be a misc driver. > create mode 100644 Documentation/devicetree/bindings/misc/hisilicon,low-pin-count.txt > create mode 100644 drivers/misc/hisi_lpc.c > create mode 100644 include/linux/hisi_lpc.h > diff --git a/Documentation/devicetree/bindings/misc/hisilicon,low-pin-count.txt b/Documentation/devicetree/bindings/misc/hisilicon,low-pin-count.txt > new file mode 100644 > index 0000000..05c1e19 > --- /dev/null > +++ b/Documentation/devicetree/bindings/misc/hisilicon,low-pin-count.txt > @@ -0,0 +1,11 @@ > +Hisilicon Low Pin Count bus > + > +Required properties > +- compatible: "hisilicon,low-pin-count" > +- reg specifies low pin count address range > + > +Example: > + lpc_0: lpc@a01b0000 { > + compatible = "hisilicon,low-pin-count"; > + ret = <0x0 0xa01b0000, 0x0, 0x10000>; > + }; The name is too generic, unless you can guarantee that Hisilicon has never before made another implementation of an LPC interface, and never will again. I think you should create a child address space here using a '#address-cells' and '#size-cells'. > +#define LPC_REG_READ(reg, result) ((result) = readl(reg)) > + > +#define LPC_REG_WRITE(reg, data) writel((data), (reg)) Remove the obfuscation here. > +struct hs_lpc_dev *lpc_dev; Avoid global data structures. > + LPC_REG_WRITE(lpc_dev->regs + HS_LPC_REG_IRQ_ST, HS_LPC_IRQ_CLEAR); > + retry = 0; > + while (0 == (LPC_REG_READ(lpc_dev->regs + HS_LPC_REG_OP_STATUS, > + lpc_op_state_value) & HS_LPC_STATUS_DILE)) { > + udelay(1); > + retry++; > + if (retry >= 10000) { > + dev_err(lpc_dev->dev, "lpc W, wait idle time out\n"); > + return -ETIME; > + } > + } Better release the spinlock here and call a sleeping function for the wait. If the timeout is 10ms, you definitely don't want to keep interrupts disabled the whole time. If you can't find a good way to retry after getting the lock back, maybe use a mutex here that you can keep locked the whole time. > +void lpc_io_write_byte(u8 value, unsigned long addr) > +{ > + unsigned long flags; > + int ret; > + > + if (!lpc_dev) { > + pr_err("device is not register\n!"); > + return; > + } > + spin_lock_irqsave(&lpc_dev->lock, flags); > + ret = lpc_master_write(HS_LPC_CMD_SAMEADDR_SING, HS_LPC_CMD_TYPE_IO, > + addr, &value, 1); > + spin_unlock_irqrestore(&lpc_dev->lock, flags); > +} > +EXPORT_SYMBOL(lpc_io_write_byte); Using your own accessor functions sounds wrong here. What you have is essentially a PCI I/O space, right? As much as we all hate I/O space (in particular the kind that is not memory mapped), I think this should be hooked up to the generic inb/outb functions to allow all the generic device drivers to work. > diff --git a/include/linux/hisi_lpc.h b/include/linux/hisi_lpc.h > new file mode 100644 > index 0000000..4cf93ee > --- /dev/null > +++ b/include/linux/hisi_lpc.h Don't do a global header here, just move it into the main file. Arnd