On Thu, Sep 20, 2018 at 02:27:00AM +0000, A.s. Dong wrote:

Hi Sasha,

quoted

-----Original Message-----
From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
Sent: Thursday, September 20, 2018 3:41 AM
To: A.s. Dong <aisheng.dong@nxp.com>
Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com; Jassi Brar
[off-list ref]; dl-linux-imx [off-list ref];
kernel at pengutronix.de; Fabio Estevam [off-list ref];
shawnguo at kernel.org
Subject: Re: [PATCH V6 2/3] firmware: imx: add SCU firmware driver support

Hi Dong,

Looks mostly ok for me now. Some small things inside.

Great, thanks for the keeping review.

quoted

On Wed, Sep 19, 2018 at 11:04:05AM +0800, Dong Aisheng wrote:

quoted

The System Controller Firmware (SCFW) is a low-level system function
which runs on a dedicated Cortex-M core to provide power, clock, and
resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
(QM, QP), and i.MX8QX (QXP, DX).

This patch implements the SCU firmware IPC function and the common
message sending API sc_call_rpc.

+int sc_ipc_get_handle(struct sc_ipc *ipc) {
+	if (!scu_ipc_handle)
+		return -EPROBE_DEFER;
+
+	ipc = scu_ipc_handle;

You are modifying a local pointer here instead of returning it.

My bad mistake.
It should be:
int sc_ipc_get_handle(struct sc_ipc **ipc)
{
        if (!scu_ipc_handle)
                return -EPROBE_DEFER;

        *ipc = scu_ipc_handle;
        return 0;
}

quoted

quoted

+	return 0;
+}
+EXPORT_SYMBOL(sc_ipc_get_handle);

Could you give the exported functions a better namespace like imx_scu_*?

Good idea.
Should we change all the rest as well?

You mean enums and such? Yes, that would be good.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |