Hi Alexandre Belloni,
Thanks for the feedback.
> Subject: Re: [PATCH RFC 5/6] rtc: isl1208: Add support for the built-in RTC
> on the PMIC RAA215300
>
> On 03/05/2023 09:46:07+0100, Biju Das wrote:
> > The built-in RTC found on PMIC RAA215300 is the same as ISL1208.
> > However, the external oscillator polarity is determined by the PMIC
> > version. For eg: the PMIC version has inverted polarity for the
> > external oscillator and the corresponding bit in RTC need to be
> > inverted(XTOSCB). This info needs to be shared from PMIC driver to RTC
> > driver, so that it can support all versions without any code changes.
> >
> > Add a new compatible renesas,raa215300-isl1208 to support RTC found on
> > PMIC RAA215300 and renesas,raa215300-pmic property to support
> > different versions.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > drivers/rtc/rtc-isl1208.c | 50
> > +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 50 insertions(+)
> >
> > diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
> > index 73cc6aaf9b8b..f4ea19691ac1 100644
> > --- 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_ISL1208,
> > 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_pmic_parent: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_ISL1208] = { "isl1208", 2, false, false, true },
> > };
> >
> > static const struct i2c_device_id isl1208_id[] = { @@ -104,6 +107,10
> > @@ static const __maybe_unused struct of_device_id isl1208_of_match[] = {
> > { .compatible = "isil,isl1209", .data = &isl1208_configs[TYPE_ISL1209]
> },
> > { .compatible = "isil,isl1218", .data = &isl1208_configs[TYPE_ISL1218]
> },
> > { .compatible = "isil,isl1219", .data =
> > &isl1208_configs[TYPE_ISL1219] },
> > + {
> > + .compatible = "renesas,raa215300-isl1208",
> > + .data = &isl1208_configs[TYPE_RAA215300_ISL1208]
> > + },
> > { }
> > };
> > MODULE_DEVICE_TABLE(of, isl1208_of_match); @@ -166,6 +173,43 @@
> > isl1208_i2c_validate_client(struct i2c_client *client)
> > return 0;
> > }
> >
> > +static bool isl1208_is_xtosc_polarity_inverted(struct i2c_client
> > +*client)
>
> I'd remove polarity from the name of this function
Agreed.
>
> > +{
> > + struct device *dev = &client->dev;
> > + struct i2c_client *pmic_dev;
> > + unsigned int *pmic_version;
> > + struct device_node *np;
> > + bool ret = false;
> > +
> > + np = of_parse_phandle(dev->of_node, "renesas,raa215300-pmic", 0);
> > + if (np)
> > + pmic_dev = of_find_i2c_device_by_node(np);
> > +
> > + of_node_put(np);
> > + if (!pmic_dev)
> > + return ret;
> > +
> > + pmic_version = dev_get_drvdata(&pmic_dev->dev);
> > + /* External Oscillator polarity is inverted on revision 0x12 onwards
> > +*/
>
> s/polarity/bit/
>
> My understanding is that the bit meaning is inverted. It is still a on/off
> bit.
Yes, that is correct bit is inverted.
PMIC version 0x11
-----------------
bit 0: Disable internal crystal oscillator
1: Enable internal crystal oscillator
PMIC version 0x12
----------------
bit 0: Enable internal crystal oscillator
1: Disable internal crystal oscillator
Cheers,
Biju
>
> > + if (*pmic_version >= 0x12)
> > + ret = true;
> > +
> > + put_device(&pmic_dev->dev);
> > +
> > + return ret;
> > +}
> > +
> > +static int
> > +isl1208_set_ext_osc_based_on_pmic_version(struct i2c_client *client,
> > +int rc) {
> > + if (isl1208_is_xtosc_polarity_inverted(client))
> > + rc &= ~ISL1208_REG_SR_XTOSCB;
> > + else
> > + rc |= ISL1208_REG_SR_XTOSCB;
> > +
> > + return i2c_smbus_write_byte_data(client, ISL1208_REG_SR, rc); }
> > +
> > static int
> > isl1208_i2c_get_sr(struct i2c_client *client) { @@ -845,6 +889,12 @@
> > isl1208_probe(struct i2c_client *client)
> > return rc;
> > }
> >
> > + if (isl1208->config->has_pmic_parent) {
> > + rc = isl1208_set_ext_osc_based_on_pmic_version(client, rc);
> > + if (rc)
> > + return rc;
> > + }
> > +
> > if (rc & ISL1208_REG_SR_RTCF)
> > dev_warn(&client->dev, "rtc power failure detected, "
> > "please set clock.\n");
> > --
> > 2.25.1
> >
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.co/
> m%2F&data=05%7C01%7Cbiju.das.jz%40bp.renesas.com%7C68856b4bc4f14d42055608db4
> bc51ccd%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638187082188797517%7CUn
> known%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6Mn0%3D%7C3000%7C%7C%7C&sdata=mzGMSc1wg7XjQ97oMyCzQZ6UEHbuViyvOErFefp8pF0
> %3D&reserved=0
