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

[Ville Syrjälä]

On Thu, Feb 04, 2016 at 06:21:23PM +0200, Jani Nikula wrote:
> On Thu, 04 Feb 2016, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> > On Thu, Feb 04, 2016 at 12:50:51PM +0200, Jani Nikula wrote:
> >> From: vkorjani <vikas.korjani at 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 at intel.com>
> >> Signed-off-by: Deepak M <m.deepak at intel.com>
> >> Signed-off-by: Jani Nikula <jani.nikula at 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.

> 
> 
> BR,
> Jani.
> 
> 
> >
> >> +	adapter = i2c_get_adapter(bus_number);
> >> +	if (!adapter) {
> >> +		DRM_ERROR("i2c_get_adapter(%u)\n", bus_number);
> >> +		goto out;
> >> +	}
> >> +
> >> +	transmit_buffer = kmalloc(1 + payload_size, GFP_TEMPORARY);
> >> +	if (!transmit_buffer)
> >> +		goto out_put;
> >> +
> >> +	transmit_buffer[0] = reg_offset;
> >> +	memcpy(&transmit_buffer[1], data, payload_size);
> >> +
> >> +	msg.addr = slave_add;
> >> +	msg.flags = 0;
> >> +	msg.len = payload_size + 1;
> >> +	msg.buf = &transmit_buffer[0];
> >> +
> >> +	for (i = 0; i < 6; i++) {
> >> +		ret = i2c_transfer(adapter, &msg, 1);
> >> +		if (ret == 1) {
> >> +			goto out_free;
> >> +		} else if (ret == -EAGAIN) {
> >> +			usleep_range(1000, 2500);
> >> +		} else {
> >> +			break;
> >> +		}
> >> +	}
> >> +
> >> +	DRM_ERROR("i2c transfer failed: %d\n", ret);
> >> +out_free:
> >> +	kfree(transmit_buffer);
> >> +out_put:
> >> +	i2c_put_adapter(adapter);
> >> +out:
> >> +	return data + payload_size;
> >>  }
> >>  
> >>  typedef const u8 * (*fn_mipi_elem_exec)(struct intel_dsi *intel_dsi,
> >> @@ -246,7 +304,7 @@ static const fn_mipi_elem_exec exec_elem[] = {
> >>  	[MIPI_SEQ_ELEM_SEND_PKT] = mipi_exec_send_packet,
> >>  	[MIPI_SEQ_ELEM_DELAY] = mipi_exec_delay,
> >>  	[MIPI_SEQ_ELEM_GPIO] = mipi_exec_gpio,
> >> -	[MIPI_SEQ_ELEM_I2C] = mipi_exec_i2c_skip,
> >> +	[MIPI_SEQ_ELEM_I2C] = mipi_exec_i2c,
> >>  };
> >>  
> >>  /*
> >> -- 
> >> 2.1.4
> >> 
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC