Date: Tue, 11 Apr 2006 12:24:43 -0700 From: Nate Lawson <nate@root.org> To: Mitsuru IWASAKI <iwasaki@jp.FreeBSD.org> Cc: acpi@freebsd.org, current@freebsd.org Subject: Re: CFR: ACPI Dock driver Message-ID: <443C027B.7050002@root.org> In-Reply-To: <20060410.215024.32344167.iwasaki@jp.FreeBSD.org> References: <20060408.032151.07645075.iwasaki@jp.FreeBSD.org> <20060410.215024.32344167.iwasaki@jp.FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?443C027B.7050002>