On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
> This patch adds ARM MHU specific mailbox interface for SCMI.
>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>
> Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
