[v1,1/5] rtc: rtc-cmos: Call cmos_wake_setup() from cmos_do_probe()
Commit Message
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Notice that cmos_wake_setup() is the only user of acpi_rtc_info and it
can operate on the cmos_rtc variable directly, so it need not set the
platform_data pointer before cmos_do_probe() is called. Instead, it
can be called by cmos_do_probe() in the case when the platform_data
pointer is not set to implement the default behavior (which is to use
the FADT information as long as ACPI support is enabled).
Modify the code accordingly.
While at it, drop a comment that doesn't really match the code it is
supposed to be describing.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/rtc/rtc-cmos.c | 47 ++++++++++++++++++++++-------------------------
1 file changed, 22 insertions(+), 25 deletions(-)
Comments
On Mon, 2022-11-07 at 20:59 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Notice that cmos_wake_setup() is the only user of acpi_rtc_info and
> it
> can operate on the cmos_rtc variable directly, so it need not set the
> platform_data pointer before cmos_do_probe() is called. Instead, it
> can be called by cmos_do_probe() in the case when the platform_data
> pointer is not set to implement the default behavior (which is to use
> the FADT information as long as ACPI support is enabled).
>
...
>
> @@ -827,19 +829,27 @@ cmos_do_probe(struct device *dev, struct
> if (info->address_space)
> address_space = info->address_space;
>
> - if (info->rtc_day_alarm && info->rtc_day_alarm < 128)
> - cmos_rtc.day_alrm = info->rtc_day_alarm;
> - if (info->rtc_mon_alarm && info->rtc_mon_alarm < 128)
> - cmos_rtc.mon_alrm = info->rtc_mon_alarm;
> - if (info->rtc_century && info->rtc_century < 128)
> - cmos_rtc.century = info->rtc_century;
> + cmos_rtc.day_alrm = info->rtc_day_alarm;
> + cmos_rtc.mon_alrm = info->rtc_mon_alarm;
> + cmos_rtc.century = info->rtc_century;
>
> if (info->wake_on && info->wake_off) {
> cmos_rtc.wake_on = info->wake_on;
> cmos_rtc.wake_off = info->wake_off;
> }
> + } else {
> + cmos_wake_setup(dev);
> }
>
>
Previously, before commit a474aaedac99 ("rtc-cmos: move wake setup from
ACPI glue into RTC driver"), dev->platform_data is set in
drivers/acpi/glue.c, and the above commit moves it to cmos_wake_setup()
in this file.
Now, with this patch, my understanding is that dev->platform_data is
never set, thus we can remove the 'info' variable and the
if (info)
check above.
thanks,
rui
On Tue, Nov 8, 2022 at 3:31 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> On Mon, 2022-11-07 at 20:59 +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Notice that cmos_wake_setup() is the only user of acpi_rtc_info and
> > it
> > can operate on the cmos_rtc variable directly, so it need not set the
> > platform_data pointer before cmos_do_probe() is called. Instead, it
> > can be called by cmos_do_probe() in the case when the platform_data
> > pointer is not set to implement the default behavior (which is to use
> > the FADT information as long as ACPI support is enabled).
> >
>
> ...
>
> >
> > @@ -827,19 +829,27 @@ cmos_do_probe(struct device *dev, struct
> > if (info->address_space)
> > address_space = info->address_space;
> >
> > - if (info->rtc_day_alarm && info->rtc_day_alarm < 128)
> > - cmos_rtc.day_alrm = info->rtc_day_alarm;
> > - if (info->rtc_mon_alarm && info->rtc_mon_alarm < 128)
> > - cmos_rtc.mon_alrm = info->rtc_mon_alarm;
> > - if (info->rtc_century && info->rtc_century < 128)
> > - cmos_rtc.century = info->rtc_century;
> > + cmos_rtc.day_alrm = info->rtc_day_alarm;
> > + cmos_rtc.mon_alrm = info->rtc_mon_alarm;
> > + cmos_rtc.century = info->rtc_century;
> >
> > if (info->wake_on && info->wake_off) {
> > cmos_rtc.wake_on = info->wake_on;
> > cmos_rtc.wake_off = info->wake_off;
> > }
> > + } else {
> > + cmos_wake_setup(dev);
> > }
> >
> >
>
> Previously, before commit a474aaedac99 ("rtc-cmos: move wake setup from
> ACPI glue into RTC driver"), dev->platform_data is set in
> drivers/acpi/glue.c, and the above commit moves it to cmos_wake_setup()
> in this file.
>
> Now, with this patch, my understanding is that dev->platform_data is
> never set, thus we can remove the 'info' variable and the
> if (info)
> check above.
There are other users of this driver which can be found by grepping
for cmos_rtc_board_info.
They create platform device objects with platform_data set which are
then bound to by this driver.
On Tue, 2022-11-08 at 14:09 +0100, Rafael J. Wysocki wrote:
> On Tue, Nov 8, 2022 at 3:31 AM Zhang Rui <rui.zhang@intel.com> wrote:
> > On Mon, 2022-11-07 at 20:59 +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > Notice that cmos_wake_setup() is the only user of acpi_rtc_info
> > > and
> > > it
> > > can operate on the cmos_rtc variable directly, so it need not set
> > > the
> > > platform_data pointer before cmos_do_probe() is called. Instead,
> > > it
> > > can be called by cmos_do_probe() in the case when the
> > > platform_data
> > > pointer is not set to implement the default behavior (which is to
> > > use
> > > the FADT information as long as ACPI support is enabled).
> > >
> >
> > ...
> >
> > > @@ -827,19 +829,27 @@ cmos_do_probe(struct device *dev, struct
> > > if (info->address_space)
> > > address_space = info->address_space;
> > >
> > > - if (info->rtc_day_alarm && info->rtc_day_alarm <
> > > 128)
> > > - cmos_rtc.day_alrm = info->rtc_day_alarm;
> > > - if (info->rtc_mon_alarm && info->rtc_mon_alarm <
> > > 128)
> > > - cmos_rtc.mon_alrm = info->rtc_mon_alarm;
> > > - if (info->rtc_century && info->rtc_century < 128)
> > > - cmos_rtc.century = info->rtc_century;
> > > + cmos_rtc.day_alrm = info->rtc_day_alarm;
> > > + cmos_rtc.mon_alrm = info->rtc_mon_alarm;
> > > + cmos_rtc.century = info->rtc_century;
> > >
> > > if (info->wake_on && info->wake_off) {
> > > cmos_rtc.wake_on = info->wake_on;
> > > cmos_rtc.wake_off = info->wake_off;
> > > }
> > > + } else {
> > > + cmos_wake_setup(dev);
> > > }
> > >
> > >
> >
> > Previously, before commit a474aaedac99 ("rtc-cmos: move wake setup
> > from
> > ACPI glue into RTC driver"), dev->platform_data is set in
> > drivers/acpi/glue.c, and the above commit moves it to
> > cmos_wake_setup()
> > in this file.
> >
> > Now, with this patch, my understanding is that dev->platform_data
> > is
> > never set, thus we can remove the 'info' variable and the
> > if (info)
> > check above.
>
> There are other users of this driver which can be found by grepping
> for cmos_rtc_board_info.
>
> They create platform device objects with platform_data set which are
> then bound to by this driver.
yeah, I overlooked this.
thanks,
rui
===================================================================
@@ -744,6 +744,8 @@ static irqreturn_t cmos_interrupt(int ir
return IRQ_NONE;
}
+static void cmos_wake_setup(struct device *dev);
+
#ifdef CONFIG_PNP
#define INITSECTION
@@ -827,19 +829,27 @@ cmos_do_probe(struct device *dev, struct
if (info->address_space)
address_space = info->address_space;
- if (info->rtc_day_alarm && info->rtc_day_alarm < 128)
- cmos_rtc.day_alrm = info->rtc_day_alarm;
- if (info->rtc_mon_alarm && info->rtc_mon_alarm < 128)
- cmos_rtc.mon_alrm = info->rtc_mon_alarm;
- if (info->rtc_century && info->rtc_century < 128)
- cmos_rtc.century = info->rtc_century;
+ cmos_rtc.day_alrm = info->rtc_day_alarm;
+ cmos_rtc.mon_alrm = info->rtc_mon_alarm;
+ cmos_rtc.century = info->rtc_century;
if (info->wake_on && info->wake_off) {
cmos_rtc.wake_on = info->wake_on;
cmos_rtc.wake_off = info->wake_off;
}
+ } else {
+ cmos_wake_setup(dev);
}
+ if (cmos_rtc.day_alrm >= 128)
+ cmos_rtc.day_alrm = 0;
+
+ if (cmos_rtc.mon_alrm >= 128)
+ cmos_rtc.mon_alrm = 0;
+
+ if (cmos_rtc.century >= 128)
+ cmos_rtc.century = 0;
+
cmos_rtc.dev = dev;
dev_set_drvdata(dev, &cmos_rtc);
@@ -1275,13 +1285,6 @@ static void use_acpi_alarm_quirks(void)
static inline void use_acpi_alarm_quirks(void) { }
#endif
-/* Every ACPI platform has a mc146818 compatible "cmos rtc". Here we find
- * its device node and pass extra config data. This helps its driver use
- * capabilities that the now-obsolete mc146818 didn't have, and informs it
- * that this board's RTC is wakeup-capable (per ACPI spec).
- */
-static struct cmos_rtc_board_info acpi_rtc_info;
-
static void cmos_wake_setup(struct device *dev)
{
if (acpi_disabled)
@@ -1289,26 +1292,23 @@ static void cmos_wake_setup(struct devic
use_acpi_alarm_quirks();
- acpi_rtc_info.wake_on = rtc_wake_on;
- acpi_rtc_info.wake_off = rtc_wake_off;
+ cmos_rtc.wake_on = rtc_wake_on;
+ cmos_rtc.wake_off = rtc_wake_off;
- /* workaround bug in some ACPI tables */
+ /* ACPI tables bug workaround. */
if (acpi_gbl_FADT.month_alarm && !acpi_gbl_FADT.day_alarm) {
dev_dbg(dev, "bogus FADT month_alarm (%d)\n",
acpi_gbl_FADT.month_alarm);
acpi_gbl_FADT.month_alarm = 0;
}
- acpi_rtc_info.rtc_day_alarm = acpi_gbl_FADT.day_alarm;
- acpi_rtc_info.rtc_mon_alarm = acpi_gbl_FADT.month_alarm;
- acpi_rtc_info.rtc_century = acpi_gbl_FADT.century;
+ cmos_rtc.day_alrm = acpi_gbl_FADT.day_alarm;
+ cmos_rtc.mon_alrm = acpi_gbl_FADT.month_alarm;
+ cmos_rtc.century = acpi_gbl_FADT.century;
- /* NOTE: S4_RTC_WAKE is NOT currently useful to Linux */
if (acpi_gbl_FADT.flags & ACPI_FADT_S4_RTC_WAKE)
dev_info(dev, "RTC can wake from S4\n");
- dev->platform_data = &acpi_rtc_info;
-
/* RTC always wakes from S1/S2/S3, and often S4/STD */
device_init_wakeup(dev, 1);
}
@@ -1359,8 +1359,6 @@ static int cmos_pnp_probe(struct pnp_dev
{
int irq, ret;
- cmos_wake_setup(&pnp->dev);
-
if (pnp_port_start(pnp, 0) == 0x70 && !pnp_irq_valid(pnp, 0)) {
irq = 0;
#ifdef CONFIG_X86
@@ -1468,7 +1466,6 @@ static int __init cmos_platform_probe(st
int irq, ret;
cmos_of_init(pdev);
- cmos_wake_setup(&pdev->dev);
if (RTC_IOMAPPED)
resource = platform_get_resource(pdev, IORESOURCE_IO, 0);