On 01/03/2018 19:26, Andy Shevchenko wrote:
On Tue, 2018-02-27 at 00:40 +0800, John Garry wrote:
The low-pin-count(LPC) interface of Hip06/Hip07 accesses the
peripherals in
I/O port addresses. This patch implements the LPC host controller
driver
which perform the I/O operations on the underlying hardware.
We don't want to touch those existing peripherals' driver, such as
ipmi-bt.
So this driver applies the indirect-IO introduced in the previous
patch
after registering an indirect-IO node to the indirect-IO devices list
which
will be searched in the I/O accessors to retrieve the host-local I/O
port.
The driver config is set as a bool instead of a trisate. The reason
here is that, by the very nature of the driver providing a logical
PIO range, it does not make sense to have this driver as a loadable
module. Another more specific reason is that the Huawei D03 board
which includes hip06 SoC requires the LPC bus for UART console, so
should be built in.
Hi Andy,
Few minor comments below.
+static inline int wait_lpc_idle(unsigned char *mbase,
+ unsigned int waitcnt) {
+ do {
+ u32 status;
+
+ status = readl(mbase + LPC_REG_OP_STATUS);
+ if (status & LPC_REG_OP_STATUS_IDLE)
+ return (status & LPC_REG_OP_STATUS_FINISHED)
? 0 : -EIO;
+ ndelay(LPC_NSEC_PERWAIT);
+ } while (waitcnt--);
} while (--waitcnt);
Right, in reality it makes little difference, but we should honour the
argument we are passed.
+
+ return -ETIME;
+}
+
+/*
If you would like to have a documentation you need to use proper syntax,
i.e.
/**
Check the rest of the series for it.
I don't think kerneldoc should include these functions - they are static
and specific to this HW. I think the rest of the series is ok for
kerneldoc vs non-kerneldoc comments. The logic_pio code does have
kerneldoc comments. I will double check.
+ * hisi_lpc_target_in - trigger a series of LPC cycles for read
operation
+ * @lpcdev: pointer to hisi lpc device
+ * @para: some parameters used to control the lpc I/O operations
+ * @addr: the lpc I/O target port address
+ * @buf: where the read back data is stored
+ * @opcnt: how many I/O operations required, i.e. data width
+ *
+ * Returns 0 on success, non-zero on fail.
+ */
+ do {
+ *buf++ = readb(lpcdev->membase + LPC_REG_RDATA);
+ } while (--opcnt);
readsb() ?
+ do {
+ writeb(*buf++, lpcdev->membase + LPC_REG_WDATA);
+ } while (--opcnt);
writesb() ?
It should be ok to include these.
+static inline unsigned long
+hisi_lpc_pio_to_addr(struct hisi_lpc_dev *lpcdev, unsigned long pio)
+{
+ return pio - lpcdev->io_host->io_start +
+ lpcdev->io_host->hw_start;
I would rather put on one line.
I will try.
+}
+ do {
+ if (hisi_lpc_target_out(lpcdev, &iopara, addr, buf,
+ dwidth))
Fancy indentation. Perhaps put to one line?
Will try
+ break;
+ buf += dwidth;
+ } while (--count);
+ int ret;
+
+ lpcdev = devm_kzalloc(dev, sizeof(struct hisi_lpc_dev),
GFP_KERNEL);
sizeof(*lpcdev) ?
OK, will include
+ if (!lpcdev)
+ return -ENOMEM;
+ dev_info(dev, "registered range[%pa - sz:%pa]\n",
This is rather non-standard. We provide for resources the pattern like
"... [start-end]\n"
+ &lpcdev->io_host->io_start,
+ &lpcdev->io_host->size);
In the HW structure we record the IO base and size, as the size of
relevant to the bus address length. I could print in [start-end] format
no problem.
Thanks,
John
