On Fri, 17 Sep 2021, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
>On Fri, 17 Sep 2021, Lee Shawn C <shawn.c.lee@xxxxxxxxx> wrote:
>> Gmbus driver would setup all Intel i2c GMBuses. But DDC bus may
>> configured as gpio and reserved for MIPI driver to control panel power
>> on/off sequence.
>>
>> Using i2c tool to communicate to peripherals via i2c interface
>> reversed for gmbus(DDC). There will be some high/low pulse appear on
>> DDC SCL and SDA (might be host sent out i2c slave address). MIPI panel
>> would be impacted due to unexpected signal then caused abnormal
>> display or shut down issue.
>>
>> v2: gmbus driver should not add i2c adapter for DDC interface
>> if LFP display was configured to support MIPI panel.
>> v3: fix sparse warning
>>
>> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
>> Cc: Vandita Kulkarni <vandita.kulkarni@xxxxxxxxx>
>> Cc: Cooper Chiou <cooper.chiou@xxxxxxxxx>
>> Cc: William Tseng <william.tseng@xxxxxxxxx>
>> Signed-off-by: Lee Shawn C <shawn.c.lee@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/display/intel_gmbus.c | 18 +++++++++++++++++-
>> 1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_gmbus.c
>> b/drivers/gpu/drm/i915/display/intel_gmbus.c
>> index ceb1bf8a8c3c..51d2b6bf2ed2 100644
>> --- a/drivers/gpu/drm/i915/display/intel_gmbus.c
>> +++ b/drivers/gpu/drm/i915/display/intel_gmbus.c
>> @@ -141,6 +141,21 @@ bool intel_gmbus_is_valid_pin(struct drm_i915_private *dev_priv,
>> return pin < size && get_gmbus_pin(dev_priv, pin)->name; }
>>
>> +static bool intel_gmbus_ddc_reserve_for_mipi(struct drm_i915_private *dev_priv,
>> + unsigned int pin)
>> +{
>> + if (intel_bios_is_dsi_present(dev_priv, NULL)) {
>> + if (DISPLAY_VER(dev_priv) >= 11) {
>> + if ((pin == GMBUS_PIN_2_BXT && dev_priv->vbt.dsi.config->dual_link) ||
>> + pin == GMBUS_PIN_1_BXT) {
>> + return true;
>> + }
>> + }
>> + }
>> +
>> + return false;
>> +}
>> +
>> /* Intel GPIO access functions */
>>
>> #define I2C_RISEFALL_TIME 10
>> @@ -859,7 +874,8 @@ int intel_gmbus_setup(struct drm_i915_private *dev_priv)
>> init_waitqueue_head(&dev_priv->gmbus_wait_queue);
>>
>> for (pin = 0; pin < ARRAY_SIZE(dev_priv->gmbus); pin++) {
>> - if (!intel_gmbus_is_valid_pin(dev_priv, pin))
>> + if (!intel_gmbus_is_valid_pin(dev_priv, pin) ||
>> + intel_gmbus_ddc_reserve_for_mipi(dev_priv, pin))
>> continue;
>>
>> bus = &dev_priv->gmbus[pin];
>
>This does not prevent the pin from being used for e.g. HDMI,
Agree that customer may connect DDC pin to HDMI port. In my opinion, we should take care that while doing HW design review on customer board.
HW member should aware these pins were reserved for MIPI display design. And i915 driver would reserve them as well if LFP display was MIPI DSI panel.
>and things are probably going to go awfully wrong with intel_gmbus_get_adapter() when the adapter hasn't been registered. In that sense, this is no different from v1.
>
Before gmbus driver do i2c_add_adapter() or i2c_del_adapter(), it always check the pin is valid or not.
It seems we can move intel_gmbus_ddc_reserve_for_mipi() into intel_gmbus_is_valid_pin() to check pin was reserved or not.
Then gmbus driver would not went wrong in intel_gmbus_get_adapter() when the adapter hasn't been registered.
Best regards,
Shawn
>Sure, the VBT probably shouldn't do that, but that's not an excuse for us to not take it into account.
>
>Cc: Ville in case he has some clever ideas off the top of his head. I know I'd have to spend time I don't have to figure this out.
>
>
>BR,
>Jani.
>
>
>--
>Jani Nikula, Intel Open Source Graphics Center
