Hi Biju,
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.
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.
> > > 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?
> }
>
>
> >
> > > + 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.
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
