[MIPI,SEQ,PARSING,v2,01/11] drm/i915: Adding the parsing logic for the i2c element

Submitted by Deepak M on Sept. 9, 2015, 11:24 p.m.

Details

Message ID 1441841070-11532-2-git-send-email-m.deepak@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Deepak M Sept. 9, 2015, 11:24 p.m.
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

Signed-off-by: vkorjani <vikas.korjani@intel.com>
Signed-off-by: Deepak M <m.deepak@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c          |    6 +++
 drivers/gpu/drm/i915/intel_bios.h          |    1 +
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c |   61 ++++++++++++++++++++++++++++
 3 files changed, 68 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index c8acc29..0bf0942 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -714,6 +714,12 @@  static u8 *goto_next_sequence(u8 *data, int *size)
 
 			data += 3;
 			break;
+		case MIPI_SEQ_ELEM_I2C:
+			/* skip by this element payload size */
+			data += 7;
+			len = *data;
+			data += len + 1;
+			break;
 		default:
 			DRM_ERROR("Unknown element\n");
 			return NULL;
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 1b7417e..21a7f3f 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -956,6 +956,7 @@  enum mipi_seq_element {
 	MIPI_SEQ_ELEM_SEND_PKT,
 	MIPI_SEQ_ELEM_DELAY,
 	MIPI_SEQ_ELEM_GPIO,
+	MIPI_SEQ_ELEM_I2C,
 	MIPI_SEQ_ELEM_STATUS,
 	MIPI_SEQ_ELEM_MAX
 };
diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index a5e99ac..9989f61 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,65 @@  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;
+	u8 reg_offset, payload_size, retries = 5;
+	struct i2c_msg msg;
+	u8 *transmit_buffer = NULL;
+	u8 flag = *data++;
+	u8 index = *data++;
+	u8 bus_number = *data++;
+	u16 slave_add = *(u16 *)(data);
+
+	data = data + 2;
+	reg_offset = *data++;
+	payload_size = *data++;
+
+	adapter = i2c_get_adapter(bus_number);
+
+	if (!adapter) {
+		DRM_ERROR("i2c_get_adapter(%u) failed, index:%u flag: %u\n",
+				(bus_number + 1), index, flag);
+		goto out;
+	}
+
+	transmit_buffer = kmalloc(1 + payload_size, GFP_TEMPORARY);
+
+	if (!transmit_buffer)
+		goto out;
+
+	transmit_buffer[0] = reg_offset;
+	memcpy(&transmit_buffer[1], data, (size_t)payload_size);
+
+	msg.addr   = slave_add;
+	msg.flags  = 0;
+	msg.len    = payload_size + 1;
+	msg.buf    = &transmit_buffer[0];
+
+	do {
+		ret =  i2c_transfer(adapter, &msg, 1);
+		if (ret == 1)
+			break;
+		else if (ret == -EAGAIN)
+			usleep_range(1000, 2500);
+		else {
+			DRM_ERROR("i2c transfer failed, error code:%d", ret);
+			break;
+		}
+	} while (retries--);
+
+	if (retries == 0)
+		DRM_ERROR("i2c transfer failed, error code:%d", ret);
+out:
+	kfree(transmit_buffer);
+	i2c_put_adapter(adapter);
+
+	data = data + payload_size;
+	return data;
+}
+
 static inline enum port intel_dsi_seq_port_to_port(u8 port)
 {
 	return port ? PORT_C : PORT_A;
@@ -236,6 +296,7 @@  static const fn_mipi_elem_exec exec_elem[] = {
 	mipi_exec_send_packet,
 	mipi_exec_delay,
 	mipi_exec_gpio,
+	mipi_exec_i2c,
 	NULL, /* status read; later */
 };
 

Comments

On Thu, 10 Sep 2015, Deepak M <m.deepak@intel.com> 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
>
> Signed-off-by: vkorjani <vikas.korjani@intel.com>
> Signed-off-by: Deepak M <m.deepak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_bios.c          |    6 +++
>  drivers/gpu/drm/i915/intel_bios.h          |    1 +
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c |   61 ++++++++++++++++++++++++++++
>  3 files changed, 68 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index c8acc29..0bf0942 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -714,6 +714,12 @@ static u8 *goto_next_sequence(u8 *data, int *size)
>  
>  			data += 3;
>  			break;
> +		case MIPI_SEQ_ELEM_I2C:
> +			/* skip by this element payload size */
> +			data += 7;
> +			len = *data;
> +			data += len + 1;
> +			break;
>  		default:
>  			DRM_ERROR("Unknown element\n");
>  			return NULL;
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index 1b7417e..21a7f3f 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -956,6 +956,7 @@ enum mipi_seq_element {
>  	MIPI_SEQ_ELEM_SEND_PKT,
>  	MIPI_SEQ_ELEM_DELAY,
>  	MIPI_SEQ_ELEM_GPIO,
> +	MIPI_SEQ_ELEM_I2C,
>  	MIPI_SEQ_ELEM_STATUS,

Side note, MIPI_SEQ_ELEM_STATUS doesn't seem to be in spec.


>  	MIPI_SEQ_ELEM_MAX
>  };
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index a5e99ac..9989f61 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,65 @@ 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;
> +	u8 reg_offset, payload_size, retries = 5;
> +	struct i2c_msg msg;
> +	u8 *transmit_buffer = NULL;
> +	u8 flag = *data++;
> +	u8 index = *data++;
> +	u8 bus_number = *data++;
> +	u16 slave_add = *(u16 *)(data);
> +
> +	data = data + 2;
> +	reg_offset = *data++;
> +	payload_size = *data++;
> +
> +	adapter = i2c_get_adapter(bus_number);
> +
> +	if (!adapter) {
> +		DRM_ERROR("i2c_get_adapter(%u) failed, index:%u flag: %u\n",
> +				(bus_number + 1), index, flag);

Why do you log bus_number + 1 instead of what was actually tried?

I don't see why the flag/index are useful for in the message, as they're
not relevant to the i2c_get_adapter failing.

> +		goto out;
> +	}
> +
> +	transmit_buffer = kmalloc(1 + payload_size, GFP_TEMPORARY);
> +
> +	if (!transmit_buffer)
> +		goto out;
> +
> +	transmit_buffer[0] = reg_offset;
> +	memcpy(&transmit_buffer[1], data, (size_t)payload_size);

Why do you need the cast?

> +
> +	msg.addr   = slave_add;
> +	msg.flags  = 0;
> +	msg.len    = payload_size + 1;
> +	msg.buf    = &transmit_buffer[0];
> +
> +	do {
> +		ret =  i2c_transfer(adapter, &msg, 1);
> +		if (ret == 1)
> +			break;
> +		else if (ret == -EAGAIN)
> +			usleep_range(1000, 2500);
> +		else {
> +			DRM_ERROR("i2c transfer failed, error code:%d", ret);

\n missing.

> +			break;
> +		}
> +	} while (retries--);
> +
> +	if (retries == 0)

Due to the way you post-decrement retries, you may end up here even if
the transfer succeeds on the last try.

You may also end up printing two error messages, if i2c_transfer fails
on the last try.

> +		DRM_ERROR("i2c transfer failed, error code:%d", ret);

\n missing.

> +out:
> +	kfree(transmit_buffer);
> +	i2c_put_adapter(adapter);
> +
> +	data = data + payload_size;
> +	return data;
> +}
> +
>  static inline enum port intel_dsi_seq_port_to_port(u8 port)
>  {
>  	return port ? PORT_C : PORT_A;
> @@ -236,6 +296,7 @@ static const fn_mipi_elem_exec exec_elem[] = {
>  	mipi_exec_send_packet,
>  	mipi_exec_delay,
>  	mipi_exec_gpio,
> +	mipi_exec_i2c,
>  	NULL, /* status read; later */
>  };
>  
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>-----Original Message-----
>From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>Sent: Thursday, September 17, 2015 2:48 PM
>To: Deepak, M; intel-gfx@lists.freedesktop.org
>Cc: Deepak, M
>Subject: Re: [Intel-gfx] [MIPI SEQ PARSING v2 PATCH 01/11] drm/i915: Adding
>the parsing logic for the i2c element
>
>On Thu, 10 Sep 2015, Deepak M <m.deepak@intel.com> 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
>>
>> Signed-off-by: vkorjani <vikas.korjani@intel.com>
>> Signed-off-by: Deepak M <m.deepak@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_bios.c          |    6 +++
>>  drivers/gpu/drm/i915/intel_bios.h          |    1 +
>>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c |   61
>++++++++++++++++++++++++++++
>>  3 files changed, 68 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c
>> b/drivers/gpu/drm/i915/intel_bios.c
>> index c8acc29..0bf0942 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -714,6 +714,12 @@ static u8 *goto_next_sequence(u8 *data, int
>> *size)
>>
>>  			data += 3;
>>  			break;
>> +		case MIPI_SEQ_ELEM_I2C:
>> +			/* skip by this element payload size */
>> +			data += 7;
>> +			len = *data;
>> +			data += len + 1;
>> +			break;
>>  		default:
>>  			DRM_ERROR("Unknown element\n");
>>  			return NULL;
>> diff --git a/drivers/gpu/drm/i915/intel_bios.h
>> b/drivers/gpu/drm/i915/intel_bios.h
>> index 1b7417e..21a7f3f 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.h
>> +++ b/drivers/gpu/drm/i915/intel_bios.h
>> @@ -956,6 +956,7 @@ enum mipi_seq_element {
>>  	MIPI_SEQ_ELEM_SEND_PKT,
>>  	MIPI_SEQ_ELEM_DELAY,
>>  	MIPI_SEQ_ELEM_GPIO,
>> +	MIPI_SEQ_ELEM_I2C,
>>  	MIPI_SEQ_ELEM_STATUS,
>
>Side note, MIPI_SEQ_ELEM_STATUS doesn't seem to be in spec.
>
>
>>  	MIPI_SEQ_ELEM_MAX
>>  };
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> index a5e99ac..9989f61 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,65 @@ 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;
>> +	u8 reg_offset, payload_size, retries = 5;
>> +	struct i2c_msg msg;
>> +	u8 *transmit_buffer = NULL;
>> +	u8 flag = *data++;
>> +	u8 index = *data++;
>> +	u8 bus_number = *data++;
>> +	u16 slave_add = *(u16 *)(data);
>> +
>> +	data = data + 2;
>> +	reg_offset = *data++;
>> +	payload_size = *data++;
>> +
>> +	adapter = i2c_get_adapter(bus_number);
>> +
>> +	if (!adapter) {
>> +		DRM_ERROR("i2c_get_adapter(%u) failed, index:%u flag:
>%u\n",
>> +				(bus_number + 1), index, flag);
>
>Why do you log bus_number + 1 instead of what was actually tried?
[Deepak M] Will fix this.
>
>I don't see why the flag/index are useful for in the message, as they're not
>relevant to the i2c_get_adapter failing.
>
[Deepak M] Flag field is reserved and then the index is used for windows, for Linux these two can be skipped. Will not try to declare flag and index variable.
>> +		goto out;
>> +	}
>> +
>> +	transmit_buffer = kmalloc(1 + payload_size, GFP_TEMPORARY);
>> +
>> +	if (!transmit_buffer)
>> +		goto out;
>> +
>> +	transmit_buffer[0] = reg_offset;
>> +	memcpy(&transmit_buffer[1], data, (size_t)payload_size);
>
>Why do you need the cast?
>
>> +
>> +	msg.addr   = slave_add;
>> +	msg.flags  = 0;
>> +	msg.len    = payload_size + 1;
>> +	msg.buf    = &transmit_buffer[0];
>> +
>> +	do {
>> +		ret =  i2c_transfer(adapter, &msg, 1);
>> +		if (ret == 1)
>> +			break;
>> +		else if (ret == -EAGAIN)
>> +			usleep_range(1000, 2500);
>> +		else {
>> +			DRM_ERROR("i2c transfer failed, error code:%d", ret);
>
>\n missing.
>
>> +			break;
>> +		}
>> +	} while (retries--);
>> +
>> +	if (retries == 0)
>
>Due to the way you post-decrement retries, you may end up here even if the
>transfer succeeds on the last try.
>
>You may also end up printing two error messages, if i2c_transfer fails on the
>last try.
[Deepak M] okay, Will handle this.
>
>> +		DRM_ERROR("i2c transfer failed, error code:%d", ret);
>
>\n missing.
>
>> +out:
>> +	kfree(transmit_buffer);
>> +	i2c_put_adapter(adapter);
>> +
>> +	data = data + payload_size;
>> +	return data;
>> +}
>> +
>>  static inline enum port intel_dsi_seq_port_to_port(u8 port)  {
>>  	return port ? PORT_C : PORT_A;
>> @@ -236,6 +296,7 @@ static const fn_mipi_elem_exec exec_elem[] = {
>>  	mipi_exec_send_packet,
>>  	mipi_exec_delay,
>>  	mipi_exec_gpio,
>> +	mipi_exec_i2c,
>>  	NULL, /* status read; later */
>>  };
>>
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>--
>Jani Nikula, Intel Open Source Technology Center