[Intel-gfx] [PATCH] drm/i915/dsi: Parse the I2C element from the VBT MIPI sequence block
Hans de Goede
hdegoede at redhat.com
Sat Jan 4 16:44:47 UTC 2020
Hi,
On 04-01-2020 01:00, Vivek Kasireddy wrote:
> On Fri, 3 Jan 2020 12:05:11 +0100
> Hans de Goede <hdegoede at redhat.com> wrote:
> Hi Hans,
>
>> Hi Vivek,
>>
>> On 03-01-2020 01:00, Vivek Kasireddy wrote:
>>> Parsing the i2c element is mainly done to transfer the payload from
>>> the MIPI sequence block to the relevant slave device. In some
>>> cases, the commands that are part of the payload can be used to
>>> turn on the backlight.
>>>
>>> This patch is actually a refactored version of this old patch:
>>> https://lists.freedesktop.org/archives/intel-gfx/2014-December/056897.html
>>>
>>> In addition to the refactoring, the old patch is augmented by
>>> looking up the i2c bus from ACPI NS instead of relying on the bus
>>> number provided in the VBT.
>>>
>>> Cc: Deepak M <m.deepak at intel.com>
>>> Cc: Nabendu Maiti <nabendu.bikash.maiti at intel.com>
>>> Cc: Matt Roper <matthew.d.roper at intel.com>
>>> Cc: Bob Paauwe <bob.j.paauwe at intel.com>
>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy at intel.com>
>>
>> Thank you for this patch, I have been doing a lot of work to make
>> DSI panels on Bay Trail and Cherry Trail devices work better, as such
>> I've done a lot of testing of DSI panels. But I have never seen any
>> MIPI sequences actually use the i2c commands. May I ask how you have
>> tested this? Do you have a device which actually uses the i2c
>> commands?
> Oh, they sure exist; we do have a device that uses i2c commands to turn
> on the backlight that we have tested this patch on.
Ah, ok, good. I was a bit worried this patch was not tested with
any devices actually using the i2c stuff. So I'm happy to hear that it
has been tested.
Regards,
Hans
>
>>
>> I also have some small review comments inline:
>>
>>> ---
>>> drivers/gpu/drm/i915/display/intel_dsi.h | 3 +
>>> drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 93
>>> ++++++++++++++++++++ 2 files changed, 96 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsi.h
>>> b/drivers/gpu/drm/i915/display/intel_dsi.h index
>>> b15be5814599..5651bc8aa5c2 100644 ---
>>> a/drivers/gpu/drm/i915/display/intel_dsi.h +++
>>> b/drivers/gpu/drm/i915/display/intel_dsi.h @@ -68,6 +68,9 @@ struct
>>> intel_dsi { /* number of DSI lanes */
>>> unsigned int lane_count;
>>>
>>> + /* i2c bus associated with the slave device */
>>> + int i2c_bus_num;
>>> +
>>> /*
>>> * video mode pixel format
>>> *
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
>>> b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c index
>>> f90946c912ee..60441a5a3dba 100644 ---
>>> a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c +++
>>> b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c @@ -83,6 +83,12 @@
>>> static struct gpio_map vlv_gpio_table[] = { {
>>> VLV_GPIO_NC_11_PANEL1_BKLTCTL }, };
>>>
>>> +struct i2c_adapter_lookup {
>>> + u16 slave_addr;
>>> + struct intel_dsi *intel_dsi;
>>> + acpi_handle dev_handle;
>>> +};
>>> +
>>> #define CHV_GPIO_IDX_START_N 0
>>> #define CHV_GPIO_IDX_START_E 73
>>> #define CHV_GPIO_IDX_START_SW 100
>>> @@ -375,8 +381,93 @@ static const u8 *mipi_exec_gpio(struct
>>> intel_dsi *intel_dsi, const u8 *data) return data;
>>> }
>>>
>>> +static int i2c_adapter_lookup(struct acpi_resource *ares, void
>>> *data) +{
>>> + struct i2c_adapter_lookup *lookup = data;
>>> + struct intel_dsi *intel_dsi = lookup->intel_dsi;
>>> + struct acpi_resource_i2c_serialbus *sb;
>>> + struct i2c_adapter *adapter;
>>> + acpi_handle adapter_handle;
>>> + acpi_status status;
>>> +
>>> + if (intel_dsi->i2c_bus_num >= 0 ||
>>> + !i2c_acpi_get_i2c_resource(ares, &sb))
>>> + return 1;
>>> +
>>> + if (lookup->slave_addr != sb->slave_address)
>>> + return 1;
>>> +
>>> + status = acpi_get_handle(lookup->dev_handle,
>>> + sb->resource_source.string_ptr,
>>> + &adapter_handle);
>>> + if (ACPI_FAILURE(status))
>>> + return 1;
>>> +
>>> + adapter = i2c_acpi_find_adapter_by_handle(adapter_handle);
>>> + if (adapter)
>>> + intel_dsi->i2c_bus_num = adapter->nr;
>>> +
>>> + return 1;
>>> +}
>>> +
>>> static const u8 *mipi_exec_i2c(struct intel_dsi *intel_dsi, const
>>> u8 *data) {
>>> + struct drm_device *dev = intel_dsi->base.base.dev;
>>> + struct i2c_adapter *adapter;
>>> + struct acpi_device *acpi_dev;
>>> + struct list_head resource_list;
>>> + struct i2c_adapter_lookup lookup;
>>> + struct i2c_msg msg;
>>> + int ret;
>>> + u8 vbt_i2c_bus_num = *(data + 2);
>>> + u16 slave_addr = *(u16 *)(data + 3);
>>> + u8 reg_offset = *(data + 5);
>>> + u8 payload_size = *(data + 6);
>>> + u8 *payload_data;
>>> +
>>> + if (intel_dsi->i2c_bus_num < 0) {
>>> + intel_dsi->i2c_bus_num = vbt_i2c_bus_num;
>>> +
>>> + acpi_dev = ACPI_COMPANION(&dev->pdev->dev);
>>> + if (acpi_dev) {
>>> + memset(&lookup, 0, sizeof(lookup));
>>> + lookup.slave_addr = slave_addr;
>>> + lookup.intel_dsi = intel_dsi;
>>> + lookup.dev_handle =
>>> acpi_device_handle(acpi_dev); +
>>> + INIT_LIST_HEAD(&resource_list);
>>> + acpi_dev_get_resources(acpi_dev,
>>> &resource_list,
>>> + i2c_adapter_lookup,
>>> + &lookup);
>>> +
>>> acpi_dev_free_resource_list(&resource_list);
>>> + }
>>> + }
>>> +
>>> + adapter = i2c_get_adapter(intel_dsi->i2c_bus_num);
>>> + if (!adapter)
>>> + goto out;
>>
>> This should never happen, so you should put a DRM_DEV_WARN here.
> Ok, will do.
>
>>
>>> +
>>> + payload_data = kzalloc(payload_size + 1, GFP_KERNEL);
>>> + if (!payload_data)
>>> + goto out;
>>> +
>>> + payload_data[0] = reg_offset;
>>> + memcpy(&payload_data[1], (data + 7), payload_size);
>>> +
>>> + msg.addr = slave_addr;
>>> + msg.flags = 0;
>>> + msg.len = payload_size + 1;
>>> + msg.buf = payload_data;
>>> +
>>> + ret = i2c_transfer(adapter, &msg, 1);
>>> + if (ret < 0)
>>> + DRM_ERROR("i2c transfer failed");
>>
>> DRM_DEV_ERROR? And maybe some more info, like the register which
>> the transfer is going to + payload size?
> Ok, adding extra info makes sense.
>
>>
>>> +
>>> + kfree(payload_data);
>>> + i2c_put_adapter(adapter);
>>> +
>>
>> Just put out here, no need for the DRM_DEBUG (which should no
>> longer be a debug now that we implement this) below, since we
>> WARN or ERROR on all goto out; statements above.
> Ok, will do.
>
> Thanks,
> Vivek
>
>>
>> out:
>>
>>> + return data + payload_size + 7;
>>
>> And drop these 3 lines:
>>
>>> +out:
>>> DRM_DEBUG_KMS("Skipping I2C element execution\n");
>>>
>>> return data + *(data + 6) + 7;
>>
>>
>>
>>> @@ -664,6 +755,8 @@ bool intel_dsi_vbt_init(struct intel_dsi
>>> *intel_dsi, u16 panel_id) intel_dsi->panel_off_delay =
>>> pps->panel_off_delay / 10; intel_dsi->panel_pwr_cycle_delay =
>>> pps->panel_power_cycle_delay / 10;
>>> + intel_dsi->i2c_bus_num = -1;
>>> +
>>> /* a regular driver would get the device in probe */
>>> for_each_dsi_port(port, intel_dsi->ports) {
>>> mipi_dsi_attach(intel_dsi->dsi_hosts[port]->device);
>>>
>>
>> Regards,
>>
>> Hans
>>
>
More information about the Intel-gfx
mailing list