From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH 1/2] drm/i915: Update VBT data structures to have MIPI block enhancements Date: Thu, 13 Feb 2014 10:33:11 +0200 Message-ID: <87mwhvxvi0.fsf@intel.com> References: <1392272860-2535-1-git-send-email-shobhit.kumar@intel.com> <1392272860-2535-2-git-send-email-shobhit.kumar@intel.com> <87ppmrxyzi.fsf@intel.com> <52FC8041.6000002@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id EF8BCFAD40 for ; Thu, 13 Feb 2014 00:29:22 -0800 (PST) In-Reply-To: <52FC8041.6000002@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Shobhit Kumar , intel-gfx List-Id: intel-gfx@lists.freedesktop.org On Thu, 13 Feb 2014, Shobhit Kumar wrote: > Hi > > On Thursday 13 February 2014 12:47 PM, Jani Nikula wrote: >> On Thu, 13 Feb 2014, Shobhit Kumar 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