[PATCH v3 0/5] Add Renesas PMIC RAA215300 and built-in RTC support

[PATCH v3 0/5] Add Renesas PMIC RAA215300 and built-in RTC support

[Biju Das]

This patch series aims to add support for Renesas PMIC RAA215300 and
built-in RTC found on this PMIC device.

The details of PMIC can be found here[1].

Renesas PMIC RAA215300 exposes two separate i2c devices, one for the main
device and another for rtc device.

Enhance i2c_new_ancillary_device() to instantiate a real device.
(eg: Instantiate rtc device from PMIC driver)

The built-in RTC found on PMIC RAA215300 is the same as ISL1208.
However, the external oscillator bit is inverted on PMIC version
0x11. The PMIC driver detects PMIC version and instantiate appropriate
RTC device.

[1]
https://www.renesas.com/in/en/products/power-power-management/multi-channel-power-management-ics-pmics/ssdsoc-power-management-ics-pmic-and-pmus/raa215300-high-performance-9-channel-pmic-supporting-ddr-memory-built-charger-and-rtc

[2]
 https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230505091720.115675-1-biju.das.jz@bp.renesas.com/

[3]
 https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230505172530.357455-5-biju.das.jz@bp.renesas.com/

v2->v3:
 * Enhanced i2c_new_ancillary_device() to instantiate a real ancillary_device().
 * RTC device is instantiated by PMIC driver and dropped isl1208_probe_helper().
 * Added "TYPE_RAA215300_RTC_A0" to handle inverted oscillator bit case.
 * Added more detailed description for renesas,rtc-enabled property.
 * Added support for handling "renesas,rtc-enabled" property.
 * Based on PMIC version, it instantiates rtc device by calling i2c_new_
   ancillary_device().
 * Updated the logs.
RFC->v2:
 * Dropped the cross-links from bindings and used a single compatible
   with separate i2c addresses for pmic main and rtc device.
 * Dropped patch#4 and split patch#3 from this series and send as
   separate patch to ML [2].
 * Added RTC platform driver and mfd cell entry to the PMIC driver.RTC
   platform driver creates rtc device by using i2c_new_ancillary_device()
   and register the rtc device by calling the helper function provided
   by rtc-isl2108 driver.
 * Updated reg property in bindings.
 * Added optional reg-names, interrupts and renesas,rtc-enabled
   properties.
 * Fixed the node name in the binding example
 * Dropped the cross link property renesas,raa215300-rtc.
 * Updated the binding example
 * Dropped MODULE_SOFTDEP from the driver as it is added in RTC platform
   driver.
 * Dropped compatible "renesas,raa215300-isl1208" and "renesas,raa215300-pmic" property.
 * Updated the comment polarity->bit for External Oscillator.
 * Added raa215300_rtc_probe_helper() for registering raa215300_rtc device and
   added the helper function isl1208_probe_helper() to share the code.
 * Updated pmic device node on the SoM dtsi based on the bindings.

Logs:
[   15.447305] rtc-isl1208 3-006f: registered as rtc0
[   15.479493] rtc-isl1208 3-006f: setting system clock to 2023-04-27T19:31:02 UTC (1682623862)

root@smarc-rzv2l:~# hwclock -r
2023-04-27 19:33:05.499001+00:00
root@smarc-rzv2l:~# hwclock -r
2023-04-27 19:33:06.936688+00:00
root@smarc-rzv2l:~#

Biju Das (5):
  i2c: Enhance i2c_new_ancillary_device API
  rtc: isl1208: Add support for the built-in RTC on the PMIC RAA215300
  dt-bindings: mfd: Add Renesas RAA215300 PMIC bindings
  mfd: Add Renesas PMIC RAA215300 driver
  arm64: dts: renesas: rzg2l-smarc-som: Enable PMIC and built-in RTC

 .../bindings/mfd/renesas,raa215300.yaml       |  70 ++++++++++++
 .../boot/dts/renesas/rzg2l-smarc-som.dtsi     |  10 ++
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c  |   6 +-
 drivers/i2c/i2c-core-base.c                   |  38 +++++--
 drivers/media/i2c/adv748x/adv748x-core.c      |   2 +-
 drivers/media/i2c/adv7604.c                   |   3 +-
 drivers/mfd/Kconfig                           |   7 ++
 drivers/mfd/Makefile                          |   2 +
 drivers/mfd/raa215300.c                       | 102 ++++++++++++++++++
 drivers/rtc/rtc-isl1208.c                     |  21 ++++
 include/linux/i2c.h                           |   3 +-
 11 files changed, 251 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/renesas,raa215300.yaml
 create mode 100644 drivers/mfd/raa215300.c

-- 
2.25.1

[PATCH v3 1/5] i2c: Enhance i2c_new_ancillary_device API

[Biju Das]

Renesas PMIC RAA215300 exposes two separate i2c devices, one for the main
device and another for rtc device.

Enhance i2c_new_ancillary_device() to instantiate a real device.
(eg: Instantiate rtc device from PMIC driver)

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v3:
 * New patch

Ref:
 https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230505172530.357455-5-biju.das.jz@bp.renesas.com/
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c |  6 ++--
 drivers/i2c/i2c-core-base.c                  | 38 ++++++++++++++++----
 drivers/media/i2c/adv748x/adv748x-core.c     |  2 +-
 drivers/media/i2c/adv7604.c                  |  3 +-
 include/linux/i2c.h                          |  3 +-
 5 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index ddceafa7b637..86306b010a0a 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1072,7 +1072,7 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
 	int ret;
 
 	adv->i2c_cec = i2c_new_ancillary_device(adv->i2c_main, "cec",
-						ADV7511_CEC_I2C_ADDR_DEFAULT);
+				    ADV7511_CEC_I2C_ADDR_DEFAULT, NULL);
 	if (IS_ERR(adv->i2c_cec))
 		return PTR_ERR(adv->i2c_cec);
 
@@ -1261,7 +1261,7 @@ static int adv7511_probe(struct i2c_client *i2c)
 	adv7511_packet_disable(adv7511, 0xffff);
 
 	adv7511->i2c_edid = i2c_new_ancillary_device(i2c, "edid",
-					ADV7511_EDID_I2C_ADDR_DEFAULT);
+					ADV7511_EDID_I2C_ADDR_DEFAULT, NULL);
 	if (IS_ERR(adv7511->i2c_edid)) {
 		ret = PTR_ERR(adv7511->i2c_edid);
 		goto uninit_regulators;
@@ -1271,7 +1271,7 @@ static int adv7511_probe(struct i2c_client *i2c)
 		     adv7511->i2c_edid->addr << 1);
 
 	adv7511->i2c_packet = i2c_new_ancillary_device(i2c, "packet",
-					ADV7511_PACKET_I2C_ADDR_DEFAULT);
+					ADV7511_PACKET_I2C_ADDR_DEFAULT, NULL);
 	if (IS_ERR(adv7511->i2c_packet)) {
 		ret = PTR_ERR(adv7511->i2c_packet);
 		goto err_i2c_unregister_edid;
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index ae3af738b03f..4f0964326968 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1122,15 +1122,17 @@ EXPORT_SYMBOL_GPL(devm_i2c_new_dummy_device);
  * @client: Handle to the primary client
  * @name: Handle to specify which secondary address to get
  * @default_addr: Used as a fallback if no secondary address was specified
+ * @aux_device_name: Ancillary device name
  * Context: can sleep
  *
  * I2C clients can be composed of multiple I2C slaves bound together in a single
  * component. The I2C client driver then binds to the master I2C slave and needs
- * to create I2C dummy clients to communicate with all the other slaves.
+ * to create I2C ancillary clients to communicate with all the other slaves.
  *
- * This function creates and returns an I2C dummy client whose I2C address is
- * retrieved from the platform firmware based on the given slave name. If no
- * address is specified by the firmware default_addr is used.
+ * This function creates and returns an I2C ancillary client whose I2C address
+ * is retrieved from the platform firmware based on the given slave name. If no
+ * address is specified by the firmware default_addr is used. If no aux_device_
+ * name is specified by the firmware, it will create an I2C dummy client.
  *
  * On DT-based platforms the address is retrieved from the "reg" property entry
  * cell whose "reg-names" value matches the slave name.
@@ -1139,10 +1141,12 @@ EXPORT_SYMBOL_GPL(devm_i2c_new_dummy_device);
  * i2c_unregister_device(); or an ERR_PTR to describe the error.
  */
 struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client,
-						const char *name,
-						u16 default_addr)
+					    const char *name,
+					    u16 default_addr,
+					    const char *aux_device_name)
 {
 	struct device_node *np = client->dev.of_node;
+	struct i2c_client *i2c_aux_client;
 	u32 addr = default_addr;
 	int i;
 
@@ -1153,7 +1157,27 @@ struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client,
 	}
 
 	dev_dbg(&client->adapter->dev, "Address for %s : 0x%x\n", name, addr);
-	return i2c_new_dummy_device(client->adapter, addr);
+
+	if (aux_device_name) {
+		struct i2c_board_info info;
+		size_t aux_device_name_len = strlen(aux_device_name);
+
+		if (aux_device_name_len > I2C_NAME_SIZE - 1) {
+			dev_err(&client->adapter->dev, "Invalid device name\n");
+			return ERR_PTR(-EINVAL);
+		}
+
+		memset(&info, 0, sizeof(struct i2c_board_info));
+
+		memcpy(info.type, aux_device_name, aux_device_name_len);
+		info.addr = addr;
+
+		i2c_aux_client = i2c_new_client_device(client->adapter, &info);
+	} else {
+		i2c_aux_client = i2c_new_dummy_device(client->adapter, addr);
+	}
+
+	return i2c_aux_client;
 }
 EXPORT_SYMBOL_GPL(i2c_new_ancillary_device);
 
diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 4498d78a2357..5bdf7b0c6bf3 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -186,7 +186,7 @@ static int adv748x_initialise_clients(struct adv748x_state *state)
 		state->i2c_clients[i] = i2c_new_ancillary_device(
 				state->client,
 				adv748x_default_addresses[i].name,
-				adv748x_default_addresses[i].default_addr);
+				adv748x_default_addresses[i].default_addr, NULL);
 
 		if (IS_ERR(state->i2c_clients[i])) {
 			adv_err(state, "failed to create i2c client %u\n", i);
diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 3d0898c4175e..63fa44c9d27c 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -2935,7 +2935,8 @@ static struct i2c_client *adv76xx_dummy_client(struct v4l2_subdev *sd,
 	else
 		new_client = i2c_new_ancillary_device(client,
 				adv76xx_default_addresses[page].name,
-				adv76xx_default_addresses[page].default_addr);
+				adv76xx_default_addresses[page].default_addr,
+				NULL);
 
 	if (!IS_ERR(new_client))
 		io_write(sd, io_reg, new_client->addr << 1);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 13a1ce38cb0c..0ce344724209 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -489,7 +489,8 @@ devm_i2c_new_dummy_device(struct device *dev, struct i2c_adapter *adap, u16 addr
 struct i2c_client *
 i2c_new_ancillary_device(struct i2c_client *client,
 			 const char *name,
-			 u16 default_addr);
+			 u16 default_addr,
+			 const char *aux_device_name);
 
 void i2c_unregister_device(struct i2c_client *client);
 
-- 
2.25.1

[PATCH v3 2/5] rtc: isl1208: Add support for the built-in RTC on the PMIC RAA215300

[Biju Das]

The built-in RTC found on PMIC RAA215300 is the same as ISL1208.
However, the external oscillator bit is inverted on PMIC version
0x11. The PMIC driver detects PMIC version and instantiate appropriate
RTC device based on i2c_device_id.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * RTC device is instantiated by PMIC driver and dropped isl1208_probe_helper().
 * Added "TYPE_RAA215300_RTC_A0" to handle inverted oscillator bit case.
RFC->v2:
 * Dropped compatible "renesas,raa215300-isl1208" and "renesas,raa215300-pmic" property.
 * Updated the comment polarity->bit for External Oscillator.
 * Added raa215300_rtc_probe_helper() for registering raa215300_rtc device and
   added the helper function isl1208_probe_helper() to share the code.
---
 drivers/rtc/rtc-isl1208.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index 73cc6aaf9b8b..d6425780d834 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -74,6 +74,7 @@ enum isl1208_id {
 	TYPE_ISL1209,
 	TYPE_ISL1218,
 	TYPE_ISL1219,
+	TYPE_RAA215300_RTC_A0,
 	ISL_LAST_ID
 };
 
@@ -83,11 +84,13 @@ static const struct isl1208_config {
 	unsigned int	nvmem_length;
 	unsigned	has_tamper:1;
 	unsigned	has_timestamp:1;
+	unsigned	has_inverted_osc_bit:1;
 } isl1208_configs[] = {
 	[TYPE_ISL1208] = { "isl1208", 2, false, false },
 	[TYPE_ISL1209] = { "isl1209", 2, true,  false },
 	[TYPE_ISL1218] = { "isl1218", 8, false, false },
 	[TYPE_ISL1219] = { "isl1219", 2, true,  true },
+	[TYPE_RAA215300_RTC_A0] = { "rtc_a0", 2, false, false, true },
 };
 
 static const struct i2c_device_id isl1208_id[] = {
@@ -95,6 +98,7 @@ static const struct i2c_device_id isl1208_id[] = {
 	{ "isl1209", TYPE_ISL1209 },
 	{ "isl1218", TYPE_ISL1218 },
 	{ "isl1219", TYPE_ISL1219 },
+	{ "rtc_a0", TYPE_RAA215300_RTC_A0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, isl1208_id);
@@ -166,6 +170,16 @@ isl1208_i2c_validate_client(struct i2c_client *client)
 	return 0;
 }
 
+static int
+isl1208_set_external_oscillator(struct i2c_client *client, int rc,
+				bool is_inverted_oscillator_bit)
+{
+	if (is_inverted_oscillator_bit)
+		rc |= ISL1208_REG_SR_XTOSCB;
+
+	return i2c_smbus_write_byte_data(client, ISL1208_REG_SR, rc);
+}
+
 static int
 isl1208_i2c_get_sr(struct i2c_client *client)
 {
@@ -845,6 +859,13 @@ isl1208_probe(struct i2c_client *client)
 		return rc;
 	}
 
+	if (isl1208->config->has_inverted_osc_bit) {
+		rc = isl1208_set_external_oscillator(client, rc,
+						     isl1208->config->has_inverted_osc_bit);
+		if (rc)
+			return rc;
+	}
+
 	if (rc & ISL1208_REG_SR_RTCF)
 		dev_warn(&client->dev, "rtc power failure detected, "
 			 "please set clock.\n");
-- 
2.25.1

[PATCH v3 3/5] dt-bindings: mfd: Add Renesas RAA215300 PMIC bindings

[Biju Das]

Document Renesas RAA215300 PMIC bindings.

The RAA215300 is a high Performance 9-Channel PMIC supporting DDR
Memory, with Built-In Charger and RTC.

It supports DDR3, DDR3L, DDR4, and LPDDR4 memory power requirements.
The internally compensated regulators, built-in Real-Time Clock (RTC),
32kHz crystal oscillator, and coin cell battery charger provide a
highly integrated, small footprint power solution ideal for
System-On-Module (SOM) applications. A spread spectrum feature
provides an ease-of-use solution for noise-sensitive audio or RF
applications.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * Added more detailed description for renesas,rtc-enabled property.
RFC->v2:
 * Updated reg property
 * Added optional reg-names, interrupts and renesas,rtc-enabled
   properties.
 * Fixed the node name in the example
 * Dropped the cross link property renesas,raa215300-rtc.
 * Updated the example
---
 .../bindings/mfd/renesas,raa215300.yaml       | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/renesas,raa215300.yaml

diff --git a/Documentation/devicetree/bindings/mfd/renesas,raa215300.yaml b/Documentation/devicetree/bindings/mfd/renesas,raa215300.yaml
new file mode 100644
index 000000000000..04d34e5be23e
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/renesas,raa215300.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/renesas,raa215300.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RAA215300 Power Management Integrated Circuit (PMIC)
+
+maintainers:
+  - Biju Das <biju.das.jz@bp.renesas.com>
+
+description: |
+  The RAA215300 is a high-performance, low-cost 9-channel PMIC designed for
+  32-bit and 64-bit MCU and MPU applications. It supports DDR3, DDR3L, DDR4,
+  and LPDDR4 memory power requirements. The internally compensated regulators,
+  built-in Real-Time Clock (RTC), 32kHz crystal oscillator, and coin cell
+  battery charger provide a highly integrated, small footprint power solution
+  ideal for System-On-Module (SOM) applications. A spread spectrum feature
+  provides an ease-of-use solution for noise-sensitive audio or RF applications.
+
+  This device exposes two devices via I2C. One for the integrated RTC IP, and
+  one for everything else.
+
+  Link to datasheet:
+  https://www.renesas.com/in/en/products/power-power-management/multi-channel-power-management-ics-pmics/ssdsoc-power-management-ics-pmic-and-pmus/raa215300-high-performance-9-channel-pmic-supporting-ddr-memory-built-charger-and-rtc
+
+properties:
+  compatible:
+    enum:
+      - renesas,raa215300
+
+  reg:
+    minItems: 1
+    maxItems: 2
+
+  reg-names:
+    items:
+      - const: main
+      - const: rtc
+
+  interrupts:
+    maxItems: 1
+
+  renesas,rtc-enabled:
+    description:
+      To indicate RTC is enabled on the PMIC.
+      Enabling of the RTC is based on system design. System designers may
+      choose not to populate built-in RTC by grounding XIN and XOUT pins.
+    type: boolean
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        raa215300: pmic@12 {
+            compatible = "renesas,raa215300";
+            reg = <0x12>, <0x6f>;
+            reg-names = "main", "rtc";
+
+            renesas,rtc-enabled;
+        };
+    };
-- 
2.25.1

[PATCH v3 4/5] mfd: Add Renesas PMIC RAA215300 driver

[Biju Das]

The RAA215300 is a 9-channel PMIC that consists of
 * Internally compensated regulators
 * built-in Real Time Clock (RTC)
 * 32kHz crystal oscillator
 * coin cell battery charger

The RTC on RAA215300 is similar to the IP found in the ISL1208.
The existing driver for the ISL1208 works for this PMIC too,
however the RAA215300 exposes two devices via I2C, one for the RTC
IP, and one for everything else. The RTC IP has to be enabled
by the other I2C device, therefore this driver is necessary to get
the RTC to work.

The external oscillator bit is inverted on PMIC version 0x11.

Add PMIC RAA215300 driver for enabling RTC block and instantiating
RTC device based on PMIC version.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * Updated commit description
 * Added support for handling "renesas,rtc-enabled" property.
 * Based on PMIC version, it instantiates rtc device by calling i2c_new_
   ancillary_device().
RFC->V2:
 * Dropped MODULE_SOFTDEP from the driver as it is added in RTC platform
   driver.
---
 drivers/mfd/Kconfig     |   7 +++
 drivers/mfd/Makefile    |   2 +
 drivers/mfd/raa215300.c | 102 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+)
 create mode 100644 drivers/mfd/raa215300.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index e90463c4441c..9071b0f27b62 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -311,6 +311,13 @@ config MFD_CS47L92
 	help
 	  Support for Cirrus Logic CS42L92, CS47L92 and CS47L93 Smart Codecs
 
+config PMIC_RAA215300
+	tristate "Renesas RAA215300 driver"
+	select REGMAP_I2C
+	depends on I2C
+	help
+	  Support for the Renesas RAA215300 PMIC.
+
 config PMIC_DA903X
 	bool "Dialog Semiconductor DA9030/DA9034 PMIC Support"
 	depends on I2C=y
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 1d2392f06f78..d9c601120bfd 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -126,6 +126,8 @@ ifeq ($(CONFIG_SA1100_ASSABET),y)
 obj-$(CONFIG_MCP_UCB1200)	+= ucb1x00-assabet.o
 endif
 
+obj-$(CONFIG_PMIC_RAA215300)	+= raa215300.o
+
 obj-$(CONFIG_PMIC_DA903X)	+= da903x.o
 
 obj-$(CONFIG_PMIC_DA9052)	+= da9052-irq.o
diff --git a/drivers/mfd/raa215300.c b/drivers/mfd/raa215300.c
new file mode 100644
index 000000000000..aa149a88e2fb
--- /dev/null
+++ b/drivers/mfd/raa215300.c
@@ -0,0 +1,102 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RAA215300 PMIC driver
+ *
+ * Copyright (C) 2023 Renesas Electronics Corporation
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#define RAA215300_REG_BLOCK_EN	0x6c
+#define RAA215300_HW_REV	0xf8
+
+#define RAA215300_RTC_DEFAULT_ADDR 0x6f
+#define RAA215300_REG_BLOCK_EN_RTC_EN	BIT(6)
+
+static bool raa215300_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	return true;
+}
+
+static const struct regmap_config raa215300_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0xff,
+	.volatile_reg = raa215300_is_volatile_reg,
+	.cache_type = REGCACHE_FLAT,
+};
+
+static void raa215300_rtc_unregister_device(void *data)
+{
+	i2c_unregister_device(data);
+}
+
+static int raa215300_i2c_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	unsigned int pmic_version;
+	struct regmap *regmap;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+		return -EOPNOTSUPP;
+
+	regmap = devm_regmap_init_i2c(client, &raa215300_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "regmap i2c init failed\n");
+
+	ret = regmap_read(regmap, RAA215300_HW_REV, &pmic_version);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "HW rev read failed\n");
+
+	dev_dbg(dev, "RAA215300 PMIC version 0x%04x\n", pmic_version);
+
+	if (of_property_read_bool(dev->of_node, "renesas,rtc-enabled"))  {
+		struct i2c_client *rtc_client;
+
+		/* Enable RTC block */
+		regmap_update_bits(regmap, RAA215300_REG_BLOCK_EN,
+				   RAA215300_REG_BLOCK_EN_RTC_EN,
+				   RAA215300_REG_BLOCK_EN_RTC_EN);
+
+		rtc_client = i2c_new_ancillary_device(client, "rtc",
+						      RAA215300_RTC_DEFAULT_ADDR,
+						      pmic_version >= 0x12 ?
+						      "isl1208" : "rtc_a0");
+		if (IS_ERR(rtc_client))
+			return PTR_ERR(rtc_client);
+
+		ret = devm_add_action_or_reset(dev,
+					       raa215300_rtc_unregister_device,
+					       rtc_client);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id raa215300_dt_match[] = {
+	{ .compatible = "renesas,raa215300" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, raa215300_dt_match);
+
+static struct i2c_driver raa215300_i2c_driver = {
+	.driver = {
+		.name = "raa215300",
+		.of_match_table = raa215300_dt_match,
+	},
+	.probe_new = raa215300_i2c_probe,
+};
+module_i2c_driver(raa215300_i2c_driver);
+
+MODULE_DESCRIPTION("Renesas RAA215300 PMIC driver");
+MODULE_AUTHOR("Fabrizio Castro <fabrizio.castro.jz@renesas.com>");
+MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
+MODULE_LICENSE("GPL");
-- 
2.25.1

[PATCH v3 5/5] arm64: dts: renesas: rzg2l-smarc-som: Enable PMIC and built-in RTC

[Biju Das]

Enable PMIC RAA215300 and the built-in RTC on the RZ/{G2L,V2L} SMARC
EVK.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * No change.
RFC->V2:
 * Updated pmic device node based on the bindings.
---
 arch/arm64/boot/dts/renesas/rzg2l-smarc-som.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/rzg2l-smarc-som.dtsi b/arch/arm64/boot/dts/renesas/rzg2l-smarc-som.dtsi
index fbbb4f03440b..ffe19d2f0e37 100644
--- a/arch/arm64/boot/dts/renesas/rzg2l-smarc-som.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg2l-smarc-som.dtsi
@@ -351,3 +351,13 @@ &wdt1 {
 	status = "okay";
 	timeout-sec = <60>;
 };
+
+&i2c3 {
+	raa215300: pmic@12 {
+		compatible = "renesas,raa215300";
+		reg = <0x12>, <0x6f>;
+		reg-names = "main", "rtc";
+
+		renesas,rtc-enabled;
+	};
+};
-- 
2.25.1

Re: [PATCH v3 3/5] dt-bindings: mfd: Add Renesas RAA215300 PMIC bindings

[Krzysztof Kozlowski]

On 13/05/2023 18:52, Biju Das wrote:
> Document Renesas RAA215300 PMIC bindings.
> 
> The RAA215300 is a high Performance 9-Channel PMIC supporting DDR
> Memory, with Built-In Charger and RTC.
> 
> It supports DDR3, DDR3L, DDR4, and LPDDR4 memory power requirements.
> The internally compensated regulators, built-in Real-Time Clock (RTC),
> 32kHz crystal oscillator, and coin cell battery charger provide a
> highly integrated, small footprint power solution ideal for
> System-On-Module (SOM) applications. A spread spectrum feature
> provides an ease-of-use solution for noise-sensitive audio or RF
> applications.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2->v3:
>  * Added more detailed description for renesas,rtc-enabled property.
> RFC->v2:
>  * Updated reg property
>  * Added optional reg-names, interrupts and renesas,rtc-enabled
>    properties.
>  * Fixed the node name in the example
>  * Dropped the cross link property renesas,raa215300-rtc.
>  * Updated the example
> ---
>  .../bindings/mfd/renesas,raa215300.yaml       | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/renesas,raa215300.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/renesas,raa215300.yaml b/Documentation/devicetree/bindings/mfd/renesas,raa215300.yaml
> new file mode 100644
> index 000000000000..04d34e5be23e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/renesas,raa215300.yaml
> @@ -0,0 +1,70 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/renesas,raa215300.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas RAA215300 Power Management Integrated Circuit (PMIC)
> +
> +maintainers:
> +  - Biju Das <biju.das.jz@bp.renesas.com>
> +
> +description: |
> +  The RAA215300 is a high-performance, low-cost 9-channel PMIC designed for
> +  32-bit and 64-bit MCU and MPU applications. It supports DDR3, DDR3L, DDR4,
> +  and LPDDR4 memory power requirements. The internally compensated regulators,
> +  built-in Real-Time Clock (RTC), 32kHz crystal oscillator, and coin cell
> +  battery charger provide a highly integrated, small footprint power solution
> +  ideal for System-On-Module (SOM) applications. A spread spectrum feature
> +  provides an ease-of-use solution for noise-sensitive audio or RF applications.
> +
> +  This device exposes two devices via I2C. One for the integrated RTC IP, and
> +  one for everything else.
> +
> +  Link to datasheet:
> +  https://www.renesas.com/in/en/products/power-power-management/multi-channel-power-management-ics-pmics/ssdsoc-power-management-ics-pmic-and-pmus/raa215300-high-performance-9-channel-pmic-supporting-ddr-memory-built-charger-and-rtc
> +
> +properties:
> +  compatible:
> +    enum:
> +      - renesas,raa215300
> +
> +  reg:
> +    minItems: 1
> +    maxItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: main
> +      - const: rtc

I missed it last time. This does not match your reg. I suggest to drop
minItems from reg, assuming your device always has two address spaces.

> +
> +  interrupts:
> +    maxItems: 1
> +

Best regards,
Krzysztof

RE: [PATCH v3 3/5] dt-bindings: mfd: Add Renesas RAA215300 PMIC bindings

[Biju Das]

Hi Krzysztof Kozlowski,

Thanks for the feedback.

> Subject: Re: [PATCH v3 3/5] dt-bindings: mfd: Add Renesas RAA215300 PMIC
> bindings
> 
> On 13/05/2023 18:52, Biju Das wrote:
> > Document Renesas RAA215300 PMIC bindings.
> >
> > The RAA215300 is a high Performance 9-Channel PMIC supporting DDR
> > Memory, with Built-In Charger and RTC.
> >
> > It supports DDR3, DDR3L, DDR4, and LPDDR4 memory power requirements.
> > The internally compensated regulators, built-in Real-Time Clock (RTC),
> > 32kHz crystal oscillator, and coin cell battery charger provide a
> > highly integrated, small footprint power solution ideal for
> > System-On-Module (SOM) applications. A spread spectrum feature
> > provides an ease-of-use solution for noise-sensitive audio or RF
> > applications.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v2->v3:
> >  * Added more detailed description for renesas,rtc-enabled property.
> > RFC->v2:
> >  * Updated reg property
> >  * Added optional reg-names, interrupts and renesas,rtc-enabled
> >    properties.
> >  * Fixed the node name in the example
> >  * Dropped the cross link property renesas,raa215300-rtc.
> >  * Updated the example
> > ---
> >  .../bindings/mfd/renesas,raa215300.yaml       | 70
> +++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/mfd/renesas,raa215300.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/mfd/renesas,raa215300.yaml
> > b/Documentation/devicetree/bindings/mfd/renesas,raa215300.yaml
> > new file mode 100644
> > index 000000000000..04d34e5be23e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/renesas,raa215300.yaml
> > @@ -0,0 +1,70 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +
> > +title: Renesas RAA215300 Power Management Integrated Circuit (PMIC)
> > +
> > +maintainers:
> > +  - Biju Das <biju.das.jz@bp.renesas.com>
> > +
> > +description: |
> > +  The RAA215300 is a high-performance, low-cost 9-channel PMIC
> > +designed for
> > +  32-bit and 64-bit MCU and MPU applications. It supports DDR3,
> > +DDR3L, DDR4,
> > +  and LPDDR4 memory power requirements. The internally compensated
> > +regulators,
> > +  built-in Real-Time Clock (RTC), 32kHz crystal oscillator, and coin
> > +cell
> > +  battery charger provide a highly integrated, small footprint power
> > +solution
> > +  ideal for System-On-Module (SOM) applications. A spread spectrum
> > +feature
> > +  provides an ease-of-use solution for noise-sensitive audio or RF
> applications.
> > +
> > +  This device exposes two devices via I2C. One for the integrated RTC
> > + IP, and  one for everything else.
> > +
> > +  Link to datasheet:
> > +
> > + https://www.renesas.com/in/en/products/power-power-management/multi-
> > + channel-power-management-ics-pmics/ssdsoc-power-management-ics-pmic-
> > + and-pmus/raa215300-high-performance-9-channel-pmic-supporting-ddr-me
> > + mory-built-charger-and-rtc
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - renesas,raa215300
> > +
> > +  reg:
> > +    minItems: 1
> > +    maxItems: 2
> > +
> > +  reg-names:
> > +    items:
> > +      - const: main
> > +      - const: rtc
> 
> I missed it last time. This does not match your reg. I suggest to drop
> minItems from reg, assuming your device always has two address spaces.

OK, will drop minItems.

Also, in next-version, will add reg-names as required-properties .

+required:
+  - compatible
+  - reg
+  - reg-names

I will send next version, based on feedback from others.

Cheers,
Biju

Re: [PATCH v3 1/5] i2c: Enhance i2c_new_ancillary_device API

[Geert Uytterhoeven]

Hi Biju,

On Sat, May 13, 2023 at 6:52 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Renesas PMIC RAA215300 exposes two separate i2c devices, one for the main
> device and another for rtc device.
>
> Enhance i2c_new_ancillary_device() to instantiate a real device.
> (eg: Instantiate rtc device from PMIC driver)
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v3:
>  * New patch

Thanks for your patch!

Looks correct to me, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Some suggestions for improvement below...

> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1153,7 +1157,27 @@ struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client,
>         }
>
>         dev_dbg(&client->adapter->dev, "Address for %s : 0x%x\n", name, addr);
> -       return i2c_new_dummy_device(client->adapter, addr);
> +
> +       if (aux_device_name) {
> +               struct i2c_board_info info;
> +               size_t aux_device_name_len = strlen(aux_device_name);
> +
> +               if (aux_device_name_len > I2C_NAME_SIZE - 1) {
> +                       dev_err(&client->adapter->dev, "Invalid device name\n");
> +                       return ERR_PTR(-EINVAL);
> +               }

strscpy() return value?

> +
> +               memset(&info, 0, sizeof(struct i2c_board_info));

The call to memset() would not be needed if info would be initialized
at declaration time, i.e.

    struct i2c_board_info info = { .addr = addr };

Or, use I2C_BOARD_INFO(), to guarantee initialization is aligned
with whatever future changes made to i2c_board_info? But that relies
on providing the name at declaration time, which we already have in
i2c_new_dummy_device().

So I suggest to add a name parameter to i2c_new_dummy_device(),
rename it to __i2c_new_dummy_device(), and create a wrapper for
compatibility with existing users:

    struct i2c_client *__i2c_new_dummy_device(struct i2c_adapter
*adapter, u16 address,
                                             const char *name)
    {
            struct i2c_board_info info = {
                    I2C_BOARD_INFO("dummy", address),
            };

            if (name) {
                    ssize_ret = strscpy(info.type, name, sizeof(info.type));

                    if (ret < 0)
                            return ERR_PTR(dev_err_probe(&client->adapter->dev,
                                           ret, "Invalid device name\n");
            }

            return i2c_new_client_device(adapter, &info);
    }

> +
> +               memcpy(info.type, aux_device_name, aux_device_name_len);
> +               info.addr = addr;
> +
> +               i2c_aux_client = i2c_new_client_device(client->adapter, &info);
> +       } else {
> +               i2c_aux_client = i2c_new_dummy_device(client->adapter, addr);
> +       }
> +
> +       return i2c_aux_client;
>  }
>  EXPORT_SYMBOL_GPL(i2c_new_ancillary_device);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 3/5] dt-bindings: mfd: Add Renesas RAA215300 PMIC bindings

[Geert Uytterhoeven]

Hi Biju,

On Sat, May 13, 2023 at 6:52 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Document Renesas RAA215300 PMIC bindings.
>
> The RAA215300 is a high Performance 9-Channel PMIC supporting DDR
> Memory, with Built-In Charger and RTC.
>
> It supports DDR3, DDR3L, DDR4, and LPDDR4 memory power requirements.
> The internally compensated regulators, built-in Real-Time Clock (RTC),
> 32kHz crystal oscillator, and coin cell battery charger provide a
> highly integrated, small footprint power solution ideal for
> System-On-Module (SOM) applications. A spread spectrum feature
> provides an ease-of-use solution for noise-sensitive audio or RF
> applications.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2->v3:
>  * Added more detailed description for renesas,rtc-enabled property.

Thanks for your patch!

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/renesas,raa215300.yaml

> +  renesas,rtc-enabled:
> +    description:
> +      To indicate RTC is enabled on the PMIC.
> +      Enabling of the RTC is based on system design. System designers may
> +      choose not to populate built-in RTC by grounding XIN and XOUT pins.
> +    type: boolean

Perhaps you should go full DT monty and replace this logic by a clocks
property pointing to the external crystal?

However, as I only have the Short-Form Datasheet, I am wondering what
"Built-in 32kHz crystal oscillator (with bypass)" really means?
Does this mean the RTC can work without an external crystal, using an
on-chip oscillator?

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 4/5] mfd: Add Renesas PMIC RAA215300 driver

[Geert Uytterhoeven]

Hi Biju,

On Sat, May 13, 2023 at 6:53 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> The RAA215300 is a 9-channel PMIC that consists of
>  * Internally compensated regulators
>  * built-in Real Time Clock (RTC)
>  * 32kHz crystal oscillator
>  * coin cell battery charger
>
> The RTC on RAA215300 is similar to the IP found in the ISL1208.
> The existing driver for the ISL1208 works for this PMIC too,
> however the RAA215300 exposes two devices via I2C, one for the RTC
> IP, and one for everything else. The RTC IP has to be enabled
> by the other I2C device, therefore this driver is necessary to get
> the RTC to work.
>
> The external oscillator bit is inverted on PMIC version 0x11.
>
> Add PMIC RAA215300 driver for enabling RTC block and instantiating
> RTC device based on PMIC version.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2->v3:
>  * Updated commit description
>  * Added support for handling "renesas,rtc-enabled" property.
>  * Based on PMIC version, it instantiates rtc device by calling i2c_new_
>    ancillary_device().

Thanks for the update!

> RFC->V2:
>  * Dropped MODULE_SOFTDEP from the driver as it is added in RTC platform
>    driver.
> ---
>  drivers/mfd/Kconfig     |   7 +++
>  drivers/mfd/Makefile    |   2 +
>  drivers/mfd/raa215300.c | 102 ++++++++++++++++++++++++++++++++++++++++

Note that this driver no longer uses any MFD APIs...
Perhaps it should be moved to drivers/regulator/ instead?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 2/5] rtc: isl1208: Add support for the built-in RTC on the PMIC RAA215300

[Geert Uytterhoeven]

Hi Biju,

On Sat, May 13, 2023 at 6:52 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> The built-in RTC found on PMIC RAA215300 is the same as ISL1208.
> However, the external oscillator bit is inverted on PMIC version
> 0x11. The PMIC driver detects PMIC version and instantiate appropriate
> RTC device based on i2c_device_id.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2->v3:
>  * RTC device is instantiated by PMIC driver and dropped isl1208_probe_helper().
>  * Added "TYPE_RAA215300_RTC_A0" to handle inverted oscillator bit case.
> RFC->v2:
>  * Dropped compatible "renesas,raa215300-isl1208" and "renesas,raa215300-pmic" property.
>  * Updated the comment polarity->bit for External Oscillator.
>  * Added raa215300_rtc_probe_helper() for registering raa215300_rtc device and
>    added the helper function isl1208_probe_helper() to share the code.

Thanks for the update!

> --- a/drivers/rtc/rtc-isl1208.c
> +++ b/drivers/rtc/rtc-isl1208.c
> @@ -74,6 +74,7 @@ enum isl1208_id {
>         TYPE_ISL1209,
>         TYPE_ISL1218,
>         TYPE_ISL1219,
> +       TYPE_RAA215300_RTC_A0,
>         ISL_LAST_ID
>  };
>
> @@ -83,11 +84,13 @@ static const struct isl1208_config {
>         unsigned int    nvmem_length;
>         unsigned        has_tamper:1;
>         unsigned        has_timestamp:1;
> +       unsigned        has_inverted_osc_bit:1;
>  } isl1208_configs[] = {
>         [TYPE_ISL1208] = { "isl1208", 2, false, false },
>         [TYPE_ISL1209] = { "isl1209", 2, true,  false },
>         [TYPE_ISL1218] = { "isl1218", 8, false, false },
>         [TYPE_ISL1219] = { "isl1219", 2, true,  true },
> +       [TYPE_RAA215300_RTC_A0] = { "rtc_a0", 2, false, false, true },
>  };
>
>  static const struct i2c_device_id isl1208_id[] = {
> @@ -95,6 +98,7 @@ static const struct i2c_device_id isl1208_id[] = {
>         { "isl1209", TYPE_ISL1209 },
>         { "isl1218", TYPE_ISL1218 },
>         { "isl1219", TYPE_ISL1219 },
> +       { "rtc_a0", TYPE_RAA215300_RTC_A0 },

"rtc_a0" is IMHO a too-generic name.


>         { }
>  };
>  MODULE_DEVICE_TABLE(i2c, isl1208_id);
> @@ -166,6 +170,16 @@ isl1208_i2c_validate_client(struct i2c_client *client)
>         return 0;
>  }
>
> +static int
> +isl1208_set_external_oscillator(struct i2c_client *client, int rc,

s/rc/sr/

> +                               bool is_inverted_oscillator_bit)
> +{
> +       if (is_inverted_oscillator_bit)

This condition is always true, given all callers in your series.

> +               rc |= ISL1208_REG_SR_XTOSCB;

If you do decide to keep it, you probably want to add an else branch
to make sure the bit is cleared.

> +
> +       return i2c_smbus_write_byte_data(client, ISL1208_REG_SR, rc);
> +}
> +
>  static int
>  isl1208_i2c_get_sr(struct i2c_client *client)
>  {
> @@ -845,6 +859,13 @@ isl1208_probe(struct i2c_client *client)
>                 return rc;
>         }
>
> +       if (isl1208->config->has_inverted_osc_bit) {
> +               rc = isl1208_set_external_oscillator(client, rc,

Passing "rc" is confusing, this is really the status register value
obtained above...

> +                                                    isl1208->config->has_inverted_osc_bit);
> +               if (rc)
> +                       return rc;

If we get here, rc is always zero ...

> +       }
> +
>         if (rc & ISL1208_REG_SR_RTCF)

... thus breaking this check..

>                 dev_warn(&client->dev, "rtc power failure detected, "
>                          "please set clock.\n");

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v3 2/5] rtc: isl1208: Add support for the built-in RTC on the PMIC RAA215300

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v3 2/5] rtc: isl1208: Add support for the built-in
> RTC on the PMIC RAA215300
> 
> Hi Biju,
> 
> On Sat, May 13, 2023 at 6:52 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > The built-in RTC found on PMIC RAA215300 is the same as ISL1208.
> > However, the external oscillator bit is inverted on PMIC version 0x11.
> > The PMIC driver detects PMIC version and instantiate appropriate RTC
> > device based on i2c_device_id.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v2->v3:
> >  * RTC device is instantiated by PMIC driver and dropped
> isl1208_probe_helper().
> >  * Added "TYPE_RAA215300_RTC_A0" to handle inverted oscillator bit
> case.
> > RFC->v2:
> >  * Dropped compatible "renesas,raa215300-isl1208" and
> "renesas,raa215300-pmic" property.
> >  * Updated the comment polarity->bit for External Oscillator.
> >  * Added raa215300_rtc_probe_helper() for registering raa215300_rtc
> device and
> >    added the helper function isl1208_probe_helper() to share the code.
> 
> Thanks for the update!
> 
> > --- a/drivers/rtc/rtc-isl1208.c
> > +++ b/drivers/rtc/rtc-isl1208.c
> > @@ -74,6 +74,7 @@ enum isl1208_id {
> >         TYPE_ISL1209,
> >         TYPE_ISL1218,
> >         TYPE_ISL1219,
> > +       TYPE_RAA215300_RTC_A0,
> >         ISL_LAST_ID
> >  };
> >
> > @@ -83,11 +84,13 @@ static const struct isl1208_config {
> >         unsigned int    nvmem_length;
> >         unsigned        has_tamper:1;
> >         unsigned        has_timestamp:1;
> > +       unsigned        has_inverted_osc_bit:1;
> >  } isl1208_configs[] = {
> >         [TYPE_ISL1208] = { "isl1208", 2, false, false },
> >         [TYPE_ISL1209] = { "isl1209", 2, true,  false },
> >         [TYPE_ISL1218] = { "isl1218", 8, false, false },
> >         [TYPE_ISL1219] = { "isl1219", 2, true,  true },
> > +       [TYPE_RAA215300_RTC_A0] = { "rtc_a0", 2, false, false, true },
> >  };
> >
> >  static const struct i2c_device_id isl1208_id[] = { @@ -95,6 +98,7 @@
> > static const struct i2c_device_id isl1208_id[] = {
> >         { "isl1209", TYPE_ISL1209 },
> >         { "isl1218", TYPE_ISL1218 },
> >         { "isl1219", TYPE_ISL1219 },
> > +       { "rtc_a0", TYPE_RAA215300_RTC_A0 },
> 
> "rtc_a0" is IMHO a too-generic name.

I tried to squeeze with string length "8".

What about changing it to "raa215300_a0" and changing length to
"12"? as version A0 of RAA215300 pmic chip have this inverted oscillator bit.

> 
> 
> >         { }
> >  };
> >  MODULE_DEVICE_TABLE(i2c, isl1208_id); @@ -166,6 +170,16 @@
> > isl1208_i2c_validate_client(struct i2c_client *client)
> >         return 0;
> >  }
> >
> > +static int
> > +isl1208_set_external_oscillator(struct i2c_client *client, int rc,
> 
> s/rc/sr/

Agreed. But I plan to drop this function in next version.
Please see below.

> 
> > +                               bool is_inverted_oscillator_bit) {
> > +       if (is_inverted_oscillator_bit)
> 
> This condition is always true, given all callers in your series.

Ok.

> 
> > +               rc |= ISL1208_REG_SR_XTOSCB;
> 
> If you do decide to keep it, you probably want to add an else branch to
> make sure the bit is cleared.

No, I am planning to drop it. Please see below.


> 
> > +
> > +       return i2c_smbus_write_byte_data(client, ISL1208_REG_SR, rc);
> > +}
> > +
> >  static int
> >  isl1208_i2c_get_sr(struct i2c_client *client)  { @@ -845,6 +859,13 @@
> > isl1208_probe(struct i2c_client *client)
> >                 return rc;
> >         }
> >
> > +       if (isl1208->config->has_inverted_osc_bit) {
> > +               rc = isl1208_set_external_oscillator(client, rc,
> 
> Passing "rc" is confusing, this is really the status register value
> obtained above...

I am planning to drop this function in next version and will use the below logic instead.
Is it ok?

         if (isl1208->config->has_inverted_osc_bit) {   
		    int sr;
                          
                 sr = i2c_smbus_write_byte_data(client, ISL1208_REG_SR,           
                                              rc | ISL1208_REG_SR_XTOSCB);      
                 if (sr)                                                          
                         return sr;                                               
         }                                                                        
           

> 
> > +                                                    isl1208->config-
> >has_inverted_osc_bit);
> > +               if (rc)
> > +                       return rc;
> 
> If we get here, rc is always zero ...
> 
> > +       }
> > +
> >         if (rc & ISL1208_REG_SR_RTCF)
> 
> ... thus breaking this check..

Oops, missed it.

Cheers,
Biju

RE: [PATCH v3 4/5] mfd: Add Renesas PMIC RAA215300 driver

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v3 4/5] mfd: Add Renesas PMIC RAA215300 driver
> 
> Hi Biju,
> 
> On Sat, May 13, 2023 at 6:53 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > The RAA215300 is a 9-channel PMIC that consists of
> >  * Internally compensated regulators
> >  * built-in Real Time Clock (RTC)
> >  * 32kHz crystal oscillator
> >  * coin cell battery charger
> >
> > The RTC on RAA215300 is similar to the IP found in the ISL1208.
> > The existing driver for the ISL1208 works for this PMIC too, however
> > the RAA215300 exposes two devices via I2C, one for the RTC IP, and one
> > for everything else. The RTC IP has to be enabled by the other I2C
> > device, therefore this driver is necessary to get the RTC to work.
> >
> > The external oscillator bit is inverted on PMIC version 0x11.
> >
> > Add PMIC RAA215300 driver for enabling RTC block and instantiating RTC
> > device based on PMIC version.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v2->v3:
> >  * Updated commit description
> >  * Added support for handling "renesas,rtc-enabled" property.
> >  * Based on PMIC version, it instantiates rtc device by calling
> i2c_new_
> >    ancillary_device().
> 
> Thanks for the update!
> 
> > RFC->V2:
> >  * Dropped MODULE_SOFTDEP from the driver as it is added in RTC
> platform
> >    driver.
> > ---
> >  drivers/mfd/Kconfig     |   7 +++
> >  drivers/mfd/Makefile    |   2 +
> >  drivers/mfd/raa215300.c | 102
> > ++++++++++++++++++++++++++++++++++++++++
> 
> Note that this driver no longer uses any MFD APIs...
> Perhaps it should be moved to drivers/regulator/ instead?

Agreed, Will move it to drivers/regulators.

Cheers,
Biju

Re: [PATCH v3 2/5] rtc: isl1208: Add support for the built-in RTC on the PMIC RAA215300

[Geert Uytterhoeven]

Hi Biju,

On Tue, May 16, 2023 at 10:46 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v3 2/5] rtc: isl1208: Add support for the built-in
> > RTC on the PMIC RAA215300
> > On Sat, May 13, 2023 at 6:52 PM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > The built-in RTC found on PMIC RAA215300 is the same as ISL1208.
> > > However, the external oscillator bit is inverted on PMIC version 0x11.
> > > The PMIC driver detects PMIC version and instantiate appropriate RTC
> > > device based on i2c_device_id.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > > v2->v3:
> > >  * RTC device is instantiated by PMIC driver and dropped
> > isl1208_probe_helper().
> > >  * Added "TYPE_RAA215300_RTC_A0" to handle inverted oscillator bit
> > case.
> > > RFC->v2:
> > >  * Dropped compatible "renesas,raa215300-isl1208" and
> > "renesas,raa215300-pmic" property.
> > >  * Updated the comment polarity->bit for External Oscillator.
> > >  * Added raa215300_rtc_probe_helper() for registering raa215300_rtc
> > device and
> > >    added the helper function isl1208_probe_helper() to share the code.
> >
> > Thanks for the update!
> >
> > > --- a/drivers/rtc/rtc-isl1208.c
> > > +++ b/drivers/rtc/rtc-isl1208.c
> > > @@ -74,6 +74,7 @@ enum isl1208_id {
> > >         TYPE_ISL1209,
> > >         TYPE_ISL1218,
> > >         TYPE_ISL1219,
> > > +       TYPE_RAA215300_RTC_A0,
> > >         ISL_LAST_ID
> > >  };
> > >
> > > @@ -83,11 +84,13 @@ static const struct isl1208_config {
> > >         unsigned int    nvmem_length;
> > >         unsigned        has_tamper:1;
> > >         unsigned        has_timestamp:1;
> > > +       unsigned        has_inverted_osc_bit:1;
> > >  } isl1208_configs[] = {
> > >         [TYPE_ISL1208] = { "isl1208", 2, false, false },
> > >         [TYPE_ISL1209] = { "isl1209", 2, true,  false },
> > >         [TYPE_ISL1218] = { "isl1218", 8, false, false },
> > >         [TYPE_ISL1219] = { "isl1219", 2, true,  true },
> > > +       [TYPE_RAA215300_RTC_A0] = { "rtc_a0", 2, false, false, true },
> > >  };
> > >
> > >  static const struct i2c_device_id isl1208_id[] = { @@ -95,6 +98,7 @@
> > > static const struct i2c_device_id isl1208_id[] = {
> > >         { "isl1209", TYPE_ISL1209 },
> > >         { "isl1218", TYPE_ISL1218 },
> > >         { "isl1219", TYPE_ISL1219 },
> > > +       { "rtc_a0", TYPE_RAA215300_RTC_A0 },
> >
> > "rtc_a0" is IMHO a too-generic name.
>
> I tried to squeeze with string length "8".
>
> What about changing it to "raa215300_a0" and changing length to
> "12"? as version A0 of RAA215300 pmic chip have this inverted oscillator bit.

Ah, because of the size limit of isl1208_config.name[]?
Note that that field is only initialized, but further unused, so you
can just drop it.

BTW, isl1208_id[].driver_data could store a pointer to the config,
like for DT-based matching, making I2C and DT-based matching
more similar.

> > >  isl1208_i2c_get_sr(struct i2c_client *client)  { @@ -845,6 +859,13 @@
> > > isl1208_probe(struct i2c_client *client)
> > >                 return rc;
> > >         }
> > >
> > > +       if (isl1208->config->has_inverted_osc_bit) {
> > > +               rc = isl1208_set_external_oscillator(client, rc,
> >
> > Passing "rc" is confusing, this is really the status register value
> > obtained above...
>
> I am planning to drop this function in next version and will use the below logic instead.
> Is it ok?
>
>          if (isl1208->config->has_inverted_osc_bit) {
>                     int sr;
>
>                  sr = i2c_smbus_write_byte_data(client, ISL1208_REG_SR,
>                                               rc | ISL1208_REG_SR_XTOSCB);
>                  if (sr)
>                          return sr;

Isn't this more confusing: "rc" is the Status Register value, and "sr"
is the Return Code?


>          }
>
>
> >
> > > +                                                    isl1208->config-
> > >has_inverted_osc_bit);
> > > +               if (rc)
> > > +                       return rc;
> >
> > If we get here, rc is always zero ...
> >
> > > +       }
> > > +
> > >         if (rc & ISL1208_REG_SR_RTCF)
> >
> > ... thus breaking this check..
>
> Oops, missed it.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v3 3/5] dt-bindings: mfd: Add Renesas RAA215300 PMIC bindings

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Tuesday, May 16, 2023 9:00 AM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Lee Jones <lee@kernel.org>; Rob Herring <robh+dt@kernel.org>;
> Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Magnus Damm
> <magnus.damm@gmail.com>; devicetree@vger.kernel.org; linux-renesas-
> soc@vger.kernel.org; Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> Subject: Re: [PATCH v3 3/5] dt-bindings: mfd: Add Renesas RAA215300 PMIC
> bindings
> 
> Hi Biju,
> 
> On Sat, May 13, 2023 at 6:52 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> >
> > Document Renesas RAA215300 PMIC bindings.
> >
> > The RAA215300 is a high Performance 9-Channel PMIC supporting DDR
> > Memory, with Built-In Charger and RTC.
> >
> > It supports DDR3, DDR3L, DDR4, and LPDDR4 memory power requirements.
> > The internally compensated regulators, built-in Real-Time Clock (RTC),
> > 32kHz crystal oscillator, and coin cell battery charger provide a
> > highly integrated, small footprint power solution ideal for
> > System-On-Module (SOM) applications. A spread spectrum feature
> > provides an ease-of-use solution for noise-sensitive audio or RF
> > applications.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v2->v3:
> >  * Added more detailed description for renesas,rtc-enabled property.
> 
> Thanks for your patch!
> 
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/renesas,raa215300.yaml
> 
> > +  renesas,rtc-enabled:
> > +    description:
> > +      To indicate RTC is enabled on the PMIC.
> > +      Enabling of the RTC is based on system design. System designers
> may
> > +      choose not to populate built-in RTC by grounding XIN and XOUT
> pins.
> > +    type: boolean
> 
> Perhaps you should go full DT monty and replace this logic by a clocks
> property pointing to the external crystal?

OK for me. Krzysztof Kozlowski, Rob are you ok with this?

> 
> However, as I only have the Short-Form Datasheet, I am wondering what
> "Built-in 32kHz crystal oscillator (with bypass)" really means?

It is not mentioned in original doc as well.

> Does this mean the RTC can work without an external crystal, using an
> on-chip oscillator?

I am checking with hardware team. I will update you once I get this info.

Cheers,
Biju

RE: [PATCH v3 2/5] rtc: isl1208: Add support for the built-in RTC on the PMIC RAA215300

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Tuesday, May 16, 2023 9:58 AM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; Magnus Damm <magnus.damm@gmail.com>;
> Lee Jones <lee@kernel.org>; linux-rtc@vger.kernel.org; linux-renesas-
> soc@vger.kernel.org; Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> Subject: Re: [PATCH v3 2/5] rtc: isl1208: Add support for the built-in
> RTC on the PMIC RAA215300
> 
> Hi Biju,
> 
> On Tue, May 16, 2023 at 10:46 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH v3 2/5] rtc: isl1208: Add support for the
> > > built-in RTC on the PMIC RAA215300 On Sat, May 13, 2023 at 6:52 PM
> > > Biju Das <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > The built-in RTC found on PMIC RAA215300 is the same as ISL1208.
> > > > However, the external oscillator bit is inverted on PMIC version
> 0x11.
> > > > The PMIC driver detects PMIC version and instantiate appropriate
> > > > RTC device based on i2c_device_id.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > ---
> > > > v2->v3:
> > > >  * RTC device is instantiated by PMIC driver and dropped
> > > isl1208_probe_helper().
> > > >  * Added "TYPE_RAA215300_RTC_A0" to handle inverted oscillator bit
> > > case.
> > > > RFC->v2:
> > > >  * Dropped compatible "renesas,raa215300-isl1208" and
> > > "renesas,raa215300-pmic" property.
> > > >  * Updated the comment polarity->bit for External Oscillator.
> > > >  * Added raa215300_rtc_probe_helper() for registering
> > > > raa215300_rtc
> > > device and
> > > >    added the helper function isl1208_probe_helper() to share the
> code.
> > >
> > > Thanks for the update!
> > >
> > > > --- a/drivers/rtc/rtc-isl1208.c
> > > > +++ b/drivers/rtc/rtc-isl1208.c
> > > > @@ -74,6 +74,7 @@ enum isl1208_id {
> > > >         TYPE_ISL1209,
> > > >         TYPE_ISL1218,
> > > >         TYPE_ISL1219,
> > > > +       TYPE_RAA215300_RTC_A0,
> > > >         ISL_LAST_ID
> > > >  };
> > > >
> > > > @@ -83,11 +84,13 @@ static const struct isl1208_config {
> > > >         unsigned int    nvmem_length;
> > > >         unsigned        has_tamper:1;
> > > >         unsigned        has_timestamp:1;
> > > > +       unsigned        has_inverted_osc_bit:1;
> > > >  } isl1208_configs[] = {
> > > >         [TYPE_ISL1208] = { "isl1208", 2, false, false },
> > > >         [TYPE_ISL1209] = { "isl1209", 2, true,  false },
> > > >         [TYPE_ISL1218] = { "isl1218", 8, false, false },
> > > >         [TYPE_ISL1219] = { "isl1219", 2, true,  true },
> > > > +       [TYPE_RAA215300_RTC_A0] = { "rtc_a0", 2, false, false,
> > > > + true },
> > > >  };
> > > >
> > > >  static const struct i2c_device_id isl1208_id[] = { @@ -95,6 +98,7
> > > > @@ static const struct i2c_device_id isl1208_id[] = {
> > > >         { "isl1209", TYPE_ISL1209 },
> > > >         { "isl1218", TYPE_ISL1218 },
> > > >         { "isl1219", TYPE_ISL1219 },
> > > > +       { "rtc_a0", TYPE_RAA215300_RTC_A0 },
> > >
> > > "rtc_a0" is IMHO a too-generic name.
> >
> > I tried to squeeze with string length "8".
> >
> > What about changing it to "raa215300_a0" and changing length to "12"?
> > as version A0 of RAA215300 pmic chip have this inverted oscillator
> bit.
> 
> Ah, because of the size limit of isl1208_config.name[]?
> Note that that field is only initialized, but further unused, so you can
> just drop it.

Agreed. It will look like

static const struct isl1208_config {
-       const char      name[8];
        unsigned int    nvmem_length;
        unsigned        has_tamper:1;
        unsigned        has_timestamp:1;
        unsigned        has_inverted_osc_bit:1;
 } isl1208_configs[] = {
-       [TYPE_ISL1208] = { "isl1208", 2, false, false },
-       [TYPE_ISL1209] = { "isl1209", 2, true,  false },
-       [TYPE_ISL1218] = { "isl1218", 8, false, false },
-       [TYPE_ISL1219] = { "isl1219", 2, true,  true },
-       [TYPE_RAA215300_RTC_A0] = { "rtc_a0", 2, false, false, true },
+       [TYPE_ISL1208] = { 2, false, false },
+       [TYPE_ISL1209] = { 2, true,  false },
+       [TYPE_ISL1218] = { 8, false, false },
+       [TYPE_ISL1219] = { 2, true,  true },
+       [TYPE_RAA215300_RTC_A0] = { 2, false, false, true },
 };

> 
> BTW, isl1208_id[].driver_data could store a pointer to the config, like
> for DT-based matching, making I2C and DT-based matching more similar.

OK. But some type casting required

+       { "isl1208", .driver_data = (unsigned long)&isl1208_configs[TYPE_ISL1208] },
+       { "isl1209", .driver_data = (unsigned long)&isl1208_configs[TYPE_ISL1209] },
+       { "isl1218", .driver_data = (unsigned long)&isl1208_configs[TYPE_ISL1218] },
+       { "isl1219", .driver_data = (unsigned long)&isl1208_configs[TYPE_ISL1219] },
+       { "raa215300_rtc_a0", .driver_data = (unsigned long)&isl1208_configs[TYPE_RAA215300_RTC_A0] },


And

In probe()

-               if (id->driver_data >= ISL_LAST_ID)
+               if (!id)
                        return -ENODEV;
-               isl1208->config = &isl1208_configs[id->driver_data];
+               isl1208->config = (struct isl1208_config *)id->driver_data;

> 
> > > >  isl1208_i2c_get_sr(struct i2c_client *client)  { @@ -845,6
> > > > +859,13 @@ isl1208_probe(struct i2c_client *client)
> > > >                 return rc;
> > > >         }
> > > >
> > > > +       if (isl1208->config->has_inverted_osc_bit) {
> > > > +               rc = isl1208_set_external_oscillator(client, rc,
> > >
> > > Passing "rc" is confusing, this is really the status register value
> > > obtained above...
> >
> > I am planning to drop this function in next version and will use the
> below logic instead.
> > Is it ok?
> >
> >          if (isl1208->config->has_inverted_osc_bit) {
> >                     int sr;
> >
> >                  sr = i2c_smbus_write_byte_data(client,
> ISL1208_REG_SR,
> >                                               rc |
> ISL1208_REG_SR_XTOSCB);
> >                  if (sr)
> >                          return sr;
> 
> Isn't this more confusing: "rc" is the Status Register value, and "sr"
> is the Return Code?

OK will use "ret" instead.

      if (isl1208->config->has_inverted_osc_bit) {
+               int ret;
+
+               ret = i2c_smbus_write_byte_data(client, ISL1208_REG_SR,
+                                               rc | ISL1208_REG_SR_XTOSCB);
+               if (ret)
+                       return ret;
        }

Cheers,
Biju

RE: [PATCH v3 1/5] i2c: Enhance i2c_new_ancillary_device API

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v3 1/5] i2c: Enhance i2c_new_ancillary_device API
> 
> Hi Biju,
> 
> On Sat, May 13, 2023 at 6:52 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > Renesas PMIC RAA215300 exposes two separate i2c devices, one for the
> > main device and another for rtc device.
> >
> > Enhance i2c_new_ancillary_device() to instantiate a real device.
> > (eg: Instantiate rtc device from PMIC driver)
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v3:
> >  * New patch
> 
> Thanks for your patch!
> 
> Looks correct to me, so
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Some suggestions for improvement below...

OK.

> 
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -1153,7 +1157,27 @@ struct i2c_client
> *i2c_new_ancillary_device(struct i2c_client *client,
> >         }
> >
> >         dev_dbg(&client->adapter->dev, "Address for %s : 0x%x\n",
> name, addr);
> > -       return i2c_new_dummy_device(client->adapter, addr);
> > +
> > +       if (aux_device_name) {
> > +               struct i2c_board_info info;
> > +               size_t aux_device_name_len = strlen(aux_device_name);
> > +
> > +               if (aux_device_name_len > I2C_NAME_SIZE - 1) {
> > +                       dev_err(&client->adapter->dev, "Invalid device
> name\n");
> > +                       return ERR_PTR(-EINVAL);
> > +               }
> 
> strscpy() return value?
> 
> > +
> > +               memset(&info, 0, sizeof(struct i2c_board_info));
> 
> The call to memset() would not be needed if info would be initialized at
> declaration time, i.e.
> 
>     struct i2c_board_info info = { .addr = addr };
> 
> Or, use I2C_BOARD_INFO(), to guarantee initialization is aligned with
> whatever future changes made to i2c_board_info? But that relies on
> providing the name at declaration time, which we already have in
> i2c_new_dummy_device().
> 
> So I suggest to add a name parameter to i2c_new_dummy_device(), rename
> it to __i2c_new_dummy_device(), and create a wrapper for compatibility
> with existing users:
> 
>     struct i2c_client *__i2c_new_dummy_device(struct i2c_adapter
> *adapter, u16 address,
>                                              const char *name)
>     {
>             struct i2c_board_info info = {
>                     I2C_BOARD_INFO("dummy", address),
>             };
> 
>             if (name) {
>                     ssize_ret = strscpy(info.type, name,
> sizeof(info.type));
> 
>                     if (ret < 0)
>                             return ERR_PTR(dev_err_probe(&client-
> >adapter->dev,
>                                            ret, "Invalid device
> name\n");
>             }
> 
>             return i2c_new_client_device(adapter, &info);
>     }

OK will introduce, static function

static struct i2c_client *__i2c_new_dummy_device(struct i2c_adapter *adapter,
                                                u16 address, const char *name)

and is called by both "i2c_new_dummy_device" and "i2c_new_ancillary_device"

Cheers,
Biju


> 
> > +
> > +               memcpy(info.type, aux_device_name,
> aux_device_name_len);
> > +               info.addr = addr;
> > +
> > +               i2c_aux_client = i2c_new_client_device(client-
> >adapter, &info);
> > +       } else {
> > +               i2c_aux_client = i2c_new_dummy_device(client->adapter,
> addr);
> > +       }
> > +
> > +       return i2c_aux_client;
> >  }
> >  EXPORT_SYMBOL_GPL(i2c_new_ancillary_device);

Re: [PATCH v3 2/5] rtc: isl1208: Add support for the built-in RTC on the PMIC RAA215300

[Geert Uytterhoeven]

Hi Biju,

On Tue, May 16, 2023 at 12:22 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > -----Original Message-----
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > Sent: Tuesday, May 16, 2023 9:58 AM
> > To: Biju Das <biju.das.jz@bp.renesas.com>
> > Cc: Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni
> > <alexandre.belloni@bootlin.com>; Magnus Damm <magnus.damm@gmail.com>;
> > Lee Jones <lee@kernel.org>; linux-rtc@vger.kernel.org; linux-renesas-
> > soc@vger.kernel.org; Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > Subject: Re: [PATCH v3 2/5] rtc: isl1208: Add support for the built-in
> > RTC on the PMIC RAA215300
> >
> > On Tue, May 16, 2023 at 10:46 AM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > > Subject: Re: [PATCH v3 2/5] rtc: isl1208: Add support for the
> > > > built-in RTC on the PMIC RAA215300 On Sat, May 13, 2023 at 6:52 PM
> > > > Biju Das <biju.das.jz@bp.renesas.com>
> > > > wrote:
> > > > > The built-in RTC found on PMIC RAA215300 is the same as ISL1208.
> > > > > However, the external oscillator bit is inverted on PMIC version
> > 0x11.
> > > > > The PMIC driver detects PMIC version and instantiate appropriate
> > > > > RTC device based on i2c_device_id.
> > > > >
> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > ---
> > > > > v2->v3:
> > > > >  * RTC device is instantiated by PMIC driver and dropped
> > > > isl1208_probe_helper().
> > > > >  * Added "TYPE_RAA215300_RTC_A0" to handle inverted oscillator bit
> > > > case.
> > > > > RFC->v2:
> > > > >  * Dropped compatible "renesas,raa215300-isl1208" and
> > > > "renesas,raa215300-pmic" property.
> > > > >  * Updated the comment polarity->bit for External Oscillator.
> > > > >  * Added raa215300_rtc_probe_helper() for registering
> > > > > raa215300_rtc
> > > > device and
> > > > >    added the helper function isl1208_probe_helper() to share the
> > code.
> > > >
> > > > Thanks for the update!
> > > >
> > > > > --- a/drivers/rtc/rtc-isl1208.c
> > > > > +++ b/drivers/rtc/rtc-isl1208.c
> > > > > @@ -74,6 +74,7 @@ enum isl1208_id {
> > > > >         TYPE_ISL1209,
> > > > >         TYPE_ISL1218,
> > > > >         TYPE_ISL1219,
> > > > > +       TYPE_RAA215300_RTC_A0,
> > > > >         ISL_LAST_ID
> > > > >  };
> > > > >
> > > > > @@ -83,11 +84,13 @@ static const struct isl1208_config {
> > > > >         unsigned int    nvmem_length;
> > > > >         unsigned        has_tamper:1;
> > > > >         unsigned        has_timestamp:1;
> > > > > +       unsigned        has_inverted_osc_bit:1;
> > > > >  } isl1208_configs[] = {
> > > > >         [TYPE_ISL1208] = { "isl1208", 2, false, false },
> > > > >         [TYPE_ISL1209] = { "isl1209", 2, true,  false },
> > > > >         [TYPE_ISL1218] = { "isl1218", 8, false, false },
> > > > >         [TYPE_ISL1219] = { "isl1219", 2, true,  true },
> > > > > +       [TYPE_RAA215300_RTC_A0] = { "rtc_a0", 2, false, false,
> > > > > + true },
> > > > >  };
> > > > >
> > > > >  static const struct i2c_device_id isl1208_id[] = { @@ -95,6 +98,7
> > > > > @@ static const struct i2c_device_id isl1208_id[] = {
> > > > >         { "isl1209", TYPE_ISL1209 },
> > > > >         { "isl1218", TYPE_ISL1218 },
> > > > >         { "isl1219", TYPE_ISL1219 },
> > > > > +       { "rtc_a0", TYPE_RAA215300_RTC_A0 },
> > > >
> > > > "rtc_a0" is IMHO a too-generic name.
> > >
> > > I tried to squeeze with string length "8".
> > >
> > > What about changing it to "raa215300_a0" and changing length to "12"?
> > > as version A0 of RAA215300 pmic chip have this inverted oscillator
> > bit.
> >
> > Ah, because of the size limit of isl1208_config.name[]?
> > Note that that field is only initialized, but further unused, so you can
> > just drop it.
>
> Agreed. It will look like
>
> static const struct isl1208_config {
> -       const char      name[8];
>         unsigned int    nvmem_length;
>         unsigned        has_tamper:1;
>         unsigned        has_timestamp:1;
>         unsigned        has_inverted_osc_bit:1;
>  } isl1208_configs[] = {
> -       [TYPE_ISL1208] = { "isl1208", 2, false, false },
> -       [TYPE_ISL1209] = { "isl1209", 2, true,  false },
> -       [TYPE_ISL1218] = { "isl1218", 8, false, false },
> -       [TYPE_ISL1219] = { "isl1219", 2, true,  true },
> -       [TYPE_RAA215300_RTC_A0] = { "rtc_a0", 2, false, false, true },
> +       [TYPE_ISL1208] = { 2, false, false },
> +       [TYPE_ISL1209] = { 2, true,  false },
> +       [TYPE_ISL1218] = { 8, false, false },
> +       [TYPE_ISL1219] = { 2, true,  true },
> +       [TYPE_RAA215300_RTC_A0] = { 2, false, false, true },
>  };
>
> >
> > BTW, isl1208_id[].driver_data could store a pointer to the config, like
> > for DT-based matching, making I2C and DT-based matching more similar.
>
> OK. But some type casting required
>
> +       { "isl1208", .driver_data = (unsigned long)&isl1208_configs[TYPE_ISL1208] },
> +       { "isl1209", .driver_data = (unsigned long)&isl1208_configs[TYPE_ISL1209] },
> +       { "isl1218", .driver_data = (unsigned long)&isl1208_configs[TYPE_ISL1218] },
> +       { "isl1219", .driver_data = (unsigned long)&isl1208_configs[TYPE_ISL1219] },
> +       { "raa215300_rtc_a0", .driver_data = (unsigned long)&isl1208_configs[TYPE_RAA215300_RTC_A0] },

Now there are no more users left of isl1208_configs[], you can split
the array in individual variables, and make lines shorter by referring
to e.g. &config_isl1219 instead of &isl1208_configs[TYPE_ISL1219].

> > > > >  isl1208_i2c_get_sr(struct i2c_client *client)  { @@ -845,6
> > > > > +859,13 @@ isl1208_probe(struct i2c_client *client)
> > > > >                 return rc;
> > > > >         }
> > > > >
> > > > > +       if (isl1208->config->has_inverted_osc_bit) {
> > > > > +               rc = isl1208_set_external_oscillator(client, rc,
> > > >
> > > > Passing "rc" is confusing, this is really the status register value
> > > > obtained above...
> > >
> > > I am planning to drop this function in next version and will use the
> > below logic instead.
> > > Is it ok?
> > >
> > >          if (isl1208->config->has_inverted_osc_bit) {
> > >                     int sr;
> > >
> > >                  sr = i2c_smbus_write_byte_data(client,
> > ISL1208_REG_SR,
> > >                                               rc |
> > ISL1208_REG_SR_XTOSCB);
> > >                  if (sr)
> > >                          return sr;
> >
> > Isn't this more confusing: "rc" is the Status Register value, and "sr"
> > is the Return Code?
>
> OK will use "ret" instead.

Use "ret" instead of what? ;-)
Just declare "int sr" at the top, and use that to store the Status Register
value, and use rc everywhere else?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v3 2/5] rtc: isl1208: Add support for the built-in RTC on the PMIC RAA215300

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Tuesday, May 16, 2023 1:20 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; Magnus Damm <magnus.damm@gmail.com>;
> Lee Jones <lee@kernel.org>; linux-rtc@vger.kernel.org; linux-renesas-
> soc@vger.kernel.org; Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> Subject: Re: [PATCH v3 2/5] rtc: isl1208: Add support for the built-in
> RTC on the PMIC RAA215300
> 
> Hi Biju,
> 
> On Tue, May 16, 2023 at 12:22 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > -----Original Message-----
> > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > Sent: Tuesday, May 16, 2023 9:58 AM
> > > To: Biju Das <biju.das.jz@bp.renesas.com>
> > > Cc: Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni
> > > <alexandre.belloni@bootlin.com>; Magnus Damm
> > > <magnus.damm@gmail.com>; Lee Jones <lee@kernel.org>;
> > > linux-rtc@vger.kernel.org; linux-renesas- soc@vger.kernel.org;
> > > Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > > Subject: Re: [PATCH v3 2/5] rtc: isl1208: Add support for the
> > > built-in RTC on the PMIC RAA215300
> > >
> > > On Tue, May 16, 2023 at 10:46 AM Biju Das
> > > <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > > Subject: Re: [PATCH v3 2/5] rtc: isl1208: Add support for the
> > > > > built-in RTC on the PMIC RAA215300 On Sat, May 13, 2023 at 6:52 
> > > > > PM Biju Das <biju.das.jz@bp.renesas.com>
> > > > > wrote:
> > > > > > The built-in RTC found on PMIC RAA215300 is the same as
> ISL1208.
> > > > > > However, the external oscillator bit is inverted on PMIC
> > > > > > version
> > > 0x11.
> > > > > > The PMIC driver detects PMIC version and instantiate
> > > > > > appropriate RTC device based on i2c_device_id.
> > > > > >
> > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > ---
> > > > > > v2->v3:
> > > > > >  * RTC device is instantiated by PMIC driver and dropped
> > > > > isl1208_probe_helper().
> > > > > >  * Added "TYPE_RAA215300_RTC_A0" to handle inverted oscillator
> > > > > > bit
> > > > > case.
> > > > > > RFC->v2:
> > > > > >  * Dropped compatible "renesas,raa215300-isl1208" and
> > > > > "renesas,raa215300-pmic" property.
> > > > > >  * Updated the comment polarity->bit for External Oscillator.
> > > > > >  * Added raa215300_rtc_probe_helper() for registering
> > > > > > raa215300_rtc
> > > > > device and
> > > > > >    added the helper function isl1208_probe_helper() to share
> > > > > > the
> > > code.
> > > > >
> > > > > Thanks for the update!
> > > > >
> > > > > > --- a/drivers/rtc/rtc-isl1208.c
> > > > > > +++ b/drivers/rtc/rtc-isl1208.c
> > > > > > @@ -74,6 +74,7 @@ enum isl1208_id {
> > > > > >         TYPE_ISL1209,
> > > > > >         TYPE_ISL1218,
> > > > > >         TYPE_ISL1219,
> > > > > > +       TYPE_RAA215300_RTC_A0,
> > > > > >         ISL_LAST_ID
> > > > > >  };
> > > > > >
> > > > > > @@ -83,11 +84,13 @@ static const struct isl1208_config {
> > > > > >         unsigned int    nvmem_length;
> > > > > >         unsigned        has_tamper:1;
> > > > > >         unsigned        has_timestamp:1;
> > > > > > +       unsigned        has_inverted_osc_bit:1;
> > > > > >  } isl1208_configs[] = {
> > > > > >         [TYPE_ISL1208] = { "isl1208", 2, false, false },
> > > > > >         [TYPE_ISL1209] = { "isl1209", 2, true,  false },
> > > > > >         [TYPE_ISL1218] = { "isl1218", 8, false, false },
> > > > > >         [TYPE_ISL1219] = { "isl1219", 2, true,  true },
> > > > > > +       [TYPE_RAA215300_RTC_A0] = { "rtc_a0", 2, false, false,
> > > > > > + true },
> > > > > >  };
> > > > > >
> > > > > >  static const struct i2c_device_id isl1208_id[] = { @@ -95,6
> > > > > > +98,7 @@ static const struct i2c_device_id isl1208_id[] = {
> > > > > >         { "isl1209", TYPE_ISL1209 },
> > > > > >         { "isl1218", TYPE_ISL1218 },
> > > > > >         { "isl1219", TYPE_ISL1219 },
> > > > > > +       { "rtc_a0", TYPE_RAA215300_RTC_A0 },
> > > > >
> > > > > "rtc_a0" is IMHO a too-generic name.
> > > >
> > > > I tried to squeeze with string length "8".
> > > >
> > > > What about changing it to "raa215300_a0" and changing length to
> "12"?
> > > > as version A0 of RAA215300 pmic chip have this inverted oscillator
> > > bit.
> > >
> > > Ah, because of the size limit of isl1208_config.name[]?
> > > Note that that field is only initialized, but further unused, so you
> > > can just drop it.
> >
> > Agreed. It will look like
> >
> > static const struct isl1208_config {
> > -       const char      name[8];
> >         unsigned int    nvmem_length;
> >         unsigned        has_tamper:1;
> >         unsigned        has_timestamp:1;
> >         unsigned        has_inverted_osc_bit:1;
> >  } isl1208_configs[] = {
> > -       [TYPE_ISL1208] = { "isl1208", 2, false, false },
> > -       [TYPE_ISL1209] = { "isl1209", 2, true,  false },
> > -       [TYPE_ISL1218] = { "isl1218", 8, false, false },
> > -       [TYPE_ISL1219] = { "isl1219", 2, true,  true },
> > -       [TYPE_RAA215300_RTC_A0] = { "rtc_a0", 2, false, false, true },
> > +       [TYPE_ISL1208] = { 2, false, false },
> > +       [TYPE_ISL1209] = { 2, true,  false },
> > +       [TYPE_ISL1218] = { 8, false, false },
> > +       [TYPE_ISL1219] = { 2, true,  true },
> > +       [TYPE_RAA215300_RTC_A0] = { 2, false, false, true },
> >  };
> >
> > >
> > > BTW, isl1208_id[].driver_data could store a pointer to the config,
> > > like for DT-based matching, making I2C and DT-based matching more
> similar.
> >
> > OK. But some type casting required
> >
> > +       { "isl1208", .driver_data = (unsigned
> long)&isl1208_configs[TYPE_ISL1208] },
> > +       { "isl1209", .driver_data = (unsigned
> long)&isl1208_configs[TYPE_ISL1209] },
> > +       { "isl1218", .driver_data = (unsigned
> long)&isl1208_configs[TYPE_ISL1218] },
> > +       { "isl1219", .driver_data = (unsigned
> long)&isl1208_configs[TYPE_ISL1219] },
> > +       { "raa215300_rtc_a0", .driver_data = (unsigned
> > + long)&isl1208_configs[TYPE_RAA215300_RTC_A0] },
> 
> Now there are no more users left of isl1208_configs[], you can split the
> array in individual variables, and make lines shorter by referring to
> e.g. &config_isl1219 instead of &isl1208_configs[TYPE_ISL1219].

It looks better now,

-/* ISL1208 various variants */
-enum isl1208_id {
-       TYPE_ISL1208 = 0,
-       TYPE_ISL1209,
-       TYPE_ISL1218,
-       TYPE_ISL1219,
-       TYPE_RAA215300_RTC_A0,
-       ISL_LAST_ID
-};
-

-static const struct isl1208_config {
+struct isl1208_config {
        unsigned int    nvmem_length;
        unsigned        has_tamper:1;
        unsigned        has_timestamp:1;
        unsigned        has_inverted_osc_bit:1;
-} isl1208_configs[] = {
-       [TYPE_ISL1208] = { 2, false, false },
-       [TYPE_ISL1209] = { 2, true,  false },
-       [TYPE_ISL1218] = { 8, false, false },
-       [TYPE_ISL1219] = { 2, true,  true },
-       [TYPE_RAA215300_RTC_A0] = { 2, false, false, true },
+};
+
+static const struct isl1208_config config_isl1208 = {
+       .nvmem_length = 2,
+       .has_tamper = false,
+       .has_timestamp = false
+};
+
+static const struct isl1208_config config_isl1209 = {
+       .nvmem_length = 2,
+       .has_tamper = true,
+       .has_timestamp = false
+};
+
+static const struct isl1208_config config_isl1218 = {
+       .nvmem_length = 8,
+       .has_tamper = false,
+       .has_timestamp = false
+};
+
+static const struct isl1208_config config_isl1219 = {
+       .nvmem_length = 2,
+       .has_tamper = true,
+       .has_timestamp = true
+};
+
+static const struct isl1208_config config_raa215300_a0 = {
+       .nvmem_length = 2,
+       .has_tamper = false,
+       .has_timestamp = false,
+       .has_inverted_osc_bit = true
 };
 
 static const struct i2c_device_id isl1208_id[] = {
-       { "isl1208", .driver_data = (unsigned long)&isl1208_configs[TYPE_ISL1208] },
-       { "isl1209", .driver_data = (unsigned long)&isl1208_configs[TYPE_ISL1209] },
-       { "isl1218", .driver_data = (unsigned long)&isl1208_configs[TYPE_ISL1218] },
-       { "isl1219", .driver_data = (unsigned long)&isl1208_configs[TYPE_ISL1219] },
-       { "raa215300_rtc_a0", .driver_data = (unsigned long)&isl1208_configs[TYPE_RAA215300_RTC_A0] },
+       { "isl1208", .driver_data = (unsigned long)&config_isl1208 },
+       { "isl1209", .driver_data = (unsigned long)&config_isl1209 },
+       { "isl1218", .driver_data = (unsigned long)&config_isl1218 },
+       { "isl1219", .driver_data = (unsigned long)&config_isl1219 },
+       { "raa215300_a0", .driver_data = (unsigned long)&config_raa215300_a0 },
        { }
 };
 MODULE_DEVICE_TABLE(i2c, isl1208_id);
 
 static const __maybe_unused struct of_device_id isl1208_of_match[] = {
-       { .compatible = "isil,isl1208", .data = &isl1208_configs[TYPE_ISL1208] },
-       { .compatible = "isil,isl1209", .data = &isl1208_configs[TYPE_ISL1209] },
-       { .compatible = "isil,isl1218", .data = &isl1208_configs[TYPE_ISL1218] },
-       { .compatible = "isil,isl1219", .data = &isl1208_configs[TYPE_ISL1219] },
+       { .compatible = "isil,isl1208", .data = &config_isl1208 },
+       { .compatible = "isil,isl1209", .data = &config_isl1209 },
+       { .compatible = "isil,isl1218", .data = &config_isl1218 },
+       { .compatible = "isil,isl1219", .data = &config_isl1219 },


> 
> > > > > >  isl1208_i2c_get_sr(struct i2c_client *client)  { @@ -845,6
> > > > > > +859,13 @@ isl1208_probe(struct i2c_client *client)
> > > > > >                 return rc;
> > > > > >         }
> > > > > >
> > > > > > +       if (isl1208->config->has_inverted_osc_bit) {
> > > > > > +               rc = isl1208_set_external_oscillator(client,
> > > > > > + rc,
> > > > >
> > > > > Passing "rc" is confusing, this is really the status register
> > > > > value obtained above...
> > > >
> > > > I am planning to drop this function in next version and will use
> > > > the
> > > below logic instead.
> > > > Is it ok?
> > > >
> > > >          if (isl1208->config->has_inverted_osc_bit) {
> > > >                     int sr;
> > > >
> > > >                  sr = i2c_smbus_write_byte_data(client,
> > > ISL1208_REG_SR,
> > > >                                               rc |
> > > ISL1208_REG_SR_XTOSCB);
> > > >                  if (sr)
> > > >                          return sr;
> > >
> > > Isn't this more confusing: "rc" is the Status Register value, and
> "sr"
> > > is the Return Code?
> >
> > OK will use "ret" instead.
> 
> Use "ret" instead of what? ;-)
> Just declare "int sr" at the top, and use that to store the Status
> Register value, and use rc everywhere else?

Agreed.

Maybe will create 2-3 patches. Is it ok to send RTC patches separate from the original series,
as there is no dependency?

First patch for removing name variable and also using sr variable to read status register in probe

Second patch for isl1208_id[].driver_data to store a pointer to the config, like for DT-based matching, making I2C and DT-based matching more similar and dropping enum isl1208_id.

Third patch is for adding support for inv oscillator bit.

Please let me know.

Cheers,
Biju

RE: [PATCH v3 3/5] dt-bindings: mfd: Add Renesas RAA215300 PMIC bindings

[Biju Das]

Hi Geert,

> Subject: RE: [PATCH v3 3/5] dt-bindings: mfd: Add Renesas RAA215300 PMIC
> bindings
> 
> Hi Geert,
> 
> Thanks for the feedback.
> 
> > -----Original Message-----
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > Sent: Tuesday, May 16, 2023 9:00 AM
> > To: Biju Das <biju.das.jz@bp.renesas.com>
> > Cc: Lee Jones <lee@kernel.org>; Rob Herring <robh+dt@kernel.org>;
> > Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Magnus Damm
> > <magnus.damm@gmail.com>; devicetree@vger.kernel.org; linux-renesas-
> > soc@vger.kernel.org; Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > Subject: Re: [PATCH v3 3/5] dt-bindings: mfd: Add Renesas RAA215300
> > PMIC bindings
> >
> > Hi Biju,
> >
> > On Sat, May 13, 2023 at 6:52 PM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > >
> > > Document Renesas RAA215300 PMIC bindings.
> > >
> > > The RAA215300 is a high Performance 9-Channel PMIC supporting DDR
> > > Memory, with Built-In Charger and RTC.
> > >
> > > It supports DDR3, DDR3L, DDR4, and LPDDR4 memory power requirements.
> > > The internally compensated regulators, built-in Real-Time Clock
> > > (RTC), 32kHz crystal oscillator, and coin cell battery charger
> > > provide a highly integrated, small footprint power solution ideal
> > > for System-On-Module (SOM) applications. A spread spectrum feature
> > > provides an ease-of-use solution for noise-sensitive audio or RF
> > > applications.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > > v2->v3:
> > >  * Added more detailed description for renesas,rtc-enabled property.
> >
> > Thanks for your patch!
> >
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mfd/renesas,raa215300.yaml
> >
> > > +  renesas,rtc-enabled:
> > > +    description:
> > > +      To indicate RTC is enabled on the PMIC.
> > > +      Enabling of the RTC is based on system design. System
> > > + designers
> > may
> > > +      choose not to populate built-in RTC by grounding XIN and XOUT
> > pins.
> > > +    type: boolean
> >
> > Perhaps you should go full DT monty and replace this logic by a clocks
> > property pointing to the external crystal?
> 
> OK for me. Krzysztof Kozlowski, Rob are you ok with this?
> 
> >
> > However, as I only have the Short-Form Datasheet, I am wondering what
> > "Built-in 32kHz crystal oscillator (with bypass)" really means?
> 
> It is not mentioned in original doc as well.
> 
> > Does this mean the RTC can work without an external crystal, using an
> > on-chip oscillator?
> 
> I am checking with hardware team. I will update you once I get this
> info.

Please find the answer from HW team, it is inline with current implementation.

Q1) Can you please clarify "Built-in 32kHz crystal oscillator (with bypass)" really means?

Ans:
The RTC on RAA215300 can work with an external 32.768kHz crystal or an external clock IC. So you can use it in two different ways:
1) Connect an external 32.768kHz crystal to XIN/XOUT. The on-chip oscillator needs to be enabled in this case, meaning the XTOSCB bit(0x07[6]) needs to be at ‘0’ (default value).
2) Connect an external clock IC of 32.768kHz to the XIN pin. The on-chip oscillator needs to be disabled (bypassed) in this case, meaning the XTOSCB bit needs to be at ‘1’.

Q2) Does this mean the RTC can work without an external crystal, using an on-chip oscillator?

Ans:
I guess you meant using the on-chip oscillator only without connecting anything to XIN/XOUT. That is not possible.

Cheers,
Biju