From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755331AbbLAH64 (ORCPT ); Tue, 1 Dec 2015 02:58:56 -0500 Received: from szxga02-in.huawei.com ([119.145.14.65]:18064 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752483AbbLAH6z (ORCPT ); Tue, 1 Dec 2015 02:58:55 -0500 Subject: Re: [PATCH] Hisilicon LPC driver To: Arnd Bergmann , Rongrong Zou References: <1448888837-6148-1-git-send-email-zourongrong@gmail.com> <2285486.C9K5rVAjVJ@wuerfel> CC: , , , , , , lijianhua , Will Deacon , Catalin Marinas From: Rongrong Zou Message-ID: <565D532C.9040501@huawei.com> Date: Tue, 1 Dec 2015 15:58:36 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <2285486.C9K5rVAjVJ@wuerfel> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.177.30.66] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020206.565D5338.002D,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: cc49cfc593bf6f334bceca87847561bb Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 在 2015/11/30 21:19, Arnd Bergmann 写道: > 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. I an not sure which subsystem to place, do you have any sugguestion? > >> 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. OK, I will fix it. > > I think you should create a child address space here using a > '#address-cells' and '#size-cells'. There are some mistake,it should be wrote like: reg = <0x0 0xa01b0000 0x0 0x10000>; > >> +#define LPC_REG_READ(reg, result) ((result) = readl(reg)) >> + >> +#define LPC_REG_WRITE(reg, data) writel((data), (reg)) > > Remove the obfuscation here. OK > >> +struct hs_lpc_dev *lpc_dev; > > Avoid global data structures. > OK >> + 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. > The interface "lpc_io_read_byte" may be called in IRQ context by UART driver, and in process context by ipmi driver. >> +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. It is not a PCI I/O space, although we want access it like IO space. Could you explain how to hook up to the generic inb/outb functions. > >> 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. > Because in previous design, the uart driver should call lpc_io_write_byte and lpc_io_write_byte. the header file must be included in uart_driver.c to access its exported interface. > Arnd > > . > Thanks for your comment. Rongrong