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 Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v3 2/5] rtc: isl1208: Add support for the built-in
> RTC on the PMIC RAA215300
> 
> 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.

I tried to squeeze with string length "8".

What about changing it to "raa215300_a0" and changing length to
"12"? as version A0 of RAA215300 pmic chip have this inverted oscillator bit.

> 
> 
> >         { }
> >  };
> >  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/

Agreed. But I plan to drop this function in next version.
Please see below.

> 
> > +                               bool is_inverted_oscillator_bit) {
> > +       if (is_inverted_oscillator_bit)
> 
> This condition is always true, given all callers in your series.

Ok.

> 
> > +               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.

No, I am planning to drop it. Please see below.


> 
> > +
> > +       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...

I am planning to drop this function in next version and will use the below logic instead.
Is it ok?

         if (isl1208->config->has_inverted_osc_bit) {   
		    int sr;
                          
                 sr = i2c_smbus_write_byte_data(client, ISL1208_REG_SR,           
                                              rc | ISL1208_REG_SR_XTOSCB);      
                 if (sr)                                                          
                         return sr;                                               
         }                                                                        
           

> 
> > +                                                    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..

Oops, missed it.

Cheers,
Biju





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

  Powered by Linux