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

[Biju Das <biju.das.jz@bp.renesas.com>]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Tuesday, May 9, 2023 10:40 AM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Lee Jones <lee@kernel.org>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; Alessandro Zummo <a.zummo@towertech.it>;
> Magnus Damm <magnus.damm@gmail.com>; linux-renesas-soc@vger.kernel.org;
> Fabrizio Castro <fabrizio.castro.jz@renesas.com>; Wolfram Sang <wsa-
> dev@sang-engineering.com>
> Subject: Re: [PATCH v2 4/5] mfd: Add Renesas PMIC RAA215300 RTC driver
> 
> Hi Biju,
> 
> On Tue, May 9, 2023 at 11:32 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > Subject: Re: [PATCH v2 4/5] mfd: Add Renesas PMIC RAA215300 RTC
> > > driver On Tue, May 9, 2023 at 9:35 AM Biju Das
> <biju.das.jz@bp.renesas.com> wrote:
> > > > > Subject: Re: [PATCH v2 4/5] mfd: Add Renesas PMIC RAA215300 RTC
> > > > > driver On Tue, May 9, 2023 at 9:06 AM Biju Das
> > > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > Subject: [PATCH v2 4/5] mfd: Add Renesas PMIC RAA215300 RTC
> > > > > > > driver
> > > > > > >
> > > > > > > Currently, it is not possible to instantiate the i2c client
> > > > > > > driver using MFD cell as it is not a platform driver. Add
> > > > > > > support for Renesas PMIC
> > > > > > > RAA215300 RTC platform driver, so that it can be
> > > > > > > instantiated by MFD
> > > > > API.
> > > > > > > The rtc device is created by using
> > > > > > > i2c_new_ancillary_device() and it register as the rtc device
> > > > > > > by the helper function provided by
> > > > > > > rtc-isl2108 driver.
> > > > > >
> > > > > > Not sure this platform driver has to be placed in RTC
> > > > > > subsystem rather than MFD subsystem as PMIC MFD driver, can
> > > > > > instantiate it using
> > > MFD cell??
> > > > >
> > > > > Can't you just instantiate the i2c ancillary device from the
> > > > > PMIC driver, and drop the new mfd and platform rtc drivers?
> > > >
> > > > Yes, it is possible.
> > > >
> > > > The only reason MFD is used for future expansion like adding
> > > > support for
> > > >
> > > > 1) battery charger
> > > > Or
> > > > 2) regulator
> > >
> > > I'd just start with a skeleton regulator ("PMIC") driver...
> > >
> > > > In that case, we can share regmap to sub devices. But these
> > > > drivers are
> > > not currently planned.
> > > >
> > > > Apart from that,
> > > >
> > > > 1) It avoids subsystem dependencies, ie, PMIC driver directly
> > > > calling rtc
> > > driver
> > > >    for registering RTC device.
> > >
> > > You mean the direct call into raa215300_rtc_probe_helper()?
> >
> > Yes, indeed.
> >
> > > I think that can be solved by enhancing i2c_new_ancillary_device()
> > > to take a proper device name, instead of using "dummy"?
> >
> > Geert, Wolfram,
> >
> > Any pointers for instantiating i2c_client device(for eg:
> > rtc_isl1208.c) by enhancing i2c_new_ancillary_device() api?
> >
> > Apart for this, since a single compatible(the pmic one) is used,
> >
> > In rtc device probe, How do we identify this PMIC RTC device and use
> > the "TYPE_ISL1208" config in RTC driver, once we instantiate RTC device?
> 
> You create ancillary devices with different names, based on the detected
> PMIC version, and add new types for them to the isl1208 driver?

OK, I have prototyped this.

Wolfram,

What is your opinion, should I enhance "i2c_new_ancillary_device" like
below[1]?  or directly call "i2c_new_client_device" from PMIC driver to
instantiate the client device?

Please let me know.

[1]
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index ae3af738b03f..79808e852f2f 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1122,15 +1122,17 @@ EXPORT_SYMBOL_GPL(devm_i2c_new_dummy_device);
  * @client: Handle to the primary client
  * @name: Handle to specify which secondary address to get
  * @default_addr: Used as a fallback if no secondary address was specified
+ * @aux_device_name: Optional 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 i2c dummy client.
  *
  * On DT-based platforms the address is retrieved from the "reg" property entry
  * cell whose "reg-names" value matches the slave name.
@@ -1139,10 +1141,12 @@ EXPORT_SYMBOL_GPL(devm_i2c_new_dummy_device);
  * i2c_unregister_device(); or an ERR_PTR to describe the error.
  */
 struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client,
-						const char *name,
-						u16 default_addr)
+					    const char *name,
+					    u16 default_addr,
+					    const char *aux_device_name)
 {
 	struct device_node *np = client->dev.of_node;
+	struct i2c_client *i2c_aux_client;
 	u32 addr = default_addr;
 	int i;
 
@@ -1153,7 +1157,27 @@ struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client,
 	}
 
 	dev_dbg(&client->adapter->dev, "Address for %s : 0x%x\n", name, addr);
-	return i2c_new_dummy_device(client->adapter, addr);
+
+	if (aux_device_name) {
+		struct i2c_board_info info;
+		size_t device_name_len = strlen(device_name);
+
+		if (aux_device_name_len > I2C_NAME_SIZE - 1) {
+			dev_err(&client->adapter->dev, "Invalid device name\n");
+			return ERR_PTR(-EINVAL);
+		}
+
+		memset(&info, 0, sizeof(struct i2c_board_info));
+
+		memcpy(info.type, device_name, device_name_len);
+		info.addr = addr;
+
+		i2c_aux_client = i2c_new_client_device(client->adapter, &info);
+	} else {
+		i2c_aux_client = i2c_new_dummy_device(client->adapter, addr);
+	}
+
+	return i2c_aux_client;
 }
 EXPORT_SYMBOL_GPL(i2c_new_ancillary_device);


Cheers,
Biju