Re: Boolean properties

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

On Tue, Dec 06, 2011 at 04:01:20PM -0800, Simon Glass wrote:
> Hi David,
> 
> On Tue, Dec 6, 2011 at 3:12 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Mon, Dec 05, 2011 at 04:29:22PM -0800, Simon Glass wrote:
> >> Hi,
> >>
> >> As I understand it, boolean properties in the device tree are defined
> >> as follows:
> >>
> >> - if the property is present, then it is true
> >> - if the property is absent, then it is false
> >
> > That's the common convention.  Although an individual binding could
> > define something else, and I think some already do define a
> > boolean~=int approach instead (e.g. the ibm,dfp cpu property).  In
> > some cases that's anticipating future expansion to values other than 0
> > and 1.
> 
> OK I didn't know that.
> 
> >
> >> The fdtget and fdtput utilities deal with reading and changing these.
> >> At present fdtget returns the value of the property and is not
> >> suitable for boolean properties. fdtput allows setting of a property
> >> value but not removing it.
> >>
> >> To deal with boolean properties these utilities need to change. One
> >> option is to use something like:
> >>
> >> $ fdtget -t f /path/to/node property
> >> (prints 'true' or 'false')
> >> $ fdtput -t f /path/to/node property false/true
> >> $ fdtput -t f /path/to/node property 0/1
> >> (deletes the property for false and 0; adds it with no value for true and 1)
> >>
> >> (Unfortunately the 'b' type is taken for byte at present, so it is
> >> unclear to be so far what letter we should use for 'f' in the -t flag)
> >
> > Hrm.  Rather than treating this as a boolean "type", I think it would
> > be less ambiguous to instead add options to explicitly delete a
> > property (either e.g. fdtput -d or a new fdtdel), and to create an
> > empty valued property.
> 
> That was my first thought, but I thought it would be painful since
> callers would need to do something like:
> 
> if [ $value -eq 0 ]
>    fdtput -d /path/to/node property
> else
>    fdtput /path/to/node property <some tag that means nothing>
>     (or maybe    fdtput -z /path/to/node property)
> fi
> 
> so better IMO to provide boolean support in the tool. But it does
> illustrate that presence/absence is not always ideal.

Hrm.  I think that's a lesser evil than having a data "type" that can
actually completely delete the property.

> >> But first I would like to suggest that we support boolean properties
> >> with a value, i.e. change to:
> >>
> >> - if the property is absent, then it is false
> >> - if the property is present, then it is true
> >>     - unless the property has a 0 value, iwc it is false
> >
> > So the first question here is who/what exactly constitutes "we" here -
> > kernel, u-boot, dtc?  Using the above logic sounds like a good idea
> > for fdt readers on the "be liberal in what you accept" principle.  But
> > it's something I'd like to see Mitch Bradley and/or Segher comment on
> > - they'd have a better idea of whether there's existing practice that
> > might break with that.
> 
> I mean device tree users (kernel, u-boot) rather than dtc (which I
> suspect would be unaffected).

Well, like I say, I have no in principle objection to using this as
the usual boolean test, but actually changing it is obviously
something you'd need to convince the maintainers of the relevant
projects.

> >> There are three reasons:
> >> 1. it is very confusing for the poor user to set a property value to 0
> >> and have it come back as 'true' from a function that checks these
> >> sorts of things. This is particularly true since fdt properties do not
> >> have types, or even type hints.  The function I have in mind is
> >> something like this (modified to fit the discussion):
> >>
> >> /**
> >>  * Look up a boolean property in a node and return it.
> >>  *
> >>  * A boolean properly is true if present in the device tree and false if not
> >>  * present (regardless of any property data/value).
> >>  *
> >>  * @param blob        FDT blob
> >>  * @param node        node to examine
> >>  * @param prop_name   name of property to find
> >>  * @return 1 if the properly is present; 0 if it isn't present
> >>  */
> >> int fdtdec_get_bool(const void *blob, int node, const char *prop_name);
> >>
> >> An alternative might be to print an error (and refuse to return a
> >> value) when a boolean property has a value:
> >>
> >>  * @return 1 if the properly is present; 0 if it isn't present, -1 if
> >> it has any data (and is therefore illegal as a boolean property)
> >
> > Kind of a pain for C.
> 
> I don't like it either. But we currently have a situation where a zero
> value is considered true, and this can be confusing. I would prefer to
> expand the meaning.

Actually, on further thought, I don't mind the above so much, because
even if you use it without checking for the error value, you'll at
least get the right behaviour for the unambiguous cases.

> >> 2. it allows us to adjust the boolean value without requiring a change
> >> in the size of the device tree blob (faster, easier)
> >>
> >> 3. Related to 2 it allows us to put template values in the fdt which
> >> can then be *optionally* updated in the build system or in U-Boot to
> >> the required value before passing to the kernel.
> >>
> >> I presume this discussion has happened before - can anyone please
> >> point me to the thread?

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