Re: CFR: ACPI Dock driver

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