Re: [PATCH v5 5/7] firmware: imx: add i.MX95 MISC driver

[Cristian Marussi]

On Fri, Jun 21, 2024 at 03:04:40PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@xxxxxxx>
> 
> The i.MX95 System manager exports SCMI MISC protocol for linux to do
> various settings, such as set board gpio expander as wakeup source.
> 
> The driver is to add the support.
> 

Hi,

a small style nitpick down below.

Other than that,

LGTM.
Reviewed-by: Cristian Marussi <cristian.marussi@xxxxxxx>

> Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> ---
>  drivers/firmware/imx/Kconfig    |  11 ++++
>  drivers/firmware/imx/Makefile   |   1 +
>  drivers/firmware/imx/sm-misc.c  | 119 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/firmware/imx/sm.h |  33 +++++++++++
>  4 files changed, 164 insertions(+)
> 
> diff --git a/drivers/firmware/imx/Kconfig b/drivers/firmware/imx/Kconfig
> index 183613f82a11..477d3f32d99a 100644
> --- a/drivers/firmware/imx/Kconfig
> +++ b/drivers/firmware/imx/Kconfig
> @@ -22,3 +22,14 @@ config IMX_SCU
>  
>  	  This driver manages the IPC interface between host CPU and the
>  	  SCU firmware running on M4.
> +
> +config IMX_SCMI_MISC_DRV
> +	tristate "IMX SCMI MISC Protocol driver"
> +	depends on IMX_SCMI_MISC_EXT || COMPILE_TEST
> +	default y if ARCH_MXC
> +	help
> +	  The System Controller Management Interface firmware (SCMI FW) is
> +	  a low-level system function which runs on a dedicated Cortex-M
> +	  core that could provide misc functions such as board control.
> +
> +	  This driver can also be built as a module.
> diff --git a/drivers/firmware/imx/Makefile b/drivers/firmware/imx/Makefile
> index 8f9f04a513a8..8d046c341be8 100644
> --- a/drivers/firmware/imx/Makefile
> +++ b/drivers/firmware/imx/Makefile
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_IMX_DSP)		+= imx-dsp.o
>  obj-$(CONFIG_IMX_SCU)		+= imx-scu.o misc.o imx-scu-irq.o rm.o imx-scu-soc.o
> +obj-${CONFIG_IMX_SCMI_MISC_DRV}	+= sm-misc.o
> diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
> new file mode 100644
> index 000000000000..342e1254a356
> --- /dev/null
> +++ b/drivers/firmware/imx/sm-misc.c
> @@ -0,0 +1,119 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2024 NXP
> + */
> +
> +#include <linux/firmware/imx/sm.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/scmi_imx_protocol.h>
> +
> +static const struct scmi_imx_misc_proto_ops *imx_misc_ctrl_ops;
> +static struct scmi_protocol_handle *ph;
> +struct notifier_block scmi_imx_misc_ctrl_nb;
> +
> +int scmi_imx_misc_ctrl_set(u32 id, u32 val)
> +{
> +	if (!ph)
> +		return -EPROBE_DEFER;
> +
> +	return imx_misc_ctrl_ops->misc_ctrl_set(ph, id, 1, &val);
> +};
> +EXPORT_SYMBOL(scmi_imx_misc_ctrl_set);
> +
> +int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val)
> +{
> +	if (!ph)
> +		return -EPROBE_DEFER;
> +
> +	return imx_misc_ctrl_ops->misc_ctrl_get(ph, id, num, val);
> +}
> +EXPORT_SYMBOL(scmi_imx_misc_ctrl_get);
> +
> +static int scmi_imx_misc_ctrl_notifier(struct notifier_block *nb,
> +				       unsigned long event, void *data)
> +{
> +	/*
> +	 * notifier_chain_register requires a valid notifier_block and
> +	 * valid notifier_call. SCMI_EVENT_IMX_MISC_CONTROL is needed
> +	 * to let SCMI firmware enable control events, but the hook here
> +	 * is just a dummy function to avoid kernel panic as of now.
> +	 */
> +	return 0;
> +}
> +
> +static int scmi_imx_misc_ctrl_probe(struct scmi_device *sdev)
> +{
> +	const struct scmi_handle *handle = sdev->handle;
> +	struct device_node *np = sdev->dev.of_node;
> +	u32 src_id, flags;
> +	int ret, i, num;
> +
> +	if (!handle)
> +		return -ENODEV;
> +
> +	if (imx_misc_ctrl_ops) {
> +		dev_err(&sdev->dev, "misc ctrl already initialized\n");
> +		return -EEXIST;
> +	}
> +
> +	imx_misc_ctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_IMX_MISC, &ph);
> +	if (IS_ERR(imx_misc_ctrl_ops))
> +		return PTR_ERR(imx_misc_ctrl_ops);
> +
> +	num = of_property_count_u32_elems(np, "nxp,ctrl-ids");
> +	if (num % 2) {
> +		dev_err(&sdev->dev, "Invalid wakeup-sources\n");
> +		return -EINVAL;
> +	}
> +
> +	scmi_imx_misc_ctrl_nb.notifier_call = &scmi_imx_misc_ctrl_notifier;
> +	for (i = 0; i < num; i += 2) {
> +		ret = of_property_read_u32_index(np, "nxp,ctrl-ids", i, &src_id);
> +		if (ret) {
> +			dev_err(&sdev->dev, "Failed to read ctrl-id: %i\n", i);
> +			continue;
> +		}
> +
> +		ret = of_property_read_u32_index(np, "nxp,ctrl-ids", i + 1, &flags);
> +		if (ret) {
> +			dev_err(&sdev->dev, "Failed to read ctrl-id value: %d\n", i + 1);
> +			continue;
> +		}
> +
> +		ret = handle->notify_ops->devm_event_notifier_register(sdev, SCMI_PROTOCOL_IMX_MISC,
> +								       SCMI_EVENT_IMX_MISC_CONTROL,
> +								       &src_id,
> +								       &scmi_imx_misc_ctrl_nb);
> +		if (ret)

missing {
		dev_err(&sdev->dev, "Failed to register scmi misc event: %d\n", src_id);

	}


Thanks,
Cristian