This is something to think about before we rip out all the ACPI core-style debug stuff. > -----Original Message----- > From: linux-acpi-owner@xxxxxxxxxxxxxxx [mailto:linux-acpi- > owner@xxxxxxxxxxxxxxx] On Behalf Of Kristen Accardi > Sent: Wednesday, April 19, 2006 10:09 AM > To: Patrick Mochel > Cc: Prarit Bhargava; Andrew Morton; Brown, Len; greg@xxxxxxxxx; linux- > acpi@xxxxxxxxxxxxxxx; pcihpd-discuss@xxxxxxxxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; arjan@xxxxxxxxxxxxxxx; > muneda.takahiro@xxxxxxxxxxxxxx; pavel@xxxxxx; temnota@xxxxxx > Subject: Re: [patch 1/3] acpi: dock driver > > On Tue, 2006-04-18 at 15:54 -0700, Patrick Mochel wrote: > > On Tue, Apr 18, 2006 at 11:03:16AM -0700, Kristen Accardi wrote: > > > Create a driver which lives in the acpi subsystem to handle dock > events. This > > > driver is not an acpi driver, because acpi drivers require that the > object > > > be present when the driver is loaded. > > > > A few comments.. > > > > > > > --- /dev/null > > > +++ 2.6-git-kca2/drivers/acpi/dock.c > > > @@ -0,0 +1,652 @@ > > > > > +#define ACPI_DOCK_COMPONENT 0x10000000 > > > +#define ACPI_DOCK_DRIVER_NAME "ACPI Dock Station Driver" > > > +#define _COMPONENT ACPI_DOCK_COMPONENT > > > > These aren't necessary for code that is outside of the ACPI-CA. > > Originally I did not include these, but it turns out if you wish to use > the ACPI_DEBUG macro, you need to have these things defined. I did go > ahead and use this macro in a couple places, mainly because I felt that > even though this isn't strictly an acpi driver (using the acpi driver > model), it does live in drivers/acpi and perhaps people might expect to > be able to debug it the same way. > > > > > > +struct dock_station { > > > + acpi_handle handle; > > > + unsigned long last_dock_time; > > > + u32 flags; > > > + spinlock_t dd_lock; > > > + spinlock_t hp_lock; > > > + struct list_head dependent_devices; > > > + struct list_head hotplug_devices; > > > +}; > > > + > > > +struct dock_dependent_device { > > > + struct list_head list; > > > + struct list_head hotplug_list; > > > + acpi_handle handle; > > > + acpi_notify_handler handler; > > > + void *context; > > > +}; > > > + > > > +#define DOCK_DOCKING 0x00000001 > > > + > > > +static struct dock_station *dock_station; > > > > Does this need to be dynamically allocated? Static initialization > > would be a bit cleaner and obviate the need for the NULL checks in > > several of the functions below. > > > > It could be statically allocated, but I have a preference towards not > allocating statically in this case. I will consider the option of > making it static. > > > > +/** > > > + * eject_dock - respond to a dock eject request > > > + * @ds: the dock station > > > + * > > > + * This is called after _DCK is called, to execute the dock station's > > > + * _EJ0 method. > > > + */ > > > +static void eject_dock(struct dock_station *ds) > > > +{ > > > + struct acpi_object_list arg_list; > > > + union acpi_object arg; > > > + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > > > + union acpi_object *obj; > > > + > > > + acpi_get_name(ds->handle, ACPI_FULL_PATHNAME, &buffer); > > > + obj = buffer.pointer; > > > + > > > + arg_list.count = 1; > > > + arg_list.pointer = &arg; > > > + arg.type = ACPI_TYPE_INTEGER; > > > + arg.integer.value = 1; > > > > Minor nit (that is replicated in many of the ACPI drivers). This can be > > done by just describing the data better: > > > > struct acpi_object arg = { > > .type = ACPI_TYPE_INTEGER, > > .integer = { > > .value = 1, > > }, > > }; > > struct acpi_object_list arg_list = { > > .count = 1, > > .pointer = &arg, > > }; > > > > ... > > > > In the long run, since the same exact code exists in dozens of places > > in the ACPI drivers, there should just be a helper for it. E.g.: > > > > > > int ret; > > unsigned long value; > > > > ret = acpi_get_int(ds->handle, "_EJO", &value); > > if (!ret) > > /* Use Value */ > > else > > /* Error */ > > > > ...and get rid of the awkward object/object list handling. > > > > > +static inline void begin_dock(struct dock_station *ds) > > > +{ > > > + ds->flags |= DOCK_DOCKING; > > > +} > > > + > > > +static inline void complete_dock(struct dock_station *ds) > > > +{ > > > + ds->flags &= ~(DOCK_DOCKING); > > > + ds->last_dock_time = jiffies; > > > +} > > > + > > > +/** > > > + * dock_in_progress - see if we are in the middle of handling a dock > event > > > + * @ds: the dock station > > > + * > > > + * Sometimes while docking, false dock events can be sent to the > driver > > > + * because good connections aren't made or some other reason. Ignore > these > > > + * if we are in the middle of doing something. > > > + */ > > > +static int dock_in_progress(struct dock_station *ds) > > > +{ > > > + if ((ds->flags & DOCK_DOCKING) || > > > + time_before(jiffies, (ds->last_dock_time + HZ))) > > > + return 1; > > > + return 0; > > > +} > > > > These seem racy. It seems the flag should should at least be an > atomic_t. But, > > if it's that, then it might as well be a mutex, eh? And, if it's a > mutex, then > > do we need the other spinlocks? > > > > yes, the flag might be racy. we do need the other spinlocks however, > because they are locking lists within the dock_station struct, but not > the entire struct (unless I just change to something that locks the > entire struct). > > > > +acpi_status > > > +register_hotplug_dock_device(acpi_handle handle, acpi_notify_handler > handler, > > > + void *context) > > > > If this is called from outside drivers/acpi/, you should return an int > with a > > real errno value. The AE_* values shouldn't be used outside of the ACPI > CA. > > > > Really? We use these all over the place in drivers/pci/hotplug. In > fact, you kinda have to use them if you are calling certain acpi > symbols, since they return these types. > > For example, here are some functions will return acpi_status that we use > in hotplug land. > > pci_osc_control_set() > acpi_run_oshp() > acpi_walk_namespace requires its use. > > I felt that by returning acpi_status I was being consistent with how > other acpi calls acted. Is this another example of the iceberg that Len > was talking about in a previous email?? (ugh.) > > > > > > > +acpi_status unregister_hotplug_dock_device(acpi_handle handle) > > > > Does unregister need to return an error? > > > > No probably not. > > > > > Thanks, > > > > > > Pat > > thanks for reviewing again :). > > Kristen > - > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html - To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
