From: Alexandre Belloni <alexandre.belloni@bootlin.com>
To: Dong Aisheng <aisheng.dong@nxp.com>
Cc: linux-arm-kernel@lists.infradead.org, linux-rtc@vger.kernel.org,
devicetree@vger.kernel.org, linux-imx@nxp.com,
kernel@pengutronix.de, dongas86@gmail.com, robh+dt@kernel.org,
shawnguo@kernel.org, peng.fan@nxp.com,
Alessandro Zummo <a.zummo@towertech.it>
Subject: Re: [PATCH 2/2] rtc: imx-rpmsg: Add i.MX RPMSG RTC support
Date: Sat, 25 Sep 2021 23:53:47 +0200 [thread overview]
Message-ID: <YU+aa52b2zptByFu@piout.net> (raw)
In-Reply-To: <20210805033546.1390950-3-aisheng.dong@nxp.com>
Hello,
On 05/08/2021 11:35:46+0800, Dong Aisheng wrote:
> +static int rtc_send_message(struct rtc_rpmsg_info *info,
> + struct rtc_rpmsg_data *msg, bool ack)
> +{
> + struct device *dev = &info->rpdev->dev;
> + int err;
> +
> + mutex_lock(&info->lock);
> +
> + cpu_latency_qos_add_request(&info->pm_qos_req, 0);
> + reinit_completion(&info->cmd_complete);
> +
> + err = rpmsg_send(info->rpdev->ept, (void *)msg, sizeof(*msg));
> + if (err) {
> + dev_err(dev, "rpmsg send failed: %d\n", err);
> + goto err_out;
> + }
> +
> + if (ack) {
> + err = wait_for_completion_timeout(&info->cmd_complete,
> + msecs_to_jiffies(RPMSG_TIMEOUT));
> + if (!err) {
> + dev_err(dev, "rpmsg send timeout\n");
> + err = -ETIMEDOUT;
> + goto err_out;
> + }
> +
> + if (info->msg->ret != 0) {
> + dev_err(dev, "rpmsg not ack %d\n", info->msg->ret);
This is very verbose and I guess nobody will ever read those, maybe use
dev_dbg?
> + err = -EINVAL;
> + goto err_out;
> + }
> +
> + err = 0;
> + }
> +
> +err_out:
> + cpu_latency_qos_remove_request(&info->pm_qos_req);
> + mutex_unlock(&info->lock);
> +
> + return err;
> +}
> +
> +static int imx_rpmsg_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct rtc_rpmsg_info *rtc_rpmsg = dev_get_drvdata(dev);
> + struct rtc_rpmsg_data msg;
> + int ret;
> +
> + msg.header.cate = IMX_RPMSG_RTC;
> + msg.header.major = IMX_RMPSG_MAJOR;
> + msg.header.minor = IMX_RMPSG_MINOR;
> + msg.header.type = RTC_RPMSG_SEND;
> + msg.header.cmd = RTC_RPMSG_GET_TIME;
> +
> + ret = rtc_send_message(rtc_rpmsg, &msg, true);
> + if (ret)
> + return ret;
> +
Is there a way to know when the time is invalid (e.g. it has not been
set yet)?
> + rtc_time64_to_tm(rtc_rpmsg->msg->sec, tm);
> +
> + return 0;
> +}
> +
> +static int imx_rpmsg_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct rtc_rpmsg_info *rtc_rpmsg = dev_get_drvdata(dev);
> + struct rtc_rpmsg_data msg;
> + unsigned long time;
> + int ret;
> +
> + time = rtc_tm_to_time64(tm);
> +
> + msg.header.cate = IMX_RPMSG_RTC;
> + msg.header.major = IMX_RMPSG_MAJOR;
> + msg.header.minor = IMX_RMPSG_MINOR;
> + msg.header.type = RTC_RPMSG_SEND;
> + msg.header.cmd = RTC_RPMSG_SET_TIME;
> + msg.sec = time;
> +
> + ret = rtc_send_message(rtc_rpmsg, &msg, true);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int imx_rpmsg_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> + struct rtc_rpmsg_info *rtc_rpmsg = dev_get_drvdata(dev);
> + struct rtc_rpmsg_data msg;
> + int ret;
> +
> + msg.header.cate = IMX_RPMSG_RTC;
> + msg.header.major = IMX_RMPSG_MAJOR;
> + msg.header.minor = IMX_RMPSG_MINOR;
> + msg.header.type = RTC_RPMSG_SEND;
> + msg.header.cmd = RTC_RPMSG_GET_ALARM;
> +
> + ret = rtc_send_message(rtc_rpmsg, &msg, true);
> + if (ret)
> + return ret;
> +
> + rtc_time64_to_tm(rtc_rpmsg->msg->sec, &alrm->time);
> + alrm->pending = rtc_rpmsg->msg->pending;
> +
> + return 0;
> +}
> +
> +static int imx_rpmsg_rtc_alarm_irq_enable(struct device *dev,
> + unsigned int enable)
> +{
> + struct rtc_rpmsg_info *rtc_rpmsg = dev_get_drvdata(dev);
> + struct rtc_rpmsg_data msg;
> + int ret;
> +
> + msg.header.cate = IMX_RPMSG_RTC;
> + msg.header.major = IMX_RMPSG_MAJOR;
> + msg.header.minor = IMX_RMPSG_MINOR;
> + msg.header.type = RTC_RPMSG_SEND;
> + msg.header.cmd = RTC_RPMSG_ENABLE_ALARM;
> + msg.enable = enable;
> +
> + ret = rtc_send_message(rtc_rpmsg, &msg, true);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int imx_rpmsg_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> + struct rtc_rpmsg_info *rtc_rpmsg = dev_get_drvdata(dev);
> + struct rtc_rpmsg_data msg;
> + unsigned long time;
> + int ret;
> +
> + time = rtc_tm_to_time64(&alrm->time);
> +
> + msg.header.cate = IMX_RPMSG_RTC;
> + msg.header.major = IMX_RMPSG_MAJOR;
> + msg.header.minor = IMX_RMPSG_MINOR;
> + msg.header.type = RTC_RPMSG_SEND;
> + msg.header.cmd = RTC_RPMSG_SET_ALARM;
> + msg.sec = time;
> + msg.enable = alrm->enabled;
> +
> + ret = rtc_send_message(rtc_rpmsg, &msg, true);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static const struct rtc_class_ops imx_rpmsg_rtc_ops = {
> + .read_time = imx_rpmsg_rtc_read_time,
> + .set_time = imx_rpmsg_rtc_set_time,
> + .read_alarm = imx_rpmsg_rtc_read_alarm,
> + .set_alarm = imx_rpmsg_rtc_set_alarm,
> + .alarm_irq_enable = imx_rpmsg_rtc_alarm_irq_enable,
> +};
> +
> +static int rtc_rpmsg_probe(struct rpmsg_device *rpdev)
> +{
> + struct device *dev = &rpdev->dev;
> + struct rtc_rpmsg_info *rtc_rpmsg;
> +
> + dev_info(dev, "new channel: 0x%x -> 0x%x\n", rpdev->src, rpdev->dst);
> +
Same comment, please try to cut down on the number of strings in the
driver, your customers will thank you.
> + rtc_rpmsg = devm_kzalloc(dev, sizeof(*rtc_rpmsg), GFP_KERNEL);
> + if (!rtc_rpmsg)
> + return -ENOMEM;
> +
> + rtc_rpmsg->rpdev = rpdev;
> + mutex_init(&rtc_rpmsg->lock);
> + init_completion(&rtc_rpmsg->cmd_complete);
> +
> + dev_set_drvdata(dev, rtc_rpmsg);
> +
> + device_init_wakeup(dev, true);
> +
> + rtc_rpmsg->rtc = devm_rtc_device_register(dev, "rtc-rpmsg",
> + &imx_rpmsg_rtc_ops,
> + THIS_MODULE);
> + if (IS_ERR(rtc_rpmsg->rtc)) {
> + dev_err(dev, "failed to register rtc rpmsg: %ld\n", PTR_ERR(rtc_rpmsg->rtc));
> + return PTR_ERR(rtc_rpmsg->rtc);
> + }
Please use devm_rtc_allocate_device/devm_rtc_register_device. Also, it
would be nice to set .range_min and .range_max if you actually know what
the underlying RTC supports.
> +
> + return 0;
> +}
> +
> +static void rtc_rpmsg_remove(struct rpmsg_device *rpdev)
> +{
> + dev_info(&rpdev->dev, "rtc rpmsg driver is removed\n");
Seriously, drop this function...
> +}
> +
> +static int rtc_rpmsg_cb(struct rpmsg_device *rpdev, void *data, int len,
> + void *priv, u32 src)
> +{
> + struct rtc_rpmsg_info *rtc_rpmsg = dev_get_drvdata(&rpdev->dev);
> + struct rtc_rpmsg_data *msg = (struct rtc_rpmsg_data *)data;
> +
> + rtc_rpmsg->msg = msg;
> +
> + if (msg->header.type == RTC_RPMSG_RECEIVE)
> + complete(&rtc_rpmsg->cmd_complete);
> + else if (msg->header.type == RTC_RPMSG_NOTIFY)
> + rtc_update_irq(rtc_rpmsg->rtc, 1, RTC_IRQF);
> + else
> + dev_err(&rpdev->dev, "wrong command type!\n");
> +
> + return 0;
> +}
> +
> +static const struct rpmsg_device_id rtc_rpmsg_id_table[] = {
> + { .name = "rpmsg-rtc-channel" },
> + { },
> +};
> +
> +static struct rpmsg_driver rtc_rpmsg_driver = {
> + .drv.name = "imx_rtc_rpmsg",
> + .probe = rtc_rpmsg_probe,
> + .remove = rtc_rpmsg_remove,
> + .callback = rtc_rpmsg_cb,
> + .id_table = rtc_rpmsg_id_table,
> +};
> +
> +/*
> + * imx m4 has a limitation that we can't read data during ns process.
> + * So register rtc a little bit late as rtc core will read data during
> + * register process
> + */
> +static int __init rtc_rpmsg_init(void)
> +{
> + return register_rpmsg_driver(&rtc_rpmsg_driver);
> +}
> +late_initcall(rtc_rpmsg_init);
> +
> +MODULE_AUTHOR("Dong Aisheng <aisheng.dong@nxp.com>");
> +MODULE_DESCRIPTION("NXP i.MX RPMSG RTC Driver");
> +MODULE_ALIAS("platform:imx_rtc_rpmsg");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/firmware/imx/rpmsg.h b/include/linux/firmware/imx/rpmsg.h
> new file mode 100644
> index 000000000000..20bcce23c917
> --- /dev/null
> +++ b/include/linux/firmware/imx/rpmsg.h
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2019-2021 NXP.
> + */
> +
> +#ifndef __LINUX_IMX_RPMSG_H__
> +#define __LINUX_IMX_RPMSG_H__
> +
> +#include <linux/types.h>
> +
> +/*
> + * Global header file for iMX RPMSG
> + */
> +
> +/* Category define */
> +#define IMX_RMPSG_LIFECYCLE 1
> +#define IMX_RPMSG_PMIC 2
> +#define IMX_RPMSG_AUDIO 3
> +#define IMX_RPMSG_KEY 4
> +#define IMX_RPMSG_GPIO 5
> +#define IMX_RPMSG_RTC 6
> +#define IMX_RPMSG_SENSOR 7
> +
> +/* rpmsg version */
> +#define IMX_RMPSG_MAJOR 1
> +#define IMX_RMPSG_MINOR 0
> +
> +struct imx_rpmsg_head {
> + u8 cate;
> + u8 major;
> + u8 minor;
> + u8 type;
> + u8 cmd;
> + u8 reserved[5];
> +} __packed;
> +
> +#endif /* __LINUX_IMX_RPMSG_H__ */
> --
> 2.25.1
>
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
prev parent reply other threads:[~2021-09-25 21:53 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-05 3:35 [PATCH 0/2] rtc: add imx rpmsg rtc support Dong Aisheng
2021-08-05 3:35 ` [PATCH 1/2] ARM: dts: imx7ulp: add cm4 support Dong Aisheng
2021-08-05 3:35 ` [PATCH 2/2] rtc: imx-rpmsg: Add i.MX RPMSG RTC support Dong Aisheng
2021-08-05 8:13 ` Alexandre Belloni
2021-08-05 8:48 ` Dong Aisheng
2021-09-25 21:53 ` Alexandre Belloni [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YU+aa52b2zptByFu@piout.net \
--to=alexandre.belloni@bootlin.com \
--cc=a.zummo@towertech.it \
--cc=aisheng.dong@nxp.com \
--cc=devicetree@vger.kernel.org \
--cc=dongas86@gmail.com \
--cc=kernel@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-imx@nxp.com \
--cc=linux-rtc@vger.kernel.org \
--cc=peng.fan@nxp.com \
--cc=robh+dt@kernel.org \
--cc=shawnguo@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).