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.
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);
> +
> + 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.
> + * 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() ?
> +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.
> +}
> + do {
> + if (hisi_lpc_target_out(lpcdev, &iopara, addr, buf,
> + dwidth))
Fancy indentation. Perhaps put to one line?
> + break;
> + buf += dwidth;
> + } while (--count);
> + int ret;
> +
> + lpcdev = devm_kzalloc(dev, sizeof(struct hisi_lpc_dev),
> GFP_KERNEL);
sizeof(*lpcdev) ?
> + 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);
--
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy
