| Message ID | 1246079912-17619-2-git-send-email-david@hardeman.nu (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Sat, 27 Jun 2009 07:18:31 +0200 David H__rdeman <david@hardeman.nu> wrote: > This reintroduces the .shutdown method which is used by the > winbond-cir driver. A normal revert wasn't possible since there > had been other changes to include/acpi/acpi_bus.h since. > > Signed-off-by: David H__rdeman <david@hardeman.nu> > --- > drivers/acpi/scan.c | 12 ++++++++++++ > include/acpi/acpi_bus.h | 2 ++ > 2 files changed, 14 insertions(+), 0 deletions(-) > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > index 781435d..c94ab13 100644 > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -464,10 +464,22 @@ static int acpi_device_remove(struct device * dev) > return 0; > } > > +static void acpi_device_shutdown(struct device *dev) > +{ > + struct acpi_device *acpi_dev = to_acpi_device(dev); > + struct acpi_driver *acpi_drv = acpi_dev->driver; > + > + if (acpi_drv && acpi_drv->ops.shutdown) > + acpi_drv->ops.shutdown(acpi_dev); > + > + return ; > +} > + > struct bus_type acpi_bus_type = { > .name = "acpi", > .suspend = acpi_device_suspend, > .resume = acpi_device_resume, > + .shutdown = acpi_device_shutdown, > .match = acpi_bus_match, > .probe = acpi_device_probe, > .remove = acpi_device_remove, > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index c65e4ce..52da89a 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -96,6 +96,7 @@ typedef int (*acpi_op_resume) (struct acpi_device * device); > typedef int (*acpi_op_bind) (struct acpi_device * device); > typedef int (*acpi_op_unbind) (struct acpi_device * device); > typedef void (*acpi_op_notify) (struct acpi_device * device, u32 event); > +typedef int (*acpi_op_shutdown) (struct acpi_device * device); > > struct acpi_bus_ops { > u32 acpi_op_add:1; > @@ -112,6 +113,7 @@ struct acpi_device_ops { > acpi_op_bind bind; > acpi_op_unbind unbind; > acpi_op_notify notify; > + acpi_op_shutdown shutdown; > }; > > #define ACPI_DRIVER_ALL_NOTIFY_EVENTS 0x1 /* system AND device events */ Len, Bjorn: is this OK? Or is there some other mechanism which the driver should have used? Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 30 June 2009 3:10:47 pm Andrew Morton wrote: > On Sat, 27 Jun 2009 07:18:31 +0200 > David H__rdeman <david@hardeman.nu> wrote: > > > This reintroduces the .shutdown method which is used by the > > winbond-cir driver. A normal revert wasn't possible since there > > had been other changes to include/acpi/acpi_bus.h since. > > > > Signed-off-by: David H__rdeman <david@hardeman.nu> > > --- > > drivers/acpi/scan.c | 12 ++++++++++++ > > include/acpi/acpi_bus.h | 2 ++ > > 2 files changed, 14 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > > index 781435d..c94ab13 100644 > > --- a/drivers/acpi/scan.c > > +++ b/drivers/acpi/scan.c > > @@ -464,10 +464,22 @@ static int acpi_device_remove(struct device * dev) > > return 0; > > } > > > > +static void acpi_device_shutdown(struct device *dev) > > +{ > > + struct acpi_device *acpi_dev = to_acpi_device(dev); > > + struct acpi_driver *acpi_drv = acpi_dev->driver; > > + > > + if (acpi_drv && acpi_drv->ops.shutdown) > > + acpi_drv->ops.shutdown(acpi_dev); > > + > > + return ; > > +} > > + > > struct bus_type acpi_bus_type = { > > .name = "acpi", > > .suspend = acpi_device_suspend, > > .resume = acpi_device_resume, > > + .shutdown = acpi_device_shutdown, > > .match = acpi_bus_match, > > .probe = acpi_device_probe, > > .remove = acpi_device_remove, > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > > index c65e4ce..52da89a 100644 > > --- a/include/acpi/acpi_bus.h > > +++ b/include/acpi/acpi_bus.h > > @@ -96,6 +96,7 @@ typedef int (*acpi_op_resume) (struct acpi_device * device); > > typedef int (*acpi_op_bind) (struct acpi_device * device); > > typedef int (*acpi_op_unbind) (struct acpi_device * device); > > typedef void (*acpi_op_notify) (struct acpi_device * device, u32 event); > > +typedef int (*acpi_op_shutdown) (struct acpi_device * device); > > > > struct acpi_bus_ops { > > u32 acpi_op_add:1; > > @@ -112,6 +113,7 @@ struct acpi_device_ops { > > acpi_op_bind bind; > > acpi_op_unbind unbind; > > acpi_op_notify notify; > > + acpi_op_shutdown shutdown; > > }; > > > > #define ACPI_DRIVER_ALL_NOTIFY_EVENTS 0x1 /* system AND device events */ > > Len, Bjorn: is this OK? Or is there some other mechanism which the > driver should have used? I'm on vacation and don't have time to read the new winbond driver right now, but maybe it could be changed so that wbcir_shutdown() is an internal function called by wbcir_suspend() (as it is already) and wbcir_remove(). I hate to re-introduce .shutdown when it's only used by a single driver. That makes me think either we have a bunch of drivers that are buggy because they *should* have .shutdown methods but don't, or the single user of .shutdown doesn't have a real dependency on it. The winbond driver does not use any ACPI-specific functionality, so it might be simpler to write it as a PNP driver (which would depend on PNPACPI, of course). Nice looking driver, by the way. Even from a cursory glance it's obvious that you've taken a lot of care with it. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, July 1, 2009 01:11, Bjorn Helgaas wrote: > On Tuesday 30 June 2009 3:10:47 pm Andrew Morton wrote: >> On Sat, 27 Jun 2009 07:18:31 +0200 >> David Härdeman <david@hardeman.nu> wrote: >>> >>> This reintroduces the .shutdown method which is used by the >>> winbond-cir driver. A normal revert wasn't possible since there >>> had been other changes to include/acpi/acpi_bus.h since. >> >> Len, Bjorn: is this OK? Or is there some other mechanism which the >> driver should have used? > > I'm on vacation and don't have time to read the new winbond driver > right now, but maybe it could be changed so that wbcir_shutdown() > is an internal function called by wbcir_suspend() (as it is already) > and wbcir_remove(). I didn't want to call wbcir_shutdown from wbcir_remove since wbcir_remove is called on rmmod and the wbcir_shutdown method programs the chip so that wake-on-a-specific-ir-command is enabled (and I imagine that people would expect rmmod to disable the hardware completely). I will clarify the function names a bit in the next version of the patch... As far as I could understand from my testing, the acpi .suspend method is not called during shutdown which is why I needed to hook into both .suspend and .shutdown separately. > I hate to re-introduce .shutdown when it's only used by a single > driver. That makes me think either we have a bunch of drivers that > are buggy because they *should* have .shutdown methods but don't, > or the single user of .shutdown doesn't have a real dependency on it. I think this single user does have a real dependency because of its wake-from-poweroff and wake-from-suspend capability. But I could be mistaken... > The winbond driver does not use any ACPI-specific functionality, so > it might be simpler to write it as a PNP driver (which would depend > on PNPACPI, of course). As far as I could tell from a quick look at include/linux/pnp.h, a pnp_driver doesn't seem to have any .shutdown methods either, so I'm not sure how it would help? (On a related note, it seems inconsistent to me that platform_driver has a .shutdown method while pnp_driver and acpi_driver doesn't.) If you disagree with acpi_driver regaining the .shutdown method I guess the only options are to use register_reboot_notifier or to rewrite the driver as a platform_driver (Alan Cox seemed to suggest earlier that the driver was not a good fit for a platform driver). > Nice looking driver, by the way. Even from a cursory glance it's > obvious that you've taken a lot of care with it. Thank you, and have a nice vacation. Regards, David -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 01 July 2009 02:20:59 am David Härdeman wrote: > On Wed, July 1, 2009 01:11, Bjorn Helgaas wrote: > > The winbond driver does not use any ACPI-specific functionality, so > > it might be simpler to write it as a PNP driver (which would depend > > on PNPACPI, of course). > > As far as I could tell from a quick look at include/linux/pnp.h, a > pnp_driver doesn't seem to have any .shutdown methods either, so I'm not > sure how it would help? PNPACPI parses the device resources for you, so you could get rid of the _CRS stuff in your driver. The only reason PNP and ACPI don't have .shutdown is because nobody has needed it yet. If you need it (and it sounds like you do), I think the cleanest thing would be to add it to PNP and turn your driver into a PNP driver. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 08 July 2009 10:52:19 am Bjorn Helgaas wrote: > On Wednesday 01 July 2009 02:20:59 am David Härdeman wrote: > > On Wed, July 1, 2009 01:11, Bjorn Helgaas wrote: > > > The winbond driver does not use any ACPI-specific functionality, so > > > it might be simpler to write it as a PNP driver (which would depend > > > on PNPACPI, of course). > > > > As far as I could tell from a quick look at include/linux/pnp.h, a > > pnp_driver doesn't seem to have any .shutdown methods either, so I'm not > > sure how it would help? > > PNPACPI parses the device resources for you, so you could get rid > of the _CRS stuff in your driver. > > The only reason PNP and ACPI don't have .shutdown is because nobody > has needed it yet. If you need it (and it sounds like you do), I > think the cleanest thing would be to add it to PNP and turn your > driver into a PNP driver. Here are two sample patches to show you what I was suggesting. These apply on top of the two Winbond patches currently in -mm. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, July 9, 2009 23:38, Bjorn Helgaas wrote: > Here are two sample patches to show you what I was suggesting. These > apply on top of the two Winbond patches currently in -mm. Thanks, great service :) I'll test the patches as soon as I have time and access to the hardware. One question though, the resulting patch series (if AKPM merges these patches in addition to the existing ones and drops the patch adding .shutdown to acpi_driver) will be git bisect unfriendly. Should I roll two new patches instead to replace the three patches currently in -mm?
On Friday 10 July 2009 01:55:27 am David Härdeman wrote: > On Thu, July 9, 2009 23:38, Bjorn Helgaas wrote: > > Here are two sample patches to show you what I was suggesting. These > > apply on top of the two Winbond patches currently in -mm. > > Thanks, great service :) > > I'll test the patches as soon as I have time and access to the hardware. > > One question though, the resulting patch series (if AKPM merges these > patches in addition to the existing ones and drops the patch adding > .shutdown to acpi_driver) will be git bisect unfriendly. > > Should I roll two new patches instead to replace the three patches > currently in -mm? If you like the approach and it works, I would take the latter approach (replace the three patches with two new ones -- e.g., add-pnp-shutdown-method and add-winbond-pnp-driver). I don't think we have to worry about bisectability until they show up in a git tree. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 781435d..c94ab13 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -464,10 +464,22 @@ static int acpi_device_remove(struct device * dev) return 0; } +static void acpi_device_shutdown(struct device *dev) +{ + struct acpi_device *acpi_dev = to_acpi_device(dev); + struct acpi_driver *acpi_drv = acpi_dev->driver; + + if (acpi_drv && acpi_drv->ops.shutdown) + acpi_drv->ops.shutdown(acpi_dev); + + return ; +} + struct bus_type acpi_bus_type = { .name = "acpi", .suspend = acpi_device_suspend, .resume = acpi_device_resume, + .shutdown = acpi_device_shutdown, .match = acpi_bus_match, .probe = acpi_device_probe, .remove = acpi_device_remove, diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index c65e4ce..52da89a 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -96,6 +96,7 @@ typedef int (*acpi_op_resume) (struct acpi_device * device); typedef int (*acpi_op_bind) (struct acpi_device * device); typedef int (*acpi_op_unbind) (struct acpi_device * device); typedef void (*acpi_op_notify) (struct acpi_device * device, u32 event); +typedef int (*acpi_op_shutdown) (struct acpi_device * device); struct acpi_bus_ops { u32 acpi_op_add:1; @@ -112,6 +113,7 @@ struct acpi_device_ops { acpi_op_bind bind; acpi_op_unbind unbind; acpi_op_notify notify; + acpi_op_shutdown shutdown; }; #define ACPI_DRIVER_ALL_NOTIFY_EVENTS 0x1 /* system AND device events */
This reintroduces the .shutdown method which is used by the winbond-cir driver. A normal revert wasn't possible since there had been other changes to include/acpi/acpi_bus.h since. Signed-off-by: David Härdeman <david@hardeman.nu> --- drivers/acpi/scan.c | 12 ++++++++++++ include/acpi/acpi_bus.h | 2 ++ 2 files changed, 14 insertions(+), 0 deletions(-)