[PATCH v4 3/6] firmware: arm_scmi: add initial support for i.MX BBM protocol

[Peng Fan]

> Subject: Re: [PATCH v4 3/6] firmware: arm_scmi: add initial support for
> i.MX BBM protocol
> 
> On Fri, May 24, 2024 at 04:56:45PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan at nxp.com>
> >
> > i.MX95 has a battery-backed module(BBM), which has persistent
> storage
> > (GPR), an RTC, and the ON/OFF button. The System Manager(SM)
> firmware
> > use SCMI vendor protocol(SCMI BBM) to let agent be able to use GPR,
> > RTC and ON/OFF button.
> >
> > Signed-off-by: Peng Fan <peng.fan at nxp.com>
> > ---
> >  drivers/firmware/arm_scmi/Kconfig          |   2 +
> >  drivers/firmware/arm_scmi/Makefile         |   1 +
> >  drivers/firmware/arm_scmi/imx/Kconfig      |  14 ++
> >  drivers/firmware/arm_scmi/imx/Makefile     |   2 +
> >  drivers/firmware/arm_scmi/imx/imx-sm-bbm.c | 380
> +++++++++++++++++++++++++++++
> >  include/linux/scmi_imx_protocol.h          |  42 ++++
> >  6 files changed, 441 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_scmi/Kconfig
> > b/drivers/firmware/arm_scmi/Kconfig
> > index aa5842be19b2..79846cbaf71b 100644
> > --- a/drivers/firmware/arm_scmi/Kconfig
> > +++ b/drivers/firmware/arm_scmi/Kconfig
> > @@ -180,4 +180,6 @@ config ARM_SCMI_POWER_CONTROL
> >  	  called scmi_power_control. Note this may needed early in
> boot to catch
> >  	  early shutdown/reboot SCMI requests.
> >
> > +source "drivers/firmware/arm_scmi/imx/Kconfig"
> > +
> 
> It could be that we fold all the Vendor drivers under
> drivers/firmware/arm_scmi/vendors once it is merged...but we will
> take care of this reowrk/refctor...still not sure about this details.

ok. Sudeep may comment more on this.

> 
> >  endmenu
> > diff --git a/drivers/firmware/arm_scmi/Makefile
> > b/drivers/firmware/arm_scmi/Makefile
> > index fd59f58ce8a2..fb9407fef60c 100644
> > --- a/drivers/firmware/arm_scmi/Makefile
> > +++ b/drivers/firmware/arm_scmi/Makefile

.....
> > +
> > +	ret = scmi_imx_bbm_attributes_get(ph, binfo);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return ph->set_priv(ph, binfo, version); }
> 
> I would move this init down below, right before the scmi_imx_bbm and
> after the proto_ops definition,  for consistency and readability.

Yeah. Fix in v5.

> 
> > +

....
> > +	.button_get = scmi_imx_bbm_button_get, };
> > +
> 
> ...just here the init
> 
> > +static const struct scmi_protocol scmi_imx_bbm = {
> > +	.id = SCMI_PROTOCOL_IMX_BBM,
> > +	.owner = THIS_MODULE,
> > +	.instance_init = &scmi_imx_bbm_protocol_init,
> > +	.ops = &scmi_imx_bbm_proto_ops,
> > +	.events = &scmi_imx_bbm_protocol_events,
> > +	.supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION,
> > +	.vendor_id = "NXP",
> > +	.sub_vendor_id = "i.MX95 EVK",
> > +};
> > +
> 
> Beside this, LGTM.
> 
> Reviewed-by: Cristian Marussi <cristian.marussi at arm.com>

Thanks for help reviewing the patches.

Thanks,
Peng.

> 
> Thanks,
> Cristian