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@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx> 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:
signature.asc
Description: PGP signature
