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

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4323 bytes --]

On Tue, Feb 23, 2016 at 08:35:46AM -0600, Rob Herring wrote:
> On Mon, Feb 22, 2016 at 11:47 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Mon, Feb 22, 2016 at 10:51:46AM -0600, Rob Herring wrote:
> >> On Thu, Feb 18, 2016 at 11:07 PM, David Gibson
> >> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >> > On Thu, Feb 11, 2016 at 02:46:59PM -0600, Rob Herring wrote:
> >> >> 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.

> >> > 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.
> >
> >> 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.

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.

> >> If we do have
> >> some bus with wacky addresses, it should definitely have a bus
> >> compatible and then we can simply exclude it from the check.
> >>
> >> Another option would be skipping the test if there are any commas (or
> >> periods, etc.) in the unit address. That's pretty rare to begin with
> >> and a single number is pretty much not a bus specific unit-address.
> >
> > Um.. no.. there are definitely bus types that don't typically use
> > commas.  ISA, for one.
> 
> All the cases of ISA in the kernel tree at least would pass this test.
> But we could either blacklist ISA or skip if any non-hex characters
> are present.
> 
> BTW, my next patch is stricter node and property name checks on the
> use of '#', '?', '.', '+', '*', and '_'. So if you don't think these
> kinds to checks belong in dtc, then tell me and suggest how we should
> check for this.

That sounds reasonable on the face of it.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]