On 07/01/2015 11:38 PM, Viresh Kumar wrote:

On 01-07-15, 18:13, Stephen Boyd wrote:

quoted

On 06/15/2015 04:57 AM, Viresh Kumar wrote:

quoted

---
 drivers/base/power/opp.c | 238 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 213 insertions(+), 25 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 2ac48ff9c1ef..3198c3e77224 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c

@@ -49,12 +49,17 @@
  *		are protected by the dev_opp_list_lock for integrity.
  *		IMPORTANT: the opp nodes should be maintained in increasing
  *		order.
- * @dynamic:	not-created from static DT entries.
  * @available:	true/false - marks if this OPP as available or not
+ * @dynamic:	not-created from static DT entries.

Why move dynamic?

To match its position, as it is present in the struct below. Yes it
could have been done in a separate patch, but its also fine to fix
such silly mistakes in another patch :)

Oh I thought kernel-doc didn't care what order things were documented
in, so moving it around doesn't really help unless someone cares that
they match the struct ordering.

quoted

quoted

+
+	new_opp->np = np;
+	new_opp->dynamic = false;
+	new_opp->available = true;
+
+	/*
+	 * TODO: Support multiple regulators
+	 *
+	 * read opp-microvolt array
+	 */
+	ret = of_property_count_u32_elems(np, "opp-microvolt");
+	if (ret == 1 || ret == 3) {
+		/* There can be one or three elements here */
+		ret = of_property_read_u32_array(np, "opp-microvolt",
+						 (u32 *)&new_opp->u_volt, ret);

It seems fragile to rely on the struct packing here. Maybe the same
comment in the struct should be copied here, and possibly some better
way of doing this so the code can't be subtly broken?

Any example of how things will break? Aren't these guaranteed to be
present at 3 consecutive 32 bit positions ?

I'm mostly worried about someone jumbling fields in the struct. Maybe
I'm too paranoid... Maybe we can have some sort of BUILD_BUG_ON() check
here?

BUILD_BUG_ON(offsetof(struct dev_pm_opp, u_volt_max) - offsetof(struct
dev_pm_opp, u_volt) != sizeof(u32) * 3);

Have you tried compiling that on 64-bit? sizeof(unsigned long) !=
sizeof(u32).

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project