This is something to think about before we rip out all the ACPI
core-style debug stuff.
> -----Original Message-----
> From: [email protected] [mailto:linux-acpi-
> [email protected]] On Behalf Of Kristen Accardi
> Sent: Wednesday, April 19, 2006 10:09 AM
> To: Patrick Mochel
> Cc: Prarit Bhargava; Andrew Morton; Brown, Len; [email protected]; linux-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
