lkml.org 
[lkml]   [2017]   [Oct]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3 16/22] firmware: arm_scmi: add arm_mhu specific mailbox interface
On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> This patch adds ARM MHU specific mailbox interface for SCMI.
>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

This clearly needs an explanation why we need another driver.

> +union mhu_data {
> + void *ptr;
> + u32 val;
> +};
> +
> +static void mhu_tx_prepare(struct mbox_client *cl, void *m)
> +{
> + struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl);
> + union mhu_data tmp;
> +
> + scmi_generic_tx_prepare(cinfo, m);
> +
> + /* clear only the relavant bit */
> + tmp.ptr = cinfo->priv;
> + *(u32 *)m &= tmp.val;
> +}

Why do you use the first 32 bits of the pointer here? That doesn't
seem to make any sense, in particular on big-endian 64-bit
architectures.

> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 97285a22dfaa..bdc9c566e6c1 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -648,6 +648,9 @@ EXPORT_SYMBOL_GPL(devm_scmi_handle_get);
> /* Each compatible listed below must have descriptor associated with it */
> static const struct of_device_id scmi_of_match[] = {
> { .compatible = "arm,scmi", .data = &scmi_generic_desc },
> +#if IS_REACHABLE(CONFIG_ARM_MHU)
> + { .compatible = "arm,mhu-scmi", .data = &mhu_scmi_desc },
> +#endif
> { /* Sentinel */ },
> };
>

This again is a bad abstraction, if the main part needs to know about
each mailbox that it could use.

Turn the registration around so rather than referring to an exported
symbol from mhu_scmi_desc.c in the main driver, you export
a symbol to register the operations and make the mhu
part its own module.

Arnd

\
 
 \ /
  Last update: 2017-10-04 13:37    [W:0.290 / U:6.716 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site