From owner-freebsd-current@FreeBSD.ORG Tue Apr 11 19:26:26 2006 Return-Path: X-Original-To: current@freebsd.org Delivered-To: freebsd-current@FreeBSD.ORG Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 9A5D716A400; Tue, 11 Apr 2006 19:26:26 +0000 (UTC) (envelope-from nate@root.org) Received: from mail.aloha.net (mail.aloha.net [64.75.176.98]) by mx1.FreeBSD.org (Postfix) with ESMTP id 1B95F43D48; Tue, 11 Apr 2006 19:26:25 +0000 (GMT) (envelope-from nate@root.org) Received: from localhost (localhost [127.0.0.1]) by mail.aloha.net (Postfix) with ESMTP id ABC00F1A18; Tue, 11 Apr 2006 09:26:23 -1000 (HST) X-Virus-Scanned: by amavisd-new at aloha.net Received: from mail.aloha.net ([127.0.0.1]) by localhost (lacewood.aloha.net [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id gm2A0yRjE-+I; Tue, 11 Apr 2006 09:26:22 -1000 (HST) Received: from [10.0.0.150] (ip64-75-163-14.hsia.aloha.net [64.75.163.14]) by mail.aloha.net (Postfix) with ESMTP id 8B912F1A37; Tue, 11 Apr 2006 09:26:21 -1000 (HST) Message-ID: <443C027B.7050002@root.org> Date: Tue, 11 Apr 2006 12:24:43 -0700 From: Nate Lawson User-Agent: Thunderbird 1.5 (Windows/20051201) MIME-Version: 1.0 To: Mitsuru IWASAKI References: <20060408.032151.07645075.iwasaki@jp.FreeBSD.org> <20060410.215024.32344167.iwasaki@jp.FreeBSD.org> In-Reply-To: <20060410.215024.32344167.iwasaki@jp.FreeBSD.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: acpi@freebsd.org, current@freebsd.org Subject: Re: CFR: ACPI Dock driver X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 11 Apr 2006 19:26:26 -0000 Mitsuru IWASAKI wrote: > Hi, I've ported ACPI Dock driver to 7-CURRENT. > http://www.freebsd.org/~iwasaki/acpi/acpi_dock-CURRENT-20060410.tar.gz > > It seems to be OK so far. I'm planning to commit this > coming week-end. > > Thanks > Thanks for doing this work, Iwasaki-san. A few comments: * I prefer the logic in the device not present case to be reworked a little for clarity: ACPI_HANDLE h; // local var at top of fn if (type == ACPI_TYPE_DEVICE && !acpi_DeviceIsPresent(child)) { /* Never disable PCI link devices. */ if (acpi_MatchHid(handle, "PNP0C0F")) break; /* * Docking stations should remain enabled since the system may * may be undocked at boot. */ if (ACPI_SUCCESS(AcpiGetHandle(handle, "_DCK", &h))) { break; } device_disable(child); break; * Minor style for (retry = 0; retry < ACPI_CMBAT_RETRY_MAX; retry++, AcpiOsSleep(10000)) { /* batteries on DOCK can be ejected w/ DOCK during retrying */ if (!device_is_attached(dev)) return; * Instead of rolling your own, try acpi_GetInteger() instead of code like this: argobj.Type = ACPI_TYPE_INTEGER; argobj.Integer.Value = dock; args.Count = 1; args.Pointer = &argobj; buf.Pointer = &retobj; buf.Length = sizeof(retobj); * There also might be an acpi_GetReference() helper function to use. (I think I wrote one) * I'm not sure what this code is trying to do: #if (ACPI_CA_VERSION <= 0x20041119) ACPI_INIT_WALK_INFO Info; AcpiNsWalkNamespace(ACPI_TYPE_ANY, handle, 100, TRUE, AcpiNsInitOneDevice, &Info, NULL); #endif * Is it safe to do probe/attach from a timeout handler? If you want to do this, you can also use AcpiOsQueueHandler(). It queues a task on our thread taskq and you can sleep in the context. timeout(acpi_dock_attach_later, (caddr_t)dev, hz*3); * Use explicit names for constants like status: if (sc->_sta & 0x9) * Having a global status.. is this ok with SMP? Could acpi_dock_insert() and acpi_dock_removal() have a race condition with setting this variable? /* Global docking status, for avoiding duplicated docking */ static int acpi_dock_status = ACPI_DOCK_STATUS_UNKNOWN; Hope this helps, -- Nate