Re: [PATCH v3 2/5] rtc: isl1208: Add support for the built-in RTC on the PMIC RAA215300

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Biju,

On Sat, May 13, 2023 at 6:52 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> 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
> RTC device based on i2c_device_id.
>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> ---
> 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.

Thanks for the update!

> --- a/drivers/rtc/rtc-isl1208.c
> +++ b/drivers/rtc/rtc-isl1208.c
> @@ -74,6 +74,7 @@ enum isl1208_id {
>         TYPE_ISL1209,
>         TYPE_ISL1218,
>         TYPE_ISL1219,
> +       TYPE_RAA215300_RTC_A0,
>         ISL_LAST_ID
>  };
>
> @@ -83,11 +84,13 @@ static const struct isl1208_config {
>         unsigned int    nvmem_length;
>         unsigned        has_tamper:1;
>         unsigned        has_timestamp:1;
> +       unsigned        has_inverted_osc_bit: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_RAA215300_RTC_A0] = { "rtc_a0", 2, false, false, true },
>  };
>
>  static const struct i2c_device_id isl1208_id[] = {
> @@ -95,6 +98,7 @@ static const struct i2c_device_id isl1208_id[] = {
>         { "isl1209", TYPE_ISL1209 },
>         { "isl1218", TYPE_ISL1218 },
>         { "isl1219", TYPE_ISL1219 },
> +       { "rtc_a0", TYPE_RAA215300_RTC_A0 },

"rtc_a0" is IMHO a too-generic name.


>         { }
>  };
>  MODULE_DEVICE_TABLE(i2c, isl1208_id);
> @@ -166,6 +170,16 @@ isl1208_i2c_validate_client(struct i2c_client *client)
>         return 0;
>  }
>
> +static int
> +isl1208_set_external_oscillator(struct i2c_client *client, int rc,

s/rc/sr/

> +                               bool is_inverted_oscillator_bit)
> +{
> +       if (is_inverted_oscillator_bit)

This condition is always true, given all callers in your series.

> +               rc |= ISL1208_REG_SR_XTOSCB;

If you do decide to keep it, you probably want to add an else branch
to make sure the bit is cleared.

> +
> +       return i2c_smbus_write_byte_data(client, ISL1208_REG_SR, rc);
> +}
> +
>  static int
>  isl1208_i2c_get_sr(struct i2c_client *client)
>  {
> @@ -845,6 +859,13 @@ isl1208_probe(struct i2c_client *client)
>                 return rc;
>         }
>
> +       if (isl1208->config->has_inverted_osc_bit) {
> +               rc = isl1208_set_external_oscillator(client, rc,

Passing "rc" is confusing, this is really the status register value
obtained above...

> +                                                    isl1208->config->has_inverted_osc_bit);
> +               if (rc)
> +                       return rc;

If we get here, rc is always zero ...

> +       }
> +
>         if (rc & ISL1208_REG_SR_RTCF)

... thus breaking this check..

>                 dev_warn(&client->dev, "rtc power failure detected, "
>                          "please set clock.\n");

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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



[Index of Archives]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux