eeh@netbsd.org writes:
> 3	Description
> 
> A property is a tuple that consists of a pointer to a device node (struct
> device *), string, and an arbitrary amount of data.  This tuple is established
> by dev_setprop(), retrieved by dev_getprop(), and destroyed by dev_delprop().  In
> addition, there is a machine dependent hook, md_getprop()[1] that is called if
> attempting to retrieve a property fails.  md_getprop() can then use any
> arbitrary method to generate property data if it so desires, or cause a failure
> to be reported.
> 
> This functionality will be implemented in two phases.  The first phase will
> add these functions, but retain the existing behavior for compatilbility.
> dev_create(9) will generate a dummy device node that the parent can use to 
> attach properties to.  config_attach*(9) functions will then create the real
> device node and migrate properties to it.

s/migrate/copy/ please.


> The second phase will separate the `struct device' from the device's softc.
> dev_create(9) will create the true device node, which config_found*(9) will
> attach to the device tree.

It's not clear that you want to go that far.  I still think you want:

(1) fake device is mostly a container,

(2) config_found or dev_config_found() or whatever makes a new one and
attaches it.

The problem is again one of handling of non-configured devices.  It's
just easier for code be be written as:

	create properties container
	config_search(... properties container ...)
	destroy properties container

or:

	create properties container
	set up properties that will never change
	loop {
		set up properties that change from dev to dev.
		set immutable
		config_found(... properties container ...)
		clear immutable
	}
	destroy properties container.

than it is e.g.:

	create properties container
	if (config_search(... properties_container) == NULL)
		destroy_properties_container

or similar for the second example.

Having to worry about freeing depending on whether or not the device
actually was attached is a pain, and pains mean that the code will get
written wrong, and the code getting written wrong will mean probable
memory leaks or data structure corruption.

Yeah, reusing the storage may be a bit more efficient, but we've
pretty much already said that for this interface efficiency is not a
primary concern.  I'd rather have something which makes code
correctness easier.


To this end, I suggest you make the following changes to existing
device stuff (in addition to adding the properties list).

	* add a new devclass, DV_PROPERTIES (for non-devices that are
	  solely property containers.)

	* add a new flag, DV_PROP_IMMUTABLE, which indiates that
	  a device's properties are immutable.


> 3.1	New Functionality
> 
> int dev_setprop(struct device *dev, const char *name, void *val, size_t len, 
> 			int flags);
> 
> Create a property `name' and attach it to `dev' with a `len' byte value 
> copied from location `val'.  If PROP_WAIT is not specified then dev_setprop(9)
> will not sleep for resource shortage.  Returns 0 on success or an error value.

(1) if immutable, error.

(2) if not found on device specified, adds it there (i.e. doesn't
somehow traverse up-chain).

(3) if found on device specified, replaces existing entry.


> int dev_copyprops(struct device *source, struct device *dest, int flags);
> 
> Copy all properties associated with `source' device to `dest' device
> structure.  If PROP_WAIT is not specified then dev_copyprops(9) will not sleep
> for resource shortage.  Returns 0 on success or an error value.

Is this a deep copy, or a shallow copy?  (i.e., are only the
properties directly associated with the named device copied, or are
those properties and all inherited properties also copied?)

I suspect you mean 'only the properties directly associated,' but it
should be speficied.


> size_t dev_getprop(struct device *dev, const char *name, void *val, 
> 			size_t len, int flags);
> 
> Retrieve a property called `name' associated with `dev'.  If the property is
> not found dev_getprop(9) will call md_getprop(9).  If the flag PROP_INHERIT is
> set, and there is no property with that name associated with this device node,
> it will try to retrieve the property from any parent device nodes.

Is the ordering on this correct?  i'd expect it would be:

	(1) look at current device
	(2) if (PROP_INHERIT) look at parents for interhited devices,
	(3) call md_getprop() (or whatever it'll be called.


> int dev_delprop(struct device *dev, const char *name, int flags);
> 
> Remove a property from a device node.  If the flag PROP_INHERIT is set it will
> look for that property in any parent device nodes.  If the flag PROP_ALL is
> set it will remove the property from that device node and all parent device
> nodes.  If a NULL is supplied for the name, dev_delprop(9) will remove all
> properties from a device node.  If a NULL is supplied for the name and
> PROP_ALL is set, dev_delprop(9) will remove ALL properties from a device node
> and all its parents.  It returns 0 on success or an error value.

I don't think it's right for there to be a mechanism by which a device
can modify the properties of its parents.  (Under what circumstances
do you think this is a valid thing to do?)

There definitely needs to be a way to 'reset' the inheritance, though,
i.e. terminate the inheritance chain.  Probably DVF_PROP_NOINHERIT,
which says that this node doesn't inherit from its parents.

That way, to clear a device's slate, you might do:

	(1) set NOINHERIT,
	(2) delete all.

Actually makes me wonder if you want to have the equivalent of a
'whiteout'.  (it might be useful to see how often that'd be used in
practice, and leave the door open to implementing it later...)  If
that type of thing isn't supported, it might be useful to say
something like:

	(1) copy all inherited values down from parent,
	(2) set NOINHERIT.



> struct device *config_found_smd(struct device *dev, void *aux, 	cfprint_t print, 
> 			cfmatch_t submatch, struct device *propdev);
> 
> In addition to config_found(9), and config_found_sm(9), config_found_smd(9)
> can be used to supply device properties to child devices during probe and 
> attach.  The `propdev' parameter is passed down to config_search(9) and
> config_attach(9).
> 
> 
> struct device *dev_config_found(struct device *dev, struct device *propdev,
> 		 	cfprint_t print, cfmatch_t submatch);
> 
> dev_config_found(9) is intended as an interface for bus drivers that no longer
> need to supply an `aux'.  The `propdev' parameter will initally be a dummy
> container but later will be the real child device node.

How does it make sense to provide both of these as public interfaces,
assuing you're going to replace the 'aux' with the propdev for
config_found_smd()?

config_found_smd() can sanely be called either with:

	(1) aux set appropriately && propdev == NULL, or
	(2) aux to be ignored && propdev != NULL.

(1) == config_found_sm().
(2) == dev_config_found().


> struct cfdata *dev_config_search(cfmatch_t fn, struct device *parent, void *aux, 
> 			struct device *propdev);
> 
> dev_config_search(9) takes an additional parameter `propdev' which is a
> pointer to a device which provides a set of properties for devices to use
> during probe.  If `propdev' is not NULL, locator data are attached to it as
> properties, it is passed to the probe routine as the `aux' parameter and
> the value passed in as `aux' is ignored.  The probe routine needs to know to
> cast `aux' to a pointer to a device and extract properties rather than use it
> as a pointer to configuration data.
> 
> struct device *dev_config_attach(struct device *parent, struct cfdata *cf, 
> 			void *aux, cfprint_t print, struct device *propdev)
> 
> If a non-NULL `propdev' is passed in to dev_config_attach(9), all properties
> attached to it are moved to the newly created device node for the device
> being attached.

Again, for providing public interfaces, why bother with the 'aux'
parameter if it's just going to be replaced by propdev if the latter
is supplied?

It only makes sense to take both if you're going to provide both to
underlying match/attach rtns (to allow backward compatiblity.)


If you're going to mandate an all-or-nothing transition per bus, it
makes sense to have two disjoint set of function entry points: one
which takes aux, and one which takes propdev.

If you're going to allow a smooth transition, then you need entry
points which take both and wrappers around them to take either.


To encourage smooth transition between your two phases, I suggest you
do the following also at the time you introduce the properties stuff:

(1) add a new fn/macro dev_private_date(struct device *), which
returns a pointer to the private data.

(2) provide macro CFATTACH_DEFINE(attach_name, private_size) (or
better name) that would:

	provide struct cfattach attach_name##ca (for now using
		private_size + sizeof struct device as size)
	provide static protos for attach_name##match and
		attach_name##attach

Aux vectors should be in the prototypes, but probably named specially
so they can't easily be used.

(3) provide macros CFATTACH_MATCH_DEFINE(attach_name) and
CFATTACH_ATTACH_DEFINE(attach_name) (and i suppose one for detach)
that can be used to head the function definitions and match the protos
provided by (2).

I'd say that initially they'd start out taking both aux and propdev,
but eventually we could flip a switch and make them only take propdev.

In the mean time, the 'cfattach' could still be defined to take only
aux, etc., so we could transition drivers, etc.  But flip that switch,
and the aux support goes away entirely, no editing required.

The transition would go something like:

(0) provide the infrastructure to do conversion, as described above,
and to provide the new property interfaces.

(1) convert busses to provide both properties and aux values.

(2) convert drivers on those busses to use new CFATTACH declaration
macros, and to exclusively use properties.

(3) after a period of time when the new macros have been in place and
all in-tree drivers have been converted, flip the switch to allow
drivers to only take properties, and nuke the aux-only versions of the
config_found, etc. functions.

(4) deprecate the versions of the config functions that include both
properties and aux values, and as time permits, rip the aux value code
out of busses.

(5) after a period of time has elapsed and all of the busses have been
converted, rip out the aux-tolerant config_found, etc. functions.

I say do that at time of addition of properties, because otherwise you
need to worry about editing the cfattach() stuff in all the existing
drivers, etc., multiple times.  This can allow you to hide the gross
casts necessary for backward compatibility inside the macros.



chris