From: Alexandre Belloni <alexandre.belloni@bootlin.com>
To: Shubhi Garg <shgarg@nvidia.com>
Cc: Lee Jones <lee@kernel.org>, Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzk+dt@kernel.org>,
Conor Dooley <conor+dt@kernel.org>,
Catalin Marinas <catalin.marinas@arm.com>,
Will Deacon <will@kernel.org>,
Jonathan Hunter <jonathanh@nvidia.com>,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-rtc@vger.kernel.org, linux-tegra@vger.kernel.org
Subject: Re: [PATCH v6 3/4] rtc: nvvrs: add NVIDIA VRS RTC device driver
Date: Tue, 23 Sep 2025 17:40:48 +0200 [thread overview]
Message-ID: <202509231540480daf8b56@mail.local> (raw)
In-Reply-To: <20250919140229.10546-1-shgarg@nvidia.com>
On 19/09/2025 14:02:29+0000, Shubhi Garg wrote:
> +static int nvvrs_rtc_enable_alarm(struct nvvrs_rtc_info *info)
> +{
> + int ret;
> +
> + /* Set RTC_WAKE bit for autonomous wake from sleep */
> + ret = nvvrs_update_bits(info, NVVRS_REG_CTL_2, NVVRS_REG_CTL_2_RTC_WAKE,
> + NVVRS_REG_CTL_2_RTC_WAKE);
> + if (ret < 0) {
> + dev_err(info->dev, "Failed to set RTC_WAKE bit (%d)\n", ret);
This should be either a dev_dbg or removed
> + return ret;
> + }
> +
> + /* Set RTC_PU bit for autonomous wake from shutdown */
> + ret = nvvrs_update_bits(info, NVVRS_REG_CTL_2, NVVRS_REG_CTL_2_RTC_PU,
> + NVVRS_REG_CTL_2_RTC_PU);
> + if (ret < 0) {
> + dev_err(info->dev, "Failed to set RTC_PU bit (%d)\n", ret);
Ditto
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int nvvrs_rtc_disable_alarm(struct nvvrs_rtc_info *info)
> +{
> + struct i2c_client *client = info->client;
> + u8 val[4];
> + int ret;
> +
> + /* Clear RTC_WAKE bit */
> + ret = nvvrs_update_bits(info, NVVRS_REG_CTL_2, NVVRS_REG_CTL_2_RTC_WAKE,
> + 0);
> + if (ret < 0) {
> + dev_err(info->dev, "Failed to clear RTC_WAKE bit (%d)\n", ret);
Ditto
> + return ret;
> + }
> +
> + /* Clear RTC_PU bit */
> + ret = nvvrs_update_bits(info, NVVRS_REG_CTL_2, NVVRS_REG_CTL_2_RTC_PU,
> + 0);
> + if (ret < 0) {
> + dev_err(info->dev, "Failed to clear RTC_PU bit (%d)\n", ret);
Ditto
> + return ret;
> + }
> +
> + /* Write ALARM_RESET_VAL in RTC Alarm register to disable alarm */
> + val[0] = 0xff;
> + val[1] = 0xff;
> + val[2] = 0xff;
> + val[3] = 0xff;
> +
> + ret = nvvrs_rtc_write_alarm(client, val);
> + if (ret < 0)
> + dev_err(info->dev, "Failed to disable Alarm (%d)\n", ret);
Ditto
> +
> + return 0;
Plus it fails but then returns 0
> +}
> +
> +static int nvvrs_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct nvvrs_rtc_info *info = dev_get_drvdata(dev);
> + time64_t secs = 0;
> + int ret;
> + u8 val;
> +
> + mutex_lock(&info->lock);
This lock is unnecessary once you use rtc_lock/rtc_unlock in the IRQ
handler.
> +
> + /*
> + * Multi-byte transfers are not supported with PEC enabled
> + * Read MSB first to avoid coherency issues
> + */
> + ret = i2c_smbus_read_byte_data(info->client, NVVRS_REG_RTC_T3);
> + if (ret < 0)
> + goto out;
> +
> + val = (u8)ret;
> + secs |= (time64_t)val << 24;
> +
> + ret = i2c_smbus_read_byte_data(info->client, NVVRS_REG_RTC_T2);
> + if (ret < 0)
> + goto out;
> +
> + val = (u8)ret;
> + secs |= (time64_t)val << 16;
> +
> + ret = i2c_smbus_read_byte_data(info->client, NVVRS_REG_RTC_T1);
> + if (ret < 0)
> + goto out;
> +
> + val = (u8)ret;
> + secs |= (time64_t)val << 8;
> +
> + ret = i2c_smbus_read_byte_data(info->client, NVVRS_REG_RTC_T0);
> + if (ret < 0)
> + goto out;
> +
> + val = (u8)ret;
> + secs |= val;
> +
> + rtc_time64_to_tm(secs, tm);
> + ret = 0;
> +out:
> + mutex_unlock(&info->lock);
> + return ret;
> +}
> +
> +static int nvvrs_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct nvvrs_rtc_info *info = dev_get_drvdata(dev);
> + time64_t secs;
> + u8 time[4];
> + int ret;
> +
> + mutex_lock(&info->lock);
Ditto
> +
> + secs = rtc_tm_to_time64(tm);
> + time[0] = secs & 0xff;
> + time[1] = (secs >> 8) & 0xff;
> + time[2] = (secs >> 16) & 0xff;
> + time[3] = (secs >> 24) & 0xff;
> +
> + ret = i2c_smbus_write_byte_data(info->client, NVVRS_REG_RTC_T3, time[3]);
> + if (ret < 0)
> + goto out;
> +
> + ret = i2c_smbus_write_byte_data(info->client, NVVRS_REG_RTC_T2, time[2]);
> + if (ret < 0)
> + goto out;
> +
> + ret = i2c_smbus_write_byte_data(info->client, NVVRS_REG_RTC_T1, time[1]);
> + if (ret < 0)
> + goto out;
> +
> + ret = i2c_smbus_write_byte_data(info->client, NVVRS_REG_RTC_T0, time[0]);
> +out:
> + mutex_unlock(&info->lock);
> + return ret;
> +}
> +
> +static int nvvrs_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> + struct nvvrs_rtc_info *info = dev_get_drvdata(dev);
> + time64_t alarm_val = 0;
> + int ret;
> + u8 val;
> +
> + mutex_lock(&info->lock);
Ditto
> +
> + /* Multi-byte transfers are not supported with PEC enabled */
> + ret = i2c_smbus_read_byte_data(info->client, NVVRS_REG_RTC_A3);
> + if (ret < 0)
> + goto out;
> +
> + val = (u8)ret;
> + alarm_val |= (time64_t)val << 24;
> +
> + ret = i2c_smbus_read_byte_data(info->client, NVVRS_REG_RTC_A2);
> + if (ret < 0)
> + goto out;
> +
> + val = (u8)ret;
> + alarm_val |= (time64_t)val << 16;
> +
> + ret = i2c_smbus_read_byte_data(info->client, NVVRS_REG_RTC_A1);
> + if (ret < 0)
> + goto out;
> +
> + val = (u8)ret;
> + alarm_val |= (time64_t)val << 8;
> +
> + ret = i2c_smbus_read_byte_data(info->client, NVVRS_REG_RTC_A0);
> + if (ret < 0)
> + goto out;
> +
> + val = (u8)ret;
> + alarm_val |= val;
> +
> + if (alarm_val == ALARM_RESET_VAL)
> + alrm->enabled = 0;
> + else
> + alrm->enabled = 1;
> +
> + rtc_time64_to_tm(alarm_val, &alrm->time);
> + ret = 0;
> +out:
> + mutex_unlock(&info->lock);
> + return ret;
> +}
> +
> +static int nvvrs_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> + struct nvvrs_rtc_info *info = dev_get_drvdata(dev);
> + time64_t secs;
> + u8 time[4];
> + int ret;
> +
> + mutex_lock(&info->lock);
> +
Ditto
> + if (!alrm->enabled) {
> + ret = nvvrs_rtc_disable_alarm(info);
> + if (ret < 0)
> + goto out;
> + }
> +
> + ret = nvvrs_rtc_enable_alarm(info);
> + if (ret < 0)
> + goto out;
> +
> + secs = rtc_tm_to_time64(&alrm->time);
> + time[0] = secs & 0xff;
> + time[1] = (secs >> 8) & 0xff;
> + time[2] = (secs >> 16) & 0xff;
> + time[3] = (secs >> 24) & 0xff;
> +
> + ret = nvvrs_rtc_write_alarm(info->client, time);
> +
> +out:
> + mutex_unlock(&info->lock);
> + return ret;
> +}
> +
> +static int nvvrs_pseq_irq_clear(struct nvvrs_rtc_info *info)
> +{
> + unsigned int i;
> + int ret;
> +
> + for (i = 0; i < NVVRS_IRQ_REG_COUNT; i++) {
> + ret = i2c_smbus_read_byte_data(info->client,
> + NVVRS_REG_INT_SRC1 + i);
> + if (ret < 0) {
> + dev_err(info->dev, "Failed to read INT_SRC%d : %d\n",
> + i + 1, ret);
> + return ret;
> + }
> +
> + ret = i2c_smbus_write_byte_data(info->client,
> + NVVRS_REG_INT_SRC1 + i,
> + (u8)ret);
> + if (ret < 0) {
> + dev_err(info->dev, "Failed to clear INT_SRC%d : %d\n",
> + i + 1, ret);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static irqreturn_t nvvrs_rtc_irq_handler(int irq, void *data)
> +{
> + struct nvvrs_rtc_info *info = data;
> + int ret;
> +
> + /* Check for RTC alarm interrupt */
> + ret = i2c_smbus_read_byte_data(info->client, NVVRS_REG_INT_SRC1);
> + if (ret < 0) {
> + dev_err(info->dev, "Failed to read INT_SRC1: %d\n", ret);
dev_dbg or remove
> + return IRQ_NONE;
> + }
> +
> + if (ret & NVVRS_INT_SRC1_RTC_MASK) {
> + rtc_lock(info->rtc);
> + rtc_update_irq(info->rtc, 1, RTC_IRQF | RTC_AF);
> + rtc_unlock(info->rtc);
> + }
> +
> + /* Clear all interrupts */
> + if (nvvrs_pseq_irq_clear(info) < 0)
> + return IRQ_NONE;
> +
> + return IRQ_HANDLED;
> +}
> +
> diff --git a/include/linux/rtc/rtc-nvidia-vrs10.h b/include/linux/rtc/rtc-nvidia-vrs10.h
> new file mode 100644
> index 000000000000..3c9c46abf555
> --- /dev/null
> +++ b/include/linux/rtc/rtc-nvidia-vrs10.h
Just to be sure, do you expect to use this include in another driver?
Else you should merge it back in the c file.
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
prev parent reply other threads:[~2025-09-23 15:41 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-19 14:02 [PATCH v6 3/4] rtc: nvvrs: add NVIDIA VRS RTC device driver Shubhi Garg
2025-09-23 15:40 ` 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=202509231540480daf8b56@mail.local \
--to=alexandre.belloni@bootlin.com \
--cc=catalin.marinas@arm.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=jonathanh@nvidia.com \
--cc=krzk+dt@kernel.org \
--cc=lee@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-rtc@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=robh@kernel.org \
--cc=shgarg@nvidia.com \
--cc=will@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.