lkml.org 
[lkml]   [2018]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v15 6/9] HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings
    From
    Date
    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


    \
     
     \ /
      Last update: 2018-03-02 11:46    [W:4.388 / U:0.396 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site