> Subject: Re: [PATCH v4 5/6] firmware: imx: support i.MX95 BBM
> module
>
> On Fri, May 24, 2024 at 04:56:47PM +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.
> >
> > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > ---
> > drivers/firmware/imx/Makefile | 1 +
> > drivers/firmware/imx/sm-bbm.c | 314
> > ++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 315 insertions(+)
> >
> > diff --git a/drivers/firmware/imx/Makefile
> > b/drivers/firmware/imx/Makefile index 8f9f04a513a8..fb20e22074e1
> > 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_BBM_EXT} += sm-bbm.o
> > diff --git a/drivers/firmware/imx/sm-bbm.c
> > b/drivers/firmware/imx/sm-bbm.c new file mode 100644 index
> > 000000000000..5e7083bf8fd3
> > --- /dev/null
> > +++ b/drivers/firmware/imx/sm-bbm.c
> > @@ -0,0 +1,314 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2024 NXP.
> > + */
> > +
> > +#include <linux/input.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/rtc.h>
> > +#include <linux/scmi_protocol.h>
> > +#include <linux/scmi_imx_protocol.h>
> > +#include <linux/suspend.h>
> > +
> > +#define DEBOUNCE_TIME 30
> > +#define REPEAT_INTERVAL 60
> > +
> > +struct scmi_imx_bbm {
> > + struct rtc_device *rtc_dev;
> > + struct scmi_protocol_handle *ph;
> > + const struct scmi_imx_bbm_proto_ops *ops;
> > + struct notifier_block nb;
> > + int keycode;
> > + int keystate; /* 1:pressed */
> > + bool suspended;
> > + struct delayed_work check_work;
> > + struct input_dev *input;
> > +};
>
> You could pull out the *ops in a global like
>
> static const struct scmi_imx_bbm_proto_ops *bbm_ops;
>
> since the protocol ops handle are always the same you will end up
> overwriting it with the same value if this driver is probed multiple
> times (which is harmless)...you will get anyway a different *ph handle
> to operate on that you are already storing...
>
> ..up to you really
I prefer to keep as it is
>
> > +
> > +static int scmi_imx_bbm_read_time(struct device *dev, struct
> rtc_time
> > +*tm) {
> > + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev);
> > + struct scmi_protocol_handle *ph = bbnsm->ph;
> > + u64 val;
> > + int ret;
> > +
> > + ret = bbnsm->ops->rtc_time_get(ph, 0, &val);
> > + if (ret)
> > + dev_err(dev, "%s: %d\n", __func__, ret);
> > +
>
> ..so if you fail to get the time via SCMI you just carry on without caring ?
>
> > + rtc_time64_to_tm(val, tm);
>
> ... converting some random val from the stack into tm ?
>
I will fix in v5, should return an error value.
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_imx_bbm_set_time(struct device *dev, struct
> rtc_time
> > +*tm) {
> > + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev);
> > + struct scmi_protocol_handle *ph = bbnsm->ph;
> > + u64 val;
> > + int ret;
> > +
> > + val = rtc_tm_to_time64(tm);
> > +
> > + ret = bbnsm->ops->rtc_time_set(ph, 0, val);
> > + if (ret)
> > + dev_err(dev, "%s: %d\n", __func__, ret);
> > +
>
> same here why you dont report any error ?
>
Should return error. Fix in v5.
> > + return 0;
> > +}
> > +
> > +static int scmi_imx_bbm_alarm_irq_enable(struct device *dev,
> unsigned
> > +int enable) {
> > + return 0;
> > +}
> > +
> > +static int scmi_imx_bbm_set_alarm(struct device *dev, struct
> > +rtc_wkalrm *alrm) {
> > + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev);
> > + struct scmi_protocol_handle *ph = bbnsm->ph;
> > + struct rtc_time *alrm_tm = &alrm->time;
> > + u64 val;
> > + int ret;
> > +
> > + val = rtc_tm_to_time64(alrm_tm);
> > +
> > + ret = bbnsm->ops->rtc_alarm_set(ph, 0, val);
> > + if (ret)
> > + dev_err(dev, "%s: %d\n", __func__, ret);
> > +
>
> same ...
>
> ....mmm... hang on ... this reminds me of something...
>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> lore.kernel.org%2Flinux-arm-
> kernel%2FZdjgSxFx9YRP107y%40pluto%2F&data=05%7C02%7Cpeng.f
> an%40nxp.com%7C91000402f63d4e893d8208dc8f7c9399%7C686ea1
> d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638543012417198736
> %7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l
> uMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=nK
> leYfoIRJtMN13TNQB9xYH8aX1tUYk9UI202tYl49I%3D&reserved=0
>
> ...I did exactly the same comments on V1 around these 2 BBM/MISC
> SCMI drivers....but you never replied, addressed or disputed those
> issues.
>
> You DID address other review/comments in protocols and DT in later
> versions so I suppose you just forget these latter drivers reviewes and
> just rebased on.
>
> So, this review stops here for now, please address or reply to my V1
> concerns on these last 2 BBM/MISC drivers.
My bad. I will re-collect all the comments and address in new version.
Thanks,
Peng.
>
> Thanks,
> Cristian
