Good progress! This is almost commitable :-)
Some nits: use `cvs diff -uNp` and be careful about spacing.
Going to comment more on the diff inline.
diff -p -u5 -r -N /usr/src.orig/sys/arch/i386/conf/GENERIC
/usr/src/sys/arch/i386/conf/GENERIC
--- /usr/src.orig/sys/arch/i386/conf/GENERIC Fri May 27 20:19:23 2011
+++ /usr/src/sys/arch/i386/conf/GENERIC Fri May 27 22:08:02 2011
@@ -60,10 +60,11 @@ acpimcfg* at acpi?
acpiprt* at acpi?
acpitz* at acpi?
acpiasus* at acpi?
acpisony* at acpi?
acpithinkpad* at acpi?
+acpitoshiba* at acpi?
Is this i386 only?
acpivideo* at acpi?
acpivout* at acpivideo?
acpipwrres* at acpi?
aibs* at acpi?
diff -p -u5 -r -N /usr/src.orig/sys/dev/acpi/acpi.c /usr/src/sys/dev/acpi/acpi.c
--- /usr/src.orig/sys/dev/acpi/acpi.c Fri May 27 20:19:49 2011
+++ /usr/src/sys/dev/acpi/acpi.c Fri May 27 23:47:07 2011
@@ -2332,10 +2332,12 @@ acpi_foundhid(struct aml_node *node, void *arg)
!strcmp(dev, ACPI_DEV_LENOVO)) {
aaa.aaa_name = "acpithinkpad";
acpi_thinkpad_enabled = 1;
} else if (!strcmp(dev, ACPI_DEV_ASUSAIBOOSTER))
aaa.aaa_name = "aibs";
+ else if ( !strcmp(dev, ACPI_DEV_TOSHIBA))
+ aaa.aaa_name = "acpitoshiba";
Is it okay for acpitoshiba and acpivideo to coexist? I don't think so.
if (aaa.aaa_name)
config_found(self, &aaa, acpi_print);
aml_freevalue(&res);
diff -p -u5 -r -N /usr/src.orig/sys/dev/acpi/acpireg.h
/usr/src/sys/dev/acpi/acpireg.h
--- /usr/src.orig/sys/dev/acpi/acpireg.h Fri May 27 20:19:49 2011
+++ /usr/src/sys/dev/acpi/acpireg.h Fri May 27 23:46:26 2011
@@ -714,7 +714,9 @@ struct acpi_ivrs {
#define ACPI_DEV_FFB "FIXEDBUTTON" /* Fixed Feature Button */
#define ACPI_DEV_ASUS "ASUS010" /* ASUS Hotkeys */
#define ACPI_DEV_IBM "IBM0068" /* IBM ThinkPad support */
#define ACPI_DEV_LENOVO "LEN0068" /* Lenovo ThinkPad support */
#define ACPI_DEV_ASUSAIBOOSTER "ATK0110" /* ASUSTeK AI Booster */
+#define ACPI_DEV_TOSHIBA "TOS6208" /* Toshiba support */
+
Extra newline. Not needed.
#endif /* !_DEV_ACPI_ACPIREG_H_ */
diff -p -u5 -r -N /usr/src.orig/sys/dev/acpi/acpitoshiba.c
/usr/src/sys/dev/acpi/acpitoshiba.c
--- /usr/src.orig/sys/dev/acpi/acpitoshiba.c Thu Jan 1 01:00:00 1970
+++ /usr/src/sys/dev/acpi/acpitoshiba.c Sun May 29 00:48:48 2011
@@ -0,0 +1,461 @@
+/*
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
I think you have a trimmed copyright. It should include the author and
the clauses as well ;-)
+
+#include <sys/param.h>
+#include <sys/systm.h>
+#include <sys/proc.h>
+
+#include <dev/acpi/acpireg.h>
+#include <dev/acpi/acpivar.h>
+#include <dev/acpi/acpidev.h>
+#include <dev/acpi/amltypes.h>
+#include <dev/acpi/dsdt.h>
+
+#include <machine/apmvar.h>
Why is this needed?
+#include <dev/wscons/wsconsio.h>
+
+/** HCI register definitions */
+#define HCI_WORDS 6 /* Number of register */
+#define HCI_REG_AX 0 /* Operation, then return value */
+#define HCI_REG_BX 1 /* Function */
+#define HCI_REG_CX 2 /* Argument (in or out) */
+#define HCI_REG_DX 3 /* Unused? */
+#define HCI_REG_SI 4 /* Unused? */
+#define HCI_REG_DI 5 /* Unused? */
If unused better leave them out or give the constant a proper comment.
+
+/** Operations */
+#define HCI_SET 0xFF00
+#define HCI_GET 0xFE00
+
+ /* Functions */
+#define HCI_REG_SYSTEM_EVENT 0x0016
+#define HCI_REG_VIDEO_OUTPUT 0x001C
+#define HCI_REG_HOTKEY_EVENT 0x001E
Unused?
+#define HCI_REG_LCD_BRIGHTNESS 0x002A
+
+
+/* Toshiba ACPI method paths */
+#define METHOD_HCI "GHCI"
+#define METHOD_HCI_ENABLE "ENAB"
+#define METHOD_VIDEO "DSSX"
Spaces instead of tabs. And where is this used anyway?
+
+
+
+/** Toshiba button events */
+#define TOSHIBA_BUTTON_VIDEO_OUTPUT 0x01BF
Spaces instead of tabs.
+#define TOSHIBA_BUTTON_BRIGHTNESS_DOWN 0x01C0
+#define TOSHIBA_BUTTON_BRIGHTNESS_UP 0x01C1
Also, be consistent about identing your defines.
+
+
+/* Field definitions */
+#define HCI_FAN_SHIFT 7
Unused?
+#define HCI_LCD_BRIGHTNESS_BITS 3
+#define HCI_LCD_BRIGHTNESS_SHIFT (16 - HCI_LCD_BRIGHTNESS_BITS)
+#define HCI_LCD_BRIGHTNESS_MAX ((1 << HCI_LCD_BRIGHTNESS_BITS) - 1)
+#define HCI_LCD_BRIGHTNESS_MIN 0
+#define HCI_VIDEO_OUTPUT_FLAG 0x0100
+
+/* default definitions */
+#define MIN_BRIGHTNESS_LEVEL 0
+#define MAX_BRIGHTNESS_LEVEL 7
+#define MIN_VIDEO_OUTPUT_CYCLE 0
+#define MAX_VIDEO_OUTPUT_CYCLE 7
Yet again, spaces instead of tabs. You really need to fix this all
through the diff. I will stop commenting these spacing nits now.
Go ahead and check them yourself for the rest of the diff.
+
+
+struct acpitoshiba_softc {
+ struct device sc_dev;
+
+ struct acpi_softc *sc_acpi;
+ struct aml_node *sc_devnode;
+
+};
+
+int toshiba_enable_events(struct acpitoshiba_softc *);
+int toshiba_enable_hotkeys(struct acpitoshiba_softc *);
+int toshiba_read_events(struct acpitoshiba_softc *);
+int toshiba_match(struct device *, void *, void *);
+void toshiba_attach(struct device *, struct device *, void *);
+int toshiba_hotkey(struct aml_node *, int, void *);
+int toshiba_get_brightness(struct acpitoshiba_softc *, u_int32_t * );
+int toshiba_set_brightness(struct acpitoshiba_softc *, u_int32_t *);
+int toshiba_get_video_output(struct acpitoshiba_softc *, u_int32_t * );
+int toshiba_set_video_output(struct acpitoshiba_softc *, u_int32_t * );
+int toshiba_find_brightness(struct acpitoshiba_softc *, int *);
+
+/* wconsole hook functions */
+int acpitoshiba_get_param(struct wsdisplay_param *);
+int acpitoshiba_set_param(struct wsdisplay_param *);
+extern int (*ws_get_param)(struct wsdisplay_param *);
+extern int (*ws_set_param)(struct wsdisplay_param *);
+
+
+struct cfattach acpitoshiba_ca = {
+ sizeof(struct acpitoshiba_softc), toshiba_match, toshiba_attach
+};
+
+struct cfdriver acpitoshiba_cd = {
+ NULL, "acpitoshiba", DV_DULL
+};
+
+const char *acpitoshiba_hids[] = { ACPI_DEV_TOSHIBA, 0 };
This is useless, see further note down.
+
+struct acpitoshiba_softc * selfp;
Where is this used? Dead code?
+
+
+int
+acpitoshiba_get_param(struct wsdisplay_param *dp)
+{
+ struct acpitoshiba_softc *sc = NULL;
+ int i;
+
+ switch (dp->param) {
+ case WSDISPLAYIO_PARAM_BRIGHTNESS:
+ for (i = 0; i < acpitoshiba_cd.cd_ndevs; i++) {
+ if (acpitoshiba_cd.cd_devs[i] == NULL)
+ continue;
+ sc = (struct acpitoshiba_softc
*)acpitoshiba_cd.cd_devs[i];
+ }
+ if (sc != NULL) {
+ rw_enter_write(&sc->sc_acpi->sc_lck);
+ dp->min = MIN_BRIGHTNESS_LEVEL;
+ dp->max = MAX_BRIGHTNESS_LEVEL;
+ toshiba_get_brightness(sc, &dp->curval);
+ rw_exit_write(&sc->sc_acpi->sc_lck);
+ if (dp->curval != -1)
+ return 0;
+ }
+ return -1;
+ default:
+ return -1;
+ }
+}
+
+int
+acpitoshiba_set_param(struct wsdisplay_param *dp)
+{
+ struct acpitoshiba_softc *sc = NULL;
+ int i;
+
+ switch (dp->param) {
+ case WSDISPLAYIO_PARAM_BRIGHTNESS:
+ for (i = 0; i < acpitoshiba_cd.cd_ndevs; i++) {
+ if (acpitoshiba_cd.cd_devs[i] == NULL)
+ continue;
+ sc = (struct acpitoshiba_softc
*)acpitoshiba_cd.cd_devs[i];
+ }
+ if (sc != NULL) {
+ rw_enter_write(&sc->sc_acpi->sc_lck);
+ toshiba_find_brightness(sc, &dp->curval);
This can fail as well, why not check it as in get_param()?
Or check the return value at least. But be consistent in both cases.
+ rw_exit_write(&sc->sc_acpi->sc_lck);
+ return 0;
+ }
+ return -1;
+ default:
+ return -1;
+ }
+}
+
+int
+toshiba_find_brightness(struct acpitoshiba_softc * sc, int * set_blevel)
+{
+ int brightness_level = 0;
+ toshiba_get_brightness(sc, &brightness_level);
Why don't you check the return value here? Or the brightness_level (as
you did above in get_param()?
+
+ if ( brightness_level != *set_blevel)
+ {
+ if ( *set_blevel >= MAX_BRIGHTNESS_LEVEL)
+ *set_blevel = brightness_level = MAX_BRIGHTNESS_LEVEL;
+ else if (*set_blevel <= MIN_BRIGHTNESS_LEVEL)
+ *set_blevel = brightness_level = MIN_BRIGHTNESS_LEVEL;
+ else
+ brightness_level = *set_blevel;
+
+ toshiba_set_brightness(sc, &brightness_level);
This can fail, remeber to signal it or make the function return type
void.
+ }
+
+ return 0;
Sometimes you put parentheses around the return value, sometimes you
don't. Pick one style and use it all through the file.
+}
+
+int
+toshiba_match(struct device *parent, void *match, void *aux)
+{
+ struct acpi_attach_args *aa = aux;
+ struct cfdata *cf = match;
+
+
+
+ return (acpi_matchhids(aa, acpitoshiba_hids, cf->cf_driver->cd_name));
Might be missing something, but didn't you do that in acpi.c already?
Something like this is just fine:
if (aa->aaa_name == NULL ||
strcmp(aa->aaa_name, cf->cf_driver->cd_name) != 0 ||
aa->aaa_table != NULL)
return (0);
return (1);
+}
+
+
+int
+toshiba_enable_events(struct acpitoshiba_softc *sc)
+{
+
+ if (aml_evalname(sc->sc_acpi, sc->sc_devnode, METHOD_HCI_ENABLE,
+ 0, NULL, NULL)) {
style(9).
+ printf("%s: couldn't toggle METHOD_HCI\n", DEVNAME(sc));
+ return (1);
+ }
+
+ return (0);
+}
+
+int
+toshiba_read_events(struct acpitoshiba_softc *sc)
+{
+
+ struct aml_value args[HCI_WORDS];
+ struct aml_value res;
+ bzero(args, sizeof(args));
+ bzero(&res, sizeof(res));
+ int val;
+
+
+ int i;
+ for (i = 0; i < HCI_WORDS; ++i)
+ {
+ args[i].type = AML_OBJTYPE_INTEGER;
+ args[i].v_integer = 0;
+ }
+
+ args[HCI_REG_AX].v_integer = HCI_GET;
+ args[HCI_REG_BX].v_integer = HCI_REG_SYSTEM_EVENT;
+
+ if (aml_evalname(sc->sc_acpi, sc->sc_devnode, METHOD_HCI,
+ i, args, &res)) {
+ printf("%s: couldn't toggle METHOD_HCI\n", DEVNAME(sc));
+ return (1);
+ }
+
+ /** We receive a package type so we need to get the event
+ * value from the HCI_REG_CX
+ */
style(9).
+ val = aml_val2int(res.v_package[HCI_REG_CX]);
+ aml_freevalue(&res);
+
+ return val;
+}
+
+void
+toshiba_attach(struct device *parent, struct device *self, void *aux)
+{
+ struct acpitoshiba_softc *sc = (struct acpitoshiba_softc *)self;
+ struct acpi_attach_args *aa = aux;
+
+ sc->sc_acpi = (struct acpi_softc *)parent;
+ sc->sc_devnode = aa->aaa_node;
+
+ printf("\n");
+
+ /* enable events and hotkeys */
+ toshiba_enable_events(sc);
What if it fails?
+
+ /* Run toshiba_hotkey on button presses */
+ aml_register_notify(sc->sc_devnode, aa->aaa_dev,
+ toshiba_hotkey, sc, ACPIDEV_NOPOLL);
+
+ /** wsconsctl purpose */
style(9)
+ ws_get_param = acpitoshiba_get_param;
+ ws_set_param = acpitoshiba_set_param;
+
+}
+
+int
+toshiba_hotkey(struct aml_node *node, int notify, void *arg)
+{
+ struct acpitoshiba_softc *sc = arg;
+
+ int event = toshiba_read_events(sc);
+ if ( event == 0)
+ return (0);
+
+ u_int32_t brightness_level;
+ u_int32_t video_output;
style(9)! style(9)! style(9)!
+
+ switch (event) {
+ case TOSHIBA_BUTTON_BRIGHTNESS_UP:
+ toshiba_get_brightness(sc, &brightness_level);
+ if (brightness_level++ == MAX_BRIGHTNESS_LEVEL)
+ brightness_level = MAX_BRIGHTNESS_LEVEL;
+ else
+ toshiba_set_brightness(sc, &brightness_level);
+ break;
+ case TOSHIBA_BUTTON_BRIGHTNESS_DOWN:
+ toshiba_get_brightness(sc, &brightness_level);
+ if (brightness_level-- == MIN_BRIGHTNESS_LEVEL)
+ brightness_level = MIN_BRIGHTNESS_LEVEL;
+ else
+ toshiba_set_brightness(sc, &brightness_level);
+ break;
+ case TOSHIBA_BUTTON_VIDEO_OUTPUT:
+ /* Cycle through video outputs. */
+ toshiba_get_video_output(sc, &video_output);
+ video_output = (video_output + 1) %
+ MAX_VIDEO_OUTPUT_CYCLE;
+ toshiba_set_video_output(sc, &video_output);
+ break;
+ default:
+ break;
+ }
+
+ return (0);
+}
Yet again, you have to check the return values all through these cases.
+
+int
+toshiba_set_brightness(struct acpitoshiba_softc * sc, u_int32_t * brightness)
+{
+ struct aml_value args[HCI_WORDS];
+
+ bzero(args, sizeof(args));
+
+ int i;
+ for (i = 0; i < HCI_WORDS; ++i)
+ {
+ args[i].type = AML_OBJTYPE_INTEGER;
+ args[i].v_integer = 0;
+ }
+
+ if ((*brightness < HCI_LCD_BRIGHTNESS_MIN) ||
+ (*brightness > HCI_LCD_BRIGHTNESS_MAX))
+ return (EINVAL);
+
+ *brightness <<= HCI_LCD_BRIGHTNESS_SHIFT;
+
+
+ args[HCI_REG_AX].v_integer = HCI_SET;
+ args[HCI_REG_BX].v_integer = HCI_REG_LCD_BRIGHTNESS;
+ args[HCI_REG_CX].v_integer = *brightness;
+
+
+ if (aml_evalname(sc->sc_acpi, sc->sc_devnode, METHOD_HCI,
+ i, args, NULL)) {
+ printf("%s: set brightness failed\n", DEVNAME(sc));
+ return (1);
+ }
+
+ return (0);
+}
+
+int
+toshiba_get_brightness(struct acpitoshiba_softc *sc, u_int32_t * brightness)
+{
+
+ struct aml_value args[HCI_WORDS];
+ struct aml_value res;
+
+ bzero(args, sizeof(args));
+ bzero(&res, sizeof(res));
+
+ int i;
+ for (i = 0; i < HCI_WORDS; ++i)
+ {
+ args[i].type = AML_OBJTYPE_INTEGER;
+ args[i].v_integer = 0;
+ }
+
+ args[HCI_REG_AX].v_integer = HCI_GET;
+ args[HCI_REG_BX].v_integer = HCI_REG_LCD_BRIGHTNESS;
+
+ if (aml_evalname(sc->sc_acpi, sc->sc_devnode, METHOD_HCI,
+ i, args, &res)) {
+ printf("%s: get brightness failed\n", DEVNAME(sc));
+ return (1);
+ }
+
+ /** We receive a package type so we need to get the event
+ * value from the HCI_REG_CX
+ */
style(9)
+ *brightness = aml_val2int(res.v_package[HCI_REG_CX]);
+
+ *brightness >>= HCI_LCD_BRIGHTNESS_SHIFT;
+
+ aml_freevalue(&res);
+
+ return (0);
+}
+
+int
+toshiba_get_video_output(struct acpitoshiba_softc *sc, u_int32_t *
video_output)
+{
+ struct aml_value args[HCI_WORDS];
+ struct aml_value res;
+
+ bzero(args, sizeof(args));
+ bzero(&res, sizeof(res));
+
+ int i;
+ for (i = 0; i < HCI_WORDS; ++i)
+ {
+ args[i].type = AML_OBJTYPE_INTEGER;
+ args[i].v_integer = 0;
+ }
+
+ args[HCI_REG_AX].v_integer = HCI_GET;
+ args[HCI_REG_BX].v_integer = HCI_REG_VIDEO_OUTPUT;
+
+ if (aml_evalname(sc->sc_acpi, sc->sc_devnode, METHOD_HCI,
+ i, args, &res)) {
+ printf("%s: get video output failed\n", DEVNAME(sc));
+ return (1);
+ }
+
+ /** We receive a package type so we need to get the event
+ * value from the HCI_REG_CX
+ */
style(9)
+ *video_output = aml_val2int(res.v_package[HCI_REG_CX]);
+
+ *video_output &= 0xff;
+
+ aml_freevalue(&res);
+
+ return (0);
+}
+
+int
+toshiba_set_video_output(struct acpitoshiba_softc *sc, u_int32_t *
video_output)
+{
+
+ struct aml_value args[HCI_WORDS];
+
+ bzero(args, sizeof(args));
+
+ if ((*video_output < MIN_VIDEO_OUTPUT_CYCLE) ||
+ (*video_output > MAX_VIDEO_OUTPUT_CYCLE))
+ return (EINVAL);
+
+ *video_output |= HCI_VIDEO_OUTPUT_FLAG;
+
+ int i;
That's a no-no!
+ for (i = 0; i < HCI_WORDS; ++i)
+ {
+ args[i].type = AML_OBJTYPE_INTEGER;
+ args[i].v_integer = 0;
Perhaps useless since you bzero-ed.
+ }
+
+
+ args[HCI_REG_AX].v_integer = HCI_SET;
+ args[HCI_REG_BX].v_integer = HCI_REG_VIDEO_OUTPUT;
+ args[HCI_REG_CX].v_integer = *video_output;
+
+ if (aml_evalname(sc->sc_acpi, sc->sc_devnode, METHOD_HCI,
+ i, args, NULL)) {
+ printf("%s: set video output failed\n", DEVNAME(sc));
+ return (1);
+ }
+
+ return (0);
+}
+
diff -p -u5 -r -N /usr/src.orig/sys/dev/acpi/files.acpi
/usr/src/sys/dev/acpi/files.acpi
--- /usr/src.orig/sys/dev/acpi/files.acpi Fri May 27 20:19:49 2011
+++ /usr/src/sys/dev/acpi/files.acpi Fri May 27 22:19:42 2011
@@ -79,10 +79,15 @@ file dev/acpi/acpiasus.c acpiasus
# IBM/Lenovo ThinkPad support
device acpithinkpad
attach acpithinkpad at acpi
file dev/acpi/acpithinkpad.c acpithinkpad
+# Toshiba support
+device acpitoshiba
+attach acpitoshiba at acpi
+file dev/acpi/acpitoshiba.c acpitoshiba
+
# Sony support
device acpisony
attach acpisony at acpi
file dev/acpi/acpisony.c acpisony
