Re: [PATCH 08/12] drm: Add lspcon (custom type2 dp->hdmi) support

["Sharma, Shashank" <shashank.sharma@intel.com>]

Regards
Shashank

On 5/3/2016 9:29 PM, Ville Syrjälä wrote:
> On Tue, May 03, 2016 at 09:15:48PM +0530, Sharma, Shashank wrote:
>> Regards
>> Shashank
>>
>> On 5/2/2016 7:19 PM, Ville Syrjälä wrote:
>>> On Mon, Apr 04, 2016 at 05:31:44PM +0530, Shashank Sharma wrote:
>>>> This patch adds support for LSPCON devices in dp++ adaptor
>>>> helper layer. LSPCON is DP++ type-2 adaptor with some customized
>>>> functionalities, to provide additional features in Intel Gen9 HW.
>>>>
>>>> LSPCON needs I2C-over-aux support to read/write control and
>>>> data registers. This patch adds following:
>>>>     - Functions for I2C over aux read/write
>>>>     - Function to read EDID using I2c-over-aux
>>>>     - Function to identify LSPCON among type-2 DP adaptors
>>>>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_dp_dual_mode_helper.c | 100 ++++++++++++++++++++++++++++++
>>>>    include/drm/drm_dp_dual_mode_helper.h     |  10 +++
>>>>    2 files changed, 110 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>>>> index b72b7bb..ce8e11c 100644
>>>> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>>>> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>>>> @@ -132,6 +132,99 @@ ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,
>>>>    }
>>>>    EXPORT_SYMBOL(drm_dp_dual_mode_write);
>>>>
>>>> +int drm_dp_dual_mode_get_edid(void *data,
>>>> +	u8 *buf, unsigned int block, size_t len)
>>>> +{
>>>> +	struct i2c_adapter *adapter = data;
>>>> +	unsigned char start = block * EDID_LENGTH;
>>>> +	unsigned char segment = block >> 1;
>>>> +	unsigned char xfers = segment ? 3 : 2;
>>>> +	int ret, retries = 5;
>>>> +
>>>> +	do {
>>>> +		struct i2c_msg msgs[] = {
>>>> +			{
>>>> +				.addr   = DP_DUAL_MODE_DDC_SEGMENT_ADDR,
>>>> +				.flags  = 0,
>>>> +				.len    = 1,
>>>> +				.buf    = &segment,
>>>> +			}, {
>>>> +				.addr   = DDC_ADDR,
>>>> +				.flags  = 0,
>>>> +				.len    = 1,
>>>> +				.buf    = &start,
>>>> +			}, {
>>>> +				.addr   = DDC_ADDR,
>>>> +				.flags  = I2C_M_RD,
>>>> +				.len    = len,
>>>> +				.buf    = buf,
>>>> +			}
>>>> +		};
>>>> +
>>>> +		ret = adapter->algo->master_xfer(adapter, &msgs[3 - xfers],
>>>> +						xfers);
>>>> +
>>>> +		if (ret == -ENXIO) {
>>>> +			DRM_ERROR("Non-existent adapter %s\n",
>>>> +				adapter->name);
>>>> +			break;
>>>> +		}
>>>> +	} while (ret != xfers && --retries);
>>>> +
>>>> +	return ret == xfers ? 0 : -1;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_dp_dual_mode_get_edid);
>>>
>>> This sort of stuff shouldn't be needed. All that should be needed is
>>> passing the right i2c adapter to drm_get_edid() and whatnot.
>>>
>> Yeah, agree, we can optimize this.
>>>> +
>>>> +/*
>>>> +* drm_dp_dual_mode_ioa_xfer
>>>> +* Few dp->hdmi type 2 adaptors allow i2c_over_aux read/write
>>>> +* to the control and status registers. These functions help
>>>> +* to read/write from those.
>>>> +*/
>>>> +static int drm_dp_dual_mode_ioa_xfer(struct i2c_adapter *adapter,
>>>> +		u8 *buffer, u8 offset, u8 no_of_bytes, u8 rw_flag)
>>>> +{
>>>> +	int err = 0;
>>>> +
>>>> +	struct i2c_msg msgs[] = {
>>>> +			{
>>>> +				.addr	= DP_DUAL_MODE_SLAVE_ADDRESS,
>>>> +				.flags	= 0,
>>>> +				.len	= 1,
>>>> +				.buf	= &offset,
>>>> +			}, {
>>>> +				.addr	= DP_DUAL_MODE_SLAVE_ADDRESS,
>>>> +				.flags	= rw_flag,
>>>> +				.len	= no_of_bytes,
>>>> +				.buf	= buffer,
>>>> +			}
>>>> +	};
>>>> +
>>>> +	/* I2C over AUX here */
>>>> +	err = adapter->algo->master_xfer(adapter, msgs, 2);
>>>> +	if (err < 0)
>>>> +		DRM_ERROR("LSPCON: Failed I2C over Aux read(addr=0x%x)\n",
>>>> +				(unsigned int)offset);
>>>> +
>>>> +	return err;
>>>> +}
>>>> +
>>>> +int drm_dp_dual_mode_ioa_read(struct i2c_adapter *adapter, u8 *buffer,
>>>> +		u8 offset, u8 no_of_bytes)
>>>> +{
>>>> +	return drm_dp_dual_mode_ioa_xfer(adapter, buffer, offset,
>>>> +		no_of_bytes, I2C_M_RD);
>>>> +}
>>>> +EXPORT_SYMBOL(drm_dp_dual_mode_ioa_read);
>>>> +
>>>> +int drm_dp_dual_mode_ioa_write(struct i2c_adapter *adapter, u8 *buffer,
>>>> +		u8 offset, u8 no_of_bytes)
>>>> +{
>>>> +	return drm_dp_dual_mode_ioa_xfer(adapter, buffer, offset,
>>>> +		no_of_bytes, 0);
>>>> +}
>>>> +EXPORT_SYMBOL(drm_dp_dual_mode_ioa_write);
>>>
>>> Aren't these just dupes of the dual mode helper read/write functions?
>> No, I2C over aux read uses adapter->algo->master_xfer function, which is
>> different in case of aux channel read/write. dp_dual_mode uses i2c_xfer.
>
> .master_xfer() is just an internal implementation detail of the i2c
> bus/algo driver. You're not meant call it directly. i2c_transfer() is
> what you call, and it will end up calling .master_xfer() internally.
>
Please correct me if its not the case, but the master_xfer function 
getting loaded in drm_dp_helper layer is drm_dp_i2c_xfer() which does a 
lot of stuff. You mean to say i2c_xfer() function call will map to 
drm_dp_i2c_xfer ?
>>>
>>>> +
>>>>    static bool is_hdmi_adaptor(const char hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN])
>>>>    {
>>>>    	static const char dp_dual_mode_hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN] =
>>>> @@ -141,6 +234,11 @@ static bool is_hdmi_adaptor(const char hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN])
>>>>    		      sizeof(dp_dual_mode_hdmi_id)) == 0;
>>>>    }
>>>>
>>>> +bool is_lspcon_adaptor(const uint8_t adaptor_id)
>>>> +{
>>>> +	return adaptor_id == 0xa8;
>>>> +}
>>>> +
>>>>    /**
>>>>     * drm_dp_dual_mode_detect - Identyfy the DP dual mode adaptor
>>>>     * adapter: I2C adapter for the DDC bus
>>>> @@ -197,6 +295,8 @@ enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct i2c_adapter *adapter)
>>>>    				    &adaptor_id, sizeof(adaptor_id));
>>>>    	if (ret || (adaptor_id != (DP_DUAL_MODE_TYPE_TYPE2 |
>>>>    				   DP_DUAL_MODE_REV_TYPE2))) {
>>>> +		if (is_lspcon_adaptor(adaptor_id))
>>>> +			return DRM_DP_DUAL_MODE_TYPE2_LSPCON;
>>>
>>> We should reorganize this a bit I think. Probably should end up looking
>>> somehting like:
>>>
>>> bool is_type2_adaptor();
>>> bool is_lspcon_adaptor();
>>>
>>> if (ret == 0) {
>>> 	if (is_lspcon_adaptor())
>>> 		return TYPE2_LSPCON;
>>> 	else if (is_type2_adaptor() && is_hdmi_adaptor)
>>> 		return TYPE2_HDMI;
>>> 	else if (is_type2_adaptor())
>>> 		return TYPE2_DVI;
>>> }
>>>
>>> if (is_hdmi...)
>>> 	return TYPE1_HDMI;
>>> else
>>> 	return TYPE1_DVI;
>>>
>>>
>> Yep, this is a good suggestion.
>
> I already reogranized the type1/2 detection to match this pattern in my
> repost of the dp++ stuff, so should be easy to drop this in:
> https://lists.freedesktop.org/archives/dri-devel/2016-May/106535.html
>
Yep, just started looking into these patches, I will reuse this.
>>>>    		if (is_hdmi_adaptor(hdmi_id))
>>>>    			return DRM_DP_DUAL_MODE_TYPE1_HDMI;
>>>>    		else
>>>> diff --git a/include/drm/drm_dp_dual_mode_helper.h b/include/drm/drm_dp_dual_mode_helper.h
>>>> index 8f8a8dc..26e3dd8 100644
>>>> --- a/include/drm/drm_dp_dual_mode_helper.h
>>>> +++ b/include/drm/drm_dp_dual_mode_helper.h
>>>> @@ -24,6 +24,7 @@
>>>>    #define DRM_DP_DUAL_MODE_HELPER_H
>>>>
>>>>    #include <linux/types.h>
>>>> +#include <drm/drm_edid.h>
>>>>
>>>>    /*
>>>>     * Optional for type 1 DVI adaptors
>>>> @@ -55,6 +56,7 @@
>>>>    #define  DP_DUAL_MODE_CEC_ENABLE 0x01
>>>>    #define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22
>>>>    #define DP_DUAL_MODE_LAST_RESERVED 0xff
>>>> +#define DP_DUAL_MODE_DDC_SEGMENT_ADDR 0x30
>>>>
>>>>    struct i2c_adapter;
>>>>
>>>> @@ -69,6 +71,7 @@ enum drm_dp_dual_mode_type {
>>>>    	DRM_DP_DUAL_MODE_TYPE1_HDMI,
>>>>    	DRM_DP_DUAL_MODE_TYPE2_DVI,
>>>>    	DRM_DP_DUAL_MODE_TYPE2_HDMI,
>>>> +	DRM_DP_DUAL_MODE_TYPE2_LSPCON,
>>>>    };
>>>>
>>>>    enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct i2c_adapter *adapter);
>>>> @@ -81,4 +84,11 @@ drm_dp_dual_mode_set_tmds_output(struct i2c_adapter *adapter, bool enable);
>>>>    const char
>>>>    *drm_dp_get_dual_mode_type_name(enum drm_dp_dual_mode_type type);
>>>>
>>>> +int drm_dp_dual_mode_ioa_read(struct i2c_adapter *adapter, u8 *buffer,
>>>> +		u8 offset, u8 no_of_bytes);
>>>> +int drm_dp_dual_mode_ioa_write(struct i2c_adapter *adapter, u8 *buffer,
>>>> +		u8 offset, u8 no_of_bytes);
>>>> +int drm_dp_dual_mode_get_edid(void *data,
>>>> +	u8 *buf, unsigned int block, size_t len);
>>>> +
>>>>    #endif
>>>> --
>>>> 1.9.1
>>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx