[RFC/PATCH 1/2] of: move of_property_read_bool further down

[RFC/PATCH 1/2] of: move of_property_read_bool further down

[Felipe Balbi]

A follow-up patch will make use of of_property_read_u32()
to allow for boolean properties to have a value, this
is just in preparation for that patch in order to make
review easier.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 include/linux/of.h | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/include/linux/of.h b/include/linux/of.h
index dfde07e77a63..76c055b20fef 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -765,22 +765,6 @@ static inline int of_property_read_string_index(struct device_node *np,
 	return rc < 0 ? rc : 0;
 }
 
-/**
- * of_property_read_bool - Findfrom 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.
- * Returns true if the property exist false otherwise.
- */
-static inline bool of_property_read_bool(const struct device_node *np,
-					 const char *propname)
-{
-	struct property *prop = of_find_property(np, propname, NULL);
-
-	return prop ? true : false;
-}
-
 static inline int of_property_read_u8(const struct device_node *np,
 				       const char *propname,
 				       u8 *out_value)
@@ -802,6 +786,22 @@ static inline int of_property_read_u32(const struct device_node *np,
 	return of_property_read_u32_array(np, propname, out_value, 1);
 }
 
+/**
+ * of_property_read_bool - Findfrom 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.
+ * Returns true if the property exist false otherwise.
+ */
+static inline bool of_property_read_bool(const struct device_node *np,
+					 const char *propname)
+{
+	struct property *prop = of_find_property(np, propname, NULL);
+
+	return prop ? true : false;
+}
+
 static inline int of_property_read_s32(const struct device_node *np,
 				       const char *propname,
 				       s32 *out_value)
-- 
2.3.0-rc1

[RFC/PATCH 2/2] of: allow for boolean flags to have value

[Felipe Balbi]

allowing values to boolean flags lets us setup
defaults on DTSI which can get disabled later
at board DTS if, for whatever reason, board can't
use that default.

One such example is DM81xx EVM where we can't use
MUSB's multipoint feature even though SoC supports
it. Something at the board level prevents us from
using the feature.

Instead of removing "multipoint;" from DTSI and
adding it to all board DTS just so we can remove
it from our quirky board seems like overkill when
we could just add:

	multipoint = <0>;

to that quirky board's DTS.

Note that the description here is but one example
and it's likely many others have faced something
similar.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 include/linux/of.h | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/include/linux/of.h b/include/linux/of.h
index 76c055b20fef..c5ee9320f237 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -792,14 +792,32 @@ static inline int of_property_read_u32(const struct device_node *np,
  * @propname:	name of the property to be searched.
  *
  * Search for a property in a device node.
- * Returns true if the property exist false otherwise.
+ * Returns true if the property exist and has a value greater than zero,
+ * fals otherwise.
  */
 static inline bool of_property_read_bool(const struct device_node *np,
 					 const char *propname)
 {
 	struct property *prop = of_find_property(np, propname, NULL);
+	int rc;
+	u32 val;
 
-	return prop ? true : false;
+	if (!prop)
+		return false;
+
+	rc = of_property_read_u32(np, propname, &val);
+
+	/*
+	 * if property doesn't have a value, or prop->length == 0 and
+	 * we overflow, treat it as simple value-less flag.
+	 */
+	if (rc == -ENODATA || rc == -EOVERFLOW)
+		return true;
+	if (WARN(rc < 0, "failed to read '%s' value -> %d\n",
+				propname, rc))
+		return false;
+
+	return !!val;
 }
 
 static inline int of_property_read_s32(const struct device_node *np,
-- 
2.3.0-rc1

[RFC/PATCH 2/2] of: allow for boolean flags to have value

[Arnd Bergmann]

On Thursday 05 February 2015 12:01:06 Felipe Balbi wrote:
> +       /*
> +        * if property doesn't have a value, or prop->length == 0 and
> +        * we overflow, treat it as simple value-less flag.
> +        */
> +       if (rc == -ENODATA || rc == -EOVERFLOW)
> +               return true;
> +       if (WARN(rc < 0, "failed to read '%s' value -> %d\n",
> +                               propname, rc))
> +               return false;
> 

I think there are drivers today that use of_property_read_bool()
to check for the presence of a property that is not already empty.
If the property starts with a zero cell, that would break here.

	Arnd

[RFC/PATCH 2/2] of: allow for boolean flags to have value

[Mark Rutland]

On Thu, Feb 05, 2015 at 06:01:06PM +0000, Felipe Balbi wrote:
> allowing values to boolean flags lets us setup
> defaults on DTSI which can get disabled later
> at board DTS if, for whatever reason, board can't
> use that default.
> 
> One such example is DM81xx EVM where we can't use
> MUSB's multipoint feature even though SoC supports
> it. Something at the board level prevents us from
> using the feature.
> 
> Instead of removing "multipoint;" from DTSI and
> adding it to all board DTS just so we can remove
> it from our quirky board seems like overkill when
> we could just add:
> 
> 	multipoint = <0>;
> 
> to that quirky board's DTS.
> 
> Note that the description here is but one example
> and it's likely many others have faced something
> similar.
> 

While I appreciate that adding and removing properties in this way is
painful, I think that this must be dealt with at DTB compile-time rather
than kernel run-time.

There are codebases other than Linux which parse DTs, and not all
drivers call of_property_read_bool to parse boolean properties, an awful
lot still just check of_find_property. Additionally, some bindings
_explicitly_ state boolean properties are empty and have no value, which
this extension would break.

I think that this patch only adds to the inconsistency we currently
have, and given that, I would rather not have this extension to
of_property_read_bool.

Arguably of_proeprty_read_bool should warn if it encounters a non-empty
property.

Thanks,
Mark.

> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
>  include/linux/of.h | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 76c055b20fef..c5ee9320f237 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -792,14 +792,32 @@ static inline int of_property_read_u32(const struct device_node *np,
>   * @propname:	name of the property to be searched.
>   *
>   * Search for a property in a device node.
> - * Returns true if the property exist false otherwise.
> + * Returns true if the property exist and has a value greater than zero,
> + * fals otherwise.
>   */
>  static inline bool of_property_read_bool(const struct device_node *np,
>  					 const char *propname)
>  {
>  	struct property *prop = of_find_property(np, propname, NULL);
> +	int rc;
> +	u32 val;
>  
> -	return prop ? true : false;
> +	if (!prop)
> +		return false;
> +
> +	rc = of_property_read_u32(np, propname, &val);
> +
> +	/*
> +	 * if property doesn't have a value, or prop->length == 0 and
> +	 * we overflow, treat it as simple value-less flag.
> +	 */
> +	if (rc == -ENODATA || rc == -EOVERFLOW)
> +		return true;
> +	if (WARN(rc < 0, "failed to read '%s' value -> %d\n",
> +				propname, rc))
> +		return false;
> +
> +	return !!val;
>  }
>  
>  static inline int of_property_read_s32(const struct device_node *np,
> -- 
> 2.3.0-rc1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[RFC/PATCH 2/2] of: allow for boolean flags to have value

[Tony Lindgren]

* Mark Rutland <mark.rutland@arm.com> [150205 10:26]:
> On Thu, Feb 05, 2015 at 06:01:06PM +0000, Felipe Balbi wrote:
> > allowing values to boolean flags lets us setup
> > defaults on DTSI which can get disabled later
> > at board DTS if, for whatever reason, board can't
> > use that default.
> > 
> > One such example is DM81xx EVM where we can't use
> > MUSB's multipoint feature even though SoC supports
> > it. Something at the board level prevents us from
> > using the feature.
> > 
> > Instead of removing "multipoint;" from DTSI and
> > adding it to all board DTS just so we can remove
> > it from our quirky board seems like overkill when
> > we could just add:
> > 
> > 	multipoint = <0>;
> > 
> > to that quirky board's DTS.
> > 
> > Note that the description here is but one example
> > and it's likely many others have faced something
> > similar.
> > 
> 
> While I appreciate that adding and removing properties in this way is
> painful, I think that this must be dealt with at DTB compile-time rather
> than kernel run-time.
> 
> There are codebases other than Linux which parse DTs, and not all
> drivers call of_property_read_bool to parse boolean properties, an awful
> lot still just check of_find_property. Additionally, some bindings
> _explicitly_ state boolean properties are empty and have no value, which
> this extension would break.
> 
> I think that this patch only adds to the inconsistency we currently
> have, and given that, I would rather not have this extension to
> of_property_read_bool.
> 
> Arguably of_proeprty_read_bool should warn if it encounters a non-empty
> property.

How about a WARN_ON there as in that case the value will always
be set to 1 in kernel even if specified as 0 in the .dts file?

Regards,

Tony

[RFC/PATCH 2/2] of: allow for boolean flags to have value

[Uwe Kleine-König]

On Thu, Feb 05, 2015 at 12:01:06PM -0600, Felipe Balbi wrote:
> allowing values to boolean flags lets us setup
> defaults on DTSI which can get disabled later
> at board DTS if, for whatever reason, board can't
> use that default.
> 
> One such example is DM81xx EVM where we can't use
> MUSB's multipoint feature even though SoC supports
> it. Something at the board level prevents us from
> using the feature.
> 
> Instead of removing "multipoint;" from DTSI and
> adding it to all board DTS just so we can remove
> it from our quirky board seems like overkill when
> we could just add:
> 
> 	multipoint = <0>;
> 
> to that quirky board's DTS.
> 
> Note that the description here is but one example
> and it's likely many others have faced something
> similar.
And others even came up with solutions, too: The right thing to do in
this case is

	/delete-property/ multipoint;

This works since

	cd296721a964 (dtc: import latest upstream dtc)

which is in 3.7-rc1. See

http://permalink.gmane.org/gmane.linux.drivers.devicetree/19170

for the related discussion.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |