On Tue, Jan 07, 2020 at 06:49:04PM +0200, Ville Syrjälä wrote: > On Fri, Jan 03, 2020 at 04:00:54PM -0800, Vivek Kasireddy wrote: > > On Fri, 3 Jan 2020 12:05:11 +0100 > > Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxx> > > > > Cc: Nabendu Maiti <nabendu.bikash.maiti@xxxxxxxxx> > > > > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx> > > > > Cc: Bob Paauwe <bob.j.paauwe@xxxxxxxxx> > > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@xxxxxxxxx> > > > > > > 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. > > And what exactly is that device? That is valuable information that the > commit message should contain. I'm not sure if we're allowed to disclose that information. I believe Vivek is working with an engineering sample and the device itself might not have been publicly announced by the device manufacturer yet. Matt > > > > > > > > > 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 > > > > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Matt Roper Graphics Software Engineer VTT-OSGC Platform Enablement Intel Corporation (916) 356-2795 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
