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 Tue, May 16, 2023 at 12:22 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > -----Original Message-----
> > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > Sent: Tuesday, May 16, 2023 9:58 AM
> > To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > Cc: Alessandro Zummo <a.zummo@xxxxxxxxxxxx>; Alexandre Belloni
> > <alexandre.belloni@xxxxxxxxxxx>; Magnus Damm <magnus.damm@xxxxxxxxx>;
> > Lee Jones <lee@xxxxxxxxxx>; linux-rtc@xxxxxxxxxxxxxxx; linux-renesas-
> > soc@xxxxxxxxxxxxxxx; Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>
> > Subject: Re: [PATCH v3 2/5] rtc: isl1208: Add support for the built-in
> > RTC on the PMIC RAA215300
> >
> > On Tue, May 16, 2023 at 10:46 AM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > wrote:
> > > > Subject: Re: [PATCH v3 2/5] rtc: isl1208: Add support for the
> > > > built-in RTC on the PMIC RAA215300 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.
> >
> > Ah, because of the size limit of isl1208_config.name[]?
> > Note that that field is only initialized, but further unused, so you can
> > just drop it.
>
> Agreed. It will look like
>
> static const struct isl1208_config {
> -       const char      name[8];
>         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 },
> +       [TYPE_ISL1208] = { 2, false, false },
> +       [TYPE_ISL1209] = { 2, true,  false },
> +       [TYPE_ISL1218] = { 8, false, false },
> +       [TYPE_ISL1219] = { 2, true,  true },
> +       [TYPE_RAA215300_RTC_A0] = { 2, false, false, true },
>  };
>
> >
> > BTW, isl1208_id[].driver_data could store a pointer to the config, like
> > for DT-based matching, making I2C and DT-based matching more similar.
>
> OK. But some type casting required
>
> +       { "isl1208", .driver_data = (unsigned long)&isl1208_configs[TYPE_ISL1208] },
> +       { "isl1209", .driver_data = (unsigned long)&isl1208_configs[TYPE_ISL1209] },
> +       { "isl1218", .driver_data = (unsigned long)&isl1208_configs[TYPE_ISL1218] },
> +       { "isl1219", .driver_data = (unsigned long)&isl1208_configs[TYPE_ISL1219] },
> +       { "raa215300_rtc_a0", .driver_data = (unsigned long)&isl1208_configs[TYPE_RAA215300_RTC_A0] },

Now there are no more users left of isl1208_configs[], you can split
the array in individual variables, and make lines shorter by referring
to e.g. &config_isl1219 instead of &isl1208_configs[TYPE_ISL1219].

> > > > >  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;
> >
> > Isn't this more confusing: "rc" is the Status Register value, and "sr"
> > is the Return Code?
>
> OK will use "ret" instead.

Use "ret" instead of what? ;-)
Just declare "int sr" at the top, and use that to store the Status Register
value, and use rc everywhere else?

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 Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux