diff mbox series

[v6,11/11] rtc: isl1208: Add support for the built-in RTC on the PMIC RAA215300

Message ID 20230602142426.438375-12-biju.das.jz@bp.renesas.com
State Superseded
Headers show
Series Add Renesas PMIC RAA215300 and built-in RTC support | expand

Commit Message

Biju Das June 2, 2023, 2:24 p.m. UTC 
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 instantiates the
RTC device based on i2c_device_id.

The internal oscillator is enabled or not is determined by the
parent clock name.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v5->v6:
 * Added Rb tag from Geert.
 * Parsing of parent node is moved from probe->isl1208_clk_present()
 * Added comment for parsing parent node for getting clock resource.
 * Replaced XOR->NOT to make the operation more clear for the inverted case.
v4->v5:
 * Updated commit description.
 * Replaced "unsigned long"->"kernel_ulong_t" in isl1208_id[].
 * -ENOENT means clock not present, so any other errors are propagated.
 * Dropped bool inverted parameter from isl1208_set_xtoscb() instead
   using xor to compute the value of xtoscb.
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, 37 insertions(+), 7 deletions(-)

Comments

Geert Uytterhoeven June 5, 2023, 11:56 a.m. UTC | #1 
Hi Biju,

On Fri, Jun 2, 2023 at 4:25 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 instantiates the
> RTC device based on i2c_device_id.
>
> The internal oscillator is enabled or not is determined by the
> parent clock name.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v5->v6:
>  * Added Rb tag from Geert.
>  * Parsing of parent node is moved from probe->isl1208_clk_present()
>  * Added comment for parsing parent node for getting clock resource.
>  * Replaced XOR->NOT to make the operation more clear for the inverted case.

Thanks for the update!

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

> @@ -822,14 +831,32 @@ isl1208_clk_present(struct i2c_client *client, const char *name)
>         struct clk *clk;
>         int ret;
>
> -       clk = devm_clk_get_optional(&client->dev, name);
> -       if (IS_ERR(clk)) {
> -               ret = PTR_ERR(clk);
> -       } else {
> -               if (!clk)
> -                       ret = 0;
> -               else
> +       /*
> +        * For the built-in RTC found on PMIC RAA21530, enabling of the
> +        * internal oscillator is based on the parent clock. So parse the
> +        * parent node to get the details.
> +        */
> +       if (client->dev.parent->type == &i2c_client_type) {
> +               clk = of_clk_get_by_name(client->dev.parent->of_node, name);
> +               if (IS_ERR(clk)) {
> +                       if (PTR_ERR(clk) != -ENOENT)
> +                               ret = PTR_ERR(clk);
> +                       else
> +                               ret = 0;
> +               } else {
> +                       clk_put(clk);
>                         ret = 1;
> +               }

Perhaps reshuffle the conditions to decrease indentation?
And return early?

    if (!IS_ERR(clk)) {
            clk_put(clk);
            return 1;
    }

    if (PTR_ERR(clk) != -ENOENT)
            return PTR_ERR(clk);

    return 0;


> +       } else {
> +               clk = devm_clk_get_optional(&client->dev, name);
> +               if (IS_ERR(clk)) {
> +                       ret = PTR_ERR(clk);
> +               } else {
> +                       if (!clk)
> +                               ret = 0;
> +                       else
> +                               ret = 1;
> +               }

Same comments as [PATCH v6 10/11].

>         }
>
>         return ret;

Gr{oetje,eeting}s,

                        Geert
Biju Das June 5, 2023, 12:22 p.m. UTC | #2 
Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v6 11/11] rtc: isl1208: Add support for the built-in
> RTC on the PMIC RAA215300
> 
> Hi Biju,
> 
> On Fri, Jun 2, 2023 at 4:25 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 instantiates the RTC device
> > based on i2c_device_id.
> >
> > The internal oscillator is enabled or not is determined by the parent
> > clock name.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > v5->v6:
> >  * Added Rb tag from Geert.
> >  * Parsing of parent node is moved from probe->isl1208_clk_present()
> >  * Added comment for parsing parent node for getting clock resource.
> >  * Replaced XOR->NOT to make the operation more clear for the inverted
> case.
> 
> Thanks for the update!
> 
> > --- a/drivers/rtc/rtc-isl1208.c
> > +++ b/drivers/rtc/rtc-isl1208.c
> 
> > @@ -822,14 +831,32 @@ isl1208_clk_present(struct i2c_client *client,
> const char *name)
> >         struct clk *clk;
> >         int ret;
> >
> > -       clk = devm_clk_get_optional(&client->dev, name);
> > -       if (IS_ERR(clk)) {
> > -               ret = PTR_ERR(clk);
> > -       } else {
> > -               if (!clk)
> > -                       ret = 0;
> > -               else
> > +       /*
> > +        * For the built-in RTC found on PMIC RAA21530, enabling of
> the
> > +        * internal oscillator is based on the parent clock. So parse
> the
> > +        * parent node to get the details.
> > +        */
> > +       if (client->dev.parent->type == &i2c_client_type) {
> > +               clk = of_clk_get_by_name(client->dev.parent->of_node,
> name);
> > +               if (IS_ERR(clk)) {
> > +                       if (PTR_ERR(clk) != -ENOENT)
> > +                               ret = PTR_ERR(clk);
> > +                       else
> > +                               ret = 0;
> > +               } else {
> > +                       clk_put(clk);
> >                         ret = 1;
> > +               }
> 
> Perhaps reshuffle the conditions to decrease indentation?
> And return early?
> 
>     if (!IS_ERR(clk)) {
>             clk_put(clk);
>             return 1;
>     }
> 
>     if (PTR_ERR(clk) != -ENOENT)
>             return PTR_ERR(clk);
> 
>     return 0;

Agreed.

> 
> 
> > +       } else {
> > +               clk = devm_clk_get_optional(&client->dev, name);
> > +               if (IS_ERR(clk)) {
> > +                       ret = PTR_ERR(clk);
> > +               } else {
> > +                       if (!clk)
> > +                               ret = 0;
> > +                       else
> > +                               ret = 1;
> > +               }
> 
> Same comments as [PATCH v6 10/11].

OK, will do this change in v7.

Cheers,
Biju

> 
> >         }
> >
> >         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
diff mbox series

Patch

diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index e0f06677b91b..4c157d99aed6 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 = (kernel_ulong_t)&config_isl1208 },
 	{ "isl1209", .driver_data = (kernel_ulong_t)&config_isl1209 },
 	{ "isl1218", .driver_data = (kernel_ulong_t)&config_isl1218 },
 	{ "isl1219", .driver_data = (kernel_ulong_t)&config_isl1219 },
+	{ "raa215300_a0", .driver_data = (kernel_ulong_t)&config_raa215300_a0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, isl1208_id);
@@ -822,14 +831,32 @@  isl1208_clk_present(struct i2c_client *client, const char *name)
 	struct clk *clk;
 	int ret;
 
-	clk = devm_clk_get_optional(&client->dev, name);
-	if (IS_ERR(clk)) {
-		ret = PTR_ERR(clk);
-	} else {
-		if (!clk)
-			ret = 0;
-		else
+	/*
+	 * For the built-in RTC found on PMIC RAA21530, enabling of the
+	 * internal oscillator is based on the parent clock. So parse the
+	 * parent node to get the details.
+	 */
+	if (client->dev.parent->type == &i2c_client_type) {
+		clk = of_clk_get_by_name(client->dev.parent->of_node, name);
+		if (IS_ERR(clk)) {
+			if (PTR_ERR(clk) != -ENOENT)
+				ret = PTR_ERR(clk);
+			else
+				ret = 0;
+		} else {
+			clk_put(clk);
 			ret = 1;
+		}
+	} else {
+		clk = devm_clk_get_optional(&client->dev, name);
+		if (IS_ERR(clk)) {
+			ret = PTR_ERR(clk);
+		} else {
+			if (!clk)
+				ret = 0;
+			else
+				ret = 1;
+		}
 	}
 
 	return ret;
@@ -899,6 +926,9 @@  isl1208_probe(struct i2c_client *client)
 		return sr;
 	}
 
+	if (isl1208->config->has_inverted_osc_bit)
+		xtosb_val = !xtosb_val;
+
 	rc = isl1208_set_xtoscb(client, sr, xtosb_val);
 	if (rc)
 		return rc;