Re: [PATCH v3 02/15] Driver core: Unified device properties interface for platform firmware

[Rafael J. Wysocki]

On Wednesday, October 01, 2014 09:47:40 AM Arnd Bergmann wrote:
> On Wednesday 01 October 2014 04:10:03 Rafael J. Wysocki wrote:
> > From: "Rafael J. Wysocki" <rafael.j.wysocki@xxxxxxxxx>
> > 
> > Add a uniform interface by which device drivers can request device
> > properties from the platform firmware by providing a property name
> > and the corresponding data type.  The purpose of it is to help to
> > write portable code that won't depend on any particular platform
> > firmware interface.
> > 
> > Three general helper functions, device_get_property(),
> > device_read_property() and device_read_property_array() are provided.
> > The first one allows the raw value of a given device property to be
> > accessed.  The remaining two allow the value of a numeric or string
> > property and multiple numeric or string values of one array
> > property to be acquired, respectively.  Static inline wrappers are also
> > provided for the various property data types that can be passed to
> > device_read_property() or device_read_property_array() for extra type
> > checking.
> 
> These look great!
> 
> > In addition to that, new generic routines are provided for retrieving
> > properties from device description objects in the platform firmware
> > in case a device driver needs/wants to access properties of a child
> > object of a given device object.  There are cases in which there is
> > no struct device representation of such child objects and this
> > additional API is useful then.  Again, three functions are provided,
> > device_get_child_property(), device_read_child_property(),
> > device_read_child_property_array(), in analogy with device_get_property(),
> > device_read_property() and device_read_property_array() described above,
> > respectively, along with static inline wrappers for all of the propery
> > data types that can be used.  For all of them, the first argument is
> > a struct device pointer to the parent device object and the second
> > argument is a (void *) pointer to the child description provided by
> > the platform firmware (either ACPI or FDT).
> 
> I still have my reservations against the child accessors, and would
> like to hear what other people think. Passing a void pointer rather
> than struct fw_dev_node has both advantages and disadvantages, and
> I won't complain about either one if enough other people on the DT
> side would like to see the addition of the child functions.

I actually would rather like to know if the people on the DT side have any
problems with the child functions.

Because, suppose that they wouldn't like those functions at all.  What are we
supposed to do, then, honestly?  Add the whole DT vs ACPI logic to the leds-gpio
and gpio_keys_polled drivers?  But these drivers have no reason whatsoever
to include that.  Zero.

So suggestions welcome.

[BTW, In principle we also could use something like

typedef union dev_node {
	struct acpi_device *acpi_node;
	struct device_node *of_node;
} dev_node_t;

instead of the (void *) for more type safety.  It still is useful to pass the
parent pointer along with that, though.]

> > Finally, device_for_each_child_node() is added for iterating over
> > the children of the device description object associated with a
> > given device.
> > 
> > The interface covers both ACPI and Device Trees.
> > 
> > This change set includes material from Mika Westerberg and Aaron Lu.
> > 
> 
> Regarding device_for_each_child_node(), the syntax is inconsistent
> with what we normally use, which can probably be changed. All of the
> DT for_each_* helpers are macros that are used like
> 
> 	struct device *dev = ...;
> 	void *child; /* iterator */
> 
> 	device_for_each_child_node(dev, child) {
> 		u32 something;
> 		device_child_property_read_u32(dev, child, "propname", &something);
> 
> 		do_something(dev, something);
> 	}
> 
> If we get a consensus on having the child interfaces, I'd rather see
> them done this way than with a callback pointer, for consistency
> reasons.

That certainly is doable, although the resulting macro would generate a rather
large chunk of code each time it is used.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/