在 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 <[email protected]>
Signed-off-by: lijianhua <[email protected]>
---
.../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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
