lkml.org 
[lkml]   [2015]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] Hisilicon LPC driver
From
Date
在 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


\
 
 \ /
  Last update: 2015-12-01 09:21    [W:0.080 / U:0.640 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site