On Tue, Aug 28, 2018 at 08:53:34AM +0000, A.s. Dong wrote:

quoted

-----Original Message-----
From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
Sent: Tuesday, August 28, 2018 2:21 PM
To: A.s. Dong <aisheng.dong@nxp.com>
Cc: Jassi Brar <jassisinghbrar@gmail.com>; dongas86 at gmail.com;
kernel at pengutronix.de; shawnguo at kernel.org; Fabio Estevam
[off-list ref]; dl-linux-imx [off-list ref];
linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH V5 0/5] soc: imx: add scu firmware api support

On Mon, Aug 27, 2018 at 10:21:37AM +0000, A.s. Dong wrote:

quoted

quoted

From: A.s. Dong

quoted

From: Sascha Hauer [mailto:s.hauer at pengutronix.de] On Fri, Aug 24,
2018 at 07:36:38AM +0000, A.s. Dong wrote:

quoted

Hi Jassi & Sasha,

Do you have some suggestions about this patch series?

I still think that the users like clk and pinctrl should directly
call sc_call_rpc(), no need for this shim at all.

Sorry, I'm a bit confuse.
What specific shim do you mean not needed?

Do you mean we don't need all the SCU SVC APIs (clk/pm/pinctrl/misc
and etc) Implemented in this patch series and instead directly
implement them in client driver?
For example:
drivers/soc/imx/sc/svc/pm/rpc_clnt.c
sc_err_t sc_pm_get_clock_rate(sc_ipc_t ipc, sc_rsrc_t resource,
                              sc_pm_clk_t clk, sc_pm_clock_rate_t
*rate) Change to:
drivers/clk/imx/scu/scu_clock.c ?
sc_err_t sc_pm_get_clock_rate(sc_ipc_t ipc, sc_rsrc_t resource,
                              sc_pm_clk_t clk, sc_pm_clock_rate_t
*rate)

I thought even make client driver to directly to sc_call_rpc(), it may
still better to Make them into a serrate function call like
sc_pm_get_clock_rate() but just put In client driver instead of in the central

SCU SVC place.

quoted

Not sure the point to do that.
Would you please help clarify a bit more?

I think translating the sc message setup into a function only adds additional
overhead that the reader has to follow. I do not see the point for doing this.

The original point is to separate the SCU service API implementation from the client
drivers. Client drivers don't have to know the internal details of the API implementation,
they just use the service API provided by SCU firmware. API implementation and client
users will be maintained independently.

I would buy this argument if the 'API implementation' was more than a
shim layer that directly translates between function arguments and a
message struct. With this you effectively can't change the API
implementation without changing the API you provide to the client
drivers.

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 |