Patchwork [1/2] ACPI: reintroduce acpi_device_ops .shutdown method

login
register
mail settings
Submitter David Härdeman
Date June 27, 2009, 5:18 a.m.
Message ID <1246079912-17619-2-git-send-email-david@hardeman.nu>
Download mbox | patch
Permalink /patch/32657/
State RFC, archived
Headers show

Comments

David Härdeman - June 27, 2009, 5:18 a.m.
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(-)
Andrew Morton - June 30, 2009, 9:10 p.m.
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
Bjorn Helgaas - June 30, 2009, 11:11 p.m.
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
David Härdeman - July 1, 2009, 8:20 a.m.
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
Bjorn Helgaas - July 8, 2009, 4:52 p.m.
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
Bjorn Helgaas - July 9, 2009, 9:38 p.m.
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
David Härdeman - July 10, 2009, 7:55 a.m.
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?
Bjorn Helgaas - July 10, 2009, 4:01 p.m.
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

Patch

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 */