Re: [PATCH] Add Intel Poulsbo Stub Driver

["Joey Lee" <jlee@novell.com>]

[-- Attachment #1: Type: text/plain, Size: 3267 bytes --]

Hi Matthew, 

於 四，2010-09-09 於 22:36 -0600，Joey Lee 提到：
> Hi Randy, 
> 
> First, thank's for your review!
> And, sorry I am late to reply the mail.
> 
> 於 三，2010-09-01 於 10:11 -0700，Randy Dunlap 提到：
> > On Wed,  1 Sep 2010 17:08:39 +0800 Lee, Chun-Yi wrote:
> > 
> > > Currently, there have no Poulsbo native video driver to support
> > > intel opregion. So, use this stub driver to enable the acpi backlight
> > > control sysfs entry files by requrest acpi_video_register.
> > > 
> > > Signed-off-by: Lee, Chun-Yi <jlee@novell.com>
> > > ---
> > >  drivers/gpu/Makefile       |    2 +-
> > >  drivers/gpu/stub/Kconfig   |   13 +++++++++
> > >  drivers/gpu/stub/Makefile  |    1 +
> > >  drivers/gpu/stub/poulsbo.c |   64 ++++++++++++++++++++++++++++++++++++++++++++
> > >  drivers/video/Kconfig      |    2 +
> > >  5 files changed, 81 insertions(+), 1 deletions(-)
> > >  create mode 100644 drivers/gpu/stub/Kconfig
> > >  create mode 100644 drivers/gpu/stub/Makefile
> > >  create mode 100644 drivers/gpu/stub/poulsbo.c
> > > 
> > > diff --git a/drivers/gpu/Makefile b/drivers/gpu/Makefile
> > > index 30879df..cc92778 100644
> > > --- a/drivers/gpu/Makefile
> > > +++ b/drivers/gpu/Makefile
> > > @@ -1 +1 @@
> > > -obj-y			+= drm/ vga/
> > > +obj-y			+= drm/ vga/ stub/
> > > diff --git a/drivers/gpu/stub/Kconfig b/drivers/gpu/stub/Kconfig
> > > new file mode 100644
> > > index 0000000..c564140
> > > --- /dev/null
> > > +++ b/drivers/gpu/stub/Kconfig
> > > @@ -0,0 +1,13 @@
> > > +config STUB_POULSBO
> > > +	tristate "Intel Poulsbo Stub Driver"
> > > +        default m
> > 
> > Drop the "default m".
> > 
> 
> Thank's! Will drop "default m".
> 
> > > +        # Poulsbo stub depends on ACPI_VIDEO when ACPI is enabled
> > > +        # but for select to work, need to select ACPI_VIDEO's dependencies, ick
> > > +        select ACPI_VIDEO if ACPI
> > 
> > What does it do when ACPI is not enabled?  Does the driver even build
> > when APCI is not enabled?  (given the acpi_video* calls in it)
> > 
> 
> This driver will not do anything if API is not enabled. It only call
> acpi_video_register/acpi_video_unregister, and acpi/video.h handle it
> like the following:
> 
> #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
> extern int acpi_video_register(void);
> extern void acpi_video_unregister(void);
> extern int acpi_video_get_edid(struct acpi_device *device, int type,
>                                int device_id, void **edid);
> #else
> static inline int acpi_video_register(void) { return 0; }
> static inline void acpi_video_unregister(void) { return; }
> static inline int acpi_video_get_edid(struct acpi_device *device, int
> type,
>                                       int device_id, void **edid)
> {
>         return -ENODEV;
> }
> #endif
> 
> So, I think don't need add "depends on ACPI_VIDEO". How do you think?
> 
> > It also has pci_* calls in it, so it looks like it needs to depend
> > on PCI && ACPI, or at least on PCI.
> > 
> > 
> Yes, thank's for your review, I will add:
> 	depends on PCI
> 

Thank's for Greg and Randy's review, and I follow their suggestions to
modified patch like the attacted file.

Please kindly help to review my attached patch.


Thank's a lot!
Joey Lee


[-- Attachment #2: 0001-Add-Intel-Poulsbo-Stub-Driver.patch --]
[-- Type: text/plain, Size: 4136 bytes --]

>From ec59c5ccb9216c688641d51dcfdccc8a129dded5 Mon Sep 17 00:00:00 2001
From: Lee, Chun-Yi <jlee@novell.com>
Date: Fri, 10 Sep 2010 17:42:01 +0800
Subject: [PATCH] Add Intel Poulsbo Stub Driver

Currently, there have no Poulsbo native video driver to support
intel opregion. So, use this stub driver to enable the acpi backlight
control sysfs entry files by requrest acpi_video_register.

Signed-off-by: Lee, Chun-Yi <jlee@novell.com>
---
 drivers/gpu/Makefile       |    2 +-
 drivers/gpu/stub/Kconfig   |   13 +++++++++
 drivers/gpu/stub/Makefile  |    1 +
 drivers/gpu/stub/poulsbo.c |   63 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/video/Kconfig      |    2 +
 5 files changed, 80 insertions(+), 1 deletions(-)
 create mode 100644 drivers/gpu/stub/Kconfig
 create mode 100644 drivers/gpu/stub/Makefile
 create mode 100644 drivers/gpu/stub/poulsbo.c

diff --git a/drivers/gpu/Makefile b/drivers/gpu/Makefile
index 30879df..cc92778 100644
--- a/drivers/gpu/Makefile
+++ b/drivers/gpu/Makefile
@@ -1 +1 @@
-obj-y			+= drm/ vga/
+obj-y			+= drm/ vga/ stub/
diff --git a/drivers/gpu/stub/Kconfig b/drivers/gpu/stub/Kconfig
new file mode 100644
index 0000000..9795890
--- /dev/null
+++ b/drivers/gpu/stub/Kconfig
@@ -0,0 +1,13 @@
+config STUB_POULSBO
+	tristate "Intel Poulsbo Stub Driver"
+	depends on PCI
+        # Poulsbo stub depends on ACPI_VIDEO when ACPI is enabled
+        # but for select to work, need to select ACPI_VIDEO's dependencies, ick
+        select ACPI_VIDEO if ACPI
+	help
+	  Choose this option if you have a system that has Intel Poulsbo
+	  integrated graphics. If M is selected, the module will be called
+	  Poulsbo. This driver is a stub driver for Poulsbo that will call
+	  poulsbo.ko to enable the acpi backlight control sysfs entry file
+	  because there have no poulsbo native driver can support intel
+	  opregion.
diff --git a/drivers/gpu/stub/Makefile b/drivers/gpu/stub/Makefile
new file mode 100644
index 0000000..cd940cc
--- /dev/null
+++ b/drivers/gpu/stub/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_STUB_POULSBO) += poulsbo.o
diff --git a/drivers/gpu/stub/poulsbo.c b/drivers/gpu/stub/poulsbo.c
new file mode 100644
index 0000000..d8aa636
--- /dev/null
+++ b/drivers/gpu/stub/poulsbo.c
@@ -0,0 +1,63 @@
+/*
+ * Intel Poulsbo Stub driver
+ *
+ * Copyright (C) 2010 Novell <jlee@novell.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <acpi/video.h>
+
+#define DRIVER_NAME "poulsbo"
+
+enum {
+	CHIP_PSB_8108 = 0,
+	CHIP_PSB_8109 = 1,
+};
+
+static struct pci_device_id pciidlist[] = {
+	{0x8086, 0x8108, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_PSB_8108}, \
+	{0x8086, 0x8109, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_PSB_8109}, \
+	{0, 0, 0}
+};
+
+static int poulsbo_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	return acpi_video_register();
+}
+
+static void poulsbo_remove(struct pci_dev *pdev)
+{
+	acpi_video_unregister();
+}
+
+static struct pci_driver poulsbo_driver = {
+	.name = DRIVER_NAME,
+	.id_table = pciidlist,
+	.probe = poulsbo_probe,
+	.remove = poulsbo_remove,
+};
+
+static int __init poulsbo_init(void)
+{
+	return pci_register_driver(&poulsbo_driver);
+}
+
+static void __exit poulsbo_exit(void)
+{
+	pci_unregister_driver(&poulsbo_driver);
+}
+
+module_init(poulsbo_init);
+module_exit(poulsbo_exit);
+
+MODULE_AUTHOR("Lee, Chun-Yi <jlee@novell.com>");
+MODULE_DESCRIPTION("Poulsbo Stub Driver");
+MODULE_LICENSE("GPL");
+
+MODULE_DEVICE_TABLE(pci, pciidlist);
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 8b31fdf..8807ae5 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -17,6 +17,8 @@ source "drivers/gpu/vga/Kconfig"
 
 source "drivers/gpu/drm/Kconfig"
 
+source "drivers/gpu/stub/Kconfig"
+
 config VGASTATE
        tristate
        default n
-- 
1.6.0.2