On 07/01/2015 11:38 PM, Viresh Kumar wrote:
On 01-07-15, 18:13, Stephen Boyd wrote:
On 06/15/2015 04:57 AM, Viresh Kumar wrote:
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 increasingorder.
- @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.
- 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).