Re: [PATCH 3/8] drm/i915: Adding the parsing logic for the i2c element

From: Daniel Vetter <daniel@ffwll.ch>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 3/8] drm/i915: Adding the parsing logic for the i2c element
Date: Mon, 15 Feb 2016 17:48:54 +0100	[thread overview]
Message-ID: <20160215164854.GU11240@phenom.ffwll.local> (raw)
In-Reply-To: <20160204163622.GR23290@intel.com>

On Thu, Feb 04, 2016 at 06:36:22PM +0200, Ville Syrjälä wrote:
> On Thu, Feb 04, 2016 at 06:21:23PM +0200, Jani Nikula wrote:
> > On Thu, 04 Feb 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > On Thu, Feb 04, 2016 at 12:50:51PM +0200, Jani Nikula wrote:
> > >> From: vkorjani <vikas.korjani@intel.com>
> > >> 
> > >> New sequence element for i2c is been added in the
> > >> mipi sequence block of the VBT. This patch parses
> > >> and executes the i2c sequence.
> > >> 
> > >> v2: Add i2c_put_adapter call(Jani), rebase
> > >> 
> > >> v3: corrected the retry loop(Jani), rebase
> > >> 
> > >> v4 by Jani:
> > >>  - don't put the adapter if get fails
> > >>  - print an error message if all retries exhausted
> > >>  - use a for loop
> > >>  - fix warnings for unused variables
> > >> 
> > >> v5 by Jani:
> > >>  - rebase on the skip i2c element patch
> > >> 
> > >> v6: by Jani:
> > >>  - ignore the gmbus i2c elements (Ville)
> > >> 
> > >> Signed-off-by: vkorjani <vikas.korjani@intel.com>
> > >> Signed-off-by: Deepak M <m.deepak@intel.com>
> > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > >> ---
> > >>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 64 ++++++++++++++++++++++++++++--
> > >>  1 file changed, 61 insertions(+), 3 deletions(-)
> > >> 
> > >> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> > >> index 6f013efba45b..f4d303ee538b 100644
> > >> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> > >> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> > >> @@ -31,6 +31,7 @@
> > >>  #include <drm/drm_panel.h>
> > >>  #include <linux/slab.h>
> > >>  #include <video/mipi_display.h>
> > >> +#include <linux/i2c.h>
> > >>  #include <asm/intel-mid.h>
> > >>  #include <video/mipi_display.h>
> > >>  #include "i915_drv.h"
> > >> @@ -235,9 +236,66 @@ out:
> > >>  	return data;
> > >>  }
> > >>  
> > >> -static const u8 *mipi_exec_i2c_skip(struct intel_dsi *intel_dsi, const u8 *data)
> > >> +static const u8 *mipi_exec_i2c(struct intel_dsi *intel_dsi, const u8 *data)
> > >>  {
> > >> -	return data + *(data + 6) + 7;
> > >> +	struct i2c_adapter *adapter;
> > >> +	int ret, i;
> > >> +	u8 reg_offset, payload_size;
> > >> +	struct i2c_msg msg;
> > >> +	u8 *transmit_buffer;
> > >> +	u8 flag, resource_id, bus_number;
> > >> +	u16 slave_add;
> > >> +
> > >> +	flag = *data++;
> > >> +	resource_id = *data++;
> > >> +	bus_number = *data++;
> > >> +	slave_add = *(u16 *)(data);
> > >> +	data += 2;
> > >> +	reg_offset = *data++;
> > >> +	payload_size = *data++;
> > >> +
> > >> +	if (resource_id == 0xff || bus_number == 0xff) {
> > >> +		DRM_DEBUG_KMS("ignoring gmbus (resource id %02x, bus %02x)\n",
> > >> +			      resource_id, bus_number);
> > >> +		goto out;
> > >> +	}
> > >> +
> > >
> > > Still missing the check for __i2c_first_dynamic_bus_num which I think
> > > would at least provide some kind of half arsed protection against
> > > something not registering these magic i2c busses.
> > 
> > I meant to reply to that part of the review but forgot.
> > 
> > Problem is, we'd have to include drivers/i2c/i2c-core.h directly, which
> > also has this warning,
> > 
> > /* These symbols are exported ONLY FOR the i2c core.
> >  * No other users will be supported.
> >  */
> > 
> > and there are no users outside of drivers/i2c. I'm quite reluctant to
> > add that.
> 
> The we need some other way to look up the adapter. Passing in
> essentially random adapter numbers could give us more or less
> any i2c bus on the system, and if we go poke there we could do
> real damage.
> 
> The whole scheme is very poorly thoght out I think. There really
> should be some kind of ACPI ID or something that lets us look up
> exactly the right thing.

Agreed this is super fragile, but we should at least try to make sure we
don't end up getting some random dynamic i2c adapter. Maybe add an
i2c_get_board_adapter or similar, which does this check in the i2c core?

Definitely need to pull in the i2c maintainers here (and help them with
avoid hair-pulling exercises on their end ...).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx