| Message ID | 1850290.tdWV9SEqCh@kreacher |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rtc: rtc-cmos: Assorted ACPI-related cleanups and fixes | expand |
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
Index: linux-pm/drivers/rtc/rtc-cmos.c =================================================================== --- linux-pm.orig/drivers/rtc/rtc-cmos.c +++ linux-pm/drivers/rtc/rtc-cmos.c @@ -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);