Hi Wolfram,
> -----Original Message-----
> From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> Sent: Tuesday, May 9, 2023 6:05 PM
> To: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>; Wolfram Sang <wsa-dev@sang-
> engineering.com>
> 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>
> Subject: RE: [PATCH v2 4/5] mfd: Add Renesas PMIC RAA215300 RTC driver
>
> 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.
I will post a proper patch based on [1], so that we can discuss it there.
Cheers,
Biju
>
> [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
