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
