-----Original Message-----
From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
Sent: Thursday, September 20, 2018 2:34 PM
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

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

quoted

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.

I mean other functions and struct names. All prefixed by imx_scu.
Enums still not changed.
e.g.

diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
index 25e7bfe..46185a8 100644
--- a/drivers/firmware/imx/imx-scu.c
+++ b/drivers/firmware/imx/imx-scu.c

@@ -3,7 +3,7 @@
  * Copyright 2018 NXP
  *  Author: Dong Aisheng <aisheng.dong@nxp.com>
  *
- * Implementation of the IPC functions using MUs (client side).
+ * Implementation of the SCU IPC functions using MUs (client side).
  *
  */
 

@@ -23,17 +23,17 @@
 #define SCU_MU_CHAN_NUM                8
 #define MAX_RX_TIMEOUT         (msecs_to_jiffies(30))
 
-struct sc_chan {
-       struct sc_ipc *sc_ipc;
+struct imx_scu_chan {
+       struct imx_scu_ipc *sc_ipc;
 
        struct mbox_client cl;
        struct mbox_chan *ch;
        int idx;
 };
 
-struct sc_ipc {
+struct imx_scu_ipc {
        /* SCU uses 4 Tx and 4 Rx channels */
-       struct sc_chan chans[SCU_MU_CHAN_NUM];
+       struct imx_scu_chan chans[SCU_MU_CHAN_NUM];
        struct device *dev;
        struct mutex lock;
        struct completion done;

@@ -44,25 +44,25 @@ struct sc_ipc {
        u8 count;
 };
 
-static struct sc_ipc *scu_ipc_handle;
+static struct imx_scu_ipc *imx_scu_ipc_handle;

...

Regards
Dong Aisheng

Sascha

--
Pengutronix e.K.                           |
|
Industrial Linux Solutions                 |
https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
pengutronix.de%2F&data=02%7C01%7Caisheng.dong%40nxp.com%7C7a
cdea46056b4793dac008d61ec30e04%7C686ea1d3bc2b4c6fa92cd99c5c3016
35%7C0%7C0%7C636730220455595791&sdata=xoD9zOJ%2BZyQebWC
NV0zdiPkR0hFRPovWaYwxYSe8uWw%3D&reserved=0  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
|
Amtsgericht Hildesheim, HRA 2686           | Fax:
+49-5121-206917-5555 |