| Message ID | 20230513165227.13117-3-biju.das.jz@bp.renesas.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add Renesas PMIC RAA215300 and built-in RTC support | expand |
Hi Biju, On Sat, May 13, 2023 at 6:52 PM Biju Das <biju.das.jz@bp.renesas.com> 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@bp.renesas.com> > --- > 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. > { } > }; > 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/ > + bool is_inverted_oscillator_bit) > +{ > + if (is_inverted_oscillator_bit) This condition is always true, given all callers in your series. > + 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. > + > + 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... > + 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.. > dev_warn(&client->dev, "rtc power failure detected, " > "please set clock.\n"); Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
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@bp.renesas.com> > 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@bp.renesas.com> > > --- > > 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
Hi Biju, On Tue, May 16, 2023 at 10:46 AM Biju Das <biju.das.jz@bp.renesas.com> 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@bp.renesas.com> > > 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@bp.renesas.com> > > > --- > > > 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
Hi Geert, Thanks for the feedback. > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: Tuesday, May 16, 2023 9:58 AM > To: Biju Das <biju.das.jz@bp.renesas.com> > Cc: Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni > <alexandre.belloni@bootlin.com>; Magnus Damm <magnus.damm@gmail.com>; > Lee Jones <lee@kernel.org>; linux-rtc@vger.kernel.org; linux-renesas- > soc@vger.kernel.org; Fabrizio Castro <fabrizio.castro.jz@renesas.com> > 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 10:46 AM Biju Das <biju.das.jz@bp.renesas.com> > 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@bp.renesas.com> > > > 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@bp.renesas.com> > > > > --- > > > > 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] }, And In probe() - if (id->driver_data >= ISL_LAST_ID) + if (!id) return -ENODEV; - isl1208->config = &isl1208_configs[id->driver_data]; + isl1208->config = (struct isl1208_config *)id->driver_data; > > > > > 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. if (isl1208->config->has_inverted_osc_bit) { + int ret; + + ret = i2c_smbus_write_byte_data(client, ISL1208_REG_SR, + rc | ISL1208_REG_SR_XTOSCB); + if (ret) + return ret; } Cheers, Biju
Hi Biju, On Tue, May 16, 2023 at 12:22 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > -----Original Message----- > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > Sent: Tuesday, May 16, 2023 9:58 AM > > To: Biju Das <biju.das.jz@bp.renesas.com> > > Cc: Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni > > <alexandre.belloni@bootlin.com>; Magnus Damm <magnus.damm@gmail.com>; > > Lee Jones <lee@kernel.org>; linux-rtc@vger.kernel.org; linux-renesas- > > soc@vger.kernel.org; Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > 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@bp.renesas.com> > > 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@bp.renesas.com> > > > > 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@bp.renesas.com> > > > > > --- > > > > > 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
Hi Geert, Thanks for the feedback. > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: Tuesday, May 16, 2023 1:20 PM > To: Biju Das <biju.das.jz@bp.renesas.com> > Cc: Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni > <alexandre.belloni@bootlin.com>; Magnus Damm <magnus.damm@gmail.com>; > Lee Jones <lee@kernel.org>; linux-rtc@vger.kernel.org; linux-renesas- > soc@vger.kernel.org; Fabrizio Castro <fabrizio.castro.jz@renesas.com> > 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@bp.renesas.com> > wrote: > > > -----Original Message----- > > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > > Sent: Tuesday, May 16, 2023 9:58 AM > > > To: Biju Das <biju.das.jz@bp.renesas.com> > > > Cc: Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni > > > <alexandre.belloni@bootlin.com>; Magnus Damm > > > <magnus.damm@gmail.com>; Lee Jones <lee@kernel.org>; > > > linux-rtc@vger.kernel.org; linux-renesas- soc@vger.kernel.org; > > > Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > > 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@bp.renesas.com> > > > 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@bp.renesas.com> > > > > > 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@bp.renesas.com> > > > > > > --- > > > > > > 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
diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c index 73cc6aaf9b8b..d6425780d834 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_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 }, { } }; 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, + bool is_inverted_oscillator_bit) +{ + if (is_inverted_oscillator_bit) + 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 +859,13 @@ isl1208_probe(struct i2c_client *client) return rc; } + if (isl1208->config->has_inverted_osc_bit) { + rc = isl1208_set_external_oscillator(client, rc, + isl1208->config->has_inverted_osc_bit); + if (rc) + return rc; + } + if (rc & ISL1208_REG_SR_RTCF) dev_warn(&client->dev, "rtc power failure detected, " "please set clock.\n");
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@bp.renesas.com> --- 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. --- drivers/rtc/rtc-isl1208.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)