Re: [PATCH 11/15] drm/i915: Adding the parsing logic for the i2c element

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Jan 11, 2016 at 03:31:27PM +0200, Jani Nikula wrote:
> On Mon, 11 Jan 2016, Jani Nikula <jani.nikula@intel.com> wrote:
> > On Thu, 07 Jan 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >> On Mon, Dec 21, 2015 at 03:11:02PM +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
> >>> 
> >>> Cc: Jani Nikula <jani.nikula@intel.com>
> >>> 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 | 58 ++++++++++++++++++++++++++++++
> >>>  1 file changed, 58 insertions(+)
> >>> 
> >>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> >>> index ba5355506590..8fcfb0f63dc1 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"
> >>> @@ -104,6 +105,62 @@ static struct gpio_table gtable[] = {
> >>>  	{ GPIO_NC_11_PCONF0, GPIO_NC_11_PAD, 0}
> >>>  };
> >>>  
> >>> +static const u8 *mipi_exec_i2c(struct intel_dsi *intel_dsi, const u8 *data)
> >>> +{
> >>> +	struct i2c_adapter *adapter;
> >>> +	int ret, i;
> >>> +	u8 reg_offset, payload_size;
> >>> +	struct i2c_msg msg;
> >>> +	u8 *transmit_buffer;
> >>> +	u8 flag, bus_number;
> >>> +	u16 slave_add;
> >>> +
> >>> +	flag = *data++;
> >>> +	data++; /* index, unused */
> >>> +	bus_number = *data++;
> >>> +	slave_add = *(u16 *)(data);
> >>> +	data += 2;
> >>> +	reg_offset = *data++;
> >>> +	payload_size = *data++;
> >>> +
> >>> +	adapter = i2c_get_adapter(bus_number);
> >>
> >> I'm trying to find who is responsible for registering i2c busses with
> >> these magic bus numbers. So far I can't see anything that would be doing
> >> it.
> >
> > *cough* I'm not sure either *cough*
> >
> > Would you prefer adding a dummy mipi_exec_i2c that just eats and ignores
> > the data at this point? It would be a step forward. Now the driver just
> > gives up on encountering an unknown element.
> 
> I did just that, there's now a new 11/15 just skipping, and this one is
> moved at the end. Also sent the rebased version of that.

As far as the i2c bus numbering goes, I think we ought to add a check to
make sure the magic bus number is below __i2c_first_dynamic_bus_num.
That might catch the cases where some firmware related code forgot to
register the magic bus numbers.

Also the spec says 0xff would mean gmbus. So seems like we should handle
that in a special way. But I can't see where it would actually specify
the actual gmbus pins we should use, so I guessing no one actually even
tried to use this option since it simply can't work.

> 
> BR,
> Jani.
> 
> 
> >
> > BR,
> > Jani.
> >
> >
> >>
> >>> +	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;
> >>> +}
> >>> +
> >>>  static inline enum port intel_dsi_seq_port_to_port(u8 port)
> >>>  {
> >>>  	return port ? PORT_C : PORT_A;
> >>> @@ -235,6 +292,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,
> >>>  };
> >>>  
> >>>  /*
> >>> -- 
> >>> 2.1.4
> >>> 
> >>> _______________________________________________
> >>> Intel-gfx mailing list
> >>> Intel-gfx@lists.freedesktop.org
> >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx