RE: [PATCH v5 6/7] rtc: support i.MX95 BBM RTC

[Peng Fan]

> Subject: Re: [PATCH v5 6/7] rtc: support i.MX95 BBM RTC
> 
> Hello,
> 
> On 21/06/2024 15:04:41+0800, Peng Fan (OSS) wrote:
> > +	ret = bbnsm->ops->rtc_time_get(ph, 0, &val);
> > +	if (ret) {
> > +		dev_err(dev, "%s: %d\n", __func__, ret);
> 
> This is not super useful, you should drop the various dev_err or pr_err
> as there is no action the user can take to solve the erro apart from
> retrying.

Sure. I will remove the various dev_err or pr_err in v6.

> 
> > +		return ret;
> > +	}
> > +
> > +	rtc_time64_to_tm(val, tm);
> > +
> > +	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);
> > +
> > +	return ret;
> > +}
> > +
> > +static int scmi_imx_bbm_alarm_irq_enable(struct device *dev,
> unsigned
> > +int enable) {
> 
> How can userspace disable the alarm?

The SCMI notify has kernel level enable/disable
drivers/firmware/arm_scmi/notify.c

But indeed, userspace not able to disable the alarm.

I need use 
if (!enable) return -EOPNOTSUPP;

> 
> > +	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);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct rtc_class_ops smci_imx_bbm_rtc_ops = {
> > +	.read_time = scmi_imx_bbm_read_time,
> > +	.set_time = scmi_imx_bbm_set_time,
> > +	.set_alarm = scmi_imx_bbm_set_alarm,
> > +	.alarm_irq_enable = scmi_imx_bbm_alarm_irq_enable, };
> > +
> > +static int scmi_imx_bbm_rtc_notifier(struct notifier_block *nb,
> > +unsigned long event, void *data) {
> > +	struct scmi_imx_bbm *bbnsm = container_of(nb, struct
> scmi_imx_bbm, nb);
> > +	struct scmi_imx_bbm_notif_report *r = data;
> > +
> > +	if (r->is_rtc)
> > +		rtc_update_irq(bbnsm->rtc_dev, 1, RTC_AF |
> RTC_IRQF);
> > +	else
> > +		pr_err("Unexpected bbm event: %s\n", __func__);
> > +
> > +	return 0;
> > +}
> > +
> > +static int scmi_imx_bbm_rtc_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);
> > +	int ret;
> > +
> > +	bbnsm->rtc_dev = devm_rtc_allocate_device(dev);
> > +	if (IS_ERR(bbnsm->rtc_dev))
> > +		return PTR_ERR(bbnsm->rtc_dev);
> > +
> > +	bbnsm->rtc_dev->ops = &smci_imx_bbm_rtc_ops;
> > +	bbnsm->rtc_dev->range_min = 0;
> 
> range_min is set to 0 by default, this is not necessary
> 
> > +	bbnsm->rtc_dev->range_max = U32_MAX;
> > +
> > +	ret = devm_rtc_register_device(bbnsm->rtc_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	bbnsm->nb.notifier_call = &scmi_imx_bbm_rtc_notifier;
> > +	return handle->notify_ops->devm_event_notifier_register(sdev,
> SCMI_PROTOCOL_IMX_BBM,
> > +
> 	SCMI_EVENT_IMX_BBM_RTC,
> > +
> 	NULL, &bbnsm->nb);
> 
> Note that failing after devm_rtc_register_device opens the driver to a
> race condition as the character device will exist at that time.

Could you please share more info on this?

Won't the devm_rtc_register_device could automatically remove
the rtc is notifier register fails?

Thanks,
Peng.

> 
> 
> --
> Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and
> Kernel engineering
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> bootlin.com%2F&data=05%7C02%7Cpeng.fan%40nxp.com%7C91ef3c1
> 72dc04c6ed2b908dca1e8befb%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%7C0%7C0%7C638563268206282814%7CUnknown%7CTWFpbGZs
> b3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXV
> CI6Mn0%3D%7C0%7C%7C%7C&sdata=x8j754poti%2Bo07tbvO3p7XM
> QB5jrbkBPWKORIKZdE%2BU%3D&reserved=0