* Re: [PATCH 2/2] rtc: qcom: rtc-glink: Add PMIC GLINK RTC driver and extend pmic_glink
2026-08-06 20:56 ` [PATCH 2/2] rtc: qcom: rtc-glink: Add PMIC GLINK RTC driver and extend pmic_glink Kamal Wadhwa
@ 2026-08-06 21:09 ` sashiko-bot
2026-08-06 23:53 ` Bjorn Andersson
2026-08-07 20:58 ` Alexandre Belloni
2 siblings, 0 replies; 8+ messages in thread
From: sashiko-bot @ 2026-08-06 21:09 UTC (permalink / raw)
To: Kamal Wadhwa; +Cc: Alexandre Belloni, robh, devicetree, conor+dt, linux-rtc
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Racy cancellation of alarm_work and incorrect devres teardown order causes Use-After-Free on driver unbind and probe failure.
- [High] The callback extracts the property ID instead of the property value for GET_PROPERTY responses, returning the property ID as the time or alarm value.
- [High] A timeout in `qcom_rtc_glink_request` leaves the request-response state machine out of sync, allowing late firmware responses to hijack subsequent requests.
- [High] Corrupted time and alarm calculations via contradictory fallback usage of QCOM_RTC_GLINK_TIME and implicit 64-bit integer promotion.
--
--- Patch [2]: [PATCH 2/2] rtc: qcom: rtc-glink: Add PMIC GLINK RTC driver and extend pmic_glink ---
commit 453a7a32b4bd6fbb186e249935eb4448bc3ddd38
Author: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
Subject: rtc: qcom: rtc-glink: Add PMIC GLINK RTC driver and extend pmic_glink
Add a new RTC driver that communicates with the PMIC firmware over the
GLINK transport layer. It supports reading time and alarm functionality via
GET_PROPERTY and SET_PROPERTY opcodes.
> diff --git a/drivers/rtc/rtc-qcom-glink.c b/drivers/rtc/rtc-qcom-glink.c
> new file mode 100644
> index 0000000000000..611f06a3cc5ca
> --- /dev/null
> +++ b/drivers/rtc/rtc-qcom-glink.c
> @@ -0,0 +1,690 @@
[ ... ]
> +static int qcom_rtc_glink_request(struct qcom_rtc_glink *rtc_glink,
> + void *data, size_t len)
> +{
> + unsigned long flags;
> + unsigned long left;
> + int ret;
[ ... ]
> + left = wait_for_completion_timeout(&rtc_glink->ack, HZ);
> + spin_lock_irqsave(&rtc_glink->state_lock, flags);
> + rtc_glink->request_pending = false;
> + spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
[Severity: High]
If wait_for_completion_timeout() expires, request_pending is cleared. If a
new request is immediately made, request_pending becomes true again.
Could a late firmware response to the previous request arrive and be
mistakenly accepted for the new request, since the callback lacks
transaction ID or opcode validation?
> + if (!left)
> + return -ETIMEDOUT;
> +
> + return rtc_glink->error;
> +}
[ ... ]
> +static int qcom_rtc_glink_get_time(struct qcom_rtc_glink *rtc_glink,
> + struct rtc_time *time)
> +{
> + u32 ticks;
> + int ret;
[ ... ]
> + ret = qcom_rtc_glink_get_property(rtc_glink, QCOM_RTC_GLINK_TIME,
> + &ticks);
> + if (!ret && ticks != U32_MAX) {
> + rtc_time64_to_tm((time64_t)ticks + rtc_glink->offset, time);
[Severity: High]
QCOM_RTC_GLINK_TIME is set as an absolute time in qcom_rtc_glink_set_time().
Does adding rtc_glink->offset to it here in the fallback path result in an
incorrect time calculation (Absolute Time + (Absolute Time - Uptime))?
Also, since rtc_glink->offset is a u32, will a negative 32-bit offset be
zero-extended rather than sign-extended when computing
(time64_t)ticks + rtc_glink->offset?
> + return 0;
> + }
> +
> + dev_err(rtc_glink->dev, "all time sources failed\n");
> + return -EIO;
> +}
[ ... ]
> +static int qcom_rtc_glink_get_alarm(struct qcom_rtc_glink *rtc_glink,
> + struct rtc_wkalrm *alarm)
> +{
> + u32 fw_alarm_time, fw_current, alarm_en;
> + struct rtc_time now_tm;
> + time64_t now_real, secs_until_alarm, alarm_real;
> + int ret;
[ ... ]
> + ret = qcom_rtc_glink_get_time(rtc_glink, &now_tm);
> + if (ret)
> + return ret;
> + now_real = rtc_tm_to_time64(&now_tm);
> + secs_until_alarm = (time64_t)fw_alarm_time - (time64_t)fw_current;
[Severity: High]
By casting these 32-bit firmware times to time64_t before subtracting, do we
lose the 32-bit modulo arithmetic?
If the hardware counters wrap, will this produce a large negative delta
instead of the correct positive duration?
> + alarm_real = now_real + secs_until_alarm;
> + dev_dbg(rtc_glink->dev,
> + "get_alarm: now_real=%lld fw_current=%u fw_alarm=%u delta=%lld\n",
> + now_real, fw_current, fw_alarm_time, secs_until_alarm);
> + rtc_time64_to_tm(alarm_real, &alarm->time);
> + alarm->enabled = !!alarm_en;
> + return 0;
> +}
[ ... ]
> +static void qcom_rtc_glink_callback(const void *data, size_t len, void *priv)
> +{
> + struct qcom_rtc_glink *rtc_glink = priv;
> + const struct pmic_glink_hdr *hdr = data;
> + unsigned long flags;
> + bool pending;
> +
> + if (len < sizeof(*hdr))
> + return;
> +
> + dev_dbg(rtc_glink->dev,
> + "RX opcode=0x%x type=0x%x owner=0x%x len=%zu\n",
> + le32_to_cpu(hdr->opcode), le32_to_cpu(hdr->type),
> + le32_to_cpu(hdr->owner), len);
> +
> + if (le32_to_cpu(hdr->opcode) == RTC_GLINK_ALARM_EXPIRED) {
> + dev_info(rtc_glink->dev, "alarm expired\n");
> + schedule_work(&rtc_glink->alarm_work);
[Severity: High]
Does this handle the RTC_GLINK_ALARM_EXPIRED opcode and call schedule_work()
unconditionally?
If the driver is actively tearing down, could this run after the device is
freed due to the devres teardown order?
> + return;
> + }
[ ... ]
> + switch (le32_to_cpu(hdr->opcode)) {
> + case RTC_GLINK_GET_PROPERTY:
> + fallthrough;
> + case RTC_GLINK_SET_PROPERTY: {
> + const struct qcom_rtc_glink_msg *msg = data;
> +
> + if (len < sizeof(struct pmic_glink_hdr) + sizeof(__le32)) {
> + rtc_glink->error = -EINVAL;
> + break;
> + }
> + rtc_glink->resp_value = le32_to_cpu(msg->property);
[Severity: High]
For GET_PROPERTY responses, does this assign the property ID (e.g., 0 for
time, 1 for alarm) to resp_value instead of the actual data in msg->value?
Also, the bounds check only validates 16 bytes (sizeof hdr + sizeof
property). Does this need to validate 20 bytes to safely access msg->value?
> + rtc_glink->resp_valid = true;
> + rtc_glink->error = 0;
> + dev_dbg(rtc_glink->dev, "ACK opcode=0x%x resp_value=%u\n",
> + le32_to_cpu(hdr->opcode), rtc_glink->resp_value);
> + break;
> + }
[ ... ]
> + default:
> + spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> + dev_warn(rtc_glink->dev,
> + "unhandled RX opcode=0x%x len=%zu\n",
> + le32_to_cpu(hdr->opcode), len);
> + return;
> + }
> +
> + spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> + complete(&rtc_glink->ack);
[Severity: High]
Is it safe to call complete() after dropping state_lock?
Could this create a race window where a preempted callback completes a newly
issued request prematurely?
> +}
[ ... ]
> +static int qcom_rtc_glink_probe(struct auxiliary_device *adev,
> + const struct auxiliary_device_id *id)
> +{
> + struct device *dev = &adev->dev;
> + struct qcom_rtc_glink *rtc_glink;
[ ... ]
> + rtc_glink->client = devm_pmic_glink_client_alloc(dev,
> + PMIC_GLINK_OWNER_RTC,
> + qcom_rtc_glink_callback,
> + qcom_rtc_glink_pdr_notify,
> + rtc_glink);
> + if (IS_ERR(rtc_glink->client))
> + return dev_err_probe(dev, PTR_ERR(rtc_glink->client),
> + "failed to allocate glink client\n");
> +
> + pmic_glink_client_register(rtc_glink->client);
> +
> + device_init_wakeup(dev, true);
> +
> + rtc_glink->rtc = devm_rtc_allocate_device(dev);
> + if (IS_ERR(rtc_glink->rtc))
[Severity: High]
The GLINK client is registered before the RTC device is allocated.
If devm_rtc_allocate_device() fails, or during driver unbind, the GLINK
client remains active while devres unwinds. Could this allow firmware to
trigger qcom_rtc_glink_callback() on a freed rtc_glink structure?
> + return dev_err_probe(dev, PTR_ERR(rtc_glink->rtc),
> + "failed to allocate RTC device\n");
[ ... ]
> +static void qcom_rtc_glink_remove(struct auxiliary_device *adev)
> +{
> + struct qcom_rtc_glink *rtc_glink = dev_get_drvdata(&adev->dev);
> + /*
> + * Nothing left to arm alarm_work after this returns: the glink
> + * client (and its callback/pdr_notify) is torn down by devm after
> + * .remove() returns, and rtc_update_irq() below still targets a
> + * live devm-managed rtc device.
> + */
> + cancel_work_sync(&rtc_glink->alarm_work);
[Severity: High]
Since the GLINK client is managed by devm and hasn't been unregistered yet,
could an incoming ALARM_EXPIRED firmware message re-queue alarm_work
immediately after this cancel_work_sync() completes?
This might lead to a use-after-free when devres subsequently frees the
memory.
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260807-rtc-glink-cleanup-v1-0-7801e4d69c4f@oss.qualcomm.com?part=2
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 2/2] rtc: qcom: rtc-glink: Add PMIC GLINK RTC driver and extend pmic_glink
2026-08-06 20:56 ` [PATCH 2/2] rtc: qcom: rtc-glink: Add PMIC GLINK RTC driver and extend pmic_glink Kamal Wadhwa
2026-08-06 21:09 ` sashiko-bot
@ 2026-08-06 23:53 ` Bjorn Andersson
2026-08-07 20:58 ` Alexandre Belloni
2 siblings, 0 replies; 8+ messages in thread
From: Bjorn Andersson @ 2026-08-06 23:53 UTC (permalink / raw)
To: Kamal Wadhwa
Cc: Alexandre Belloni, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-rtc,
linux-arm-msm, devicetree, Raj Aryan
On Fri, Aug 07, 2026 at 02:26:31AM +0530, Kamal Wadhwa wrote:
> Add a new RTC driver that communicates with the PMIC firmware over
> the GLINK transport layer. The driver supports reading time and
> alarm functionality via GET_PROPERTY and SET_PROPERTY opcodes.
Please review the second sentence here, it doesn't match the
implementation.
In fact,
https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
says that your commit message should "describe your problem" and nowhere
in this commit message is it described why this patch exists. This is
just regurgitating the code.
N.B. There are no valid problem descriptions that starts with the word
"Add"...
>
> Time is read via GET_PROPERTY (opcode 0x62) using QCOM_RTC_GLINK_TIME
> property (0x00). Alarm set and enable are handled via SET_PROPERTY
> (opcode 0x63) using QCOM_RTC_GLINK_ALARM and QCOM_RTC_GLINK_ALARM_ENABLE
> properties respectively.
>
In both of these paragraphs you talk about reading time (and presumably
set/get alarm) - but you do implement rtc_class_ops::set_time, what's up
with that?
> GET_PROPERTY and SET_PROPERTY responses share the same firmware message
> layout, with the firmware echoing back the property value. Both opcodes
> are handled by a common callback path accordingly.
>
That's true, but it's capturing one detail of the implementation. I
don't think it's relevant in the commit message.
> Also extend pmic_glink to register the RTC as a PMIC GLINK client and
> add PMIC GLINK RTC client support for glymur.
>
The fact that those 22 lines of code is for a complete separate
subsystem/maintainer and that there isn't a build-time dependency means
that it would be much nicer if it came as separate commits.
The PMIC_GLINK_OWNER_RTC define can be carried in the rtc driver.
> Assisted-by: Claude:claude-sonnet-5
> Signed-off-by: Raj Aryan <raryan@qti.qualcomm.com>
> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
Who wrote this driver? The author seems to be you Kamal, but the first
one to sign off on the work was Raj?
> ---
> MAINTAINERS | 7 +
> drivers/rtc/Kconfig | 10 +
> drivers/rtc/Makefile | 1 +
> drivers/rtc/rtc-qcom-glink.c | 690 ++++++++++++++++++++++++++++++++++++
> drivers/soc/qcom/pmic_glink.c | 22 +-
> include/linux/soc/qcom/pmic_glink.h | 1 +
> 6 files changed, 730 insertions(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5f178a5ff026..752cf667b8ad 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22505,6 +22505,13 @@ S: Maintained
> F: Documentation/devicetree/bindings/power/supply/qcom,pmi8998-charger.yaml
> F: drivers/power/supply/qcom_smbx.c
>
> +QUALCOMM PMIC GLINK RTC DRIVER
> +M: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> +L: linux-rtc@vger.kernel.org
> +L: linux-arm-msm@vger.kernel.org
> +S: Maintained
> +F: drivers/rtc/rtc-qcom-glink.c
> +
> QUALCOMM PPE DRIVER
> M: Luo Jie <jie.luo@oss.qualcomm.com>
> L: netdev@vger.kernel.org
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 01def8231873..3ac39704b2e1 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1836,6 +1836,16 @@ config RTC_DRV_PM8XXX
> To compile this driver as a module, choose M here: the
> module will be called rtc-pm8xxx.
>
> +config RTC_DRV_QCOM_GLINK
> + tristate "Qualcomm PMIC GLINK RTC"
> + depends on QCOM_PMIC_GLINK
> + help
> + Say Y here to enable support for the RTC exposed by the PMIC
> + over the GLINK transport on Qualcomm platforms.
> +
> + This driver can also be built as a module. If so, the module
> + will be called rtc-qcom-glink.
> +
> config RTC_DRV_TEGRA
> tristate "NVIDIA Tegra Internal RTC driver"
> depends on ARCH_TEGRA || COMPILE_TEST
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 0347645b021f..8215a1fc29fb 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -73,6 +73,7 @@ obj-$(CONFIG_RTC_DRV_FM3130) += rtc-fm3130.o
> obj-$(CONFIG_RTC_DRV_FSL_FTM_ALARM) += rtc-fsl-ftm-alarm.o
> obj-$(CONFIG_RTC_DRV_FTRTC010) += rtc-ftrtc010.o
> obj-$(CONFIG_RTC_DRV_GENERIC) += rtc-generic.o
> +obj-$(CONFIG_RTC_DRV_QCOM_GLINK) += rtc-qcom-glink.o
> obj-$(CONFIG_RTC_DRV_GOLDFISH) += rtc-goldfish.o
> obj-$(CONFIG_RTC_DRV_HID_SENSOR_TIME) += rtc-hid-sensor-time.o
> obj-$(CONFIG_RTC_DRV_HYM8563) += rtc-hym8563.o
> diff --git a/drivers/rtc/rtc-qcom-glink.c b/drivers/rtc/rtc-qcom-glink.c
> new file mode 100644
> index 000000000000..611f06a3cc5c
> --- /dev/null
> +++ b/drivers/rtc/rtc-qcom-glink.c
> @@ -0,0 +1,690 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +#include <linux/auxiliary_bus.h>
> +#include <linux/completion.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/property.h>
> +#include <linux/rtc.h>
> +#include <linux/slab.h>
> +#include <linux/soc/qcom/pdr.h>
> +#include <linux/soc/qcom/pmic_glink.h>
> +#include <linux/timekeeping.h>
> +#include <linux/workqueue.h>
> +
> +#define RTC_GLINK_SET_PROPERTY 0x63
> +#define RTC_GLINK_GET_PROPERTY 0x65
> +#define RTC_GLINK_GET_RTC_TICKS 0x67
> +#define RTC_GLINK_GET_REAL_TIME 0x68
> +#define RTC_GLINK_SET_REAL_TIME 0x69
> +#define RTC_GLINK_ALARM_EXPIRED 0x6A
> +
> +enum qcom_rtc_glink_properties {
> + QCOM_RTC_GLINK_TIME = 0,
> + QCOM_RTC_GLINK_ALARM_TIME,
> + QCOM_RTC_GLINK_ALARM_ENABLE,
> +};
This isn't an enumeration, these are protocol-level constants. Please
use #define to make that clear.
> +
> +struct qcom_rtc_glink_msg {
> + struct pmic_glink_hdr hdr;
> + __le32 property;
> + __le32 value;
> +};
> +
> +struct qcom_rtc_glink_generic_req {
> + struct pmic_glink_hdr hdr;
> +};
> +
> +struct qcom_rtc_glink_status_resp {
> + struct pmic_glink_hdr hdr;
> + __le32 return_status;
> +};
> +
> +struct qcom_rtc_glink_ticks_resp {
> + struct pmic_glink_hdr hdr;
> + __le32 return_status;
> + __le32 rtc_ticks;
> +};
> +
> +struct qcom_rtc_glink_real_time_resp {
> + struct pmic_glink_hdr hdr;
> + __le32 return_status;
> + __le32 real_time_data[4];
> +};
> +
> +struct qcom_rtc_glink_set_real_time_req {
> + struct pmic_glink_hdr hdr;
> + __le32 real_time_data[4];
> +};
> +
> +struct qcom_rtc_glink {
> + struct device *dev;
> + struct pmic_glink_client *client;
> + struct rtc_device *rtc;
> +
> + /* Serializes requests: only one may be in flight at a time */
> + struct mutex lock;
> + struct completion ack;
> + struct work_struct alarm_work;
> +
> + /* Protects service_up and request_pending across callback contexts */
> + spinlock_t state_lock;
> + bool service_up;
> + bool request_pending;
> +
> + int error;
> + u32 resp_value;
> + u32 offset;
> + struct rtc_time resp_tm;
> + bool resp_valid;
> + bool allow_set_time;
> +};
> +
> +static int qcom_rtc_glink_request(struct qcom_rtc_glink *rtc_glink,
> + void *data, size_t len)
> +{
> + unsigned long flags;
> + unsigned long left;
> + int ret;
> +
> + /*
> + * Reinit the completion before request_pending becomes visible to
> + * qcom_rtc_glink_pdr_tify(), so a concurrent SSR down-transition
> + * can't complete() a stale completion that gets wiped out by
> + * reinit_completion() right after.
> + */
> + reinit_completion(&rtc_glink->ack);
> + rtc_glink->error = 0;
> + rtc_glink->resp_valid = false;
> +
> + spin_lock_irqsave(&rtc_glink->state_lock, flags);
> + if (!rtc_glink->service_up) {
> + spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> + return -ECONNRESET;
> + }
> + rtc_glink->request_pending = true;
> + spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> +
> + ret = pmic_glink_send(rtc_glink->client, data, len);
> + if (ret < 0) {
> + spin_lock_irqsave(&rtc_glink->state_lock, flags);
> + rtc_glink->request_pending = false;
> + spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> + return ret;
These 4 lines are effectively duplicated after the
wait_for_completion_timeout(), please refactor this with a goto.
> + }
> +
> + left = wait_for_completion_timeout(&rtc_glink->ack, HZ);
> + spin_lock_irqsave(&rtc_glink->state_lock, flags);
> + rtc_glink->request_pending = false;
> + spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> + if (!left)
> + return -ETIMEDOUT;
> +
> + return rtc_glink->error;
> +}
> +
> +static int qcom_rtc_glink_set_property(struct qcom_rtc_glink *rtc_glink,
> + u32 property, u32 value)
> +{
> + struct qcom_rtc_glink_msg msg = {
> + .hdr.owner = cpu_to_le32(PMIC_GLINK_OWNER_RTC),
> + .hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP),
> + .hdr.opcode = cpu_to_le32(RTC_GLINK_SET_PROPERTY),
> + .property = cpu_to_le32(property),
> + .value = cpu_to_le32(value),
> + };
> + int ret;
> +
> + dev_dbg(rtc_glink->dev,
> + "TX opcode=0x%x property=%u value=%u\n",
> + RTC_GLINK_SET_PROPERTY, property, value);
> + mutex_lock(&rtc_glink->lock);
> + ret = qcom_rtc_glink_request(rtc_glink, &msg, sizeof(msg));
> + mutex_unlock(&rtc_glink->lock);
> + return ret;
> +}
> +
> +static int qcom_rtc_glink_get_property(struct qcom_rtc_glink *rtc_glink,
> + u32 property, u32 *value)
> +{
> + struct qcom_rtc_glink_msg msg = {
> + .hdr.owner = cpu_to_le32(PMIC_GLINK_OWNER_RTC),
> + .hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP),
> + .hdr.opcode = cpu_to_le32(RTC_GLINK_GET_PROPERTY),
> + .property = cpu_to_le32(property),
> + };
> + int ret;
> +
> + dev_dbg(rtc_glink->dev,
> + "TX opcode=0x%x property=%u\n",
> + RTC_GLINK_GET_PROPERTY, property);
> + mutex_lock(&rtc_glink->lock);
> + ret = qcom_rtc_glink_request(rtc_glink, &msg, sizeof(msg));
> + if (!ret) {
The idiomatic form in the Linux kernel is to handle the error. I'd find
the following form more natural to read:
ret = qcom_rtc_glink_request(rtc_glink, &msg, sizeof(msg));
if (ret < 0) {
goto out_unlock;
} else if (!rtc_glink->resp_valid) {
ret = -EIO;
goto out_unlock;
}
*value = rtc_glink->resp_value;
out_unlock:
mutex_unlock(&rtc_glink->lock);
return ret;
> + if (!rtc_glink->resp_valid)
> + ret = -EIO;
> + else
> + *value = rtc_glink->resp_value;
> + }
> + mutex_unlock(&rtc_glink->lock);
> + return ret;
> +}
> +
> +static int qcom_rtc_glink_get_ticks(struct qcom_rtc_glink *rtc_glink,
> + u32 *ticks)
> +{
> + struct qcom_rtc_glink_generic_req msg = {
> + .hdr.owner = cpu_to_le32(PMIC_GLINK_OWNER_RTC),
> + .hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP),
> + .hdr.opcode = cpu_to_le32(RTC_GLINK_GET_RTC_TICKS),
> + };
> + int ret;
> +
> + dev_dbg(rtc_glink->dev, "TX opcode=0x%x\n", RTC_GLINK_GET_RTC_TICKS);
> + mutex_lock(&rtc_glink->lock);
> + ret = qcom_rtc_glink_request(rtc_glink, &msg, sizeof(msg));
> + if (!ret) {
> + if (!rtc_glink->resp_valid)
> + ret = -EIO;
> + else
> + *ticks = rtc_glink->resp_value;
> + }
Ditto
> + mutex_unlock(&rtc_glink->lock);
> + return ret;
> +}
> +
> +static int qcom_rtc_glink_get_real_time(struct qcom_rtc_glink *rtc_glink,
> + struct rtc_time *tm)
> +{
> + struct qcom_rtc_glink_generic_req msg = {
> + .hdr.owner = cpu_to_le32(PMIC_GLINK_OWNER_RTC),
> + .hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP),
> + .hdr.opcode = cpu_to_le32(RTC_GLINK_GET_REAL_TIME),
> + };
> + int ret;
> +
> + dev_dbg(rtc_glink->dev, "TX opcode=0x%x\n", RTC_GLINK_GET_REAL_TIME);
> + mutex_lock(&rtc_glink->lock);
> + ret = qcom_rtc_glink_request(rtc_glink, &msg, sizeof(msg));
> + if (!ret) {
> + if (!rtc_glink->resp_valid)
> + ret = -EIO;
> + else
> + *tm = rtc_glink->resp_tm;
> + }
Ditto.
> + mutex_unlock(&rtc_glink->lock);
> + return ret;
> +}
> +
> +static int qcom_rtc_glink_set_real_time(struct qcom_rtc_glink *rtc_glink,
> + struct rtc_time *tm)
> +{
> + struct qcom_rtc_glink_set_real_time_req msg = {};
> + u32 w0, w1;
> + int ret;
> +
> + w0 = ((tm->tm_year + 1900) & 0xffff) |
> + (((tm->tm_mon + 1) & 0xff) << 16) |
> + ((tm->tm_mday & 0xff) << 24);
Can't this be written as:
w0 = FIELD_PREP(0x0000ffff, tm->tm_year + 1900) |
FIELD_PREP(0x00ff0000, tm->tm_mod + 1) |
FIELD_PREP(0xff000000, tm->tm_mday);
> + w1 = (tm->tm_hour & 0xff) |
> + ((tm->tm_min & 0xff) << 8) |
> + ((tm->tm_sec & 0xff) << 16) |
> + (1U << 24);
> + msg.hdr.owner = cpu_to_le32(PMIC_GLINK_OWNER_RTC);
> + msg.hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP);
> + msg.hdr.opcode = cpu_to_le32(RTC_GLINK_SET_REAL_TIME);
> + msg.real_time_data[0] = cpu_to_le32(w0);
> + msg.real_time_data[1] = cpu_to_le32(w1);
> + msg.real_time_data[2] = 0;
> + msg.real_time_data[3] = 0;
> + dev_dbg(rtc_glink->dev,
> + "TX opcode=0x%x data=%08x %08x\n",
> + RTC_GLINK_SET_REAL_TIME, w0, w1);
> + mutex_lock(&rtc_glink->lock);
> + ret = qcom_rtc_glink_request(rtc_glink, &msg, sizeof(msg));
> + mutex_unlock(&rtc_glink->lock);
> + return ret;
It certainly wouldn't be bad to insert one or two empty lines in this 28
line block.
> +}
> +
> +static int qcom_rtc_glink_get_time(struct qcom_rtc_glink *rtc_glink,
> + struct rtc_time *time)
> +{
> + u32 ticks;
> + int ret;
> +
> + ret = qcom_rtc_glink_get_real_time(rtc_glink, time);
> + if (!ret)
> + return 0;
> + dev_warn(rtc_glink->dev, "0x68 failed (%d), falling back\n", ret);
0x68 failed you say...how about writing an error message that a human
can read?
> +
> + ret = qcom_rtc_glink_get_ticks(rtc_glink, &ticks);
> + if (!ret) {
> + rtc_time64_to_tm((time64_t)ticks + rtc_glink->offset, time);
> + return 0;
> + }
> +
> + ret = qcom_rtc_glink_get_property(rtc_glink, QCOM_RTC_GLINK_TIME,
> + &ticks);
Don't wrap lines when they are easier to read in their unwrapped form
and still less than 100 characters.
> + if (!ret && ticks != U32_MAX) {
> + rtc_time64_to_tm((time64_t)ticks + rtc_glink->offset, time);
> + return 0;
> + }
> +
> + dev_err(rtc_glink->dev, "all time sources failed\n");
> + return -EIO;
> +}
> +
> +static int qcom_rtc_glink_set_time(struct qcom_rtc_glink *rtc_glink,
> + struct rtc_time *time)
> +{
> + time64_t t = rtc_tm_to_time64(time);
> + u32 ticks;
> + int ret;
> +
> + if (!rtc_glink->allow_set_time)
> + return -EOPNOTSUPP;
> +
> + ret = qcom_rtc_glink_set_real_time(rtc_glink, time);
> + if (ret)
> + dev_warn(rtc_glink->dev, "0x69 failed (%d)\n", ret);
Write English error messages.
> +
> + ret = qcom_rtc_glink_set_property(rtc_glink, QCOM_RTC_GLINK_TIME, (u32)t);
> + if (ret)
> + return ret;
> +
> + ret = qcom_rtc_glink_get_ticks(rtc_glink, &ticks);
> + if (!ret && ticks != U32_MAX) {
> + rtc_glink->offset = (u32)t - ticks;
There's no reason to if (success) { dostuff(); } else {return -EIO; }.
Handle the error and then continue with the successful case.
> + } else {
> + dev_warn(rtc_glink->dev, "no tick source for offset\n");
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +static int qcom_rtc_glink_set_alarm_en(struct qcom_rtc_glink *rtc_glink,
> + int enabled)
> +{
> + return qcom_rtc_glink_set_property(rtc_glink,
> + QCOM_RTC_GLINK_ALARM_ENABLE,
> + enabled);
> +}
> +
> +static int qcom_rtc_glink_set_alarm(struct qcom_rtc_glink *rtc_glink,
> + struct rtc_wkalrm *alarm)
> +{
> + time64_t alarm_t = rtc_tm_to_time64(&alarm->time);
> + struct rtc_time now_tm;
> + time64_t now_real, secs_until_alarm;
> + u32 fw_current, fw_alarm;
> + int ret;
> +
> + ret = qcom_rtc_glink_get_time(rtc_glink, &now_tm);
> + if (ret)
> + return ret;
> + now_real = rtc_tm_to_time64(&now_tm);
> + secs_until_alarm = alarm_t - now_real;
> + dev_dbg(rtc_glink->dev,
> + "set_alarm: now_real=%lld alarm_t=%lld delta=%lld\n",
> + now_real, alarm_t, secs_until_alarm);
Here's 12 lines that get the time and calculate now and calculate alarm
time and print a debug message and checks for invalid value.
If you really need the debug print in here, give it a space before and
after to separate it from the actual logic.
> + if (secs_until_alarm < 0)
> + return -EINVAL;
> + if (secs_until_alarm > U32_MAX)
> + return -ERANGE;
> +
> + ret = qcom_rtc_glink_get_property(rtc_glink, QCOM_RTC_GLINK_TIME,
> + &fw_current);
Again, don't unnecessarily wrap lines that aren't too long.
> + if (ret)
> + return ret;
> + if (fw_current == U32_MAX)
> + fw_current = 0;
> + fw_alarm = fw_current + (u32)secs_until_alarm;
> + dev_dbg(rtc_glink->dev,
> + "set_alarm: fw_current=%u fw_alarm=%u\n",
> + fw_current, fw_alarm);
> +
> + ret = qcom_rtc_glink_set_alarm_en(rtc_glink, 0);
> + if (ret)
> + return ret;
> +
> + ret = qcom_rtc_glink_set_property(rtc_glink,
> + QCOM_RTC_GLINK_ALARM_TIME, fw_alarm);
> + if (ret)
> + return ret;
> +
> + return qcom_rtc_glink_set_alarm_en(rtc_glink, alarm->enabled);
> +}
> +
> +static int qcom_rtc_glink_get_alarm(struct qcom_rtc_glink *rtc_glink,
> + struct rtc_wkalrm *alarm)
> +{
> + u32 fw_alarm_time, fw_current, alarm_en;
> + struct rtc_time now_tm;
> + time64_t now_real, secs_until_alarm, alarm_real;
> + int ret;
> +
> + ret = qcom_rtc_glink_get_property(rtc_glink,
> + QCOM_RTC_GLINK_ALARM_TIME,
> + &fw_alarm_time);
> + if (ret)
> + return ret;
> +
> + ret = qcom_rtc_glink_get_property(rtc_glink, QCOM_RTC_GLINK_TIME,
> + &fw_current);
> + if (ret)
> + return ret;
> +
> + ret = qcom_rtc_glink_get_property(rtc_glink,
> + QCOM_RTC_GLINK_ALARM_ENABLE,
> + &alarm_en);
> + if (ret)
> + return ret;
> + if (fw_current == U32_MAX)
> + fw_current = 0;
> +
> + ret = qcom_rtc_glink_get_time(rtc_glink, &now_tm);
> + if (ret)
> + return ret;
> + now_real = rtc_tm_to_time64(&now_tm);
> + secs_until_alarm = (time64_t)fw_alarm_time - (time64_t)fw_current;
> + alarm_real = now_real + secs_until_alarm;
> + dev_dbg(rtc_glink->dev,
> + "get_alarm: now_real=%lld fw_current=%u fw_alarm=%u delta=%lld\n",
> + now_real, fw_current, fw_alarm_time, secs_until_alarm);
> + rtc_time64_to_tm(alarm_real, &alarm->time);
> + alarm->enabled = !!alarm_en;
> + return 0;
> +}
> +
> +static void qcom_rtc_glink_alarm_work(struct work_struct *work)
> +{
> + struct qcom_rtc_glink *rtc_glink =
> + container_of(work, struct qcom_rtc_glink, alarm_work);
> + int ret;
> +
> + ret = qcom_rtc_glink_set_alarm_en(rtc_glink, 0);
> + if (ret)
> + dev_err(rtc_glink->dev,
> + "failed to disable alarm after expiry (%d)\n", ret);
> + rtc_update_irq(rtc_glink->rtc, 1, RTC_IRQF | RTC_AF);
> +}
> +
> +static void qcom_rtc_glink_callback(const void *data, size_t len, void *priv)
> +{
> + struct qcom_rtc_glink *rtc_glink = priv;
> + const struct pmic_glink_hdr *hdr = data;
> + unsigned long flags;
> + bool pending;
> +
> + if (len < sizeof(*hdr))
> + return;
> +
> + dev_dbg(rtc_glink->dev,
> + "RX opcode=0x%x type=0x%x owner=0x%x len=%zu\n",
> + le32_to_cpu(hdr->opcode), le32_to_cpu(hdr->type),
> + le32_to_cpu(hdr->owner), len);
I don't fancy all your debug prints, but I love the fact that this isn't
crammed into logical chunks of code.
> +
> + if (le32_to_cpu(hdr->opcode) == RTC_GLINK_ALARM_EXPIRED) {
> + dev_info(rtc_glink->dev, "alarm expired\n");
Is this useful to the end user?
> + schedule_work(&rtc_glink->alarm_work);
> + return;
> + }
> +
> + /*
> + * Hold state_lock across the pending check and the response fields
> + * below: qcom_rtc_glink_pdr_notify() writes the same error/resp_*
> + * fields under this lock when it force-completes a request on an
> + * SSR down-transition, and the two must not race. Dropping the
> + * response here if !pending also covers requests that already
> + * timed out or were aborted; qcom_rtc_glink_request() will call
> + * reinit_completion() again before this response could be mistaken
> + * for a later request's.
> + */
> + spin_lock_irqsave(&rtc_glink->state_lock, flags);
> + pending = rtc_glink->request_pending;
> +
> + if (!pending) {
> + spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> + dev_dbg(rtc_glink->dev,
> + "dropping stale response opcode=0x%x\n",
> + le32_to_cpu(hdr->opcode));
> + return;
> + }
> +
> + switch (le32_to_cpu(hdr->opcode)) {
> + case RTC_GLINK_GET_PROPERTY:
> + fallthrough;
> + case RTC_GLINK_SET_PROPERTY: {
> + const struct qcom_rtc_glink_msg *msg = data;
> +
> + if (len < sizeof(struct pmic_glink_hdr) + sizeof(__le32)) {
> + rtc_glink->error = -EINVAL;
> + break;
> + }
> + rtc_glink->resp_value = le32_to_cpu(msg->property);
> + rtc_glink->resp_valid = true;
> + rtc_glink->error = 0;
> + dev_dbg(rtc_glink->dev, "ACK opcode=0x%x resp_value=%u\n",
> + le32_to_cpu(hdr->opcode), rtc_glink->resp_value);
> + break;
> + }
> +
> + case RTC_GLINK_GET_RTC_TICKS: {
> + const struct qcom_rtc_glink_ticks_resp *resp = data;
> +
> + if (len < sizeof(*resp)) {
> + rtc_glink->error = -EINVAL;
> + break;
> + }
> + if (le32_to_cpu(resp->return_status)) {
> + rtc_glink->error = -EIO;
> + break;
> + }
> + rtc_glink->resp_value = le32_to_cpu(resp->rtc_ticks);
> + rtc_glink->resp_valid = true;
> + rtc_glink->error = 0;
> + dev_dbg(rtc_glink->dev, "ACK opcode=0x%x ticks=%u\n",
> + RTC_GLINK_GET_RTC_TICKS, rtc_glink->resp_value);
> + break;
> + }
> +
> + case RTC_GLINK_GET_REAL_TIME: {
> + const struct qcom_rtc_glink_real_time_resp *resp = data;
> + u32 w0, w1;
> +
> + if (len < sizeof(*resp)) {
> + rtc_glink->error = -EINVAL;
> + break;
> + }
> + if (le32_to_cpu(resp->return_status)) {
> + rtc_glink->error = -EIO;
> + break;
> + }
> + w0 = le32_to_cpu(resp->real_time_data[0]);
> + w1 = le32_to_cpu(resp->real_time_data[1]);
> + rtc_glink->resp_tm.tm_year = (w0 & 0xffff) - 1900;
> + rtc_glink->resp_tm.tm_mon = ((w0 >> 16) & 0xff) - 1;
> + rtc_glink->resp_tm.tm_mday = (w0 >> 24) & 0xff;
> + rtc_glink->resp_tm.tm_hour = w1 & 0xff;
> + rtc_glink->resp_tm.tm_min = (w1 >> 8) & 0xff;
> + rtc_glink->resp_tm.tm_sec = (w1 >> 16) & 0xff;
Can't you FIELD_GET() these?
> + rtc_glink->resp_valid = true;
> + rtc_glink->error = 0;
> + dev_dbg(rtc_glink->dev,
> + "ACK opcode=0x%x %04d-%02d-%02d %02d:%02d:%02d\n",
> + RTC_GLINK_GET_REAL_TIME,
> + rtc_glink->resp_tm.tm_year + 1900,
> + rtc_glink->resp_tm.tm_mon + 1,
> + rtc_glink->resp_tm.tm_mday,
> + rtc_glink->resp_tm.tm_hour,
> + rtc_glink->resp_tm.tm_min,
> + rtc_glink->resp_tm.tm_sec);
> + break;
> + }
> +
> + case RTC_GLINK_SET_REAL_TIME: {
> + const struct qcom_rtc_glink_status_resp *resp = data;
> +
> + if (len < sizeof(*resp)) {
> + rtc_glink->error = -EINVAL;
> + break;
> + }
> + rtc_glink->error = le32_to_cpu(resp->return_status) ? -EIO : 0;
> + dev_dbg(rtc_glink->dev, "ACK opcode=0x%x status=%u\n",
> + RTC_GLINK_SET_REAL_TIME,
> + le32_to_cpu(resp->return_status));
> + break;
> + }
> +
> + default:
> + spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> + dev_warn(rtc_glink->dev,
> + "unhandled RX opcode=0x%x len=%zu\n",
> + le32_to_cpu(hdr->opcode), len);
> + return;
> + }
> +
> + spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> + complete(&rtc_glink->ack);
> +}
> +
> +static void qcom_rtc_glink_pdr_notify(void *priv, int state)
> +{
> + struct qcom_rtc_glink *rtc_glink = priv;
> + unsigned long flags;
> + bool up = (state == SERVREG_SERVICE_STATE_UP);
> +
> + spin_lock_irqsave(&rtc_glink->state_lock, flags);
> + rtc_glink->service_up = up;
> + if (!up && rtc_glink->request_pending) {
> + rtc_glink->error = -ECONNRESET;
> + rtc_glink->resp_valid = false;
> + complete(&rtc_glink->ack);
> + }
> + spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> +}
> +
> +static int qcom_rtc_glink_read_time(struct device *dev, struct rtc_time *tm)
> +{
> + return qcom_rtc_glink_get_time(dev_get_drvdata(dev), tm);
> +}
> +
> +static int qcom_rtc_glink_set_time_dev(struct device *dev, struct rtc_time *tm)
> +{
> + return qcom_rtc_glink_set_time(dev_get_drvdata(dev), tm);
> +}
> +
> +static int qcom_rtc_glink_read_alarm(struct device *dev,
> + struct rtc_wkalrm *alrm)
> +{
> + return qcom_rtc_glink_get_alarm(dev_get_drvdata(dev), alrm);
> +}
> +
> +static int qcom_rtc_glink_set_alarm_dev(struct device *dev,
> + struct rtc_wkalrm *alrm)
> +{
> + return qcom_rtc_glink_set_alarm(dev_get_drvdata(dev), alrm);
> +}
> +
> +static int qcom_rtc_glink_alarm_irq_enable(struct device *dev,
> + unsigned int enabled)
> +{
> + return qcom_rtc_glink_set_alarm_en(dev_get_drvdata(dev), enabled);
> +}
> +
> +static const struct rtc_class_ops qcom_rtc_glink_rtc_ops = {
> + .read_time = qcom_rtc_glink_read_time,
> + .set_time = qcom_rtc_glink_set_time_dev,
> + .read_alarm = qcom_rtc_glink_read_alarm,
> + .set_alarm = qcom_rtc_glink_set_alarm_dev,
> + .alarm_irq_enable = qcom_rtc_glink_alarm_irq_enable,
> +};
> +
> +static const struct of_device_id qcom_rtc_glink_of_variants[] = {
> + { .compatible = "qcom,glymur-pmic-glink" },
> + {}
> +};
> +
> +static int qcom_rtc_glink_probe(struct auxiliary_device *adev,
> + const struct auxiliary_device_id *id)
> +{
> + struct device *dev = &adev->dev;
> + struct qcom_rtc_glink *rtc_glink;
> +
> + if (!of_match_device(qcom_rtc_glink_of_variants, dev->parent))
> + return -ENXIO;
So in order to support this on a new target we should update both the
client_mask and qcom_rtc_glink_of_variants[]?
Why is this needed?
> +
> + rtc_glink = devm_kzalloc(dev, sizeof(*rtc_glink), GFP_KERNEL);
> + if (!rtc_glink)
> + return -ENOMEM;
> +
> + rtc_glink->dev = dev;
> + rtc_glink->allow_set_time = device_property_read_bool(dev->parent,
> + "allow-set-time");
Why do we have this property? Why would we not allow time to be set?
Regards,
Bjorn
> +
> + mutex_init(&rtc_glink->lock);
> + spin_lock_init(&rtc_glink->state_lock);
> + init_completion(&rtc_glink->ack);
> + INIT_WORK(&rtc_glink->alarm_work, qcom_rtc_glink_alarm_work);
> + dev_set_drvdata(dev, rtc_glink);
> +
> + rtc_glink->client = devm_pmic_glink_client_alloc(dev,
> + PMIC_GLINK_OWNER_RTC,
> + qcom_rtc_glink_callback,
> + qcom_rtc_glink_pdr_notify,
> + rtc_glink);
> + if (IS_ERR(rtc_glink->client))
> + return dev_err_probe(dev, PTR_ERR(rtc_glink->client),
> + "failed to allocate glink client\n");
> +
> + pmic_glink_client_register(rtc_glink->client);
> +
> + device_init_wakeup(dev, true);
> +
> + rtc_glink->rtc = devm_rtc_allocate_device(dev);
> + if (IS_ERR(rtc_glink->rtc))
> + return dev_err_probe(dev, PTR_ERR(rtc_glink->rtc),
> + "failed to allocate RTC device\n");
> +
> + rtc_glink->rtc->ops = &qcom_rtc_glink_rtc_ops;
> + rtc_glink->rtc->range_min = 0;
> + rtc_glink->rtc->range_max = U32_MAX;
> +
> + return devm_rtc_register_device(rtc_glink->rtc);
> +}
> +
> +static void qcom_rtc_glink_remove(struct auxiliary_device *adev)
> +{
> + struct qcom_rtc_glink *rtc_glink = dev_get_drvdata(&adev->dev);
> + /*
> + * Nothing left to arm alarm_work after this returns: the glink
> + * client (and its callback/pdr_notify) is torn down by devm after
> + * .remove() returns, and rtc_update_irq() below still targets a
> + * live devm-managed rtc device.
> + */
> + cancel_work_sync(&rtc_glink->alarm_work);
> +}
> +
> +static const struct auxiliary_device_id qcom_rtc_glink_id_table[] = {
> + { .name = "pmic_glink.rtc-glink" },
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(auxiliary, qcom_rtc_glink_id_table);
> +
> +static struct auxiliary_driver qcom_rtc_glink_driver = {
> + .name = "qcom_pmic_rtc_glink",
> + .id_table = qcom_rtc_glink_id_table,
> + .probe = qcom_rtc_glink_probe,
> + .remove = qcom_rtc_glink_remove,
> +};
> +
> +module_auxiliary_driver(qcom_rtc_glink_driver);
> +MODULE_DESCRIPTION("Qualcomm PMIC GLINK RTC driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
> index 3042261578aa..7ee70a31ba15 100644
> --- a/drivers/soc/qcom/pmic_glink.c
> +++ b/drivers/soc/qcom/pmic_glink.c
> @@ -21,6 +21,7 @@ enum {
> PMIC_GLINK_CLIENT_BATT = 0,
> PMIC_GLINK_CLIENT_ALTMODE,
> PMIC_GLINK_CLIENT_UCSI,
> + PMIC_GLINK_CLIENT_RTC,
> };
>
> struct pmic_glink_data {
> @@ -40,6 +41,7 @@ struct pmic_glink {
> struct auxiliary_device altmode_aux;
> struct auxiliary_device ps_aux;
> struct auxiliary_device ucsi_aux;
> + struct auxiliary_device rtc_aux;
>
> /* serializing client_state and pdr_state updates */
> struct mutex state_lock;
> @@ -341,6 +343,12 @@ static int pmic_glink_probe(struct platform_device *pdev)
> goto out_release_altmode_aux;
> }
>
> + if (pg->data->client_mask & BIT(PMIC_GLINK_CLIENT_RTC)) {
> + ret = pmic_glink_add_aux_device(pg, &pg->rtc_aux, "rtc-glink");
> + if (ret)
> + goto out_release_ps_aux;
> + }
> +
> if (pg->data->charger_pdr_service_name && pg->data->charger_pdr_service_path) {
> service = pdr_add_lookup(pg->pdr, pg->data->charger_pdr_service_name,
> pg->data->charger_pdr_service_path);
> @@ -358,6 +366,9 @@ static int pmic_glink_probe(struct platform_device *pdev)
> return 0;
>
> out_release_aux_devices:
> + if (pg->data->client_mask & BIT(PMIC_GLINK_CLIENT_RTC))
> + pmic_glink_del_aux_device(pg, &pg->rtc_aux);
> +out_release_ps_aux:
> if (pg->data->client_mask & BIT(PMIC_GLINK_CLIENT_BATT))
> pmic_glink_del_aux_device(pg, &pg->ps_aux);
> out_release_altmode_aux:
> @@ -384,11 +395,20 @@ static void pmic_glink_remove(struct platform_device *pdev)
> pmic_glink_del_aux_device(pg, &pg->altmode_aux);
> if (pg->data->client_mask & BIT(PMIC_GLINK_CLIENT_UCSI))
> pmic_glink_del_aux_device(pg, &pg->ucsi_aux);
> + if (pg->data->client_mask & BIT(PMIC_GLINK_CLIENT_RTC))
> + pmic_glink_del_aux_device(pg, &pg->rtc_aux);
>
> guard(mutex)(&__pmic_glink_lock);
> __pmic_glink = NULL;
> }
>
> +static const struct pmic_glink_data pmic_glink_glymur_data = {
> + .client_mask = BIT(PMIC_GLINK_CLIENT_BATT) |
> + BIT(PMIC_GLINK_CLIENT_ALTMODE) |
> + BIT(PMIC_GLINK_CLIENT_UCSI) |
> + BIT(PMIC_GLINK_CLIENT_RTC),
> +};
> +
> static const struct pmic_glink_data pmic_glink_adsp_data = {
> .client_mask = BIT(PMIC_GLINK_CLIENT_BATT) |
> BIT(PMIC_GLINK_CLIENT_ALTMODE) |
> @@ -404,7 +424,7 @@ static const struct pmic_glink_data pmic_glink_soccp_data = {
> };
>
> static const struct of_device_id pmic_glink_of_match[] = {
> - { .compatible = "qcom,glymur-pmic-glink", .data = &pmic_glink_soccp_data },
> + { .compatible = "qcom,glymur-pmic-glink", .data = &pmic_glink_glymur_data },
> { .compatible = "qcom,kaanapali-pmic-glink", .data = &pmic_glink_soccp_data },
> { .compatible = "qcom,pmic-glink", .data = &pmic_glink_adsp_data },
> {}
> diff --git a/include/linux/soc/qcom/pmic_glink.h b/include/linux/soc/qcom/pmic_glink.h
> index 7cddf1027752..11031b06b810 100644
> --- a/include/linux/soc/qcom/pmic_glink.h
> +++ b/include/linux/soc/qcom/pmic_glink.h
> @@ -11,6 +11,7 @@ struct pmic_glink_client;
> #define PMIC_GLINK_OWNER_BATTMGR 32778
> #define PMIC_GLINK_OWNER_USBC 32779
> #define PMIC_GLINK_OWNER_USBC_PAN 32780
> +#define PMIC_GLINK_OWNER_RTC 32784
>
> #define PMIC_GLINK_REQ_RESP 1
> #define PMIC_GLINK_NOTIFY 2
>
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 2/2] rtc: qcom: rtc-glink: Add PMIC GLINK RTC driver and extend pmic_glink
2026-08-06 20:56 ` [PATCH 2/2] rtc: qcom: rtc-glink: Add PMIC GLINK RTC driver and extend pmic_glink Kamal Wadhwa
2026-08-06 21:09 ` sashiko-bot
2026-08-06 23:53 ` Bjorn Andersson
@ 2026-08-07 20:58 ` Alexandre Belloni
2 siblings, 0 replies; 8+ messages in thread
From: Alexandre Belloni @ 2026-08-07 20:58 UTC (permalink / raw)
To: Kamal Wadhwa
Cc: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-kernel, linux-rtc, linux-arm-msm, devicetree,
Raj Aryan
On 07/08/2026 02:26:31+0530, Kamal Wadhwa wrote:
> Add a new RTC driver that communicates with the PMIC firmware over
> the GLINK transport layer. The driver supports reading time and
> alarm functionality via GET_PROPERTY and SET_PROPERTY opcodes.
>
> Time is read via GET_PROPERTY (opcode 0x62) using QCOM_RTC_GLINK_TIME
> property (0x00). Alarm set and enable are handled via SET_PROPERTY
> (opcode 0x63) using QCOM_RTC_GLINK_ALARM and QCOM_RTC_GLINK_ALARM_ENABLE
> properties respectively.
>
> GET_PROPERTY and SET_PROPERTY responses share the same firmware message
> layout, with the firmware echoing back the property value. Both opcodes
> are handled by a common callback path accordingly.
>
> Also extend pmic_glink to register the RTC as a PMIC GLINK client and
> add PMIC GLINK RTC client support for glymur.
>
> Assisted-by: Claude:claude-sonnet-5
> Signed-off-by: Raj Aryan <raryan@qti.qualcomm.com>
> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> ---
> MAINTAINERS | 7 +
> drivers/rtc/Kconfig | 10 +
> drivers/rtc/Makefile | 1 +
> drivers/rtc/rtc-qcom-glink.c | 690 ++++++++++++++++++++++++++++++++++++
> drivers/soc/qcom/pmic_glink.c | 22 +-
> include/linux/soc/qcom/pmic_glink.h | 1 +
> 6 files changed, 730 insertions(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5f178a5ff026..752cf667b8ad 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22505,6 +22505,13 @@ S: Maintained
> F: Documentation/devicetree/bindings/power/supply/qcom,pmi8998-charger.yaml
> F: drivers/power/supply/qcom_smbx.c
>
> +QUALCOMM PMIC GLINK RTC DRIVER
> +M: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> +L: linux-rtc@vger.kernel.org
> +L: linux-arm-msm@vger.kernel.org
> +S: Maintained
> +F: drivers/rtc/rtc-qcom-glink.c
> +
> QUALCOMM PPE DRIVER
> M: Luo Jie <jie.luo@oss.qualcomm.com>
> L: netdev@vger.kernel.org
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 01def8231873..3ac39704b2e1 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1836,6 +1836,16 @@ config RTC_DRV_PM8XXX
> To compile this driver as a module, choose M here: the
> module will be called rtc-pm8xxx.
>
> +config RTC_DRV_QCOM_GLINK
> + tristate "Qualcomm PMIC GLINK RTC"
> + depends on QCOM_PMIC_GLINK
> + help
> + Say Y here to enable support for the RTC exposed by the PMIC
> + over the GLINK transport on Qualcomm platforms.
> +
> + This driver can also be built as a module. If so, the module
> + will be called rtc-qcom-glink.
> +
> config RTC_DRV_TEGRA
> tristate "NVIDIA Tegra Internal RTC driver"
> depends on ARCH_TEGRA || COMPILE_TEST
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 0347645b021f..8215a1fc29fb 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -73,6 +73,7 @@ obj-$(CONFIG_RTC_DRV_FM3130) += rtc-fm3130.o
> obj-$(CONFIG_RTC_DRV_FSL_FTM_ALARM) += rtc-fsl-ftm-alarm.o
> obj-$(CONFIG_RTC_DRV_FTRTC010) += rtc-ftrtc010.o
> obj-$(CONFIG_RTC_DRV_GENERIC) += rtc-generic.o
> +obj-$(CONFIG_RTC_DRV_QCOM_GLINK) += rtc-qcom-glink.o
> obj-$(CONFIG_RTC_DRV_GOLDFISH) += rtc-goldfish.o
> obj-$(CONFIG_RTC_DRV_HID_SENSOR_TIME) += rtc-hid-sensor-time.o
> obj-$(CONFIG_RTC_DRV_HYM8563) += rtc-hym8563.o
> diff --git a/drivers/rtc/rtc-qcom-glink.c b/drivers/rtc/rtc-qcom-glink.c
> new file mode 100644
> index 000000000000..611f06a3cc5c
> --- /dev/null
> +++ b/drivers/rtc/rtc-qcom-glink.c
> @@ -0,0 +1,690 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +#include <linux/auxiliary_bus.h>
> +#include <linux/completion.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/property.h>
> +#include <linux/rtc.h>
> +#include <linux/slab.h>
> +#include <linux/soc/qcom/pdr.h>
> +#include <linux/soc/qcom/pmic_glink.h>
> +#include <linux/timekeeping.h>
> +#include <linux/workqueue.h>
> +
> +#define RTC_GLINK_SET_PROPERTY 0x63
> +#define RTC_GLINK_GET_PROPERTY 0x65
> +#define RTC_GLINK_GET_RTC_TICKS 0x67
> +#define RTC_GLINK_GET_REAL_TIME 0x68
> +#define RTC_GLINK_SET_REAL_TIME 0x69
> +#define RTC_GLINK_ALARM_EXPIRED 0x6A
> +
> +enum qcom_rtc_glink_properties {
> + QCOM_RTC_GLINK_TIME = 0,
> + QCOM_RTC_GLINK_ALARM_TIME,
> + QCOM_RTC_GLINK_ALARM_ENABLE,
> +};
> +
> +struct qcom_rtc_glink_msg {
> + struct pmic_glink_hdr hdr;
> + __le32 property;
> + __le32 value;
> +};
> +
> +struct qcom_rtc_glink_generic_req {
> + struct pmic_glink_hdr hdr;
> +};
> +
> +struct qcom_rtc_glink_status_resp {
> + struct pmic_glink_hdr hdr;
> + __le32 return_status;
> +};
> +
> +struct qcom_rtc_glink_ticks_resp {
> + struct pmic_glink_hdr hdr;
> + __le32 return_status;
> + __le32 rtc_ticks;
> +};
> +
> +struct qcom_rtc_glink_real_time_resp {
> + struct pmic_glink_hdr hdr;
> + __le32 return_status;
> + __le32 real_time_data[4];
> +};
> +
> +struct qcom_rtc_glink_set_real_time_req {
> + struct pmic_glink_hdr hdr;
> + __le32 real_time_data[4];
> +};
> +
> +struct qcom_rtc_glink {
> + struct device *dev;
> + struct pmic_glink_client *client;
> + struct rtc_device *rtc;
> +
> + /* Serializes requests: only one may be in flight at a time */
> + struct mutex lock;
This mutex is useless as all the RTC operations are already serialized.
Simply rtc_lock/rtc_unlock in qcom_rtc_glink_alarm_work.
> + struct completion ack;
> + struct work_struct alarm_work;
> +
> + /* Protects service_up and request_pending across callback contexts */
> + spinlock_t state_lock;
> + bool service_up;
> + bool request_pending;
> +
> + int error;
> + u32 resp_value;
> + u32 offset;
> + struct rtc_time resp_tm;
> + bool resp_valid;
> + bool allow_set_time;
> +};
> +
> +static int qcom_rtc_glink_request(struct qcom_rtc_glink *rtc_glink,
> + void *data, size_t len)
> +{
> + unsigned long flags;
> + unsigned long left;
> + int ret;
> +
> + /*
> + * Reinit the completion before request_pending becomes visible to
> + * qcom_rtc_glink_pdr_tify(), so a concurrent SSR down-transition
> + * can't complete() a stale completion that gets wiped out by
> + * reinit_completion() right after.
> + */
> + reinit_completion(&rtc_glink->ack);
> + rtc_glink->error = 0;
> + rtc_glink->resp_valid = false;
> +
> + spin_lock_irqsave(&rtc_glink->state_lock, flags);
> + if (!rtc_glink->service_up) {
> + spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> + return -ECONNRESET;
> + }
> + rtc_glink->request_pending = true;
> + spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> +
> + ret = pmic_glink_send(rtc_glink->client, data, len);
> + if (ret < 0) {
> + spin_lock_irqsave(&rtc_glink->state_lock, flags);
> + rtc_glink->request_pending = false;
> + spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> + return ret;
> + }
> +
> + left = wait_for_completion_timeout(&rtc_glink->ack, HZ);
> + spin_lock_irqsave(&rtc_glink->state_lock, flags);
> + rtc_glink->request_pending = false;
> + spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> + if (!left)
> + return -ETIMEDOUT;
> +
> + return rtc_glink->error;
> +}
> +
> +static int qcom_rtc_glink_set_property(struct qcom_rtc_glink *rtc_glink,
> + u32 property, u32 value)
> +{
> + struct qcom_rtc_glink_msg msg = {
> + .hdr.owner = cpu_to_le32(PMIC_GLINK_OWNER_RTC),
> + .hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP),
> + .hdr.opcode = cpu_to_le32(RTC_GLINK_SET_PROPERTY),
> + .property = cpu_to_le32(property),
> + .value = cpu_to_le32(value),
> + };
> + int ret;
> +
> + dev_dbg(rtc_glink->dev,
> + "TX opcode=0x%x property=%u value=%u\n",
> + RTC_GLINK_SET_PROPERTY, property, value);
> + mutex_lock(&rtc_glink->lock);
> + ret = qcom_rtc_glink_request(rtc_glink, &msg, sizeof(msg));
> + mutex_unlock(&rtc_glink->lock);
> + return ret;
> +}
> +
> +static int qcom_rtc_glink_get_property(struct qcom_rtc_glink *rtc_glink,
> + u32 property, u32 *value)
> +{
> + struct qcom_rtc_glink_msg msg = {
> + .hdr.owner = cpu_to_le32(PMIC_GLINK_OWNER_RTC),
> + .hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP),
> + .hdr.opcode = cpu_to_le32(RTC_GLINK_GET_PROPERTY),
> + .property = cpu_to_le32(property),
> + };
> + int ret;
> +
> + dev_dbg(rtc_glink->dev,
> + "TX opcode=0x%x property=%u\n",
> + RTC_GLINK_GET_PROPERTY, property);
> + mutex_lock(&rtc_glink->lock);
> + ret = qcom_rtc_glink_request(rtc_glink, &msg, sizeof(msg));
> + if (!ret) {
> + if (!rtc_glink->resp_valid)
> + ret = -EIO;
> + else
> + *value = rtc_glink->resp_value;
> + }
> + mutex_unlock(&rtc_glink->lock);
> + return ret;
> +}
> +
> +static int qcom_rtc_glink_get_ticks(struct qcom_rtc_glink *rtc_glink,
> + u32 *ticks)
> +{
> + struct qcom_rtc_glink_generic_req msg = {
> + .hdr.owner = cpu_to_le32(PMIC_GLINK_OWNER_RTC),
> + .hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP),
> + .hdr.opcode = cpu_to_le32(RTC_GLINK_GET_RTC_TICKS),
> + };
> + int ret;
> +
> + dev_dbg(rtc_glink->dev, "TX opcode=0x%x\n", RTC_GLINK_GET_RTC_TICKS);
> + mutex_lock(&rtc_glink->lock);
> + ret = qcom_rtc_glink_request(rtc_glink, &msg, sizeof(msg));
> + if (!ret) {
> + if (!rtc_glink->resp_valid)
> + ret = -EIO;
> + else
> + *ticks = rtc_glink->resp_value;
> + }
> + mutex_unlock(&rtc_glink->lock);
> + return ret;
> +}
> +
> +static int qcom_rtc_glink_get_real_time(struct qcom_rtc_glink *rtc_glink,
> + struct rtc_time *tm)
> +{
> + struct qcom_rtc_glink_generic_req msg = {
> + .hdr.owner = cpu_to_le32(PMIC_GLINK_OWNER_RTC),
> + .hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP),
> + .hdr.opcode = cpu_to_le32(RTC_GLINK_GET_REAL_TIME),
> + };
> + int ret;
> +
> + dev_dbg(rtc_glink->dev, "TX opcode=0x%x\n", RTC_GLINK_GET_REAL_TIME);
> + mutex_lock(&rtc_glink->lock);
> + ret = qcom_rtc_glink_request(rtc_glink, &msg, sizeof(msg));
> + if (!ret) {
> + if (!rtc_glink->resp_valid)
> + ret = -EIO;
> + else
> + *tm = rtc_glink->resp_tm;
> + }
> + mutex_unlock(&rtc_glink->lock);
> + return ret;
> +}
> +
> +static int qcom_rtc_glink_set_real_time(struct qcom_rtc_glink *rtc_glink,
> + struct rtc_time *tm)
> +{
> + struct qcom_rtc_glink_set_real_time_req msg = {};
> + u32 w0, w1;
> + int ret;
> +
> + w0 = ((tm->tm_year + 1900) & 0xffff) |
> + (((tm->tm_mon + 1) & 0xff) << 16) |
> + ((tm->tm_mday & 0xff) << 24);
> + w1 = (tm->tm_hour & 0xff) |
> + ((tm->tm_min & 0xff) << 8) |
> + ((tm->tm_sec & 0xff) << 16) |
> + (1U << 24);
> + msg.hdr.owner = cpu_to_le32(PMIC_GLINK_OWNER_RTC);
> + msg.hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP);
> + msg.hdr.opcode = cpu_to_le32(RTC_GLINK_SET_REAL_TIME);
> + msg.real_time_data[0] = cpu_to_le32(w0);
> + msg.real_time_data[1] = cpu_to_le32(w1);
> + msg.real_time_data[2] = 0;
> + msg.real_time_data[3] = 0;
> + dev_dbg(rtc_glink->dev,
> + "TX opcode=0x%x data=%08x %08x\n",
> + RTC_GLINK_SET_REAL_TIME, w0, w1);
> + mutex_lock(&rtc_glink->lock);
> + ret = qcom_rtc_glink_request(rtc_glink, &msg, sizeof(msg));
> + mutex_unlock(&rtc_glink->lock);
> + return ret;
> +}
> +
> +static int qcom_rtc_glink_get_time(struct qcom_rtc_glink *rtc_glink,
> + struct rtc_time *time)
> +{
> + u32 ticks;
> + int ret;
> +
> + ret = qcom_rtc_glink_get_real_time(rtc_glink, time);
> + if (!ret)
> + return 0;
> + dev_warn(rtc_glink->dev, "0x68 failed (%d), falling back\n", ret);
This message is useless, either fail or fallback silently
> +
> + ret = qcom_rtc_glink_get_ticks(rtc_glink, &ticks);
> + if (!ret) {
> + rtc_time64_to_tm((time64_t)ticks + rtc_glink->offset, time);
> + return 0;
> + }
> +
> + ret = qcom_rtc_glink_get_property(rtc_glink, QCOM_RTC_GLINK_TIME,
> + &ticks);
> + if (!ret && ticks != U32_MAX) {
> + rtc_time64_to_tm((time64_t)ticks + rtc_glink->offset, time);
> + return 0;
> + }
> +
> + dev_err(rtc_glink->dev, "all time sources failed\n");
Ditto
> + return -EIO;
> +}
> +
> +static int qcom_rtc_glink_set_time(struct qcom_rtc_glink *rtc_glink,
> + struct rtc_time *time)
> +{
> + time64_t t = rtc_tm_to_time64(time);
> + u32 ticks;
> + int ret;
> +
> + if (!rtc_glink->allow_set_time)
> + return -EOPNOTSUPP;
> +
> + ret = qcom_rtc_glink_set_real_time(rtc_glink, time);
> + if (ret)
> + dev_warn(rtc_glink->dev, "0x69 failed (%d)\n", ret);
Ditto
> +
> + ret = qcom_rtc_glink_set_property(rtc_glink, QCOM_RTC_GLINK_TIME, (u32)t);
> + if (ret)
> + return ret;
> +
> + ret = qcom_rtc_glink_get_ticks(rtc_glink, &ticks);
> + if (!ret && ticks != U32_MAX) {
> + rtc_glink->offset = (u32)t - ticks;
> + } else {
> + dev_warn(rtc_glink->dev, "no tick source for offset\n");
This is also probably useless. No one is going to read those messages or
do anything about them.
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +static int qcom_rtc_glink_set_alarm_en(struct qcom_rtc_glink *rtc_glink,
> + int enabled)
> +{
> + return qcom_rtc_glink_set_property(rtc_glink,
> + QCOM_RTC_GLINK_ALARM_ENABLE,
> + enabled);
> +}
> +
> +static int qcom_rtc_glink_set_alarm(struct qcom_rtc_glink *rtc_glink,
> + struct rtc_wkalrm *alarm)
> +{
> + time64_t alarm_t = rtc_tm_to_time64(&alarm->time);
> + struct rtc_time now_tm;
> + time64_t now_real, secs_until_alarm;
> + u32 fw_current, fw_alarm;
> + int ret;
> +
> + ret = qcom_rtc_glink_get_time(rtc_glink, &now_tm);
> + if (ret)
> + return ret;
> + now_real = rtc_tm_to_time64(&now_tm);
> + secs_until_alarm = alarm_t - now_real;
> + dev_dbg(rtc_glink->dev,
> + "set_alarm: now_real=%lld alarm_t=%lld delta=%lld\n",
> + now_real, alarm_t, secs_until_alarm);
> + if (secs_until_alarm < 0)
> + return -EINVAL;
> + if (secs_until_alarm > U32_MAX)
> + return -ERANGE;
> +
> + ret = qcom_rtc_glink_get_property(rtc_glink, QCOM_RTC_GLINK_TIME,
> + &fw_current);
> + if (ret)
> + return ret;
> + if (fw_current == U32_MAX)
> + fw_current = 0;
> + fw_alarm = fw_current + (u32)secs_until_alarm;
> + dev_dbg(rtc_glink->dev,
> + "set_alarm: fw_current=%u fw_alarm=%u\n",
> + fw_current, fw_alarm);
> +
> + ret = qcom_rtc_glink_set_alarm_en(rtc_glink, 0);
> + if (ret)
> + return ret;
> +
> + ret = qcom_rtc_glink_set_property(rtc_glink,
> + QCOM_RTC_GLINK_ALARM_TIME, fw_alarm);
> + if (ret)
> + return ret;
> +
> + return qcom_rtc_glink_set_alarm_en(rtc_glink, alarm->enabled);
> +}
> +
> +static int qcom_rtc_glink_get_alarm(struct qcom_rtc_glink *rtc_glink,
> + struct rtc_wkalrm *alarm)
> +{
> + u32 fw_alarm_time, fw_current, alarm_en;
> + struct rtc_time now_tm;
> + time64_t now_real, secs_until_alarm, alarm_real;
> + int ret;
> +
> + ret = qcom_rtc_glink_get_property(rtc_glink,
> + QCOM_RTC_GLINK_ALARM_TIME,
> + &fw_alarm_time);
> + if (ret)
> + return ret;
> +
> + ret = qcom_rtc_glink_get_property(rtc_glink, QCOM_RTC_GLINK_TIME,
> + &fw_current);
> + if (ret)
> + return ret;
> +
> + ret = qcom_rtc_glink_get_property(rtc_glink,
> + QCOM_RTC_GLINK_ALARM_ENABLE,
> + &alarm_en);
> + if (ret)
> + return ret;
> + if (fw_current == U32_MAX)
> + fw_current = 0;
> +
> + ret = qcom_rtc_glink_get_time(rtc_glink, &now_tm);
> + if (ret)
> + return ret;
> + now_real = rtc_tm_to_time64(&now_tm);
> + secs_until_alarm = (time64_t)fw_alarm_time - (time64_t)fw_current;
> + alarm_real = now_real + secs_until_alarm;
> + dev_dbg(rtc_glink->dev,
> + "get_alarm: now_real=%lld fw_current=%u fw_alarm=%u delta=%lld\n",
> + now_real, fw_current, fw_alarm_time, secs_until_alarm);
> + rtc_time64_to_tm(alarm_real, &alarm->time);
> + alarm->enabled = !!alarm_en;
> + return 0;
> +}
> +
> +static void qcom_rtc_glink_alarm_work(struct work_struct *work)
> +{
> + struct qcom_rtc_glink *rtc_glink =
> + container_of(work, struct qcom_rtc_glink, alarm_work);
> + int ret;
> +
> + ret = qcom_rtc_glink_set_alarm_en(rtc_glink, 0);
> + if (ret)
> + dev_err(rtc_glink->dev,
> + "failed to disable alarm after expiry (%d)\n", ret);
So what is the user supposed to do with this message? Also, do you
actually have to disable the alarm? I guess it is not going to fire again.
> + rtc_update_irq(rtc_glink->rtc, 1, RTC_IRQF | RTC_AF);
> +}
> +
> +static void qcom_rtc_glink_callback(const void *data, size_t len, void *priv)
> +{
> + struct qcom_rtc_glink *rtc_glink = priv;
> + const struct pmic_glink_hdr *hdr = data;
> + unsigned long flags;
> + bool pending;
> +
> + if (len < sizeof(*hdr))
> + return;
> +
> + dev_dbg(rtc_glink->dev,
> + "RX opcode=0x%x type=0x%x owner=0x%x len=%zu\n",
> + le32_to_cpu(hdr->opcode), le32_to_cpu(hdr->type),
> + le32_to_cpu(hdr->owner), len);
> +
> + if (le32_to_cpu(hdr->opcode) == RTC_GLINK_ALARM_EXPIRED) {
> + dev_info(rtc_glink->dev, "alarm expired\n");
This is just noise
> + schedule_work(&rtc_glink->alarm_work);
> + return;
> + }
> +
> + /*
> + * Hold state_lock across the pending check and the response fields
> + * below: qcom_rtc_glink_pdr_notify() writes the same error/resp_*
> + * fields under this lock when it force-completes a request on an
> + * SSR down-transition, and the two must not race. Dropping the
> + * response here if !pending also covers requests that already
> + * timed out or were aborted; qcom_rtc_glink_request() will call
> + * reinit_completion() again before this response could be mistaken
> + * for a later request's.
> + */
> + spin_lock_irqsave(&rtc_glink->state_lock, flags);
> + pending = rtc_glink->request_pending;
> +
> + if (!pending) {
> + spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> + dev_dbg(rtc_glink->dev,
> + "dropping stale response opcode=0x%x\n",
> + le32_to_cpu(hdr->opcode));
> + return;
> + }
> +
> + switch (le32_to_cpu(hdr->opcode)) {
> + case RTC_GLINK_GET_PROPERTY:
> + fallthrough;
> + case RTC_GLINK_SET_PROPERTY: {
> + const struct qcom_rtc_glink_msg *msg = data;
> +
> + if (len < sizeof(struct pmic_glink_hdr) + sizeof(__le32)) {
> + rtc_glink->error = -EINVAL;
> + break;
> + }
> + rtc_glink->resp_value = le32_to_cpu(msg->property);
> + rtc_glink->resp_valid = true;
> + rtc_glink->error = 0;
> + dev_dbg(rtc_glink->dev, "ACK opcode=0x%x resp_value=%u\n",
> + le32_to_cpu(hdr->opcode), rtc_glink->resp_value);
> + break;
> + }
> +
> + case RTC_GLINK_GET_RTC_TICKS: {
> + const struct qcom_rtc_glink_ticks_resp *resp = data;
> +
> + if (len < sizeof(*resp)) {
> + rtc_glink->error = -EINVAL;
> + break;
> + }
> + if (le32_to_cpu(resp->return_status)) {
> + rtc_glink->error = -EIO;
> + break;
> + }
> + rtc_glink->resp_value = le32_to_cpu(resp->rtc_ticks);
> + rtc_glink->resp_valid = true;
> + rtc_glink->error = 0;
> + dev_dbg(rtc_glink->dev, "ACK opcode=0x%x ticks=%u\n",
> + RTC_GLINK_GET_RTC_TICKS, rtc_glink->resp_value);
> + break;
> + }
> +
> + case RTC_GLINK_GET_REAL_TIME: {
> + const struct qcom_rtc_glink_real_time_resp *resp = data;
> + u32 w0, w1;
> +
> + if (len < sizeof(*resp)) {
> + rtc_glink->error = -EINVAL;
> + break;
> + }
> + if (le32_to_cpu(resp->return_status)) {
> + rtc_glink->error = -EIO;
> + break;
> + }
> + w0 = le32_to_cpu(resp->real_time_data[0]);
> + w1 = le32_to_cpu(resp->real_time_data[1]);
> + rtc_glink->resp_tm.tm_year = (w0 & 0xffff) - 1900;
> + rtc_glink->resp_tm.tm_mon = ((w0 >> 16) & 0xff) - 1;
> + rtc_glink->resp_tm.tm_mday = (w0 >> 24) & 0xff;
> + rtc_glink->resp_tm.tm_hour = w1 & 0xff;
> + rtc_glink->resp_tm.tm_min = (w1 >> 8) & 0xff;
> + rtc_glink->resp_tm.tm_sec = (w1 >> 16) & 0xff;
> + rtc_glink->resp_valid = true;
> + rtc_glink->error = 0;
> + dev_dbg(rtc_glink->dev,
> + "ACK opcode=0x%x %04d-%02d-%02d %02d:%02d:%02d\n",
> + RTC_GLINK_GET_REAL_TIME,
> + rtc_glink->resp_tm.tm_year + 1900,
> + rtc_glink->resp_tm.tm_mon + 1,
> + rtc_glink->resp_tm.tm_mday,
> + rtc_glink->resp_tm.tm_hour,
> + rtc_glink->resp_tm.tm_min,
> + rtc_glink->resp_tm.tm_sec);
> + break;
> + }
> +
> + case RTC_GLINK_SET_REAL_TIME: {
> + const struct qcom_rtc_glink_status_resp *resp = data;
> +
> + if (len < sizeof(*resp)) {
> + rtc_glink->error = -EINVAL;
> + break;
> + }
> + rtc_glink->error = le32_to_cpu(resp->return_status) ? -EIO : 0;
> + dev_dbg(rtc_glink->dev, "ACK opcode=0x%x status=%u\n",
> + RTC_GLINK_SET_REAL_TIME,
> + le32_to_cpu(resp->return_status));
> + break;
> + }
> +
> + default:
> + spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> + dev_warn(rtc_glink->dev,
> + "unhandled RX opcode=0x%x len=%zu\n",
> + le32_to_cpu(hdr->opcode), len);
Useless message
> + return;
> + }
> +
> + spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> + complete(&rtc_glink->ack);
> +}
> +
> +static void qcom_rtc_glink_pdr_notify(void *priv, int state)
> +{
> + struct qcom_rtc_glink *rtc_glink = priv;
> + unsigned long flags;
> + bool up = (state == SERVREG_SERVICE_STATE_UP);
> +
> + spin_lock_irqsave(&rtc_glink->state_lock, flags);
> + rtc_glink->service_up = up;
> + if (!up && rtc_glink->request_pending) {
> + rtc_glink->error = -ECONNRESET;
> + rtc_glink->resp_valid = false;
> + complete(&rtc_glink->ack);
> + }
> + spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> +}
> +
> +static int qcom_rtc_glink_read_time(struct device *dev, struct rtc_time *tm)
> +{
> + return qcom_rtc_glink_get_time(dev_get_drvdata(dev), tm);
> +}
> +
> +static int qcom_rtc_glink_set_time_dev(struct device *dev, struct rtc_time *tm)
> +{
> + return qcom_rtc_glink_set_time(dev_get_drvdata(dev), tm);
> +}
> +
> +static int qcom_rtc_glink_read_alarm(struct device *dev,
> + struct rtc_wkalrm *alrm)
> +{
> + return qcom_rtc_glink_get_alarm(dev_get_drvdata(dev), alrm);
> +}
> +
> +static int qcom_rtc_glink_set_alarm_dev(struct device *dev,
> + struct rtc_wkalrm *alrm)
> +{
> + return qcom_rtc_glink_set_alarm(dev_get_drvdata(dev), alrm);
> +}
> +
> +static int qcom_rtc_glink_alarm_irq_enable(struct device *dev,
> + unsigned int enabled)
> +{
> + return qcom_rtc_glink_set_alarm_en(dev_get_drvdata(dev), enabled);
> +}
> +
> +static const struct rtc_class_ops qcom_rtc_glink_rtc_ops = {
> + .read_time = qcom_rtc_glink_read_time,
> + .set_time = qcom_rtc_glink_set_time_dev,
> + .read_alarm = qcom_rtc_glink_read_alarm,
> + .set_alarm = qcom_rtc_glink_set_alarm_dev,
> + .alarm_irq_enable = qcom_rtc_glink_alarm_irq_enable,
> +};
> +
> +static const struct of_device_id qcom_rtc_glink_of_variants[] = {
> + { .compatible = "qcom,glymur-pmic-glink" },
> + {}
> +};
> +
> +static int qcom_rtc_glink_probe(struct auxiliary_device *adev,
> + const struct auxiliary_device_id *id)
> +{
> + struct device *dev = &adev->dev;
> + struct qcom_rtc_glink *rtc_glink;
> +
> + if (!of_match_device(qcom_rtc_glink_of_variants, dev->parent))
> + return -ENXIO;
> +
> + rtc_glink = devm_kzalloc(dev, sizeof(*rtc_glink), GFP_KERNEL);
> + if (!rtc_glink)
> + return -ENOMEM;
> +
> + rtc_glink->dev = dev;
> + rtc_glink->allow_set_time = device_property_read_bool(dev->parent,
> + "allow-set-time");
> +
> + mutex_init(&rtc_glink->lock);
> + spin_lock_init(&rtc_glink->state_lock);
> + init_completion(&rtc_glink->ack);
> + INIT_WORK(&rtc_glink->alarm_work, qcom_rtc_glink_alarm_work);
> + dev_set_drvdata(dev, rtc_glink);
> +
> + rtc_glink->client = devm_pmic_glink_client_alloc(dev,
> + PMIC_GLINK_OWNER_RTC,
> + qcom_rtc_glink_callback,
> + qcom_rtc_glink_pdr_notify,
> + rtc_glink);
> + if (IS_ERR(rtc_glink->client))
> + return dev_err_probe(dev, PTR_ERR(rtc_glink->client),
> + "failed to allocate glink client\n");
> +
> + pmic_glink_client_register(rtc_glink->client);
> +
> + device_init_wakeup(dev, true);
> +
> + rtc_glink->rtc = devm_rtc_allocate_device(dev);
> + if (IS_ERR(rtc_glink->rtc))
> + return dev_err_probe(dev, PTR_ERR(rtc_glink->rtc),
> + "failed to allocate RTC device\n");
> +
Useless message, there is no failure path without a message in
devm_rtc_allocate_device.
> + rtc_glink->rtc->ops = &qcom_rtc_glink_rtc_ops;
> + rtc_glink->rtc->range_min = 0;
> + rtc_glink->rtc->range_max = U32_MAX;
> +
> + return devm_rtc_register_device(rtc_glink->rtc);
> +}
> +
> +static void qcom_rtc_glink_remove(struct auxiliary_device *adev)
> +{
> + struct qcom_rtc_glink *rtc_glink = dev_get_drvdata(&adev->dev);
> + /*
> + * Nothing left to arm alarm_work after this returns: the glink
> + * client (and its callback/pdr_notify) is torn down by devm after
> + * .remove() returns, and rtc_update_irq() below still targets a
> + * live devm-managed rtc device.
> + */
> + cancel_work_sync(&rtc_glink->alarm_work);
> +}
> +
> +static const struct auxiliary_device_id qcom_rtc_glink_id_table[] = {
> + { .name = "pmic_glink.rtc-glink" },
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(auxiliary, qcom_rtc_glink_id_table);
> +
> +static struct auxiliary_driver qcom_rtc_glink_driver = {
> + .name = "qcom_pmic_rtc_glink",
> + .id_table = qcom_rtc_glink_id_table,
> + .probe = qcom_rtc_glink_probe,
> + .remove = qcom_rtc_glink_remove,
> +};
> +
> +module_auxiliary_driver(qcom_rtc_glink_driver);
> +MODULE_DESCRIPTION("Qualcomm PMIC GLINK RTC driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
> index 3042261578aa..7ee70a31ba15 100644
> --- a/drivers/soc/qcom/pmic_glink.c
> +++ b/drivers/soc/qcom/pmic_glink.c
> @@ -21,6 +21,7 @@ enum {
> PMIC_GLINK_CLIENT_BATT = 0,
> PMIC_GLINK_CLIENT_ALTMODE,
> PMIC_GLINK_CLIENT_UCSI,
> + PMIC_GLINK_CLIENT_RTC,
> };
>
> struct pmic_glink_data {
> @@ -40,6 +41,7 @@ struct pmic_glink {
> struct auxiliary_device altmode_aux;
> struct auxiliary_device ps_aux;
> struct auxiliary_device ucsi_aux;
> + struct auxiliary_device rtc_aux;
>
> /* serializing client_state and pdr_state updates */
> struct mutex state_lock;
> @@ -341,6 +343,12 @@ static int pmic_glink_probe(struct platform_device *pdev)
> goto out_release_altmode_aux;
> }
>
> + if (pg->data->client_mask & BIT(PMIC_GLINK_CLIENT_RTC)) {
> + ret = pmic_glink_add_aux_device(pg, &pg->rtc_aux, "rtc-glink");
> + if (ret)
> + goto out_release_ps_aux;
> + }
> +
> if (pg->data->charger_pdr_service_name && pg->data->charger_pdr_service_path) {
> service = pdr_add_lookup(pg->pdr, pg->data->charger_pdr_service_name,
> pg->data->charger_pdr_service_path);
> @@ -358,6 +366,9 @@ static int pmic_glink_probe(struct platform_device *pdev)
> return 0;
>
> out_release_aux_devices:
> + if (pg->data->client_mask & BIT(PMIC_GLINK_CLIENT_RTC))
> + pmic_glink_del_aux_device(pg, &pg->rtc_aux);
> +out_release_ps_aux:
> if (pg->data->client_mask & BIT(PMIC_GLINK_CLIENT_BATT))
> pmic_glink_del_aux_device(pg, &pg->ps_aux);
> out_release_altmode_aux:
> @@ -384,11 +395,20 @@ static void pmic_glink_remove(struct platform_device *pdev)
> pmic_glink_del_aux_device(pg, &pg->altmode_aux);
> if (pg->data->client_mask & BIT(PMIC_GLINK_CLIENT_UCSI))
> pmic_glink_del_aux_device(pg, &pg->ucsi_aux);
> + if (pg->data->client_mask & BIT(PMIC_GLINK_CLIENT_RTC))
> + pmic_glink_del_aux_device(pg, &pg->rtc_aux);
>
> guard(mutex)(&__pmic_glink_lock);
> __pmic_glink = NULL;
> }
>
> +static const struct pmic_glink_data pmic_glink_glymur_data = {
> + .client_mask = BIT(PMIC_GLINK_CLIENT_BATT) |
> + BIT(PMIC_GLINK_CLIENT_ALTMODE) |
> + BIT(PMIC_GLINK_CLIENT_UCSI) |
> + BIT(PMIC_GLINK_CLIENT_RTC),
> +};
> +
> static const struct pmic_glink_data pmic_glink_adsp_data = {
> .client_mask = BIT(PMIC_GLINK_CLIENT_BATT) |
> BIT(PMIC_GLINK_CLIENT_ALTMODE) |
> @@ -404,7 +424,7 @@ static const struct pmic_glink_data pmic_glink_soccp_data = {
> };
>
> static const struct of_device_id pmic_glink_of_match[] = {
> - { .compatible = "qcom,glymur-pmic-glink", .data = &pmic_glink_soccp_data },
> + { .compatible = "qcom,glymur-pmic-glink", .data = &pmic_glink_glymur_data },
> { .compatible = "qcom,kaanapali-pmic-glink", .data = &pmic_glink_soccp_data },
> { .compatible = "qcom,pmic-glink", .data = &pmic_glink_adsp_data },
> {}
> diff --git a/include/linux/soc/qcom/pmic_glink.h b/include/linux/soc/qcom/pmic_glink.h
> index 7cddf1027752..11031b06b810 100644
> --- a/include/linux/soc/qcom/pmic_glink.h
> +++ b/include/linux/soc/qcom/pmic_glink.h
> @@ -11,6 +11,7 @@ struct pmic_glink_client;
> #define PMIC_GLINK_OWNER_BATTMGR 32778
> #define PMIC_GLINK_OWNER_USBC 32779
> #define PMIC_GLINK_OWNER_USBC_PAN 32780
> +#define PMIC_GLINK_OWNER_RTC 32784
>
> #define PMIC_GLINK_REQ_RESP 1
> #define PMIC_GLINK_NOTIFY 2
>
> --
> 2.43.0
>
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 8+ messages in thread