RE: [PATCH v4 5/6] firmware: imx: support i.MX95 BBM module

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 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





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux