Hi Geert,
Thanks for the feedback.
> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: Tuesday, May 9, 2023 10:40 AM
> To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> Cc: Lee Jones <lee@xxxxxxxxxx>; Alexandre Belloni
> <alexandre.belloni@xxxxxxxxxxx>; Alessandro Zummo <a.zummo@xxxxxxxxxxxx>;
> Magnus Damm <magnus.damm@xxxxxxxxx>; linux-renesas-soc@xxxxxxxxxxxxxxx;
> Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>; Wolfram Sang <wsa-
> dev@xxxxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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
