Re: [PATCH 2/2] Warn on node name unit-addresses with '0x' or leading 0s

On Wed, Feb 24, 2016 at 11:44:56AM +1100, David Gibson wrote:

On Tue, Feb 23, 2016 at 08:35:46AM -0600, Rob Herring wrote:

quoted

On Mon, Feb 22, 2016 at 11:47 PM, David Gibson
[off-list ref] wrote:

quoted

On Mon, Feb 22, 2016 at 10:51:46AM -0600, Rob Herring wrote:

quoted

On Thu, Feb 18, 2016 at 11:07 PM, David Gibson
[off-list ref] wrote:

quoted

On Thu, Feb 11, 2016 at 02:46:59PM -0600, Rob Herring wrote:

quoted

Node name unit-addresses should never begin with 0x or leading 0s
regardless of whether they have a bus specific address (i.e. one with
commas) or not. Add warnings to check for these cases.

Hmm.. I'm pretty sure that's true in practice, but it's not true in
theory.  A bus could define it's unit address format just about
however it wants, including with leading 0s.

Only if it is not reviewed... This whole check is about what best
practices are, not what is possible.

Hmm.  dtc checks are really about checking for best practice at the
level of individual dts files, though, not bindings.

Checking simple-bus specifically would be checking a binding.

Sorry, I wasn't clear.  dtc checking the dts against a binding is
fine, but checking the sanity of the binding itself is beyond its
scope.

quoted

quoted

quoted

quoted

I think a better approach would be to add a test specific to
simple-bus devices (by looking at compatible on the parent) that fully
checks the unit address.

From there we can start adding tests for other bus types.

simple-bus is easy enough,

So, start with that, then tackle the next problem.

quoted

but then next up would be I2C and SPI. We
can't generically tell if a node is on I2C or SPI bus.

Why not?  Or perhaps.. how generically do you need?  I think having a
big list of i2c / spi controllers would be acceptable here, if not
ideal.

So someone adds a new controller, puts crap in for unit addresses, and
then no warnings until that compatible string is added to dtc. And I'm
still left spending my time in reviews telling them to fix this
trivial crap.

That's roughly 60 I2C controllers (families, so multiple compatible
strings each) plus similar number of SPI controllers, OF-graph
binding, and random other things where reg gets used.

Ah, I see.

Ok, I guess we do need to have an option for a "fallback" scheme for
unit addresses (i.e. hex) for bus types we don't specifically
recognize.  But I'd still like the logic to be:
      if (known bus type)
           check against format for this bus type
      else
           check against fallback format

Rather than putting the second test in with a hacked up set of
exclusions.

Okay, makes sense.

Do you think we still need simple-bus as an explicit type given the 
check is the same as the default? Might be useful to have if we want to 
add some checks that address translations work.

To do this nicely, I think the best way will be to add a bus_type
field to the node structure in dtc, and have it populated (with an
option for "unknown") in an early check pass, that later unit address
tests can references as a prereq.

Pointer to a struct with unit address formatting functions, with NULL
for unknown is the obvious choice to me for bus_type.

So, something like this for the first stage:

static bool pci_bus_check_is_type(struct node *node)
{
	struct property *prop;
	
	if (!node || !node->parent)
		return false;

	prop = get_property(node->parent, "device_type");
	if (!prop)
		return false;
		
	if (strcmp(prop->val, "pci") == 0)
		return true;
		
	return false;
}

static void pci_bus_check_unit_address()
{

}

struct bus_type_fns {
	.check_is_type = pci_bus_check_is_type,
	.check_unit_address = pci_bus_check_unit_address,
} pci_bus_fns;

struct bus_type_fns * {
	&pci_bus_fns,
	NULL
} bus_types;

static void fixup_bus_type(struct check *c, struct node *root,
				  struct node *node)
{
	struct bus_type_fns **bus;
	
	for (bus = bus_types; *bus != NULL; bus++) {
		if (!(*bus)->check_is_type(node))
			continue;

		node->bus_type = *bus;
		break;
	}
}
ERROR(bus_type, NULL, NULL, fixup_bus_type, NULL, NULL);


Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html