[PATCH 1/1] of: introduce helper to manage boolean

[PATCH 1/1] of: introduce helper to manage boolean

[Jean-Christophe PLAGNIOL-VILLARD]

of_property_read_bool

Search for a property in a device node and read a 32-bit value from
it. Returns 0 if <0> or if the property does not exist, 1 if <1> or none.

this will allow to disable a boolean

is-ok;          => true
is-ok = <1>;    => true
is-ok = <0>;    => false

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: devicetree-discuss at lists.ozlabs.org
---
Hi,

	if this is ok I'll rebase my mtd and i2c patch to use it

Best Regards,
J.
 drivers/of/base.c  |   30 ++++++++++++++++++++++++++++++
 include/linux/of.h |    8 ++++++++
 2 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 133908a..a0eaf08 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -686,6 +686,36 @@ int of_property_read_u64(const struct device_node *np, const char *propname,
 EXPORT_SYMBOL_GPL(of_property_read_u64);
 
 /**
+ * of_property_read_bool - Find and read a boolean from a property
+ * @np:		device node from which the property value is to be read.
+ * @propname:	name of the property to be searched.
+ *
+ * Search for a property in a device node and read a 32-bit value from
+ * it. Returns 0 if <0> or if the property does not exist, 1 if <1> or none.
+ *
+ * is-ok;	=> true
+ * is-ok = <1>;	=> true
+ * is-ok = <0>;	=> false
+ */
+int of_property_read_bool(const struct device_node *np, const char *propname)
+{
+	u32 reg;
+	int ret = of_property_read_u32(np, propname, &reg);
+
+	switch (ret) {
+	case -EINVAL:
+		return false;
+	case -ENODATA:
+		return true;
+	case 0:
+		return reg == 1;
+	default:
+		return ret;
+	}
+}
+EXPORT_SYMBOL_GPL(of_property_read_bool);
+
+/**
  * of_property_read_string - Find and read a string from a property
  * @np:		device node from which the property value is to be read.
  * @propname:	name of the property to be searched.
diff --git a/include/linux/of.h b/include/linux/of.h
index a75a831..2e5f77b 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -210,6 +210,8 @@ extern int of_property_read_u32_array(const struct device_node *np,
 				      size_t sz);
 extern int of_property_read_u64(const struct device_node *np,
 				const char *propname, u64 *out_value);
+extern int of_property_read_bool(const struct device_node *np,
+				const char *propname);
 
 extern int of_property_read_string(struct device_node *np,
 				   const char *propname,
@@ -288,6 +290,12 @@ static inline int of_property_read_u32_array(const struct device_node *np,
 	return -ENOSYS;
 }
 
+static inline int of_property_read_bool(const struct device_node *np,
+				const char *propname)
+{
+	return -ENOSYS;
+}
+
 static inline int of_property_read_string(struct device_node *np,
 					  const char *propname,
 					  const char **out_string)
-- 
1.7.7

[PATCH 1/1] of: introduce helper to manage boolean

[Rob Herring]

On 02/06/2012 10:13 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> of_property_read_bool
> 
> Search for a property in a device node and read a 32-bit value from
> it. Returns 0 if <0> or if the property does not exist, 1 if <1> or none.
> 
> this will allow to disable a boolean
> 
> is-ok;          => true
> is-ok = <1>;    => true
> is-ok = <0>;    => false
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: devicetree-discuss at lists.ozlabs.org

Acked-by: Rob Herring <rob.herring@calxeda.com>

> ---
> Hi,
> 
> 	if this is ok I'll rebase my mtd and i2c patch to use it
> 

I'm assuming you'll want to merge this patch with your mtd and i2c
patches. If you want me to apply, let me know.

Rob

> Best Regards,
> J.
>  drivers/of/base.c  |   30 ++++++++++++++++++++++++++++++
>  include/linux/of.h |    8 ++++++++
>  2 files changed, 38 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 133908a..a0eaf08 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -686,6 +686,36 @@ int of_property_read_u64(const struct device_node *np, const char *propname,
>  EXPORT_SYMBOL_GPL(of_property_read_u64);
>  
>  /**
> + * of_property_read_bool - Find and read a boolean from a property
> + * @np:		device node from which the property value is to be read.
> + * @propname:	name of the property to be searched.
> + *
> + * Search for a property in a device node and read a 32-bit value from
> + * it. Returns 0 if <0> or if the property does not exist, 1 if <1> or none.
> + *
> + * is-ok;	=> true
> + * is-ok = <1>;	=> true
> + * is-ok = <0>;	=> false
> + */
> +int of_property_read_bool(const struct device_node *np, const char *propname)
> +{
> +	u32 reg;
> +	int ret = of_property_read_u32(np, propname, &reg);
> +
> +	switch (ret) {
> +	case -EINVAL:
> +		return false;
> +	case -ENODATA:
> +		return true;
> +	case 0:
> +		return reg == 1;
> +	default:
> +		return ret;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(of_property_read_bool);
> +
> +/**
>   * of_property_read_string - Find and read a string from a property
>   * @np:		device node from which the property value is to be read.
>   * @propname:	name of the property to be searched.
> diff --git a/include/linux/of.h b/include/linux/of.h
> index a75a831..2e5f77b 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -210,6 +210,8 @@ extern int of_property_read_u32_array(const struct device_node *np,
>  				      size_t sz);
>  extern int of_property_read_u64(const struct device_node *np,
>  				const char *propname, u64 *out_value);
> +extern int of_property_read_bool(const struct device_node *np,
> +				const char *propname);
>  
>  extern int of_property_read_string(struct device_node *np,
>  				   const char *propname,
> @@ -288,6 +290,12 @@ static inline int of_property_read_u32_array(const struct device_node *np,
>  	return -ENOSYS;
>  }
>  
> +static inline int of_property_read_bool(const struct device_node *np,
> +				const char *propname)
> +{
> +	return -ENOSYS;
> +}
> +
>  static inline int of_property_read_string(struct device_node *np,
>  					  const char *propname,
>  					  const char **out_string)

[PATCH 1/1] of: introduce helper to manage boolean

[Jean-Christophe PLAGNIOL-VILLARD]

On 09:13 Tue 07 Feb     , Rob Herring wrote:
> On 02/06/2012 10:13 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > of_property_read_bool
> > 
> > Search for a property in a device node and read a 32-bit value from
> > it. Returns 0 if <0> or if the property does not exist, 1 if <1> or none.
> > 
> > this will allow to disable a boolean
> > 
> > is-ok;          => true
> > is-ok = <1>;    => true
> > is-ok = <0>;    => false
> > 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > Cc: devicetree-discuss at lists.ozlabs.org
> 
> Acked-by: Rob Herring <rob.herring@calxeda.com>
> 
> > ---
> > Hi,
> > 
> > 	if this is ok I'll rebase my mtd and i2c patch to use it
> > 
> 
> I'm assuming you'll want to merge this patch with your mtd and i2c
> patches. If you want me to apply, let me know.
yes please

I rebase the of_mtd over it

if you can apply it too
Acked-by Grant

Best Regards,
J.
> 
> Rob
> 
> > Best Regards,
> > J.
> >  drivers/of/base.c  |   30 ++++++++++++++++++++++++++++++
> >  include/linux/of.h |    8 ++++++++
> >  2 files changed, 38 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index 133908a..a0eaf08 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -686,6 +686,36 @@ int of_property_read_u64(const struct device_node *np, const char *propname,
> >  EXPORT_SYMBOL_GPL(of_property_read_u64);
> >  
> >  /**
> > + * of_property_read_bool - Find and read a boolean from a property
> > + * @np:		device node from which the property value is to be read.
> > + * @propname:	name of the property to be searched.
> > + *
> > + * Search for a property in a device node and read a 32-bit value from
> > + * it. Returns 0 if <0> or if the property does not exist, 1 if <1> or none.
> > + *
> > + * is-ok;	=> true
> > + * is-ok = <1>;	=> true
> > + * is-ok = <0>;	=> false
> > + */
> > +int of_property_read_bool(const struct device_node *np, const char *propname)
> > +{
> > +	u32 reg;
> > +	int ret = of_property_read_u32(np, propname, &reg);
> > +
> > +	switch (ret) {
> > +	case -EINVAL:
> > +		return false;
> > +	case -ENODATA:
> > +		return true;
> > +	case 0:
> > +		return reg == 1;
> > +	default:
> > +		return ret;
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(of_property_read_bool);
> > +
> > +/**
> >   * of_property_read_string - Find and read a string from a property
> >   * @np:		device node from which the property value is to be read.
> >   * @propname:	name of the property to be searched.
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index a75a831..2e5f77b 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -210,6 +210,8 @@ extern int of_property_read_u32_array(const struct device_node *np,
> >  				      size_t sz);
> >  extern int of_property_read_u64(const struct device_node *np,
> >  				const char *propname, u64 *out_value);
> > +extern int of_property_read_bool(const struct device_node *np,
> > +				const char *propname);
> >  
> >  extern int of_property_read_string(struct device_node *np,
> >  				   const char *propname,
> > @@ -288,6 +290,12 @@ static inline int of_property_read_u32_array(const struct device_node *np,
> >  	return -ENOSYS;
> >  }
> >  
> > +static inline int of_property_read_bool(const struct device_node *np,
> > +				const char *propname)
> > +{
> > +	return -ENOSYS;
> > +}
> > +
> >  static inline int of_property_read_string(struct device_node *np,
> >  					  const char *propname,
> >  					  const char **out_string)

[PATCH 1/1] of: introduce helper to manage boolean

[Grant Likely]

On Tue,  7 Feb 2012 05:13:48 +0100, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> of_property_read_bool
> 
> Search for a property in a device node and read a 32-bit value from
> it. Returns 0 if <0> or if the property does not exist, 1 if <1> or none.
> 
> this will allow to disable a boolean
> 
> is-ok;          => true
> is-ok = <1>;    => true
> is-ok = <0>;    => false
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: devicetree-discuss at lists.ozlabs.org
> ---
> Hi,
> 
> 	if this is ok I'll rebase my mtd and i2c patch to use it
> 
> Best Regards,
> J.
>  drivers/of/base.c  |   30 ++++++++++++++++++++++++++++++
>  include/linux/of.h |    8 ++++++++
>  2 files changed, 38 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 133908a..a0eaf08 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -686,6 +686,36 @@ int of_property_read_u64(const struct device_node *np, const char *propname,
>  EXPORT_SYMBOL_GPL(of_property_read_u64);
>  
>  /**
> + * of_property_read_bool - Find and read a boolean from a property
> + * @np:		device node from which the property value is to be read.
> + * @propname:	name of the property to be searched.
> + *
> + * Search for a property in a device node and read a 32-bit value from
> + * it. Returns 0 if <0> or if the property does not exist, 1 if <1> or none.
> + *
> + * is-ok;	=> true
> + * is-ok = <1>;	=> true
> + * is-ok = <0>;	=> false
> + */
> +int of_property_read_bool(const struct device_node *np, const char *propname)
> +{
> +	u32 reg;
> +	int ret = of_property_read_u32(np, propname, &reg);
> +
> +	switch (ret) {
> +	case -EINVAL:
> +		return false;
> +	case -ENODATA:
> +		return true;
> +	case 0:
> +		return reg == 1;

Ugh.  so any value other than 1 returns false?  I think that will surprise
most people.

I don't like this api or binding.  If it is a bool property, then why isn't
simply testing for the property existance sufficient?

g.

[PATCH 1/1] of: introduce helper to manage boolean

[Jean-Christophe PLAGNIOL-VILLARD]

On 18:44 Thu 08 Mar     , Grant Likely wrote:
> On Tue,  7 Feb 2012 05:13:48 +0100, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> > of_property_read_bool
> > 
> > Search for a property in a device node and read a 32-bit value from
> > it. Returns 0 if <0> or if the property does not exist, 1 if <1> or none.
> > 
> > this will allow to disable a boolean
> > 
> > is-ok;          => true
> > is-ok = <1>;    => true
> > is-ok = <0>;    => false
> > 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > Cc: devicetree-discuss at lists.ozlabs.org
> > ---
> > Hi,
> > 
> > 	if this is ok I'll rebase my mtd and i2c patch to use it
> > 
> > Best Regards,
> > J.
> >  drivers/of/base.c  |   30 ++++++++++++++++++++++++++++++
> >  include/linux/of.h |    8 ++++++++
> >  2 files changed, 38 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index 133908a..a0eaf08 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -686,6 +686,36 @@ int of_property_read_u64(const struct device_node *np, const char *propname,
> >  EXPORT_SYMBOL_GPL(of_property_read_u64);
> >  
> >  /**
> > + * of_property_read_bool - Find and read a boolean from a property
> > + * @np:		device node from which the property value is to be read.
> > + * @propname:	name of the property to be searched.
> > + *
> > + * Search for a property in a device node and read a 32-bit value from
> > + * it. Returns 0 if <0> or if the property does not exist, 1 if <1> or none.
> > + *
> > + * is-ok;	=> true
> > + * is-ok = <1>;	=> true
> > + * is-ok = <0>;	=> false
> > + */
> > +int of_property_read_bool(const struct device_node *np, const char *propname)
> > +{
> > +	u32 reg;
> > +	int ret = of_property_read_u32(np, propname, &reg);
> > +
> > +	switch (ret) {
> > +	case -EINVAL:
> > +		return false;
> > +	case -ENODATA:
> > +		return true;
> > +	case 0:
> > +		return reg == 1;
> 
> Ugh.  so any value other than 1 returns false?  I think that will surprise
> most people.
> 
> I don't like this api or binding.  If it is a bool property, then why isn't
> simply testing for the property existance sufficient?
no if you want to disable it

if a bool is define in the dtsi and want to disable it int the dts

if you we can do the the invert

if !0 => true

is-ok;			=> true
is-ok = <val != 0>;	=> true
is-ok = <0>;		=> false

Best Regards,
J.

[PATCH 1/1] of: introduce helper to manage boolean

[Grant Likely]

On Fri, 9 Mar 2012 11:04:35 +0100, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> On 18:44 Thu 08 Mar     , Grant Likely wrote:
> > On Tue,  7 Feb 2012 05:13:48 +0100, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> > > of_property_read_bool
> > > 
> > > Search for a property in a device node and read a 32-bit value from
> > > it. Returns 0 if <0> or if the property does not exist, 1 if <1> or none.
> > > 
> > > this will allow to disable a boolean
> > > 
> > > is-ok;          => true
> > > is-ok = <1>;    => true
> > > is-ok = <0>;    => false
> > > 
> > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > > Cc: devicetree-discuss at lists.ozlabs.org
> > > ---
> > > Hi,
> > > 
> > > 	if this is ok I'll rebase my mtd and i2c patch to use it
> > > 
> > > Best Regards,
> > > J.
> > >  drivers/of/base.c  |   30 ++++++++++++++++++++++++++++++
> > >  include/linux/of.h |    8 ++++++++
> > >  2 files changed, 38 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > > index 133908a..a0eaf08 100644
> > > --- a/drivers/of/base.c
> > > +++ b/drivers/of/base.c
> > > @@ -686,6 +686,36 @@ int of_property_read_u64(const struct device_node *np, const char *propname,
> > >  EXPORT_SYMBOL_GPL(of_property_read_u64);
> > >  
> > >  /**
> > > + * of_property_read_bool - Find and read a boolean from a property
> > > + * @np:		device node from which the property value is to be read.
> > > + * @propname:	name of the property to be searched.
> > > + *
> > > + * Search for a property in a device node and read a 32-bit value from
> > > + * it. Returns 0 if <0> or if the property does not exist, 1 if <1> or none.
> > > + *
> > > + * is-ok;	=> true
> > > + * is-ok = <1>;	=> true
> > > + * is-ok = <0>;	=> false
> > > + */
> > > +int of_property_read_bool(const struct device_node *np, const char *propname)
> > > +{
> > > +	u32 reg;
> > > +	int ret = of_property_read_u32(np, propname, &reg);
> > > +
> > > +	switch (ret) {
> > > +	case -EINVAL:
> > > +		return false;
> > > +	case -ENODATA:
> > > +		return true;
> > > +	case 0:
> > > +		return reg == 1;
> > 
> > Ugh.  so any value other than 1 returns false?  I think that will surprise
> > most people.
> > 
> > I don't like this api or binding.  If it is a bool property, then why isn't
> > simply testing for the property existance sufficient?
> no if you want to disable it
> 
> if a bool is define in the dtsi and want to disable it int the dts
> 
> if you we can do the the invert
> 
> if !0 => true
> 
> is-ok;			=> true
> is-ok = <val != 0>;	=> true
> is-ok = <0>;		=> false

This is a failure of the dtc tool, not the binding.  Accepting this binding
means we have to live with it for a very long time.  It needs to be fixed
in dtc instead so that properties can be deleted instead of only modified.

g.

[PATCH 1/1] of: introduce helper to manage boolean

[Jean-Christophe PLAGNIOL-VILLARD]

> > > > +int of_property_read_bool(const struct device_node *np, const char *propname)
> > > > +{
> > > > +	u32 reg;
> > > > +	int ret = of_property_read_u32(np, propname, &reg);
> > > > +
> > > > +	switch (ret) {
> > > > +	case -EINVAL:
> > > > +		return false;
> > > > +	case -ENODATA:
> > > > +		return true;
> > > > +	case 0:
> > > > +		return reg == 1;
> > > 
> > > Ugh.  so any value other than 1 returns false?  I think that will surprise
> > > most people.
> > > 
> > > I don't like this api or binding.  If it is a bool property, then why isn't
> > > simply testing for the property existance sufficient?
> > no if you want to disable it
> > 
> > if a bool is define in the dtsi and want to disable it int the dts
> > 
> > if you we can do the the invert
> > 
> > if !0 => true
> > 
> > is-ok;			=> true
> > is-ok = <val != 0>;	=> true
> > is-ok = <0>;		=> false
> 
> This is a failure of the dtc tool, not the binding.  Accepting this binding
> means we have to live with it for a very long time.  It needs to be fixed
> in dtc instead so that properties can be deleted instead of only modified.
I understand your idea but today if you put and value in the property it's true.

So is-ok = <0>; is true also which is illogical as in any language a boolean is
true (1) or false (0). When I read the property I will understand false not true

And I see no good way to add the "delete" in dtc.

Best Regards,
J.

[PATCH 1/1] of: introduce helper to manage boolean

[Scott Wood]

On 03/09/2012 10:36 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>> Ugh.  so any value other than 1 returns false?  I think that will surprise
>>>> most people.
>>>>
>>>> I don't like this api or binding.  If it is a bool property, then why isn't
>>>> simply testing for the property existance sufficient?
>>> no if you want to disable it
>>>
>>> if a bool is define in the dtsi and want to disable it int the dts
>>>
>>> if you we can do the the invert
>>>
>>> if !0 => true
>>>
>>> is-ok;			=> true
>>> is-ok = <val != 0>;	=> true
>>> is-ok = <0>;		=> false
>>
>> This is a failure of the dtc tool, not the binding.  Accepting this binding
>> means we have to live with it for a very long time.  It needs to be fixed
>> in dtc instead so that properties can be deleted instead of only modified.
> I understand your idea but today if you put and value in the property it's true.
> 
> So is-ok = <0>; is true also which is illogical as in any language a boolean is
> true (1) or false (0). When I read the property I will understand false not true

You could say similar things about is-ok = "no" or is-ok = "" or is-ok =
"I'd rather you didn't"... it's expected that violating the binding may
produce illogical results.

> And I see no good way to add the "delete" in dtc.

It shouldn't be that hard, just needs a new keyword and/or bit of syntax
to express what you want it to do.

-Scott

[PATCH 1/1] of: introduce helper to manage boolean

[Jean-Christophe PLAGNIOL-VILLARD]

On 14:39 Mon 12 Mar     , Scott Wood wrote:
> On 03/09/2012 10:36 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>>> Ugh.  so any value other than 1 returns false?  I think that will surprise
> >>>> most people.
> >>>>
> >>>> I don't like this api or binding.  If it is a bool property, then why isn't
> >>>> simply testing for the property existance sufficient?
> >>> no if you want to disable it
> >>>
> >>> if a bool is define in the dtsi and want to disable it int the dts
> >>>
> >>> if you we can do the the invert
> >>>
> >>> if !0 => true
> >>>
> >>> is-ok;			=> true
> >>> is-ok = <val != 0>;	=> true
> >>> is-ok = <0>;		=> false
> >>
> >> This is a failure of the dtc tool, not the binding.  Accepting this binding
> >> means we have to live with it for a very long time.  It needs to be fixed
> >> in dtc instead so that properties can be deleted instead of only modified.
> > I understand your idea but today if you put and value in the property it's true.
> > 
> > So is-ok = <0>; is true also which is illogical as in any language a boolean is
> > true (1) or false (0). When I read the property I will understand false not true
> 
> You could say similar things about is-ok = "no" or is-ok = "" or is-ok =
> "I'd rather you didn't"... it's expected that violating the binding may
> produce illogical results.
today is most of the binding people use a number whe the want to be able to
delete it and it's the same in most of the promgramming language

Best Regards,
J.

[PATCH 1/1] of: introduce helper to manage boolean

[Grant Likely]

On Tue, 13 Mar 2012 04:17:39 +0100, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> On 14:39 Mon 12 Mar     , Scott Wood wrote:
> > On 03/09/2012 10:36 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > >>>> Ugh.  so any value other than 1 returns false?  I think that will surprise
> > >>>> most people.
> > >>>>
> > >>>> I don't like this api or binding.  If it is a bool property, then why isn't
> > >>>> simply testing for the property existance sufficient?
> > >>> no if you want to disable it
> > >>>
> > >>> if a bool is define in the dtsi and want to disable it int the dts
> > >>>
> > >>> if you we can do the the invert
> > >>>
> > >>> if !0 => true
> > >>>
> > >>> is-ok;			=> true
> > >>> is-ok = <val != 0>;	=> true
> > >>> is-ok = <0>;		=> false
> > >>
> > >> This is a failure of the dtc tool, not the binding.  Accepting this binding
> > >> means we have to live with it for a very long time.  It needs to be fixed
> > >> in dtc instead so that properties can be deleted instead of only modified.
> > > I understand your idea but today if you put and value in the property it's true.
> > > 
> > > So is-ok = <0>; is true also which is illogical as in any language a boolean is
> > > true (1) or false (0). When I read the property I will understand false not true
> > 
> > You could say similar things about is-ok = "no" or is-ok = "" or is-ok =
> > "I'd rather you didn't"... it's expected that violating the binding may
> > produce illogical results.
> today is most of the binding people use a number whe the want to be able to
> delete it and it's the same in most of the promgramming language

It isn't yet a big pain point, so there isn't time pressure here.  Fixing the tool
is the better solution, and since the kernel carries a copy of the tool we don't
need to worry about adding a feature that isn't available by the dtc packaged by
a distribution.

Fixing the tool is the correct thing to do.

g.

[PATCH 1/1] of: introduce helper to manage boolean

[Jean-Christophe PLAGNIOL-VILLARD]

On 22:16 Mon 12 Mar     , Grant Likely wrote:
> On Tue, 13 Mar 2012 04:17:39 +0100, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> > On 14:39 Mon 12 Mar     , Scott Wood wrote:
> > > On 03/09/2012 10:36 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > >>>> Ugh.  so any value other than 1 returns false?  I think that will surprise
> > > >>>> most people.
> > > >>>>
> > > >>>> I don't like this api or binding.  If it is a bool property, then why isn't
> > > >>>> simply testing for the property existance sufficient?
> > > >>> no if you want to disable it
> > > >>>
> > > >>> if a bool is define in the dtsi and want to disable it int the dts
> > > >>>
> > > >>> if you we can do the the invert
> > > >>>
> > > >>> if !0 => true
> > > >>>
> > > >>> is-ok;			=> true
> > > >>> is-ok = <val != 0>;	=> true
> > > >>> is-ok = <0>;		=> false
> > > >>
> > > >> This is a failure of the dtc tool, not the binding.  Accepting this binding
> > > >> means we have to live with it for a very long time.  It needs to be fixed
> > > >> in dtc instead so that properties can be deleted instead of only modified.
> > > > I understand your idea but today if you put and value in the property it's true.
> > > > 
> > > > So is-ok = <0>; is true also which is illogical as in any language a boolean is
> > > > true (1) or false (0). When I read the property I will understand false not true
> > > 
> > > You could say similar things about is-ok = "no" or is-ok = "" or is-ok =
> > > "I'd rather you didn't"... it's expected that violating the binding may
> > > produce illogical results.
> > today is most of the binding people use a number whe the want to be able to
> > delete it and it's the same in most of the promgramming language
> 
> It isn't yet a big pain point, so there isn't time pressure here.  Fixing the tool
> is the better solution, and since the kernel carries a copy of the tool we don't
> need to worry about adding a feature that isn't available by the dtc packaged by
> a distribution.
> 
> Fixing the tool is the correct thing to do.
I don't like it but ok

Can I get the ack on it and apply via my tree I've more tan 10 patches pending
because of this

Best Regards,
J.

[PATCH 1/1] of: introduce helper to manage boolean

[Simon Glass]

Hi,

On Tue, Mar 13, 2012 at 5:16 AM, Grant Likely <grant.likely@secretlab.ca>wrote:

> On Tue, 13 Mar 2012 04:17:39 +0100, Jean-Christophe PLAGNIOL-VILLARD <
> plagnioj at jcrosoft.com> wrote:
> > On 14:39 Mon 12 Mar     , Scott Wood wrote:
> > > On 03/09/2012 10:36 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > >>>> Ugh.  so any value other than 1 returns false?  I think that will
> surprise
> > > >>>> most people.
> > > >>>>
> > > >>>> I don't like this api or binding.  If it is a bool property, then
> why isn't
> > > >>>> simply testing for the property existance sufficient?
> > > >>> no if you want to disable it
> > > >>>
> > > >>> if a bool is define in the dtsi and want to disable it int the dts
> > > >>>
> > > >>> if you we can do the the invert
> > > >>>
> > > >>> if !0 => true
> > > >>>
> > > >>> is-ok;                  => true
> > > >>> is-ok = <val != 0>;     => true
> > > >>> is-ok = <0>;            => false
> > > >>
> > > >> This is a failure of the dtc tool, not the binding.  Accepting this
> binding
> > > >> means we have to live with it for a very long time.  It needs to be
> fixed
> > > >> in dtc instead so that properties can be deleted instead of only
> modified.
> > > > I understand your idea but today if you put and value in the
> property it's true.
> > > >
> > > > So is-ok = <0>; is true also which is illogical as in any language a
> boolean is
> > > > true (1) or false (0). When I read the property I will understand
> false not true
> > >
> > > You could say similar things about is-ok = "no" or is-ok = "" or is-ok
> =
> > > "I'd rather you didn't"... it's expected that violating the binding may
> > > produce illogical results.
> > today is most of the binding people use a number whe the want to be able
> to
> > delete it and it's the same in most of the promgramming language
>
> It isn't yet a big pain point, so there isn't time pressure here.  Fixing
> the tool
> is the better solution, and since the kernel carries a copy of the tool we
> don't
> need to worry about adding a feature that isn't available by the dtc
> packaged by
> a distribution.
>
> Fixing the tool is the correct thing to do.
>

I just wanted to pick up on this thread. Now in the kernel, absence of a
property indicates false, and presence indicates true. This is a kernel
convention, nothing to do with the dtb format. I think it is useful to
support a boolean with a non-null value which can be 0, meaning true as
Jean-Christophe says.

My reasons are:

1. dtc does not have a way to delete a property and it isn't clear what
syntax could be used there. It also seems a little brittle to delete a
property in one place and define it in another (ordering? confusion when
people can't work out why it has gone?). So not only can dtc not do this,
but I'm not sure that it should.

2. fdtput allows updating a property. It is pretty easy to do something
like 'fdtput file.dtb /node propname $value' where value is 0 or 1. To use
the current binding we need to either do:

if [[ $value == 0 ]]; then
   fdtput -d file.dtb /node propname   # new function to delete a property?
else
   fdtput file.dtb /node propname 1
fi

or perhaps something like:

   fdtput -B file.dtb /node propname ${value}

where -B is a new option meaning either create an empty property, or delete
any property it finds, based on the value being 0 or non-zero.

3. Discoverability: it is nice to be able to see the possible options, even
if disabled. In particular, if a boolean value is true, and you later
decide with fdtput to turn it off, you effectively remove all trace of it.
This could be confusing for those who are looking for available options.

Arguably these issues could be fixed if we introduced some sort of type
hinting into the dtb file format, but that seems a long shot. I am not sure
that modifying the tools to support this arguable odd 'present/absent'
boolean binding is the right approach.

So I think Jean-Christophe's idea has significant merit.

Thoughts?

Regards,
Simon

>
> g.
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120710/80a0c4e9/attachment-0001.html>

[PATCH 1/1] of: introduce helper to manage boolean

[Scott Wood]

On 07/10/2012 07:10 AM, Simon Glass wrote:
> Hi,
> 
> On Tue, Mar 13, 2012 at 5:16 AM, Grant Likely <grant.likely@secretlab.ca
> <mailto:grant.likely@secretlab.ca>> wrote:
> 
>     On Tue, 13 Mar 2012 04:17:39 +0100, Jean-Christophe PLAGNIOL-VILLARD
>     <plagnioj at jcrosoft.com <mailto:plagnioj@jcrosoft.com>> wrote:
>     > On 14:39 Mon 12 Mar     , Scott Wood wrote:
>     > > On 03/09/2012 10:36 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>     > > >>>> Ugh.  so any value other than 1 returns false?  I think
>     that will surprise
>     > > >>>> most people.
>     > > >>>>
>     > > >>>> I don't like this api or binding.  If it is a bool
>     property, then why isn't
>     > > >>>> simply testing for the property existance sufficient?
>     > > >>> no if you want to disable it
>     > > >>>
>     > > >>> if a bool is define in the dtsi and want to disable it int
>     the dts
>     > > >>>
>     > > >>> if you we can do the the invert
>     > > >>>
>     > > >>> if !0 => true
>     > > >>>
>     > > >>> is-ok;                  => true
>     > > >>> is-ok = <val != 0>;     => true
>     > > >>> is-ok = <0>;            => false
>     > > >>
>     > > >> This is a failure of the dtc tool, not the binding.
>      Accepting this binding
>     > > >> means we have to live with it for a very long time.  It needs
>     to be fixed
>     > > >> in dtc instead so that properties can be deleted instead of
>     only modified.
>     > > > I understand your idea but today if you put and value in the
>     property it's true.
>     > > >
>     > > > So is-ok = <0>; is true also which is illogical as in any
>     language a boolean is
>     > > > true (1) or false (0). When I read the property I will
>     understand false not true
>     > >
>     > > You could say similar things about is-ok = "no" or is-ok = "" or
>     is-ok =
>     > > "I'd rather you didn't"... it's expected that violating the
>     binding may
>     > > produce illogical results.
>     > today is most of the binding people use a number whe the want to
>     be able to
>     > delete it and it's the same in most of the promgramming language
> 
>     It isn't yet a big pain point, so there isn't time pressure here.
>      Fixing the tool
>     is the better solution, and since the kernel carries a copy of the
>     tool we don't
>     need to worry about adding a feature that isn't available by the dtc
>     packaged by
>     a distribution.
> 
>     Fixing the tool is the correct thing to do.
> 
> 
> I just wanted to pick up on this thread. Now in the kernel, absence of a
> property indicates false, and presence indicates true. This is a kernel
> convention, nothing to do with the dtb format.

It's an aspect of the device binding, and was used by Open Firmware
bindings before Linux came up with flat device trees.

Also note that even non-boolean properties can mean something different
when absent.  Sometimes this is a default value; sometimes, like with
"ranges", it's something that can't be expressed with any value (empty
ranges means all translations go straight through; no ranges means no
translation is possible).

> I think it is useful to
> support a boolean with a non-null value which can be 0, meaning true as
> Jean-Christophe says.
> 
> My reasons are:
> 
> 1. dtc does not have a way to delete a property and it isn't clear what
> syntax could be used there.

Surely some syntax can be created for this.  E.g. /delete-prop/ foo;

> It also seems a little brittle to delete a
> property in one place and define it in another (ordering? confusion when
> people can't work out why it has gone?). So not only can dtc not do
> this, but I'm not sure that it should.

I don't see how it's worse than defining it one place and then
redefining it elsewhere.

> 3. Discoverability: it is nice to be able to see the possible options,
> even if disabled. 

This assumes the possible options were known in advance, or that you
don't maintain compatibility with old device trees when a new option is
added.

> In particular, if a boolean value is true, and you
> later decide with fdtput to turn it off, you effectively remove all
> trace of it. This could be confusing for those who are looking for
> available options.

There should still be a "trace of it" in the binding document.

-Scott

[PATCH 1/1] of: introduce helper to manage boolean

[Simon Glass]

Hi Scott,

On Tue, Jul 10, 2012 at 11:23 PM, Scott Wood <scottwood@freescale.com>wrote:

> On 07/10/2012 07:10 AM, Simon Glass wrote:
> > Hi,
> >
> > On Tue, Mar 13, 2012 at 5:16 AM, Grant Likely <grant.likely@secretlab.ca
> > <mailto:grant.likely@secretlab.ca>> wrote:
> >
> >     On Tue, 13 Mar 2012 04:17:39 +0100, Jean-Christophe PLAGNIOL-VILLARD
> >     <plagnioj at jcrosoft.com <mailto:plagnioj@jcrosoft.com>> wrote:
> >     > On 14:39 Mon 12 Mar     , Scott Wood wrote:
> >     > > On 03/09/2012 10:36 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >     > > >>>> Ugh.  so any value other than 1 returns false?  I think
> >     that will surprise
> >     > > >>>> most people.
> >     > > >>>>
> >     > > >>>> I don't like this api or binding.  If it is a bool
> >     property, then why isn't
> >     > > >>>> simply testing for the property existance sufficient?
> >     > > >>> no if you want to disable it
> >     > > >>>
> >     > > >>> if a bool is define in the dtsi and want to disable it int
> >     the dts
> >     > > >>>
> >     > > >>> if you we can do the the invert
> >     > > >>>
> >     > > >>> if !0 => true
> >     > > >>>
> >     > > >>> is-ok;                  => true
> >     > > >>> is-ok = <val != 0>;     => true
> >     > > >>> is-ok = <0>;            => false
> >     > > >>
> >     > > >> This is a failure of the dtc tool, not the binding.
> >      Accepting this binding
> >     > > >> means we have to live with it for a very long time.  It needs
> >     to be fixed
> >     > > >> in dtc instead so that properties can be deleted instead of
> >     only modified.
> >     > > > I understand your idea but today if you put and value in the
> >     property it's true.
> >     > > >
> >     > > > So is-ok = <0>; is true also which is illogical as in any
> >     language a boolean is
> >     > > > true (1) or false (0). When I read the property I will
> >     understand false not true
> >     > >
> >     > > You could say similar things about is-ok = "no" or is-ok = "" or
> >     is-ok =
> >     > > "I'd rather you didn't"... it's expected that violating the
> >     binding may
> >     > > produce illogical results.
> >     > today is most of the binding people use a number whe the want to
> >     be able to
> >     > delete it and it's the same in most of the promgramming language
> >
> >     It isn't yet a big pain point, so there isn't time pressure here.
> >      Fixing the tool
> >     is the better solution, and since the kernel carries a copy of the
> >     tool we don't
> >     need to worry about adding a feature that isn't available by the dtc
> >     packaged by
> >     a distribution.
> >
> >     Fixing the tool is the correct thing to do.
> >
> >
> > I just wanted to pick up on this thread. Now in the kernel, absence of a
> > property indicates false, and presence indicates true. This is a kernel
> > convention, nothing to do with the dtb format.
>
> It's an aspect of the device binding, and was used by Open Firmware
> bindings before Linux came up with flat device trees.
>

OK. When I asked about the boolean property on the devicetree-discuss I
understood that no one really cared, and it was a matter for Linux.


>
> Also note that even non-boolean properties can mean something different
> when absent.  Sometimes this is a default value; sometimes, like with
> "ranges", it's something that can't be expressed with any value (empty
> ranges means all translations go straight through; no ranges means no
> translation is possible).
>

That's fine, but I'm not sure I understand why that relates to boolean
properties, which currently mean always the same thing (false) when absent.
I don't think there is any intent to change that.


> > I think it is useful to
> > support a boolean with a non-null value which can be 0, meaning true as
> > Jean-Christophe says.
> >
> > My reasons are:
> >
> > 1. dtc does not have a way to delete a property and it isn't clear what
> > syntax could be used there.
>
> Surely some syntax can be created for this.  E.g. /delete-prop/ foo;
>

Yes, in fact I saw a patch after sending this email. So for normal values
to change the value we do

   prop = <23>;

but for booleans we must do EITHER:

   bool;

or

   /delete-prop/ bool;

depending on whether we want the make it true or false? Ick.


>
> > It also seems a little brittle to delete a
> > property in one place and define it in another (ordering? confusion when
> > people can't work out why it has gone?). So not only can dtc not do
> > this, but I'm not sure that it should.
>
> I don't see how it's worse than defining it one place and then
> redefining it elsewhere.
>

It seems worse to me, see above. Also if we end up with symbols it will be
impossible to do something like:

   bool-property = <WIBBLE_VALUE>;

You will have to do:

if WIBBLE_VALUE == 0
  /delete-prop/ bool-property;
#else
  bool-property;
#endif

or whatever, or maybe I got that around the wrong way.... Not nice IMO.

(any comments on point 2?)


>
> > 3. Discoverability: it is nice to be able to see the possible options,
> > even if disabled.
>
> This assumes the possible options were known in advance, or that you
> don't maintain compatibility with old device trees when a new option is
> added.
>

You can still add the option with a zero value - or maybe I misunderstand
what you mean.

>
> > In particular, if a boolean value is true, and you
> > later decide with fdtput to turn it off, you effectively remove all
> > trace of it. This could be confusing for those who are looking for
> > available options.
>
> There should still be a "trace of it" in the binding document.
>

Yes true, that's the saving grace.

I'm not even saying that we ever use

   bool-prop = <1>;

Clearly that is redundant. I'm just agreeing with Jean-Chrstophe that it
would be nice and clean to do something like:

   bool-prop = <0>;

and it will make it false.

Regards,
Simon


>
> -Scott
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120711/ae4b5d65/attachment-0001.html>

[PATCH 1/1] of: introduce helper to manage boolean

[Scott Wood]

On 07/10/2012 05:53 PM, Simon Glass wrote:
> Hi Scott,
> 
> On Tue, Jul 10, 2012 at 11:23 PM, Scott Wood <scottwood@freescale.com
> <mailto:scottwood@freescale.com>> wrote:
> 
>     Also note that even non-boolean properties can mean something different
>     when absent.  Sometimes this is a default value; sometimes, like with
>     "ranges", it's something that can't be expressed with any value (empty
>     ranges means all translations go straight through; no ranges means no
>     translation is possible).
> 
> 
> That's fine, but I'm not sure I understand why that relates to boolean
> properties, which currently mean always the same thing (false) when
> absent. I don't think there is any intent to change that.

The point was this isn't the only scenario where the absence of a
property means something, and where you might want to delete the property.

>     > I think it is useful to
>     > support a boolean with a non-null value which can be 0, meaning
>     true as
>     > Jean-Christophe says.
>     >
>     > My reasons are:
>     >
>     > 1. dtc does not have a way to delete a property and it isn't clear
>     what
>     > syntax could be used there.
> 
>     Surely some syntax can be created for this.  E.g. /delete-prop/ foo;
> 
> 
> Yes, in fact I saw a patch after sending this email. So for normal
> values to change the value we do
> 
>    prop = <23>;
> 
> but for booleans we must do EITHER:
> 
>    bool;
> 
> or
> 
>    /delete-prop/ bool;
> 
> depending on whether we want the make it true or false? Ick.

Doesn't seem that bad to me except in the case you show below where
you're trying to do it programmatically.

> It seems worse to me, see above. Also if we end up with symbols it will
*> be impossible to do something like:
> 
>    bool-property = <WIBBLE_VALUE>;
> 
> You will have to do:
> 
> if WIBBLE_VALUE == 0
>   /delete-prop/ bool-property;
> #else
>   bool-property;
> #endif
> 
> or whatever, or maybe I got that around the wrong way.... Not nice IMO.

Yeah, that's unpleasant.

I don't hugely object to a new boolean type for use in new bindings,
where <0> or absence can both be used to indicate false, as long as it's
clearly documented.  I'm hesitant to start doing this on existing
bindings, though, and in any case would like to see support for
property/node deletion in dtc.

> (any comments on point 2?)

It's basically the same as the above, right?

>     > 3. Discoverability: it is nice to be able to see the possible options,
>     > even if disabled.
> 
>     This assumes the possible options were known in advance, or that you
>     don't maintain compatibility with old device trees when a new option is
>     added.
> 
> 
> You can still add the option with a zero value - or maybe I
> misunderstand what you mean.

We normally want to work with existing device trees (which could be
partially produced by firmware, so changing can be unpleasant), so the
absence of the property is still going to be a possibility.  Plus
enumerating every possibility, no matter how rarely used, in every node
of a certain type seems excessively verbose.  It's not like these are
user configuration knobs.

-Scott

[PATCH 1/1] of: introduce helper to manage boolean

[Simon Glass]

Hi Scott,

On Wed, Jul 11, 2012 at 1:11 AM, Scott Wood <scottwood@freescale.com> wrote:
> On 07/10/2012 05:53 PM, Simon Glass wrote:
>> Hi Scott,
>>
>> On Tue, Jul 10, 2012 at 11:23 PM, Scott Wood <scottwood@freescale.com
>> <mailto:scottwood@freescale.com>> wrote:
>>
>>     Also note that even non-boolean properties can mean something different
>>     when absent.  Sometimes this is a default value; sometimes, like with
>>     "ranges", it's something that can't be expressed with any value (empty
>>     ranges means all translations go straight through; no ranges means no
>>     translation is possible).
>>
>>
>> That's fine, but I'm not sure I understand why that relates to boolean
>> properties, which currently mean always the same thing (false) when
>> absent. I don't think there is any intent to change that.
>
> The point was this isn't the only scenario where the absence of a
> property means something, and where you might want to delete the property.

OK I see. We don't have that option for booleans, since absence will
always mean false.

>
>>     > I think it is useful to
>>     > support a boolean with a non-null value which can be 0, meaning
>>     true as
>>     > Jean-Christophe says.
>>     >
>>     > My reasons are:
>>     >
>>     > 1. dtc does not have a way to delete a property and it isn't clear
>>     what
>>     > syntax could be used there.
>>
>>     Surely some syntax can be created for this.  E.g. /delete-prop/ foo;
>>
>>
>> Yes, in fact I saw a patch after sending this email. So for normal
>> values to change the value we do
>>
>>    prop = <23>;
>>
>> but for booleans we must do EITHER:
>>
>>    bool;
>>
>> or
>>
>>    /delete-prop/ bool;
>>
>> depending on whether we want the make it true or false? Ick.
>
> Doesn't seem that bad to me except in the case you show below where
> you're trying to do it programmatically.
>
>> It seems worse to me, see above. Also if we end up with symbols it will
> *> be impossible to do something like:
>>
>>    bool-property = <WIBBLE_VALUE>;
>>
>> You will have to do:
>>
>> if WIBBLE_VALUE == 0
>>   /delete-prop/ bool-property;
>> #else
>>   bool-property;
>> #endif
>>
>> or whatever, or maybe I got that around the wrong way.... Not nice IMO.
>
> Yeah, that's unpleasant.
>
> I don't hugely object to a new boolean type for use in new bindings,
> where <0> or absence can both be used to indicate false, as long as it's
> clearly documented.  I'm hesitant to start doing this on existing
> bindings, though, and in any case would like to see support for
> property/node deletion in dtc.

Fair enough. Jean-Christophe's patch certainly changes existing
behaviour, and without that it might be confusing - we don't want two
functions for reading boolean properties. The only plus is that
(perhaps) no one expects "bool-prop = <0>" to mean true, so it might
be safe.

>
>> (any comments on point 2?)
>
> It's basically the same as the above, right?

Yes, and you commented above.

>
>>     > 3. Discoverability: it is nice to be able to see the possible options,
>>     > even if disabled.
>>
>>     This assumes the possible options were known in advance, or that you
>>     don't maintain compatibility with old device trees when a new option is
>>     added.
>>
>>
>> You can still add the option with a zero value - or maybe I
>> misunderstand what you mean.
>
> We normally want to work with existing device trees (which could be
> partially produced by firmware, so changing can be unpleasant), so the
> absence of the property is still going to be a possibility.  Plus
> enumerating every possibility, no matter how rarely used, in every node
> of a certain type seems excessively verbose.  It's not like these are
> user configuration knobs.

Yes agreed. The point of this is just to make it easier (I think) to
make a boolean value false either in the source, or later using
fdtput.

So what do we need to do? Take a vote? Revisit Jean-Christophe's patch?

>
> -Scott
>

Regards,
Simon