[PATCH v4 00/11] Add Renesas PMIC RAA215300 and built-in RTC support

[PATCH v4 00/11] 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

Ref:
 https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230505091720.115675-1-biju.das.jz@bp.renesas.com/
 https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230505172530.357455-5-biju.das.jz@bp.renesas.com/

v3->v4:
 * Dropped Rb tag from Geert for patch#1 as there are new changes.
 * Introduced __i2c_new_dummy_device() to share the code between
   i2c_new_dummy_device and i2c_new_ancillary_device().
 * Introduced __i2c_new_client_device() to pass parent dev
   parameter, so that the ancillary device can assign its parent during
   creation.
 * Added minItems to interrupt-names in binding patch.
 * Added interrupt-names in conditional schema check.
 * Documented clock and clock-names properties.
 * Dropped unused name variable from struct isl1208_config.
 * Make similar I2C and DT-based matching.
 * Drop enum isl1208_id and split the array isl1208_configs[].
 * Introduced isl1208_set_xtoscb() to set XTOSCB bit.
 * Added support for internal oscillator enable/disable.
 * Moved PMIC bindings from mfd->regulator.
 * Dropped minItems from reg.
 * Dropped renesas,rtc-enabled property and instead used clock-names property
   to find RTC is enabled or not.
 * Added reg-names in required property.
 * Updated the example.
 * Moved from mfd->regulator as it doesn't use MFD APIs
 * Dropped handling "renesas,rtc-enabled" property and instead used
   clock-names to determine RTC is enabled or not and then instantiating
   RTC device.
 * Added clock nodes.
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 (11):
  i2c: Enhance i2c_new_ancillary_device API
  dt-bindings: rtc: isl1208: Convert to json-schema
  dt-bindings: rtc: isil,isl1208: Document clock and clock-names
    properties
  rtc: isl1208: Drop name variable
  rtc: isl1208: Make similar I2C and DT-based matching table
  rtc: isl1208: Drop enum isl1208_id and split isl1208_configs[]
  rtc: isl1208: Add isl1208_set_xtoscb()
  rtc: isl1208: Add support for the built-in RTC on the PMIC RAA215300
  regulator: dt-bindings: Add Renesas RAA215300 PMIC bindings
  regulator: Add Renesas PMIC RAA215300 driver
  arm64: dts: renesas: rzg2l-smarc-som: Enable PMIC and built-in RTC

 .../bindings/regulator/renesas,raa215300.yaml |  84 ++++++++++++
 .../devicetree/bindings/rtc/isil,isl1208.txt  |  38 ------
 .../devicetree/bindings/rtc/isil,isl1208.yaml | 102 ++++++++++++++
 .../boot/dts/renesas/rzg2l-smarc-som.dtsi     |  18 +++
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c  |   6 +-
 drivers/i2c/i2c-core-base.c                   |  90 ++++++++-----
 drivers/media/i2c/adv748x/adv748x-core.c      |   2 +-
 drivers/media/i2c/adv7604.c                   |   3 +-
 drivers/regulator/Kconfig                     |   7 +
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/raa215300.c                 | 118 +++++++++++++++++
 drivers/rtc/rtc-isl1208.c                     | 125 +++++++++++++-----
 include/linux/i2c.h                           |   3 +-
 13 files changed, 491 insertions(+), 106 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/renesas,raa215300.yaml
 delete mode 100644 Documentation/devicetree/bindings/rtc/isil,isl1208.txt
 create mode 100644 Documentation/devicetree/bindings/rtc/isil,isl1208.yaml
 create mode 100644 drivers/regulator/raa215300.c

-- 
2.25.1

[PATCH v4 01/11] 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)

Added helper function __i2c_new_dummy_device to share the code
between i2c_new_dummy_device and i2c_new_ancillary_device().

Also added helper function __i2c_new_client_device() to pass parent dev
parameter, so that the ancillary device can assign its parent during
creation.

Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v3->v4:
 * Dropped Rb tag from Geert as there are new changes.
 * Introduced __i2c_new_dummy_device() to share the code between
   i2c_new_dummy_device and i2c_new_ancillary_device().
 * Introduced __i2c_new_client_device() to pass parent dev
   parameter, so that the ancillary device can assign its parent during
   creation.
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                  | 90 +++++++++++++-------
 drivers/media/i2c/adv748x/adv748x-core.c     |  2 +-
 drivers/media/i2c/adv7604.c                  |  3 +-
 include/linux/i2c.h                          |  3 +-
 5 files changed, 67 insertions(+), 37 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..f6d2a975f8b9 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -893,24 +893,10 @@ int i2c_dev_irq_from_resources(const struct resource *resources,
 	return 0;
 }
 
-/**
- * i2c_new_client_device - instantiate an i2c device
- * @adap: the adapter managing the device
- * @info: describes one I2C device; bus_num is ignored
- * Context: can sleep
- *
- * Create an i2c device. Binding is handled through driver model
- * probe()/remove() methods.  A driver may be bound to this device when we
- * return from this function, or any later moment (e.g. maybe hotplugging will
- * load the driver module).  This call is not appropriate for use by mainboard
- * initialization logic, which usually runs during an arch_initcall() long
- * before any i2c_adapter could exist.
- *
- * This returns the new i2c client, which may be saved for later use with
- * i2c_unregister_device(); or an ERR_PTR to describe the error.
- */
-struct i2c_client *
-i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
+static struct i2c_client *
+__i2c_new_client_device(struct i2c_adapter *adap,
+			struct i2c_board_info const *info,
+			struct device *dev)
 {
 	struct i2c_client	*client;
 	int			status;
@@ -944,7 +930,7 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
 	if (status)
 		goto out_err;
 
-	client->dev.parent = &client->adapter->dev;
+	client->dev.parent = dev ? dev : &client->adapter->dev;
 	client->dev.bus = &i2c_bus_type;
 	client->dev.type = &i2c_client_type;
 	client->dev.of_node = of_node_get(info->of_node);
@@ -984,6 +970,28 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
 	kfree(client);
 	return ERR_PTR(status);
 }
+
+/**
+ * i2c_new_client_device - instantiate an i2c device
+ * @adap: the adapter managing the device
+ * @info: describes one I2C device; bus_num is ignored
+ * Context: can sleep
+ *
+ * Create an i2c device. Binding is handled through driver model
+ * probe()/remove() methods.  A driver may be bound to this device when we
+ * return from this function, or any later moment (e.g. maybe hotplugging will
+ * load the driver module).  This call is not appropriate for use by mainboard
+ * initialization logic, which usually runs during an arch_initcall() long
+ * before any i2c_adapter could exist.
+ *
+ * This returns the new i2c client, which may be saved for later use with
+ * i2c_unregister_device(); or an ERR_PTR to describe the error.
+ */
+struct i2c_client *
+i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
+{
+	return __i2c_new_client_device(adap, info, NULL);
+}
 EXPORT_SYMBOL_GPL(i2c_new_client_device);
 
 /**
@@ -1054,6 +1062,25 @@ static struct i2c_driver dummy_driver = {
 	.id_table	= dummy_id,
 };
 
+static struct i2c_client *__i2c_new_dummy_device(struct i2c_adapter *adapter,
+						 u16 address, const char *name,
+						 struct device *dev)
+{
+	struct i2c_board_info info = {
+		I2C_BOARD_INFO("dummy", address),
+	};
+
+	if (name) {
+		ssize_t ret = strscpy(info.type, name, sizeof(info.type));
+
+		if (ret < 0)
+			return ERR_PTR(dev_err_probe(&adapter->dev, ret,
+						     "Invalid device name\n"));
+	}
+
+	return __i2c_new_client_device(adapter, &info, dev);
+}
+
 /**
  * i2c_new_dummy_device - return a new i2c device bound to a dummy driver
  * @adapter: the adapter managing the device
@@ -1074,11 +1101,7 @@ static struct i2c_driver dummy_driver = {
  */
 struct i2c_client *i2c_new_dummy_device(struct i2c_adapter *adapter, u16 address)
 {
-	struct i2c_board_info info = {
-		I2C_BOARD_INFO("dummy", address),
-	};
-
-	return i2c_new_client_device(adapter, &info);
+	return __i2c_new_dummy_device(adapter, address, NULL, NULL);
 }
 EXPORT_SYMBOL_GPL(i2c_new_dummy_device);
 
@@ -1122,15 +1145,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,8 +1164,9 @@ 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;
 	u32 addr = default_addr;
@@ -1153,7 +1179,9 @@ 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);
+	return __i2c_new_dummy_device(client->adapter, addr,
+				      aux_device_name ? aux_device_name : NULL,
+				      &client->dev);
 }
 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 v4 02/11] dt-bindings: rtc: isl1208: Convert to json-schema

[Biju Das]

Convert the isl1208 RTC device tree binding documentation to json-schema.

Update the example to match reality.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
v4->v4_new:
 * Moved this patch to PMIC series
 * Added minItems to interrupt-names.
 * Added interrupt-names in conditional schema check.
v3->v4:
 * Added Rb tag from Krzysztof Kozlowski.
 * Dropped | from description 
 * Replaced the pin name #EVDET->EVDET in description.
 * Dropped oneOf from compatible.
v2->v3:
 * Updated interrupt-names property by keeping the list of names.
 * Removed Interrupts from required property as it may not be wired.
 * Removed isil,ev-evienb from required property.
RFC->v2:
 * Updated maintainers list
 * Updated description from original bindings
 * removed default from isil,ev-evienb properties to match with the original
   bindings.
 * Added conditional check for interrupts.
---
 .../devicetree/bindings/rtc/isil,isl1208.txt  | 38 --------
 .../devicetree/bindings/rtc/isil,isl1208.yaml | 89 +++++++++++++++++++
 2 files changed, 89 insertions(+), 38 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/rtc/isil,isl1208.txt
 create mode 100644 Documentation/devicetree/bindings/rtc/isil,isl1208.yaml

diff --git a/Documentation/devicetree/bindings/rtc/isil,isl1208.txt b/Documentation/devicetree/bindings/rtc/isil,isl1208.txt
deleted file mode 100644
index 51f003006f04..000000000000
--- a/Documentation/devicetree/bindings/rtc/isil,isl1208.txt
+++ /dev/null
@@ -1,38 +0,0 @@
-Intersil ISL1209/19 I2C RTC/Alarm chip with event in
-
-ISL12X9 have additional pins EVIN and #EVDET for tamper detection, while the
-ISL1208 and ISL1218 do not.  They are all use the same driver with the bindings
-described here, with chip specific properties as noted.
-
-Required properties supported by the device:
- - "compatible": Should be one of the following:
-		- "isil,isl1208"
-		- "isil,isl1209"
-		- "isil,isl1218"
-		- "isil,isl1219"
- - "reg": I2C bus address of the device
-
-Optional properties:
- - "interrupt-names": list which may contains "irq" and "evdet"
-	evdet applies to isl1209 and isl1219 only
- - "interrupts": list of interrupts for "irq" and "evdet"
-	evdet applies to isl1209 and isl1219 only
- - "isil,ev-evienb": Enable or disable internal pull on EVIN pin
-	Applies to isl1209 and isl1219 only
-	Possible values are 0 and 1
-	Value 0 enables internal pull-up on evin pin, 1 disables it.
-	Default will leave the non-volatile configuration of the pullup
-	as is.
-
-Example isl1219 node with #IRQ pin connected to SoC gpio1 pin12 and #EVDET pin
-connected to SoC gpio2 pin 24 and internal pull-up enabled in EVIN pin.
-
-	isl1219: rtc@68 {
-		compatible = "isil,isl1219";
-		reg = <0x68>;
-		interrupt-names = "irq", "evdet";
-		interrupts-extended = <&gpio1 12 IRQ_TYPE_EDGE_FALLING>,
-			<&gpio2 24 IRQ_TYPE_EDGE_FALLING>;
-		isil,ev-evienb = <1>;
-	};
-
diff --git a/Documentation/devicetree/bindings/rtc/isil,isl1208.yaml b/Documentation/devicetree/bindings/rtc/isil,isl1208.yaml
new file mode 100644
index 000000000000..565965147ce6
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/isil,isl1208.yaml
@@ -0,0 +1,89 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rtc/isil,isl1208.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intersil ISL1209/19 I2C RTC/Alarm chip with event in
+
+maintainers:
+  - Biju Das <biju.das.jz@bp.renesas.com>
+  - Trent Piepho <tpiepho@gmail.com>
+
+description:
+  ISL12X9 have additional pins EVIN and EVDET for tamper detection, while the
+  ISL1208 and ISL1218 do not.
+
+properties:
+  compatible:
+    enum:
+      - isil,isl1208
+      - isil,isl1209
+      - isil,isl1218
+      - isil,isl1219
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    minItems: 1
+    maxItems: 2
+
+  interrupt-names:
+    minItems: 1
+    items:
+      - const: irq
+      - const: evdet
+
+  isil,ev-evienb:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1 ]
+    description: |
+      Enable or disable internal pull on EVIN pin
+      Default will leave the non-volatile configuration of the pullup
+      as is.
+        <0> : Enables internal pull-up on evin pin
+        <1> : Disables internal pull-up on evin pin
+
+required:
+  - compatible
+  - reg
+
+allOf:
+  - $ref: rtc.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - isil,isl1209
+              - isil,isl1219
+    then:
+      properties:
+        interrupts:
+          maxItems: 2
+        interrupt-names:
+          items:
+            - const: irq
+            - const: evdet
+    else:
+      properties:
+        interrupts:
+          maxItems: 1
+        interrupt-names:
+          items:
+            - const: irq
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        rtc_twi: rtc@6f {
+            compatible = "isil,isl1208";
+            reg = <0x6f>;
+        };
+    };
-- 
2.25.1

[PATCH v4 03/11] dt-bindings: rtc: isil,isl1208: Document clock and clock-names properties

[Biju Das]

As per the HW manual, XTOSCB bit setting is as follows

If using an external clock signal, set the XTOSCB bit as 1 to
disable the crystal oscillator.

If using an external crystal, the XTOSCB bit needs to be set at 0
to enable the crystal oscillator.

Document clock and clock-names properties.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v4:
 * New patch
---
 .../devicetree/bindings/rtc/isil,isl1208.yaml       | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/isil,isl1208.yaml b/Documentation/devicetree/bindings/rtc/isil,isl1208.yaml
index 565965147ce6..6c270dd53605 100644
--- a/Documentation/devicetree/bindings/rtc/isil,isl1208.yaml
+++ b/Documentation/devicetree/bindings/rtc/isil,isl1208.yaml
@@ -25,6 +25,19 @@ properties:
   reg:
     maxItems: 1
 
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    description: |
+      Use xin, if connected to an external crystal.
+      Use clkin, if connected to an external clock signal.
+    oneOf:
+      - items:
+          - const: xin
+      - items:
+          - const: clkin
+
   interrupts:
     minItems: 1
     maxItems: 2
-- 
2.25.1

[PATCH v4 04/11] rtc: isl1208: Drop name variable

[Biju Das]

Drop unused name variable from struct isl1208_config.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v4:
 * New patch.
---
 drivers/rtc/rtc-isl1208.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index 73cc6aaf9b8b..a73eb78b8a40 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -79,15 +79,14 @@ enum isl1208_id {
 
 /* Chip capabilities table */
 static const struct isl1208_config {
-	const char	name[8];
 	unsigned int	nvmem_length;
 	unsigned	has_tamper:1;
 	unsigned	has_timestamp: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_ISL1208] = { 2, false, false },
+	[TYPE_ISL1209] = { 2, true,  false },
+	[TYPE_ISL1218] = { 8, false, false },
+	[TYPE_ISL1219] = { 2, true,  true },
 };
 
 static const struct i2c_device_id isl1208_id[] = {
-- 
2.25.1

[PATCH v4 05/11] rtc: isl1208: Make similar I2C and DT-based matching table

[Biju Das]

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

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v4:
 * New patch.
---
 drivers/rtc/rtc-isl1208.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index a73eb78b8a40..a6a133f719df 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -90,10 +90,10 @@ static const struct isl1208_config {
 };
 
 static const struct i2c_device_id isl1208_id[] = {
-	{ "isl1208", TYPE_ISL1208 },
-	{ "isl1209", TYPE_ISL1209 },
-	{ "isl1218", TYPE_ISL1218 },
-	{ "isl1219", TYPE_ISL1219 },
+	{ "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] },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, isl1208_id);
@@ -822,9 +822,9 @@ isl1208_probe(struct i2c_client *client)
 	} else {
 		const struct i2c_device_id *id = i2c_match_id(isl1208_id, client);
 
-		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->rtc = devm_rtc_allocate_device(&client->dev);
-- 
2.25.1

[PATCH v4 06/11] rtc: isl1208: Drop enum isl1208_id and split isl1208_configs[]

[Biju Das]

Drop enum isl1208_id and split the array isl1208_configs[] as individual
variables, and make lines shorter by referring to e.g. &config_isl1219
instead of &isl1208_configs[TYPE_ISL1219].

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v4:
 * New patch
---
 drivers/rtc/rtc-isl1208.c | 56 +++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 23 deletions(-)

diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index a6a133f719df..916e7d44c96f 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -68,41 +68,51 @@
 
 static struct i2c_driver isl1208_driver;
 
-/* ISL1208 various variants */
-enum isl1208_id {
-	TYPE_ISL1208 = 0,
-	TYPE_ISL1209,
-	TYPE_ISL1218,
-	TYPE_ISL1219,
-	ISL_LAST_ID
-};
-
 /* Chip capabilities table */
-static const struct isl1208_config {
+struct isl1208_config {
 	unsigned int	nvmem_length;
 	unsigned	has_tamper:1;
 	unsigned	has_timestamp:1;
-} isl1208_configs[] = {
-	[TYPE_ISL1208] = { 2, false, false },
-	[TYPE_ISL1209] = { 2, true,  false },
-	[TYPE_ISL1218] = { 8, false, false },
-	[TYPE_ISL1219] = { 2, true,  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 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] },
+	{ "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 },
 	{ }
 };
 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 },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, isl1208_of_match);
-- 
2.25.1

[PATCH v4 07/11] rtc: isl1208: Add isl1208_set_xtoscb()

[Biju Das]

As per the HW manual, setting of XTOSCB bit as follws

If using an external clock signal, set the XTOSCB bit as 1 to
disable the crystal oscillator.

If using an external crystal, the XTOSCB bit needs to be set at 0
to enable the crystal oscillator.

Add isl1208_set_xtoscb() to set XTOSCB bit based on the clock-names
property. Fallback is enabling the internal crystal oscillator.

While at it, introduce a variable "sr" for reading status register
in probe() as it is reused for writing.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v4:
 * New patch.
---
 drivers/rtc/rtc-isl1208.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index 916e7d44c96f..5f91a3ca5920 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/bcd.h>
+#include <linux/clk.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
@@ -175,6 +176,16 @@ isl1208_i2c_validate_client(struct i2c_client *client)
 	return 0;
 }
 
+static int isl1208_set_xtoscb(struct i2c_client *client, int sr, bool int_osc_en)
+{
+	if (int_osc_en)
+		sr &= ~ISL1208_REG_SR_XTOSCB;
+	else
+		sr |= ISL1208_REG_SR_XTOSCB;
+
+	return i2c_smbus_write_byte_data(client, ISL1208_REG_SR, sr);
+}
+
 static int
 isl1208_i2c_get_sr(struct i2c_client *client)
 {
@@ -808,9 +819,13 @@ static int isl1208_setup_irq(struct i2c_client *client, int irq)
 static int
 isl1208_probe(struct i2c_client *client)
 {
-	int rc = 0;
 	struct isl1208_state *isl1208;
+	bool int_osc_en = true;
 	int evdet_irq = -1;
+	struct clk *clkin;
+	struct clk *xin;
+	int rc = 0;
+	int sr;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
 		return -ENODEV;
@@ -837,6 +852,13 @@ isl1208_probe(struct i2c_client *client)
 		isl1208->config = (struct isl1208_config *)id->driver_data;
 	}
 
+	xin = devm_clk_get(&client->dev, "xin");
+	if (IS_ERR(xin)) {
+		clkin = devm_clk_get(&client->dev, "clkin");
+		if (!IS_ERR(clkin))
+			int_osc_en = false;
+	}
+
 	isl1208->rtc = devm_rtc_allocate_device(&client->dev);
 	if (IS_ERR(isl1208->rtc))
 		return PTR_ERR(isl1208->rtc);
@@ -848,13 +870,17 @@ isl1208_probe(struct i2c_client *client)
 	isl1208->nvmem_config.size = isl1208->config->nvmem_length;
 	isl1208->nvmem_config.priv = isl1208;
 
-	rc = isl1208_i2c_get_sr(client);
-	if (rc < 0) {
+	sr = isl1208_i2c_get_sr(client);
+	if (sr < 0) {
 		dev_err(&client->dev, "reading status failed\n");
-		return rc;
+		return sr;
 	}
 
-	if (rc & ISL1208_REG_SR_RTCF)
+	rc = isl1208_set_xtoscb(client, sr, int_osc_en);
+	if (rc)
+		return rc;
+
+	if (sr & ISL1208_REG_SR_RTCF)
 		dev_warn(&client->dev, "rtc power failure detected, "
 			 "please set clock.\n");
 
-- 
2.25.1

[PATCH v4 08/11] 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.

Enhance isl1208_set_xtoscb() to handle inverted bit and internal oscillator
is enabled or not is determined by the parent clock name.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v3->v4:
 * Added support for internal oscillator enable/disable.
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 | 44 ++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index 5f91a3ca5920..597e0126155f 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -74,6 +74,7 @@ struct isl1208_config {
 	unsigned int	nvmem_length;
 	unsigned	has_tamper:1;
 	unsigned	has_timestamp:1;
+	unsigned	has_inverted_osc_bit:1;
 };
 
 static const struct isl1208_config config_isl1208 = {
@@ -100,11 +101,19 @@ static const struct isl1208_config config_isl1219 = {
 	.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)&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);
@@ -176,12 +185,16 @@ isl1208_i2c_validate_client(struct i2c_client *client)
 	return 0;
 }
 
-static int isl1208_set_xtoscb(struct i2c_client *client, int sr, bool int_osc_en)
+static int isl1208_set_xtoscb(struct i2c_client *client, int sr,
+			      bool int_osc_en, bool inverted)
 {
+	int xtosb_set = sr | ISL1208_REG_SR_XTOSCB;
+	int xtosb_clr = sr & ~ISL1208_REG_SR_XTOSCB;
+
 	if (int_osc_en)
-		sr &= ~ISL1208_REG_SR_XTOSCB;
+		sr = inverted ? xtosb_set : xtosb_clr;
 	else
-		sr |= ISL1208_REG_SR_XTOSCB;
+		sr = inverted ? xtosb_clr : xtosb_set;
 
 	return i2c_smbus_write_byte_data(client, ISL1208_REG_SR, sr);
 }
@@ -852,11 +865,25 @@ isl1208_probe(struct i2c_client *client)
 		isl1208->config = (struct isl1208_config *)id->driver_data;
 	}
 
-	xin = devm_clk_get(&client->dev, "xin");
-	if (IS_ERR(xin)) {
-		clkin = devm_clk_get(&client->dev, "clkin");
-		if (!IS_ERR(clkin))
+	if (client->dev.parent->type == &i2c_client_type) {
+		xin = of_clk_get_by_name(client->dev.parent->of_node, "xin");
+		if (IS_ERR(xin)) {
+			clkin = of_clk_get_by_name(client->dev.parent->of_node, "clkin");
+			if (IS_ERR(clkin))
+				return -ENODEV;
+
 			int_osc_en = false;
+			clk_put(clkin);
+		} else {
+			clk_put(xin);
+		}
+	} else {
+		xin = devm_clk_get(&client->dev, "xin");
+		if (IS_ERR(xin)) {
+			clkin = devm_clk_get(&client->dev, "clkin");
+			if (!IS_ERR(clkin))
+				int_osc_en = false;
+		}
 	}
 
 	isl1208->rtc = devm_rtc_allocate_device(&client->dev);
@@ -876,7 +903,8 @@ isl1208_probe(struct i2c_client *client)
 		return sr;
 	}
 
-	rc = isl1208_set_xtoscb(client, sr, int_osc_en);
+	rc = isl1208_set_xtoscb(client, sr, int_osc_en,
+				isl1208->config->has_inverted_osc_bit);
 	if (rc)
 		return rc;
 
-- 
2.25.1

[PATCH v4 09/11] regulator: dt-bindings: 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>
---
v3->v4:
 * Moved bindings from mfd->regulator.
 * Dropped minItems from reg.
 * Dropped renesas,rtc-enabled property and instead used clock-names property
   to find RTC is enabled or not.
 * Added reg-names in required property.
 * Updated the example.
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/regulator/renesas,raa215300.yaml | 84 +++++++++++++++++++
 1 file changed, 84 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/renesas,raa215300.yaml

diff --git a/Documentation/devicetree/bindings/regulator/renesas,raa215300.yaml b/Documentation/devicetree/bindings/regulator/renesas,raa215300.yaml
new file mode 100644
index 000000000000..17b16f66f695
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/renesas,raa215300.yaml
@@ -0,0 +1,84 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/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:
+    maxItems: 2
+
+  reg-names:
+    items:
+      - const: main
+      - const: rtc
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    description: |
+      Use xin, if connected to an external crystal.
+      Use clkin, if connected to an external clock signal.
+    oneOf:
+      - items:
+          - const: xin
+      - items:
+          - const: clkin
+
+required:
+  - compatible
+  - reg
+  - reg-names
+
+additionalProperties: false
+
+examples:
+  - |
+    /* 32.768kHz crystal */
+    x2: clock-xtal {
+        compatible = "fixed-clock";
+        #clock-cells = <0>;
+        clock-frequency = <32768>;
+    };
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        raa215300: pmic@12 {
+            compatible = "renesas,raa215300";
+            reg = <0x12>, <0x6f>;
+            reg-names = "main", "rtc";
+
+            clocks = <&x2>;
+            clock-names = "xin";
+        };
+    };
-- 
2.25.1

[PATCH v4 10/11] regulator: 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>
---
v3->v4:
 * Moved from mfd->regulator as it doesn't use MFD APIs
 * Dropped handling "renesas,rtc-enabled" property and instead used
   clock-names to determine RTC is enabled or not and then instantiating
   RTC device.
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/regulator/Kconfig     |   7 ++
 drivers/regulator/Makefile    |   1 +
 drivers/regulator/raa215300.c | 118 ++++++++++++++++++++++++++++++++++
 3 files changed, 126 insertions(+)
 create mode 100644 drivers/regulator/raa215300.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 7bbaf5991268..99fd45ae2625 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1033,6 +1033,13 @@ config REGULATOR_QCOM_USB_VBUS
 	  Say M here if you want to include support for enabling the VBUS output
 	  as a module. The module will be named "qcom_usb_vbus_regulator".
 
+config REGULATOR_RAA215300
+	tristate "Renesas RAA215300 driver"
+	select REGMAP_I2C
+	depends on I2C
+	help
+	  Support for the Renesas RAA215300 PMIC.
+
 config REGULATOR_RASPBERRYPI_TOUCHSCREEN_ATTINY
 	tristate "Raspberry Pi 7-inch touchscreen panel ATTINY regulator"
 	depends on BACKLIGHT_CLASS_DEVICE
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 0b3ad1b0a999..1b885d6a050c 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -124,6 +124,7 @@ obj-$(CONFIG_REGULATOR_TPS51632) += tps51632-regulator.o
 obj-$(CONFIG_REGULATOR_PBIAS) += pbias-regulator.o
 obj-$(CONFIG_REGULATOR_PCAP) += pcap-regulator.o
 obj-$(CONFIG_REGULATOR_PCF50633) += pcf50633-regulator.o
+obj-$(CONFIG_REGULATOR_RAA215300)	+= raa215300.o
 obj-$(CONFIG_REGULATOR_RASPBERRYPI_TOUCHSCREEN_ATTINY)  += rpi-panel-attiny-regulator.o
 obj-$(CONFIG_REGULATOR_RC5T583)  += rc5t583-regulator.o
 obj-$(CONFIG_REGULATOR_RK808)   += rk808-regulator.o
diff --git a/drivers/regulator/raa215300.c b/drivers/regulator/raa215300.c
new file mode 100644
index 000000000000..cdbbbb63daec
--- /dev/null
+++ b/drivers/regulator/raa215300.c
@@ -0,0 +1,118 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RAA215300 PMIC driver
+ *
+ * Copyright (C) 2023 Renesas Electronics Corporation
+ */
+
+#include <linux/clk.h>
+#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;
+	bool int_osc_en = true;
+	struct regmap *regmap;
+	struct clk *clkin;
+	struct clk *xin;
+	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);
+
+	xin = devm_clk_get_optional(&client->dev, "xin");
+	if (IS_ERR_OR_NULL(xin)) {
+		clkin = devm_clk_get(&client->dev, "clkin");
+		if (IS_ERR(clkin))
+			return PTR_ERR(clkin);
+
+		int_osc_en = false;
+		xin = NULL;
+	} else {
+		clkin = NULL;
+	}
+
+	if (xin || clkin)  {
+		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" : "raa215300_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 v4 11/11] 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>
---
v3->v4:
 * Added clock nodes.
v2->v3:
 * No change.
RFC->V2:
 * Updated pmic device node based on the bindings.
---
 .../boot/dts/renesas/rzg2l-smarc-som.dtsi      | 18 ++++++++++++++++++
 1 file changed, 18 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..801d1c1c5bc2 100644
--- a/arch/arm64/boot/dts/renesas/rzg2l-smarc-som.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg2l-smarc-som.dtsi
@@ -73,6 +73,13 @@ vccq_sdhi0: regulator-vccq-sdhi0 {
 		gpios = <&pinctrl RZG2L_GPIO(39, 0) GPIO_ACTIVE_HIGH>;
 		regulator-always-on;
 	};
+
+	/* 32.768kHz crystal */
+	x2: clock-xtal {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <32768>;
+	};
 };
 
 &adc {
@@ -351,3 +358,14 @@ &wdt1 {
 	status = "okay";
 	timeout-sec = <60>;
 };
+
+&i2c3 {
+	raa215300: pmic@12 {
+		compatible = "renesas,raa215300";
+		reg = <0x12>, <0x6f>;
+		reg-names = "main", "rtc";
+
+		clocks = <&x2>;
+		clock-names = "xin";
+	};
+};
-- 
2.25.1

Re: [PATCH v4 09/11] regulator: dt-bindings: Add Renesas RAA215300 PMIC bindings

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 966 bytes --]

On Thu, May 18, 2023 at 12:36:41PM +0100, 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.
> +  reg:
> +    maxItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: main
> +      - const: rtc

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

Out of curiosity as much as anything else, why do you need reg-names if
the two registers are always required?

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 03/11] dt-bindings: rtc: isil,isl1208: Document clock and clock-names properties

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1584 bytes --]

On Thu, May 18, 2023 at 12:36:35PM +0100, Biju Das wrote:
> As per the HW manual, XTOSCB bit setting is as follows
> 
> If using an external clock signal, set the XTOSCB bit as 1 to
> disable the crystal oscillator.
> 
> If using an external crystal, the XTOSCB bit needs to be set at 0
> to enable the crystal oscillator.
> 
> Document clock and clock-names properties.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

I notice on v3 you appealed to Rob or Krzysztof for approval about this
approach. Clearly I not either of them, but it seems reasonable to me.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

> ---
> v4:
>  * New patch
> ---
>  .../devicetree/bindings/rtc/isil,isl1208.yaml       | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/isil,isl1208.yaml b/Documentation/devicetree/bindings/rtc/isil,isl1208.yaml
> index 565965147ce6..6c270dd53605 100644
> --- a/Documentation/devicetree/bindings/rtc/isil,isl1208.yaml
> +++ b/Documentation/devicetree/bindings/rtc/isil,isl1208.yaml
> @@ -25,6 +25,19 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    description: |
> +      Use xin, if connected to an external crystal.
> +      Use clkin, if connected to an external clock signal.
> +    oneOf:
> +      - items:
> +          - const: xin
> +      - items:
> +          - const: clkin
> +
>    interrupts:
>      minItems: 1
>      maxItems: 2
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH v4 09/11] regulator: dt-bindings: Add Renesas RAA215300 PMIC bindings

[Biju Das]

Hi Conor,

Thanks for the feedback.

> Subject: Re: [PATCH v4 09/11] regulator: dt-bindings: Add Renesas
> RAA215300 PMIC bindings
> 
> On Thu, May 18, 2023 at 12:36:41PM +0100, 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.
> > +  reg:
> > +    maxItems: 2
> > +
> > +  reg-names:
> > +    items:
> > +      - const: main
> > +      - const: rtc
> 
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> 
> Out of curiosity as much as anything else, why do you need reg-names if
> the two registers are always required?

The device has always 2 address spaces and "reg-names" provides a means
of clear differentiation compared to indices. 

By enforcing "reg-names" as required property, dt can do some schema-validation
forcing users to specify "reg-names" in device tree.

Implementation wise, we use "rtc" for getting resource details while
creating the second i2c device which overrides the default address.

Strictly speaking reg-names is not required, but from a validation
perspective and since driver is using the same "resource-name" it is
better to have it??

Please share your thoughts.

Cheers,
Biju

Re: [PATCH v4 01/11] i2c: Enhance i2c_new_ancillary_device API

[Geert Uytterhoeven]

Hi Biju,

On Thu, May 18, 2023 at 1:37 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)
>
> Added helper function __i2c_new_dummy_device to share the code
> between i2c_new_dummy_device and i2c_new_ancillary_device().
>
> Also added helper function __i2c_new_client_device() to pass parent dev
> parameter, so that the ancillary device can assign its parent during
> creation.
>
> Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v3->v4:
>  * Dropped Rb tag from Geert as there are new changes.
>  * Introduced __i2c_new_dummy_device() to share the code between
>    i2c_new_dummy_device and i2c_new_ancillary_device().
>  * Introduced __i2c_new_client_device() to pass parent dev
>    parameter, so that the ancillary device can assign its parent during
>    creation.

Thanks for the update!

LGTM, a few minor comments below.

> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -893,24 +893,10 @@ int i2c_dev_irq_from_resources(const struct resource *resources,
>         return 0;
>  }
>
> -/**
> - * i2c_new_client_device - instantiate an i2c device
> - * @adap: the adapter managing the device
> - * @info: describes one I2C device; bus_num is ignored
> - * Context: can sleep
> - *
> - * Create an i2c device. Binding is handled through driver model
> - * probe()/remove() methods.  A driver may be bound to this device when we
> - * return from this function, or any later moment (e.g. maybe hotplugging will
> - * load the driver module).  This call is not appropriate for use by mainboard
> - * initialization logic, which usually runs during an arch_initcall() long
> - * before any i2c_adapter could exist.
> - *
> - * This returns the new i2c client, which may be saved for later use with
> - * i2c_unregister_device(); or an ERR_PTR to describe the error.
> - */
> -struct i2c_client *
> -i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
> +static struct i2c_client *
> +__i2c_new_client_device(struct i2c_adapter *adap,
> +                       struct i2c_board_info const *info,
> +                       struct device *dev)

struct device *parent?

>  {
>         struct i2c_client       *client;
>         int                     status;

> @@ -1054,6 +1062,25 @@ static struct i2c_driver dummy_driver = {
>         .id_table       = dummy_id,
>  };
>
> +static struct i2c_client *__i2c_new_dummy_device(struct i2c_adapter *adapter,
> +                                                u16 address, const char *name,
> +                                                struct device *dev)
> +{
> +       struct i2c_board_info info = {
> +               I2C_BOARD_INFO("dummy", address),
> +       };
> +
> +       if (name) {
> +               ssize_t ret = strscpy(info.type, name, sizeof(info.type));
> +
> +               if (ret < 0)
> +                       return ERR_PTR(dev_err_probe(&adapter->dev, ret,
> +                                                    "Invalid device name\n"));

%s too long?

> +       }
> +
> +       return __i2c_new_client_device(adapter, &info, dev);
> +}
> +
>  /**
>   * i2c_new_dummy_device - return a new i2c device bound to a dummy driver
>   * @adapter: the adapter managing the device

> @@ -1122,15 +1145,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.

Please add something like:

    The ancillary's device parent will be set to the primary device.

>   *
>   * On DT-based platforms the address is retrieved from the "reg" property entry
>   * cell whose "reg-names" value matches the slave name.

> @@ -1153,7 +1179,9 @@ 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);
> +       return __i2c_new_dummy_device(client->adapter, addr,
> +                                     aux_device_name ? aux_device_name : NULL,

You can just pass aux_device_name.

> +                                     &client->dev);
>  }
>  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 v4 03/11] dt-bindings: rtc: isil,isl1208: Document clock and clock-names properties

[Geert Uytterhoeven]

Hi Biju,

On Thu, May 18, 2023 at 1:37 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> As per the HW manual, XTOSCB bit setting is as follows
>
> If using an external clock signal, set the XTOSCB bit as 1 to
> disable the crystal oscillator.
>
> If using an external crystal, the XTOSCB bit needs to be set at 0
> to enable the crystal oscillator.
>
> Document clock and clock-names properties.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v4:
>  * New patch

Thanks for your patch!

> --- a/Documentation/devicetree/bindings/rtc/isil,isl1208.yaml
> +++ b/Documentation/devicetree/bindings/rtc/isil,isl1208.yaml
> @@ -25,6 +25,19 @@ properties:
>    reg:
>      maxItems: 1
>
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    description: |
> +      Use xin, if connected to an external crystal.
> +      Use clkin, if connected to an external clock signal.
> +    oneOf:
> +      - items:
> +          - const: xin
> +      - items:
> +          - const: clkin
> +

I guess that oneOf scheme is equivalent to the simpler

    enum:
      - xin
      - clkin

?

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 v4 04/11] rtc: isl1208: Drop name variable

[Geert Uytterhoeven]

On Thu, May 18, 2023 at 1:37 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Drop unused name variable from struct isl1208_config.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v4:
>  * New patch.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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 v4 05/11] rtc: isl1208: Make similar I2C and DT-based matching table

[Geert Uytterhoeven]

On Thu, May 18, 2023 at 1:37 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> The isl1208_id[].driver_data could store a pointer to the config,
> like for DT-based matching, making I2C and DT-based matching
> more similar.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v4:
>  * New patch.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> @@ -822,9 +822,9 @@ isl1208_probe(struct i2c_client *client)
>         } else {
>                 const struct i2c_device_id *id = i2c_match_id(isl1208_id, client);
>
> -               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;

It's a pity there's no i2c_get_match_data() yet...

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 v4 06/11] rtc: isl1208: Drop enum isl1208_id and split isl1208_configs[]

[Geert Uytterhoeven]

On Thu, May 18, 2023 at 1:37 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Drop enum isl1208_id and split the array isl1208_configs[] as individual
> variables, and make lines shorter by referring to e.g. &config_isl1219
> instead of &isl1208_configs[TYPE_ISL1219].
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v4:
>  * New patch

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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 v4 07/11] rtc: isl1208: Add isl1208_set_xtoscb()

[Geert Uytterhoeven]

Hi Biju,

On Thu, May 18, 2023 at 1:37 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> As per the HW manual, setting of XTOSCB bit as follws

... set the XTOSCB bit as follows:

> If using an external clock signal, set the XTOSCB bit as 1 to
> disable the crystal oscillator.
>
> If using an external crystal, the XTOSCB bit needs to be set at 0
> to enable the crystal oscillator.
>
> Add isl1208_set_xtoscb() to set XTOSCB bit based on the clock-names
> property. Fallback is enabling the internal crystal oscillator.
>
> While at it, introduce a variable "sr" for reading status register

the status register

> in probe() as it is reused for writing.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v4:
>  * New patch.

Thanks for your patch!

> --- a/drivers/rtc/rtc-isl1208.c
> +++ b/drivers/rtc/rtc-isl1208.c

> @@ -837,6 +852,13 @@ isl1208_probe(struct i2c_client *client)
>                 isl1208->config = (struct isl1208_config *)id->driver_data;
>         }
>
> +       xin = devm_clk_get(&client->dev, "xin");
> +       if (IS_ERR(xin)) {
> +               clkin = devm_clk_get(&client->dev, "clkin");
> +               if (!IS_ERR(clkin))
> +                       int_osc_en = false;
> +       }

devm_clk_get_optional()
(doesn't devm_clk_get() print an error message?)

And check for NULL to find out if a clock is present.
IS_ERR() is a real error condition to be propagated.

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 v4 08/11] rtc: isl1208: Add support for the built-in RTC on the PMIC RAA215300

[Geert Uytterhoeven]

Hi Biju,

On Thu, May 18, 2023 at 1:37 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

instantiates the

> RTC device based on i2c_device_id.
>
> Enhance isl1208_set_xtoscb() to handle inverted bit and internal oscillator
> is enabled or not is determined by the parent clock name.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v3->v4:
>  * Added support for internal oscillator enable/disable.

Thanks for the update!

> --- a/drivers/rtc/rtc-isl1208.c
> +++ b/drivers/rtc/rtc-isl1208.c

> @@ -852,11 +865,25 @@ isl1208_probe(struct i2c_client *client)
>                 isl1208->config = (struct isl1208_config *)id->driver_data;
>         }
>
> -       xin = devm_clk_get(&client->dev, "xin");
> -       if (IS_ERR(xin)) {
> -               clkin = devm_clk_get(&client->dev, "clkin");
> -               if (!IS_ERR(clkin))
> +       if (client->dev.parent->type == &i2c_client_type) {
> +               xin = of_clk_get_by_name(client->dev.parent->of_node, "xin");
> +               if (IS_ERR(xin)) {

-ENOENT means clock not present, so continue below.
Any other error codes are to be propagated upstream.

> +                       clkin = of_clk_get_by_name(client->dev.parent->of_node, "clkin");
> +                       if (IS_ERR(clkin))

Likewise.

> +                               return -ENODEV;

Clock not present is not an error, as the clock is optional for DT
backwards compatibility.

> +
>                         int_osc_en = false;
> +                       clk_put(clkin);
> +               } else {
> +                       clk_put(xin);
> +               }
> +       } else {
> +               xin = devm_clk_get(&client->dev, "xin");
> +               if (IS_ERR(xin)) {
> +                       clkin = devm_clk_get(&client->dev, "clkin");
> +                       if (!IS_ERR(clkin))
> +                               int_osc_en = false;
> +               }
>         }
>
>         isl1208->rtc = devm_rtc_allocate_device(&client->dev);
> @@ -876,7 +903,8 @@ isl1208_probe(struct i2c_client *client)
>                 return sr;
>         }
>
> -       rc = isl1208_set_xtoscb(client, sr, int_osc_en);
> +       rc = isl1208_set_xtoscb(client, sr, int_osc_en,
> +                               isl1208->config->has_inverted_osc_bit);

No need to change isl1208_set_xtoscb() (but perhaps rename
the int_osc_en parameter?) if you would pass
"int_osc_en ^ isl1208->config->has_inverted_osc_bit".
(beware: C has no logical ^^, only bitwise ^).


>         if (rc)
>                 return rc;

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 v4 09/11] regulator: dt-bindings: Add Renesas RAA215300 PMIC bindings

[Geert Uytterhoeven]

Hi Biju,

On Thu, May 18, 2023 at 1:37 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>
> ---
> v3->v4:
>  * Moved bindings from mfd->regulator.
>  * Dropped minItems from reg.
>  * Dropped renesas,rtc-enabled property and instead used clock-names property
>    to find RTC is enabled or not.
>  * Added reg-names in required property.
>  * Updated the example.

Thanks for your patch!

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/renesas,raa215300.yaml
> @@ -0,0 +1,84 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/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:
> +    maxItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: main
> +      - const: rtc
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1

Perhaps add a description to make it clear that not providing clocks
is supported, and means the RTC is disabled?

> +
> +  clock-names:
> +    description: |
> +      Use xin, if connected to an external crystal.
> +      Use clkin, if connected to an external clock signal.
> +    oneOf:
> +      - items:
> +          - const: xin
> +      - items:
> +          - const: clkin

Please replace oneOf+items by enum.

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

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 v4 10/11] regulator: Add Renesas PMIC RAA215300 driver

[Geert Uytterhoeven]

Hi Biju,

On Thu, May 18, 2023 at 1:37 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>
> ---
> v3->v4:
>  * Moved from mfd->regulator as it doesn't use MFD APIs
>  * Dropped handling "renesas,rtc-enabled" property and instead used
>    clock-names to determine RTC is enabled or not and then instantiating
>    RTC device.

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/regulator/raa215300.c

> +       xin = devm_clk_get_optional(&client->dev, "xin");
> +       if (IS_ERR_OR_NULL(xin)) {
> +               clkin = devm_clk_get(&client->dev, "clkin");
> +               if (IS_ERR(clkin))
> +                       return PTR_ERR(clkin);
> +
> +               int_osc_en = false;
> +               xin = NULL;
> +       } else {
> +               clkin = NULL;
> +       }
> +
> +       if (xin || clkin)  {

Perhaps just "if (of_property_present(np, "clocks"))"?

> +               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" : "raa215300_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;
> +       }

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 v4 09/11] regulator: dt-bindings: Add Renesas RAA215300 PMIC bindings

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2154 bytes --]

On Fri, May 19, 2023 at 06:53:03AM +0000, Biju Das wrote:
> > Subject: Re: [PATCH v4 09/11] regulator: dt-bindings: Add Renesas
> > RAA215300 PMIC bindings
> > 
> > On Thu, May 18, 2023 at 12:36:41PM +0100, 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.
> > > +  reg:
> > > +    maxItems: 2
> > > +
> > > +  reg-names:
> > > +    items:
> > > +      - const: main
> > > +      - const: rtc
> > 
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - reg-names
> > 
> > Out of curiosity as much as anything else, why do you need reg-names if
> > the two registers are always required?
> 
> The device has always 2 address spaces and "reg-names" provides a means
> of clear differentiation compared to indices. 
> 
> By enforcing "reg-names" as required property, dt can do some schema-validation
> forcing users to specify "reg-names" in device tree.

Is that not what we have the following for:
  reg:
   items:
     - description: main register space...
     - description: special sauce rtc stuff...
?

The schema validation doesn't stop them putting in the wrong address
either way!

> Implementation wise, we use "rtc" for getting resource details while
> creating the second i2c device which overrides the default address.
> 
> Strictly speaking reg-names is not required, but from a validation
> perspective and since driver is using the same "resource-name" it is
> better to have it??

If the order is set by the descriptions, reg-names seem superfluous
/shrug

Cheers,
Conor.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH v4 09/11] regulator: dt-bindings: Add Renesas RAA215300 PMIC bindings

[Biju Das]

Hi Conor,

Thanks for the feedback.

> -----Original Message-----
> From: Conor Dooley <conor.dooley@microchip.com>
> Sent: Friday, May 19, 2023 3:10 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Conor Dooley <conor@kernel.org>; Rob Herring <robh+dt@kernel.org>;
> Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> <conor+dt@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown
> <broonie@kernel.org>; Geert Uytterhoeven <geert+renesas@glider.be>;
> 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 v4 09/11] regulator: dt-bindings: Add Renesas
> RAA215300 PMIC bindings
> 
> On Fri, May 19, 2023 at 06:53:03AM +0000, Biju Das wrote:
> > > Subject: Re: [PATCH v4 09/11] regulator: dt-bindings: Add Renesas
> > > RAA215300 PMIC bindings
> > >
> > > On Thu, May 18, 2023 at 12:36:41PM +0100, 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.
> > > > +  reg:
> > > > +    maxItems: 2
> > > > +
> > > > +  reg-names:
> > > > +    items:
> > > > +      - const: main
> > > > +      - const: rtc
> > >
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - reg-names
> > >
> > > Out of curiosity as much as anything else, why do you need reg-names
> > > if the two registers are always required?
> >
> > The device has always 2 address spaces and "reg-names" provides a
> > means of clear differentiation compared to indices.
> >
> > By enforcing "reg-names" as required property, dt can do some
> > schema-validation forcing users to specify "reg-names" in device tree.
> 
> Is that not what we have the following for:
>   reg:
>    items:
>      - description: main register space...
>      - description: special sauce rtc stuff...
> ?

OK, will add description.

> 
> The schema validation doesn't stop them putting in the wrong address
> either way!

OK, will drop reg-names from required property.

> 
> > Implementation wise, we use "rtc" for getting resource details while
> > creating the second i2c device which overrides the default address.
> >
> > Strictly speaking reg-names is not required, but from a validation
> > perspective and since driver is using the same "resource-name" it is
> > better to have it??
> 
> If the order is set by the descriptions, reg-names seem superfluous
> /shrug

Agreed.

Cheers,
Biju

Re: [PATCH v4 09/11] regulator: dt-bindings: Add Renesas RAA215300 PMIC bindings

[Geert Uytterhoeven]

Hi Biju,

On Fri, May 19, 2023 at 4:39 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > -----Original Message-----
> > From: Conor Dooley <conor.dooley@microchip.com>
> > Sent: Friday, May 19, 2023 3:10 PM
> > To: Biju Das <biju.das.jz@bp.renesas.com>
> > Cc: Conor Dooley <conor@kernel.org>; Rob Herring <robh+dt@kernel.org>;
> > Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> > <conor+dt@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown
> > <broonie@kernel.org>; Geert Uytterhoeven <geert+renesas@glider.be>;
> > 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 v4 09/11] regulator: dt-bindings: Add Renesas
> > RAA215300 PMIC bindings
> >
> > On Fri, May 19, 2023 at 06:53:03AM +0000, Biju Das wrote:
> > > > Subject: Re: [PATCH v4 09/11] regulator: dt-bindings: Add Renesas
> > > > RAA215300 PMIC bindings
> > > >
> > > > On Thu, May 18, 2023 at 12:36:41PM +0100, 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.
> > > > > +  reg:
> > > > > +    maxItems: 2
> > > > > +
> > > > > +  reg-names:
> > > > > +    items:
> > > > > +      - const: main
> > > > > +      - const: rtc
> > > >
> > > > > +required:
> > > > > +  - compatible
> > > > > +  - reg
> > > > > +  - reg-names
> > > >
> > > > Out of curiosity as much as anything else, why do you need reg-names
> > > > if the two registers are always required?
> > >
> > > The device has always 2 address spaces and "reg-names" provides a
> > > means of clear differentiation compared to indices.
> > >
> > > By enforcing "reg-names" as required property, dt can do some
> > > schema-validation forcing users to specify "reg-names" in device tree.
> >
> > Is that not what we have the following for:
> >   reg:
> >    items:
> >      - description: main register space...
> >      - description: special sauce rtc stuff...
> > ?
>
> OK, will add description.
>
> >
> > The schema validation doesn't stop them putting in the wrong address
> > either way!
>
> OK, will drop reg-names from required property.

Please don't, as i2c_new_ancillary_device() does rely on the name
to set the address when overriding from the default.

> > > Implementation wise, we use "rtc" for getting resource details while
> > > creating the second i2c device which overrides the default address.
> > >
> > > Strictly speaking reg-names is not required, but from a validation
> > > perspective and since driver is using the same "resource-name" it is
> > > better to have it??
> >
> > If the order is set by the descriptions, reg-names seem superfluous
> > /shrug

We have plenty of these, as it turned out to be much easier for validation
to fix the order in the bindings than to support random order ;-)
Lots of Linux drivers that do use the names don't care about the order.
If all actual DTS used the same order, usually the order was fixed during
the text to json-schema conversion...

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 v4 09/11] regulator: dt-bindings: Add Renesas RAA215300 PMIC bindings

[Geert Uytterhoeven]

On Fri, May 19, 2023 at 4:49 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Fri, May 19, 2023 at 4:39 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > From: Conor Dooley <conor.dooley@microchip.com>
> > > If the order is set by the descriptions, reg-names seem superfluous
> > > /shrug
>
> We have plenty of these, as it turned out to be much easier for validation
> to fix the order in the bindings than to support random order ;-)
> Lots of Linux drivers that do use the names don't care about the order.
> If all actual DTS used the same order, usually the order was fixed during

I meant "When all actual DTS...".

> the text to json-schema conversion...

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 v4 09/11] regulator: dt-bindings: Add Renesas RAA215300 PMIC bindings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 650 bytes --]

On Fri, May 19, 2023 at 03:10:25PM +0100, Conor Dooley wrote:
> On Fri, May 19, 2023 at 06:53:03AM +0000, Biju Das wrote:

> > By enforcing "reg-names" as required property, dt can do some schema-validation
> > forcing users to specify "reg-names" in device tree.

> Is that not what we have the following for:
>   reg:
>    items:
>      - description: main register space...
>      - description: special sauce rtc stuff...
> ?

> The schema validation doesn't stop them putting in the wrong address
> either way!

Being able to look things up by name does help make the code using the
binding more readable, and it helps with reading the DTs too.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4 09/11] regulator: dt-bindings: Add Renesas RAA215300 PMIC bindings

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1024 bytes --]

On Fri, May 19, 2023 at 04:49:44PM +0200, Geert Uytterhoeven wrote:
> On Fri, May 19, 2023 at 4:39 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > -----Original Message-----
> > > From: Conor Dooley <conor.dooley@microchip.com>
> > > On Fri, May 19, 2023 at 06:53:03AM +0000, Biju Das wrote:
> > > > > Subject: Re: [PATCH v4 09/11] regulator: dt-bindings: Add Renesas

> > > > > > +required:
> > > > > > +  - compatible
> > > > > > +  - reg
> > > > > > +  - reg-names
> > > > >
> > > > > Out of curiosity as much as anything else, why do you need reg-names
> > > > > if the two registers are always required?

> > OK, will drop reg-names from required property.
> 
> Please don't, as i2c_new_ancillary_device() does rely on the name
> to set the address when overriding from the default.

That looks like my answer! Thanks Geert & sorry for the noise here.
Modulo Geert's requested change, I think I owe you a
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Biju.

Thanks,
Conor.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH v4 01/11] i2c: Enhance i2c_new_ancillary_device API

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v4 01/11] i2c: Enhance i2c_new_ancillary_device API
> 
> Hi Biju,
> 
> On Thu, May 18, 2023 at 1:37 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)
> >
> > Added helper function __i2c_new_dummy_device to share the code between
> > i2c_new_dummy_device and i2c_new_ancillary_device().
> >
> > Also added helper function __i2c_new_client_device() to pass parent
> > dev parameter, so that the ancillary device can assign its parent
> > during creation.
> >
> > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v3->v4:
> >  * Dropped Rb tag from Geert as there are new changes.
> >  * Introduced __i2c_new_dummy_device() to share the code between
> >    i2c_new_dummy_device and i2c_new_ancillary_device().
> >  * Introduced __i2c_new_client_device() to pass parent dev
> >    parameter, so that the ancillary device can assign its parent
> during
> >    creation.
> 
> Thanks for the update!
> 
> LGTM, a few minor comments below.
> 
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -893,24 +893,10 @@ int i2c_dev_irq_from_resources(const struct
> resource *resources,
> >         return 0;
> >  }
> >
> > -/**
> > - * i2c_new_client_device - instantiate an i2c device
> > - * @adap: the adapter managing the device
> > - * @info: describes one I2C device; bus_num is ignored
> > - * Context: can sleep
> > - *
> > - * Create an i2c device. Binding is handled through driver model
> > - * probe()/remove() methods.  A driver may be bound to this device
> > when we
> > - * return from this function, or any later moment (e.g. maybe
> > hotplugging will
> > - * load the driver module).  This call is not appropriate for use by
> > mainboard
> > - * initialization logic, which usually runs during an arch_initcall()
> > long
> > - * before any i2c_adapter could exist.
> > - *
> > - * This returns the new i2c client, which may be saved for later use
> > with
> > - * i2c_unregister_device(); or an ERR_PTR to describe the error.
> > - */
> > -struct i2c_client *
> > -i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info
> > const *info)
> > +static struct i2c_client *
> > +__i2c_new_client_device(struct i2c_adapter *adap,
> > +                       struct i2c_board_info const *info,
> > +                       struct device *dev)
> 
> struct device *parent?

OK. Will use parent below as well(__i2c_new_dummy_device())

> 
> >  {
> >         struct i2c_client       *client;
> >         int                     status;
> 
> > @@ -1054,6 +1062,25 @@ static struct i2c_driver dummy_driver = {
> >         .id_table       = dummy_id,
> >  };
> >
> > +static struct i2c_client *__i2c_new_dummy_device(struct i2c_adapter
> *adapter,
> > +                                                u16 address, const
> char *name,
> > +                                                struct device *dev) {
> > +       struct i2c_board_info info = {
> > +               I2C_BOARD_INFO("dummy", address),
> > +       };
> > +
> > +       if (name) {
> > +               ssize_t ret = strscpy(info.type, name,
> > + sizeof(info.type));
> > +
> > +               if (ret < 0)
> > +                       return ERR_PTR(dev_err_probe(&adapter->dev,
> ret,
> > +                                                    "Invalid device
> > + name\n"));
> 
> %s too long?

Ok, will add %s
			return ERR_PTR(dev_err_probe(&adapter->dev, ret,
						     "Invalid device name: %s\n",
						     name));
> 
> > +       }
> > +
> > +       return __i2c_new_client_device(adapter, &info, dev); }
> > +
> >  /**
> >   * i2c_new_dummy_device - return a new i2c device bound to a dummy
> driver
> >   * @adapter: the adapter managing the device
> 
> > @@ -1122,15 +1145,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.
> 
> Please add something like:
> 
>     The ancillary's device parent will be set to the primary device.

OK, will add.

> 
> >   *
> >   * On DT-based platforms the address is retrieved from the "reg"
> property entry
> >   * cell whose "reg-names" value matches the slave name.
> 
> > @@ -1153,7 +1179,9 @@ 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);
> > +       return __i2c_new_dummy_device(client->adapter, addr,
> > +                                     aux_device_name ?
> > + aux_device_name : NULL,
> 
> You can just pass aux_device_name.

Agreed.

Cheers,
Biju

RE: [PATCH v4 03/11] dt-bindings: rtc: isil,isl1208: Document clock and clock-names properties

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v4 03/11] dt-bindings: rtc: isil,isl1208: Document
> clock and clock-names properties
> 
> Hi Biju,
> 
> On Thu, May 18, 2023 at 1:37 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > As per the HW manual, XTOSCB bit setting is as follows
> >
> > If using an external clock signal, set the XTOSCB bit as 1 to disable
> > the crystal oscillator.
> >
> > If using an external crystal, the XTOSCB bit needs to be set at 0 to
> > enable the crystal oscillator.
> >
> > Document clock and clock-names properties.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v4:
> >  * New patch
> 
> Thanks for your patch!
> 
> > --- a/Documentation/devicetree/bindings/rtc/isil,isl1208.yaml
> > +++ b/Documentation/devicetree/bindings/rtc/isil,isl1208.yaml
> > @@ -25,6 +25,19 @@ properties:
> >    reg:
> >      maxItems: 1
> >
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    description: |
> > +      Use xin, if connected to an external crystal.
> > +      Use clkin, if connected to an external clock signal.
> > +    oneOf:
> > +      - items:
> > +          - const: xin
> > +      - items:
> > +          - const: clkin
> > +
> 
> I guess that oneOf scheme is equivalent to the simpler
> 
>     enum:
>       - xin
>       - clkin

Agreed.

Cheers,
Biju

RE: [PATCH v4 05/11] rtc: isl1208: Make similar I2C and DT-based matching table

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Friday, May 19, 2023 1:39 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; linux-rtc@vger.kernel.org; Fabrizio
> Castro <fabrizio.castro.jz@renesas.com>; linux-renesas-
> soc@vger.kernel.org
> Subject: Re: [PATCH v4 05/11] rtc: isl1208: Make similar I2C and DT-
> based matching table
> 
> On Thu, May 18, 2023 at 1:37 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > The isl1208_id[].driver_data could store a pointer to the config, like
> > for DT-based matching, making I2C and DT-based matching more similar.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v4:
> >  * New patch.
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> > @@ -822,9 +822,9 @@ isl1208_probe(struct i2c_client *client)
> >         } else {
> >                 const struct i2c_device_id *id =
> > i2c_match_id(isl1208_id, client);
> >
> > -               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;
> 
> It's a pity there's no i2c_get_match_data() yet...

You mean, introduce [1] and optimize ??

	if (client->dev.of_node)
		isl1208->config = of_device_get_match_data(&client->dev);
	else
		isl1208->config = i2c_get_match_data(isl1208_id, client);

	if (!isl1208->config)
		return -ENODEV;

[1]
const void * i2c_get_match_data(const struct i2c_device_id *id, const struct i2c_client *client)
{
	if (!(id && client))
		return NULL;

	while (id->name[0]) {
		if (strcmp(client->name, id->name) == 0)
			return id->driver_data;
		id++;
	}
	return NULL;
}
EXPORT_SYMBOL(i2c_get_match_data);

Cheers,
Biju

RE: [PATCH v4 05/11] rtc: isl1208: Make similar I2C and DT-based matching table

[Biju Das]

+ Wolfram

> -----Original Message-----
> From: Biju Das <biju.das.jz@bp.renesas.com>
> Sent: Friday, May 19, 2023 5:09 PM
> To: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; linux-rtc@vger.kernel.org; Fabrizio
> Castro <fabrizio.castro.jz@renesas.com>; linux-renesas-
> soc@vger.kernel.org
> Subject: RE: [PATCH v4 05/11] rtc: isl1208: Make similar I2C and DT-
> based matching table
> 
> Hi Geert,
> 
> Thanks for the feedback.
> 
> > -----Original Message-----
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > Sent: Friday, May 19, 2023 1:39 PM
> > To: Biju Das <biju.das.jz@bp.renesas.com>
> > Cc: Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni
> > <alexandre.belloni@bootlin.com>; linux-rtc@vger.kernel.org; Fabrizio
> > Castro <fabrizio.castro.jz@renesas.com>; linux-renesas-
> > soc@vger.kernel.org
> > Subject: Re: [PATCH v4 05/11] rtc: isl1208: Make similar I2C and DT-
> > based matching table
> >
> > On Thu, May 18, 2023 at 1:37 PM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > The isl1208_id[].driver_data could store a pointer to the config,
> > > like for DT-based matching, making I2C and DT-based matching more
> similar.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > > v4:
> > >  * New patch.
> >
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > > @@ -822,9 +822,9 @@ isl1208_probe(struct i2c_client *client)
> > >         } else {
> > >                 const struct i2c_device_id *id =
> > > i2c_match_id(isl1208_id, client);
> > >
> > > -               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;
> >
> > It's a pity there's no i2c_get_match_data() yet...
> 
> You mean, introduce [1] and optimize ??
> 
> 	if (client->dev.of_node)
> 		isl1208->config = of_device_get_match_data(&client->dev);
> 	else
> 		isl1208->config = i2c_get_match_data(isl1208_id, client);
> 
> 	if (!isl1208->config)
> 		return -ENODEV;
> 
> [1]
> const void * i2c_get_match_data(const struct i2c_device_id *id, const
> struct i2c_client *client) {
> 	if (!(id && client))
> 		return NULL;
> 
> 	while (id->name[0]) {
> 		if (strcmp(client->name, id->name) == 0)
> 			return id->driver_data;
> 		id++;
> 	}
> 	return NULL;
> }
> EXPORT_SYMBOL(i2c_get_match_data);
> 
> Cheers,
> Biju

RE: [PATCH v4 07/11] rtc: isl1208: Add isl1208_set_xtoscb()

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v4 07/11] rtc: isl1208: Add isl1208_set_xtoscb()
> 
> Hi Biju,
> 
> On Thu, May 18, 2023 at 1:37 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > As per the HW manual, setting of XTOSCB bit as follws
> 
> ... set the XTOSCB bit as follows:

Oops. Will fix.

> 
> > If using an external clock signal, set the XTOSCB bit as 1 to disable
> > the crystal oscillator.
> >
> > If using an external crystal, the XTOSCB bit needs to be set at 0 to
> > enable the crystal oscillator.
> >
> > Add isl1208_set_xtoscb() to set XTOSCB bit based on the clock-names
> > property. Fallback is enabling the internal crystal oscillator.
> >
> > While at it, introduce a variable "sr" for reading status register
> 
> the status register

Ok.

> 
> > in probe() as it is reused for writing.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v4:
> >  * New patch.
> 
> Thanks for your patch!
> 
> > --- a/drivers/rtc/rtc-isl1208.c
> > +++ b/drivers/rtc/rtc-isl1208.c
> 
> > @@ -837,6 +852,13 @@ isl1208_probe(struct i2c_client *client)
> >                 isl1208->config = (struct isl1208_config *)id-
> >driver_data;
> >         }
> >
> > +       xin = devm_clk_get(&client->dev, "xin");
> > +       if (IS_ERR(xin)) {
> > +               clkin = devm_clk_get(&client->dev, "clkin");
> > +               if (!IS_ERR(clkin))
> > +                       int_osc_en = false;
> > +       }
> 
> devm_clk_get_optional()

OK, Will use optional variant.

> (doesn't devm_clk_get() print an error message?)
I will double check. If I remember correctly it didn't throw any prtints.

> 
> And check for NULL to find out if a clock is present.

OK.

> IS_ERR() is a real error condition to be propagated.

Agreed.

Cheers,
Biju

RE: [PATCH v4 08/11] 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: Friday, May 19, 2023 1:54 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; linux-rtc@vger.kernel.org; Fabrizio
> Castro <fabrizio.castro.jz@renesas.com>; linux-renesas-
> soc@vger.kernel.org
> Subject: Re: [PATCH v4 08/11] rtc: isl1208: Add support for the built-in
> RTC on the PMIC RAA215300
> 
> Hi Biju,
> 
> On Thu, May 18, 2023 at 1:37 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
> 
> instantiates the
OK.
> 
> > RTC device based on i2c_device_id.
> >
> > Enhance isl1208_set_xtoscb() to handle inverted bit and internal
> > oscillator is enabled or not is determined by the parent clock name.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v3->v4:
> >  * Added support for internal oscillator enable/disable.
> 
> Thanks for the update!
> 
> > --- a/drivers/rtc/rtc-isl1208.c
> > +++ b/drivers/rtc/rtc-isl1208.c
> 
> > @@ -852,11 +865,25 @@ isl1208_probe(struct i2c_client *client)
> >                 isl1208->config = (struct isl1208_config *)id-
> >driver_data;
> >         }
> >
> > -       xin = devm_clk_get(&client->dev, "xin");
> > -       if (IS_ERR(xin)) {
> > -               clkin = devm_clk_get(&client->dev, "clkin");
> > -               if (!IS_ERR(clkin))
> > +       if (client->dev.parent->type == &i2c_client_type) {
> > +               xin = of_clk_get_by_name(client->dev.parent->of_node,
> "xin");
> > +               if (IS_ERR(xin)) {
> 
> -ENOENT means clock not present, so continue below.
> Any other error codes are to be propagated upstream.

Agreed.

> 
> > +                       clkin = of_clk_get_by_name(client->dev.parent-
> >of_node, "clkin");
> > +                       if (IS_ERR(clkin))
> 
> Likewise.
> 
> > +                               return -ENODEV;
> 
> Clock not present is not an error, as the clock is optional for DT
> backwards compatibility.

OK.

> 
> > +
> >                         int_osc_en = false;
> > +                       clk_put(clkin);
> > +               } else {
> > +                       clk_put(xin);
> > +               }
> > +       } else {
> > +               xin = devm_clk_get(&client->dev, "xin");
> > +               if (IS_ERR(xin)) {
> > +                       clkin = devm_clk_get(&client->dev, "clkin");
> > +                       if (!IS_ERR(clkin))
> > +                               int_osc_en = false;
> > +               }
> >         }
> >
> >         isl1208->rtc = devm_rtc_allocate_device(&client->dev);
> > @@ -876,7 +903,8 @@ isl1208_probe(struct i2c_client *client)
> >                 return sr;
> >         }
> >
> > -       rc = isl1208_set_xtoscb(client, sr, int_osc_en);
> > +       rc = isl1208_set_xtoscb(client, sr, int_osc_en,
> > +
> > + isl1208->config->has_inverted_osc_bit);
> 
> No need to change isl1208_set_xtoscb() (but perhaps rename the
> int_osc_en parameter?) if you would pass "int_osc_en ^ isl1208->config-
> >has_inverted_osc_bit".
> (beware: C has no logical ^^, only bitwise ^).

OK will use "u8 xtosb_val" as the parameter.

Cheers,
Biju

> 
> 
> >         if (rc)
> >                 return rc;
> 
> 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 v4 09/11] regulator: dt-bindings: Add Renesas RAA215300 PMIC bindings

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v4 09/11] regulator: dt-bindings: Add Renesas
> RAA215300 PMIC bindings
> 
> Hi Biju,
> 
> On Thu, May 18, 2023 at 1:37 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>
> > ---
> > v3->v4:
> >  * Moved bindings from mfd->regulator.
> >  * Dropped minItems from reg.
> >  * Dropped renesas,rtc-enabled property and instead used clock-names
> property
> >    to find RTC is enabled or not.
> >  * Added reg-names in required property.
> >  * Updated the example.
> 
> Thanks for your patch!
> 
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/regulator/renesas,raa215300.ya
> > +++ ml
> > @@ -0,0 +1,84 @@
> > +# 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:
> > +    maxItems: 2
> > +
> > +  reg-names:
> > +    items:
> > +      - const: main
> > +      - const: rtc
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> 
> Perhaps add a description to make it clear that not providing clocks is
> supported, and means the RTC is disabled?

Agreed.

> 
> > +
> > +  clock-names:
> > +    description: |
> > +      Use xin, if connected to an external crystal.
> > +      Use clkin, if connected to an external clock signal.
> > +    oneOf:
> > +      - items:
> > +          - const: xin
> > +      - items:
> > +          - const: clkin
> 
> Please replace oneOf+items by enum.

OK.

Cheers,
Biju

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

RE: [PATCH v4 10/11] regulator: Add Renesas PMIC RAA215300 driver

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v4 10/11] regulator: Add Renesas PMIC RAA215300
> driver
> 
> Hi Biju,
> 
> On Thu, May 18, 2023 at 1:37 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>
> > ---
> > v3->v4:
> >  * Moved from mfd->regulator as it doesn't use MFD APIs
> >  * Dropped handling "renesas,rtc-enabled" property and instead used
> >    clock-names to determine RTC is enabled or not and then
> instantiating
> >    RTC device.
> 
> Thanks for your patch!
> 
> > --- /dev/null
> > +++ b/drivers/regulator/raa215300.c
> 
> > +       xin = devm_clk_get_optional(&client->dev, "xin");
> > +       if (IS_ERR_OR_NULL(xin)) {
> > +               clkin = devm_clk_get(&client->dev, "clkin");
> > +               if (IS_ERR(clkin))
> > +                       return PTR_ERR(clkin);
> > +
> > +               int_osc_en = false;
> > +               xin = NULL;
> > +       } else {
> > +               clkin = NULL;
> > +       }
> > +
> > +       if (xin || clkin)  {
> 
> Perhaps just "if (of_property_present(np, "clocks"))"?

Agreed.

Cheers,
Biju

> 
> > +               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" :
> "raa215300_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;
> > +       }
> 
> 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 v4 05/11] rtc: isl1208: Make similar I2C and DT-based matching table

[Geert Uytterhoeven]

Hi Biju.

On Fri, May 19, 2023 at 6:08 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > -----Original Message-----
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > Sent: Friday, May 19, 2023 1:39 PM
> > To: Biju Das <biju.das.jz@bp.renesas.com>
> > Cc: Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni
> > <alexandre.belloni@bootlin.com>; linux-rtc@vger.kernel.org; Fabrizio
> > Castro <fabrizio.castro.jz@renesas.com>; linux-renesas-
> > soc@vger.kernel.org
> > Subject: Re: [PATCH v4 05/11] rtc: isl1208: Make similar I2C and DT-
> > based matching table
> >
> > On Thu, May 18, 2023 at 1:37 PM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > The isl1208_id[].driver_data could store a pointer to the config, like
> > > for DT-based matching, making I2C and DT-based matching more similar.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > > v4:
> > >  * New patch.
> >
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > > @@ -822,9 +822,9 @@ isl1208_probe(struct i2c_client *client)
> > >         } else {
> > >                 const struct i2c_device_id *id =
> > > i2c_match_id(isl1208_id, client);
> > >
> > > -               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;
> >
> > It's a pity there's no i2c_get_match_data() yet...
>
> You mean, introduce [1] and optimize ??
>
>         if (client->dev.of_node)
>                 isl1208->config = of_device_get_match_data(&client->dev);
>         else
>                 isl1208->config = i2c_get_match_data(isl1208_id, client);
>
>         if (!isl1208->config)
>                 return -ENODEV;

Exactly.  Might be better to do that later, to avoid stalling this series.

>
> [1]
> const void * i2c_get_match_data(const struct i2c_device_id *id, const struct i2c_client *client)
> {
>         if (!(id && client))
>                 return NULL;
>
>         while (id->name[0]) {
>                 if (strcmp(client->name, id->name) == 0)
>                         return id->driver_data;
>                 id++;
>         }
>         return NULL;

Please reuse the existing i2c_match_id(), just like of_device_get_match_data()
reuses of_match_device().

> }
> EXPORT_SYMBOL(i2c_get_match_data);
>
> Cheers,
> Biju



-- 
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 v4 05/11] rtc: isl1208: Make similar I2C and DT-based matching table

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Friday, May 19, 2023 8:43 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; linux-rtc@vger.kernel.org; Fabrizio
> Castro <fabrizio.castro.jz@renesas.com>; linux-renesas-
> soc@vger.kernel.org
> Subject: Re: [PATCH v4 05/11] rtc: isl1208: Make similar I2C and DT-
> based matching table
> 
> Hi Biju.
> 
> On Fri, May 19, 2023 at 6:08 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > -----Original Message-----
> > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > Sent: Friday, May 19, 2023 1:39 PM
> > > To: Biju Das <biju.das.jz@bp.renesas.com>
> > > Cc: Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni
> > > <alexandre.belloni@bootlin.com>; linux-rtc@vger.kernel.org; Fabrizio
> > > Castro <fabrizio.castro.jz@renesas.com>; linux-renesas-
> > > soc@vger.kernel.org
> > > Subject: Re: [PATCH v4 05/11] rtc: isl1208: Make similar I2C and DT-
> > > based matching table
> > >
> > > On Thu, May 18, 2023 at 1:37 PM Biju Das
> > > <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > The isl1208_id[].driver_data could store a pointer to the config,
> > > > like for DT-based matching, making I2C and DT-based matching more
> similar.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > ---
> > > > v4:
> > > >  * New patch.
> > >
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > >
> > > > @@ -822,9 +822,9 @@ isl1208_probe(struct i2c_client *client)
> > > >         } else {
> > > >                 const struct i2c_device_id *id =
> > > > i2c_match_id(isl1208_id, client);
> > > >
> > > > -               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;
> > >
> > > It's a pity there's no i2c_get_match_data() yet...
> >
> > You mean, introduce [1] and optimize ??
> >
> >         if (client->dev.of_node)
> >                 isl1208->config = of_device_get_match_data(&client-
> >dev);
> >         else
> >                 isl1208->config = i2c_get_match_data(isl1208_id,
> > client);
> >
> >         if (!isl1208->config)
> >                 return -ENODEV;
> 
> Exactly.  Might be better to do that later, to avoid stalling this
> series.

OK, will do it later.

> 
> >
> > [1]
> > const void * i2c_get_match_data(const struct i2c_device_id *id, const
> > struct i2c_client *client) {
> >         if (!(id && client))
> >                 return NULL;
> >
> >         while (id->name[0]) {
> >                 if (strcmp(client->name, id->name) == 0)
> >                         return id->driver_data;
> >                 id++;
> >         }
> >         return NULL;
> 
> Please reuse the existing i2c_match_id(), just like
> of_device_get_match_data() reuses of_match_device().

OK, Will send this as standalone patch, as it is enhancement.

Cheers,
Biju

> 
> > }
> > EXPORT_SYMBOL(i2c_get_match_data);
> >
> > Cheers,
> > Biju
> 
> 
> 
> --
> 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