Re: [PATCH 1/2] drm/i915: Update VBT data structures to have MIPI block enhancements

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 13 Feb 2014, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> Hi
>
> On Thursday 13 February 2014 12:47 PM, Jani Nikula wrote:
>> On Thu, 13 Feb 2014, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>> Per the spec you sent me, there's 1 byte reserved, and 5 bytes of GPIO
>> indexes below.
>>
>> All in all, the size of the struct is different from the spec, shifting
>> everything for panel_type > 0. Which one is wrong?
>
> Take that the code is correct and the spec wrong. What I sent still 
> might be a little old, but the code is matched with header files used in 
> GOP code precisely for this reason that spec is not always updated 
> immediately.

:(

>>
>>> +
>>> +	/* GPIOs */
>>> +	u8 panel_enable;
>>> +	u8 bl_enable;
>>> +	u8 pwm_enable;
>>> +	u8 reset_r_n;
>>> +	u8 pwr_down_r;
>>> +	u8 stdby_r_n;
>>> +
>>>   } __packed;
>>
>> All around I would like it if the field names were slightly more
>> descriptive by themselves, particularly for the most important ones,
>> with #defines for the values in some cases. For example panel_type above
>> could clearly be "is_bridge" or similar (now it's confusing with the
>> panel_type in intel_bios.c). Same for any booleans that could be
>> expressed as "is_something" or "something_enabled". Color formats and
>> rotations could have defines, so you wouldn't need to add comments for
>> them at all. Same for byte_clk_sel.
>
> I just tried to match the names used in the VBT interface document as 
> such. But we can make some of them more descriptive. Its only that while 
> discussing parameters with those who talk in VBT document terms, it 
> helps to be on same page

There's certain value in that. The flip side of the coin is that not
everyone will have the spec handy when reading the code.

An alternative to changing the field names is adding #defines for at
least the most important ones. In this case it might be a good idea to
add the #defines within the struct, next to the relevant field.

>> I definitely do *not* mean you should rewrite all of them. If you want,
>> I can reply with detailed suggestions on a per field basis.
>>
>
> I can give a shot at improving some of them. If after that you still 
> feel changes are needed, you can suggest.

Ack. Again: fine tuning, not rewrite. :)


Thanks,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center