Hi Geert,
Thanks for the feedback.
> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: Tuesday, May 16, 2023 1:20 PM
> 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
>
> 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].
It looks better now,
-/* ISL1208 various variants */
-enum isl1208_id {
- TYPE_ISL1208 = 0,
- TYPE_ISL1209,
- TYPE_ISL1218,
- TYPE_ISL1219,
- TYPE_RAA215300_RTC_A0,
- ISL_LAST_ID
-};
-
-static const struct isl1208_config {
+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] = { 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 },
+};
+
+static const struct isl1208_config config_isl1208 = {
+ .nvmem_length = 2,
+ .has_tamper = false,
+ .has_timestamp = false
+};
+
+static const struct isl1208_config config_isl1209 = {
+ .nvmem_length = 2,
+ .has_tamper = true,
+ .has_timestamp = false
+};
+
+static const struct isl1208_config config_isl1218 = {
+ .nvmem_length = 8,
+ .has_tamper = false,
+ .has_timestamp = false
+};
+
+static const struct isl1208_config config_isl1219 = {
+ .nvmem_length = 2,
+ .has_tamper = true,
+ .has_timestamp = true
+};
+
+static const struct isl1208_config config_raa215300_a0 = {
+ .nvmem_length = 2,
+ .has_tamper = false,
+ .has_timestamp = false,
+ .has_inverted_osc_bit = true
};
static const struct i2c_device_id isl1208_id[] = {
- { "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] },
+ { "isl1208", .driver_data = (unsigned long)&config_isl1208 },
+ { "isl1209", .driver_data = (unsigned long)&config_isl1209 },
+ { "isl1218", .driver_data = (unsigned long)&config_isl1218 },
+ { "isl1219", .driver_data = (unsigned long)&config_isl1219 },
+ { "raa215300_a0", .driver_data = (unsigned long)&config_raa215300_a0 },
{ }
};
MODULE_DEVICE_TABLE(i2c, isl1208_id);
static const __maybe_unused struct of_device_id isl1208_of_match[] = {
- { .compatible = "isil,isl1208", .data = &isl1208_configs[TYPE_ISL1208] },
- { .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 = "isil,isl1208", .data = &config_isl1208 },
+ { .compatible = "isil,isl1209", .data = &config_isl1209 },
+ { .compatible = "isil,isl1218", .data = &config_isl1218 },
+ { .compatible = "isil,isl1219", .data = &config_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?
Agreed.
Maybe will create 2-3 patches. Is it ok to send RTC patches separate from the original series,
as there is no dependency?
First patch for removing name variable and also using sr variable to read status register in probe
Second patch for isl1208_id[].driver_data to store a pointer to the config, like for DT-based matching, making I2C and DT-based matching more similar and dropping enum isl1208_id.
Third patch is for adding support for inv oscillator bit.
Please let me know.
Cheers,
Biju
