Re: [PATCH v4 1/4] drivers: usb: phy: add a new driver for usb part of control module

[Mark Rutland <mark.rutland@arm.com>]

On Fri, Jan 25, 2013 at 04:23:46PM +0000, Felipe Balbi wrote:
> Hi,
> 
> On Fri, Jan 25, 2013 at 04:14:02PM +0000, Mark Rutland wrote:
> > On Fri, Jan 25, 2013 at 02:59:28PM +0000, Felipe Balbi wrote:
> > > Hi,
> > > 
> > > On Fri, Jan 25, 2013 at 12:29:43PM +0000, Mark Rutland wrote:
> > > > > > > +   depending upon omap4 or omap5.
> > > > > > > + - reg-names: The names of the register addresses corresponding to the registers
> > > > > > > +   filled in "reg".
> > > > > > > + - ti,type: This is used to differentiate whether the control module has
> > > > > > > +   usb mailbox or usb3 phy power. omap4 has usb mailbox in control module to
> > > > > > > +   notify events to the musb core and omap5 has usb3 phy power register to
> > > > > > > +   power on usb3 phy. Should be "1" if it has mailbox and "2" if it has usb3
> > > > > > > +   phy power.
> > > > > > 
> > > > > > Why not make this a string property, perhaps values "mailbox" or "register"?
> > > > > 
> > > > > NAK.
> > > > 
> > > > Can I ask what your objection to using a string property is?
> > > > 
> > > > As far as I can see, "ti,type" is only used by this driver, so there's no
> > > > common convention to stick to. Using a string makes the binding easier for
> > > > humans to read, and thus harder to mess up in a dts, and it decouples the
> > > > binding from kernel-side constants.
> > > 
> > > IIRC there is some work going on to add #define-like support for DT,
> > > which would allow us to match against integers while still having
> > > meaningful symbolic representations.
> > 
> > I was under the impression that the motivation for using the preprocessor on
> > the DT was to allow symbolic names for device/soc-specific values like
> > addresses, rather than what amounts to ABI values for the binding.
> > 
> > I don't see the point in building a binding that depends on future
> > functionality to be legible, especially as we can make it more readable,
> > robust, and just as extensible today, with a simple change to the proposed
> > binding.
> > 
> > Even ignoring the above, the driver isn't doing appropriate sanity checking.
> > If you use a string property, this sanity check is implicit in the parsing --
> > you've either matched a value you can handle or you haven't.
> 
> Even IRQs use numbers (not talking about the IRQ number, but the IRQ
> flags), why would we depend on string comparisson ? It doesn't make
> sense.

When describing interrupts, the flags are associated with the interrupt number,
and don't really make sense on their own. devicetree does not allow you to mix
strings and integers in a value, so you'd never be able to encode irq flags
with strings without completely breaking away from the way ePAPR describes how
to encode interrupts.

By using a string property, the binding is self-describing and easy for humans
to read and verify. It doesn't add a large overhead to either the driver or the
dts, and is no worse (possibly better) for future extension of bindings to
support new device variants while retaining backwards compatibility.

See the standard "status" property, which is string encoded. This would still
work were it an integer-encoded enumeration. Having it as a string makes the
meaning clear to a reader of the dts, and it's trivial for code to deal with.

I don't understand why you think encoding a more legible value in the dt does
not make sense.

Thanks,
Mark.