Re: [PATCH 4/5] firmware: imx: support BBM module

[Cristian Marussi]

On Fri, Feb 02, 2024 at 02:34:42PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@xxxxxxx>
> 
> The BBM module provides RTC and BUTTON feature. To i.MX95, this module
> is managed by System Manager. Linux could use i.MX SCMI BBM Extension
> protocol to use RTC and BUTTON feature.
> 
> This driver is to use SCMI interface to get/set RTC, enable pwrkey.
> 

Hi some further remarks questin about pwrkey down below.

> Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> ---
>  drivers/firmware/imx/Makefile |   1 +
>  drivers/firmware/imx/sm-bbm.c | 317 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 318 insertions(+)
> 
 [snip]

> +static int scmi_imx_bbm_pwrkey_init(struct scmi_device *sdev)
> +{
> +	const struct scmi_handle *handle = sdev->handle;
> +	struct device *dev = &sdev->dev;
> +	struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev);
> +	struct input_dev *input;
> +	int ret;
> +
> +	if (device_property_read_u32(dev, "linux,code", &bbnsm->keycode)) {
> +		bbnsm->keycode = KEY_POWER;
> +		dev_warn(dev, "key code is not specified, using default KEY_POWER\n");
> +	}

This linux,code binding prop is searched in the SCMI device node, BUT
your BB< protocol binding does NOT mention it at all.

> +
> +	INIT_DELAYED_WORK(&bbnsm->check_work, scmi_imx_bbm_pwrkey_check_for_events);
> +
> +	input = devm_input_allocate_device(dev);
> +	if (!input) {
> +		dev_err(dev, "failed to allocate the input device for SCMI IMX BBM\n");
> +		return -ENOMEM;
> +	}
> +
> +	input->name = dev_name(dev);
> +	input->phys = "bbnsm-pwrkey/input0";
> +	input->id.bustype = BUS_HOST;
> +
> +	input_set_capability(input, EV_KEY, bbnsm->keycode);
> +
> +	ret = devm_add_action_or_reset(dev, scmi_imx_bbm_pwrkey_act, bbnsm);
> +	if (ret) {
> +		dev_err(dev, "failed to register remove action\n");
> +		return ret;
> +	}
> +
> +	bbnsm->input = input;
> +
> +	ret = handle->notify_ops->devm_event_notifier_register(sdev, SCMI_PROTOCOL_IMX_BBM,
> +							       SCMI_EVENT_IMX_BBM_BUTTON,
> +							       NULL, &bbnsm->nb);
> +
> +	if (ret)
> +		dev_err(dev, "Failed to register BBM Button Events %d:", ret);
> +
> +	ret = input_register_device(input);
> +	if (ret) {
> +		dev_err(dev, "failed to register input device\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}

I suppose you cannot use std SystemPower protocol and scmi_power_control
existent upstream driver because you are configuring the event keycode that
is associated with your button press event using linux,code DT properies
looked up above, right ? (which you need to define somewhere as said
above..)

I was thinking that maybe handling events associated with generic button-presses
could be done via some std SCMI protocols like PINCTRL/GPIO (IF IT HAD NOTIFICATIONS)
and some custom SCMI gpio-keys driver in the future (not now clearly :D)...thoughts ?

Thanks,
Cristian