Messages in this thread | | | Subject | Re: [PATCH] Hisilicon LPC driver | From | Rongrong Zou <> | Date | Tue, 1 Dec 2015 15:58:36 +0800 |
| |
在 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 <zourongrong@gmail.com> >> Signed-off-by: lijianhua <Jueying0518@gmail.com> >> --- >> .../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
| |