Re: [PATCH] Hisilicon LPC driver

[Arnd Bergmann <arnd@arndb.de>]

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.

>  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.

I think you should create a child address space here using a
'#address-cells' and '#size-cells'.

> +#define LPC_REG_READ(reg, result) ((result) = readl(reg))
> +
> +#define LPC_REG_WRITE(reg, data)   writel((data), (reg))

Remove the obfuscation here.

> +struct hs_lpc_dev *lpc_dev;

Avoid global data structures.

> +	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.

> +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.

> 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.

	Arnd