Re: [PATCH v15 6/9] HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings

[John Garry]

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