Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
From: Bruno Thomsen <bruno.thomsen@gmail.com>
To: linux-rtc@vger.kernel.org, linux-watchdog@vger.kernel.org
Cc: alexandre.belloni@bootlin.com, a.zummo@towertech.it,
	wim@linux-watchdog.org, linux@roeck-us.net,
	u.kleine-koenig@pengutronix.de, bth@kamstrup.com,
	bruno.thomsen@gmail.com
Subject: [PATCH v3 2/5] rtc: pcf2127: cleanup register and bit defines
Date: Thu, 22 Aug 2019 15:19:33 +0200
Message-ID: <20190822131936.18772-2-bruno.thomsen@gmail.com> (raw)
In-Reply-To: <20190822131936.18772-1-bruno.thomsen@gmail.com>

Cleanup of defines to follow kernel coding style and increase code
readability by using same register and bit define style.

Change PCF2127_REG_RAM_{addr_MSB,wrt_cmd,rd_cmd} to upper case as
kernel coding guide section 12 'Macros, Enums and RTL' states
"Names of macros defining constants and labels in enums are capitalized".

Improve readability of RAM register comment by making whole sentences.

Remove parentheses from register defines as they are only used
for expressions and not constants.

As there are no clear style for name of registers and bits in the
kernel drivers, I suggest the following for at least this driver,
but hopefully also other RTC drivers.

Register name should follow this convention:
[chip]_REG_[reg name] 0xXX

Bit name should follow this convention, so it clearly states which
chip register it's part of:
[chip]_BIT_[reg name]_[bit name] BIT(X)

Additionally I suggest bit defines are always placed right below
its corresponding register define and using an extra tab indentation
for the BIT(X) part. This will visualt make it easy to see that bit
defines are part of the complete register definition.

Rename PCF2127_OSF to PCF2127_BIT_SC_OSF and move it right below
PCF2127_REG_SC. This will improve readability of bit checks as it's
easy to verify that it uses the correct register.

Move end of line comments above register defines as it's more like
a heading for 1 register define and up to 8 bit defines or a
collection of registers that are close related like timestamp
split across 6 registers.

Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
---
v3: no change.
v2: updated commit message.

 drivers/rtc/rtc-pcf2127.c | 59 ++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 58eb96506e4b..cd8def79b379 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -19,26 +19,32 @@
 #include <linux/of.h>
 #include <linux/regmap.h>
 
-#define PCF2127_REG_CTRL1       (0x00)  /* Control Register 1 */
-#define PCF2127_REG_CTRL2       (0x01)  /* Control Register 2 */
-
-#define PCF2127_REG_CTRL3       (0x02)  /* Control Register 3 */
-#define PCF2127_REG_CTRL3_BLF		BIT(2)
-
-#define PCF2127_REG_SC          (0x03)  /* datetime */
-#define PCF2127_REG_MN          (0x04)
-#define PCF2127_REG_HR          (0x05)
-#define PCF2127_REG_DM          (0x06)
-#define PCF2127_REG_DW          (0x07)
-#define PCF2127_REG_MO          (0x08)
-#define PCF2127_REG_YR          (0x09)
-
-/* the pcf2127 has 512 bytes nvmem, pcf2129 doesn't */
-#define PCF2127_REG_RAM_addr_MSB       0x1a
-#define PCF2127_REG_RAM_wrt_cmd        0x1c
-#define PCF2127_REG_RAM_rd_cmd         0x1d
+/* Control register 1 */
+#define PCF2127_REG_CTRL1		0x00
+/* Control register 2 */
+#define PCF2127_REG_CTRL2		0x01
+/* Control register 3 */
+#define PCF2127_REG_CTRL3		0x02
+#define PCF2127_BIT_CTRL3_BLF			BIT(2)
+/* Time and date registers */
+#define PCF2127_REG_SC			0x03
+#define PCF2127_BIT_SC_OSF			BIT(7)
+#define PCF2127_REG_MN			0x04
+#define PCF2127_REG_HR			0x05
+#define PCF2127_REG_DM			0x06
+#define PCF2127_REG_DW			0x07
+#define PCF2127_REG_MO			0x08
+#define PCF2127_REG_YR			0x09
+/*
+ * RAM registers
+ * PCF2127 has 512 bytes general-purpose static RAM (SRAM) that is
+ * battery backed and can survive a power outage.
+ * PCF2129 doesn't have this feature.
+ */
+#define PCF2127_REG_RAM_ADDR_MSB	0x1A
+#define PCF2127_REG_RAM_WRT_CMD		0x1C
+#define PCF2127_REG_RAM_RD_CMD		0x1D
 
-#define PCF2127_OSF             BIT(7)  /* Oscillator Fail flag */
 
 struct pcf2127 {
 	struct rtc_device *rtc;
@@ -73,11 +79,12 @@ static int pcf2127_rtc_read_time(struct device *dev, struct rtc_time *tm)
 		return ret;
 	}
 
-	if (buf[PCF2127_REG_CTRL3] & PCF2127_REG_CTRL3_BLF)
+	if (buf[PCF2127_REG_CTRL3] & PCF2127_BIT_CTRL3_BLF)
 		dev_info(dev,
 			"low voltage detected, check/replace RTC battery.\n");
 
-	if (buf[PCF2127_REG_SC] & PCF2127_OSF) {
+	/* Clock integrity is not guaranteed when OSF flag is set. */
+	if (buf[PCF2127_REG_SC] & PCF2127_BIT_SC_OSF) {
 		/*
 		 * no need clear the flag here,
 		 * it will be cleared once the new date is saved
@@ -166,7 +173,7 @@ static int pcf2127_rtc_ioctl(struct device *dev,
 		if (ret)
 			return ret;
 
-		touser = touser & PCF2127_REG_CTRL3_BLF ? 1 : 0;
+		touser = touser & PCF2127_BIT_CTRL3_BLF ? 1 : 0;
 
 		if (copy_to_user((void __user *)arg, &touser, sizeof(int)))
 			return -EFAULT;
@@ -192,12 +199,12 @@ static int pcf2127_nvmem_read(void *priv, unsigned int offset,
 	int ret;
 	unsigned char offsetbuf[] = { offset >> 8, offset };
 
-	ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_RAM_addr_MSB,
+	ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_RAM_ADDR_MSB,
 				offsetbuf, 2);
 	if (ret)
 		return ret;
 
-	ret = regmap_bulk_read(pcf2127->regmap, PCF2127_REG_RAM_rd_cmd,
+	ret = regmap_bulk_read(pcf2127->regmap, PCF2127_REG_RAM_RD_CMD,
 			       val, bytes);
 
 	return ret ?: bytes;
@@ -210,12 +217,12 @@ static int pcf2127_nvmem_write(void *priv, unsigned int offset,
 	int ret;
 	unsigned char offsetbuf[] = { offset >> 8, offset };
 
-	ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_RAM_addr_MSB,
+	ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_RAM_ADDR_MSB,
 				offsetbuf, 2);
 	if (ret)
 		return ret;
 
-	ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_RAM_wrt_cmd,
+	ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_RAM_WRT_CMD,
 				val, bytes);
 
 	return ret ?: bytes;
-- 
2.21.0


  reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-22 13:19 [PATCH v3 1/5] rtc: pcf2127: convert to devm_rtc_allocate_device Bruno Thomsen
2019-08-22 13:19 ` Bruno Thomsen [this message]
2019-08-22 21:27   ` [PATCH v3 2/5] rtc: pcf2127: cleanup register and bit defines Alexandre Belloni
2019-08-22 13:19 ` [PATCH v3 3/5] rtc: pcf2127: bugfix: read rtc disables watchdog Bruno Thomsen
2019-08-22 21:28   ` Alexandre Belloni
2019-08-22 13:19 ` [PATCH v3 4/5] rtc: pcf2127: add watchdog feature support Bruno Thomsen
2019-08-22 14:11   ` Guenter Roeck
2019-08-22 21:28   ` Alexandre Belloni
2019-08-22 13:19 ` [PATCH v3 5/5] rtc: pcf2127: add tamper detection support Bruno Thomsen
2019-08-22 21:29   ` Alexandre Belloni
2019-08-22 21:26 ` [PATCH v3 1/5] rtc: pcf2127: convert to devm_rtc_allocate_device Alexandre Belloni

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=20190822131936.18772-2-bruno.thomsen@gmail.com \
    --to=bruno.thomsen@gmail.com \
    --cc=a.zummo@towertech.it \
    --cc=alexandre.belloni@bootlin.com \
    --cc=bth@kamstrup.com \
    --cc=linux-rtc@vger.kernel.org \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=u.kleine-koenig@pengutronix.de \
    --cc=wim@linux-watchdog.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

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git