[PATCH 05/10] opp: Add support to parse "operating-points-v2" bindings

[Stephen Boyd]

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

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