[Intel-gfx] [PATCH v3 1/4] drm: Helper for lspcon in drm_dp_dual_mode
Sharma, Shashank
shashank.sharma at intel.com
Thu Aug 11 08:44:10 UTC 2016
Thanks for the review, Ville.
My comments, inline.
Regards
Shashank
On 8/11/2016 12:19 PM, Ville Syrjälä wrote:
> On Tue, Jul 05, 2016 at 06:35:47PM +0530, Shashank Sharma wrote:
>> This patch adds lspcon support in dp_dual_mode helper.
>> lspcon is essentially a dp->hdmi dongle with dual personality.
>>
>> LS mode: It works as a passive dongle, by level shifting DP++
>> signals to HDMI signals, in LS mode.
>> PCON mode: It works as a protocol converter active dongle
>> in pcon mode, by converting DP++ outputs to HDMI 2.0 outputs.
>>
>> This patch adds support for lspcon detection and mode set
>> switch operations, as a dp dual mode dongle.
>>
>> v2: Addressed review comments from Ville
>> - add adaptor id for lspcon devices (0x08), use it to identify lspcon
>> - change function names
>> old: drm_lspcon_get_current_mode/drm_lspcon_change_mode
>> new: drm_lspcon_get_mode/drm_lspcon_set_mode
>> - change drm_lspcon_get_mode type to int, to match
>> drm_dp_dual_mode_get_tmds_output
>> - change 'err' to 'ret' to match the rest of the functions
>> - remove pointless typecasting during call to dual_mode_read
>> - fix the but while setting value of data, while writing lspcon mode
>> - fix indentation
>> - change mdelay(10) -> msleep(10)
>> - return ETIMEDOUT instead of EFAULT, when lspcon mode change times out
>> - Add an empty line to separate std regs macros and lspcon regs macros
>> Indent bit definition
>>
>> v3: Addressed review comments from Rodrigo
>> - change macro name from DP_DUAL_MODE_TYPE_LSPCON to
>> DP_DUAL_MODE_TYPE_HAS_DPCD for better readability
>> - change macro name from DP_DUAL_MODE_LSPCON_MODE_PCON to
>> DP_DUAL_MODE_LSPCON_MODE_PCON for better readability
>> - add comment for MCA specific offsets like 0x40 and 0x41
>> - remove DP_DUAL_MODE_REV_TYPE2 check while checking lspcon adapter id
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
>> ---
>> drivers/gpu/drm/drm_dp_dual_mode_helper.c | 103 ++++++++++++++++++++++++++++++
>> include/drm/drm_dp_dual_mode_helper.h | 26 ++++++++
>> 2 files changed, 129 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>> index a7b2a75..4f89e0b 100644
>> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>> @@ -148,6 +148,14 @@ static bool is_type2_adaptor(uint8_t adaptor_id)
>> DP_DUAL_MODE_REV_TYPE2);
>> }
>>
>> +bool is_lspcon_adaptor(const char hdmi_id[DP_DUAL_MODE_HDMI_ID_LEN],
>> + const uint8_t adaptor_id)
>> +{
>> + return is_hdmi_adaptor(hdmi_id) &&
>> + (adaptor_id == (DP_DUAL_MODE_TYPE_TYPE2 |
>> + DP_DUAL_MODE_TYPE_HAS_DPCD));
> Looks like an indent fail here.
Can you please let me know, why do we call it indent fail ? checkpatch
doesn't complaint about this.
>> +}
>> +
>> /**
>> * drm_dp_dual_mode_detect - Identify the DP dual mode adaptor
>> * @adapter: I2C adapter for the DDC bus
>> @@ -203,6 +211,8 @@ enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct i2c_adapter *adapter)
>> ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_ADAPTOR_ID,
>> &adaptor_id, sizeof(adaptor_id));
>> if (ret == 0) {
>> + if (is_lspcon_adaptor(hdmi_id, adaptor_id))
>> + return DRM_DP_DUAL_MODE_LSPCON;
>> if (is_type2_adaptor(adaptor_id)) {
>> if (is_hdmi_adaptor(hdmi_id))
>> return DRM_DP_DUAL_MODE_TYPE2_HDMI;
>> @@ -364,3 +374,96 @@ const char *drm_dp_get_dual_mode_type_name(enum drm_dp_dual_mode_type type)
>> }
>> }
>> EXPORT_SYMBOL(drm_dp_get_dual_mode_type_name);
>> +
>> +/**
>> + * drm_lspcon_get_current_mode: Get LSPCON's current mode of operation by
>> + * by reading offset (0x80, 0x41)
>> + * @i2c_adapter: I2C-over-aux adapter
>> + * @current_mode: out vaiable, current lspcon mode of operation
>> + *
>> + * Returns:
>> + * 0 on success, sets the current_mode value to appropriate mode
>> + * -error on failure
>> + */
>> +int drm_lspcon_get_mode(struct i2c_adapter *adapter,
>> + enum drm_lspcon_mode *current_mode)
> indent fail, I would just call it 'mode'
I wanted to be more informative about what is the out parameter, I
though it makes it more readable
that its returning current mode of lspcon. Would you still prefer to
call it mode only ?
>> +{
>> + u8 data;
>> + int ret = 0;
>> +
>> + if (!current_mode) {
>> + DRM_ERROR("NULL input\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* Read Status: i2c over aux */
>> + ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
>> + (void *)&data, sizeof(data));
> indent fail, void* cast not needed.
Sure, will remove void, same question on indent as above.
>> + if (ret < 0) {
>> + DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
>> + return -EFAULT;
>> + }
>> +
>> + if (data & DP_DUAL_MODE_LSPCON_MODE_PCON)
>> + *current_mode = DRM_LSPCON_MODE_PCON;
>> + else
>> + *current_mode = DRM_LSPCON_MODE_LS;
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(drm_lspcon_get_mode);
>> +
>> +/**
>> + * drm_lspcon_change_mode: Change LSPCON's mode of operation by
>> + * by writing offset (0x80, 0x40)
>> + * @i2c_adapter: I2C-over-aux adapter
>> + * @reqd_mode: required mode of operation
>> + *
>> + * Returns:
>> + * 0 on success, -error on failure/timeout
>> + */
>> +int drm_lspcon_set_mode(struct i2c_adapter *adapter,
>> + enum drm_lspcon_mode reqd_mode)
> indent fail, I would just call it 'mode'
Same as above comments about indent.
Now, this function deals with two modes:
one which we wish to move to, and one which we get after mode change
operation.
The mode after mode change operation might not be the same as the one we
wished for.
So wouldn't it be better to make it clear by calling in reqd_mode ?
>> +{
>> + u8 data = 0;
>> + int ret;
>> + int time_out = 200;
>> + enum drm_lspcon_mode changed_mode;
> This I might call 'current_mode'. The declaration can be moved inside
> the loop.
Sure, will do that.
>> +
>> + if (reqd_mode == DRM_LSPCON_MODE_PCON)
>> + data = DP_DUAL_MODE_LSPCON_MODE_PCON;
>> +
>> + /* Change mode */
>> + ret = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_LSPCON_MODE_CHANGE,
>> + &data, sizeof(data));
> indent fail.
Same as above.
>> + if (ret < 0) {
>> + DRM_ERROR("LSPCON mode change failed\n");
>> + return ret;
>> + }
>> +
>> + /*
>> + * Confirm mode change by reading the status bit.
>> + * Sometimes, it takes a while to change the mode,
>> + * so wait and retry until time out or done.
>> + */
>> + do {
>> + ret = drm_lspcon_get_mode(adapter, &changed_mode);
>> + if (ret) {
>> + DRM_ERROR("cant confirm LSPCON mode change\n");
>> + return ret;
>> + } else {
>> + if (changed_mode != reqd_mode) {
>> + msleep(10);
>> + time_out -= 10;
>> + } else {
>> + DRM_DEBUG_KMS("LSPCON mode changed to %s\n",
>> + reqd_mode == DRM_LSPCON_MODE_LS ?
>> + "LS" : "PCON");
>> + return 0;
>> + }
>> + }
>> + } while (time_out);
>> +
>> + DRM_ERROR("LSPCON mode change timed out\n");
>> + return -ETIMEDOUT;
>> +}
>> +EXPORT_SYMBOL(drm_lspcon_set_mode);
>> diff --git a/include/drm/drm_dp_dual_mode_helper.h b/include/drm/drm_dp_dual_mode_helper.h
>> index e8a9dfd..a3c1d03 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>
> Why?
Sorry, leftover. This was required in previous patch set to get the
DDC_ADDR macro when we had separate EDID read function for LSPCON.
Will remove this.
>>
>> /*
>> * Optional for type 1 DVI adaptors
>> @@ -40,6 +41,7 @@
>> #define DP_DUAL_MODE_REV_TYPE2 0x00
>> #define DP_DUAL_MODE_TYPE_MASK 0xf0
>> #define DP_DUAL_MODE_TYPE_TYPE2 0xa0
>> +#define DP_DUAL_MODE_TYPE_HAS_DPCD 0x08
> Maybe add a comment that this came from LSPCON, and it's marked
> reserved in the official dual mode spec.
Ok, got it.
>> #define DP_DUAL_MODE_IEEE_OUI 0x11 /* 11-13*/
>> #define DP_DUAL_IEEE_OUI_LEN 3
>> #define DP_DUAL_DEVICE_ID 0x14 /* 14-19 */
>> @@ -55,6 +57,11 @@
>> #define DP_DUAL_MODE_CEC_ENABLE 0x01
>> #define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22
>>
>> +/* LSPCON specific registers, defined by MCA */
>> +#define DP_DUAL_MODE_LSPCON_MODE_CHANGE 0x40
>> +#define DP_DUAL_MODE_LSPCON_CURRENT_MODE 0x41
>> +#define DP_DUAL_MODE_LSPCON_MODE_PCON 0x1
>> +
>> struct i2c_adapter;
>>
>> ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
>> @@ -63,6 +70,19 @@ ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,
>> u8 offset, const void *buffer, size_t size);
>>
>> /**
>> +* enum drm_lspcon_mode
>> +* @lspcon_mode_ls: Level shifter mode of LSPCON
>> +* which drives DP++ to HDMI 1.4 conversion.
>> +* @lspcon_mode_pcon: Protocol converter mode of LSPCON
>> +* which drives DP++ to HDMI 2.0 active conversion.
> These don't match the enum below.
Ok, will fix it.
>> +*/
>> +enum drm_lspcon_mode {
>> + DRM_LSPCON_MODE_INVALID,
> Did we still have some need for this?
Yes, lspcon probe failure case is using this.
>> + DRM_LSPCON_MODE_LS,
>> + DRM_LSPCON_MODE_PCON,
>> +};
>> +
>> +/**
>> * enum drm_dp_dual_mode_type - Type of the DP dual mode adaptor
>> * @DRM_DP_DUAL_MODE_NONE: No DP dual mode adaptor
>> * @DRM_DP_DUAL_MODE_UNKNOWN: Could be either none or type 1 DVI adaptor
>> @@ -70,6 +90,7 @@ ssize_t drm_dp_dual_mode_write(struct i2c_adapter *adapter,
>> * @DRM_DP_DUAL_MODE_TYPE1_HDMI: Type 1 HDMI adaptor
>> * @DRM_DP_DUAL_MODE_TYPE2_DVI: Type 2 DVI adaptor
>> * @DRM_DP_DUAL_MODE_TYPE2_HDMI: Type 2 HDMI adaptor
>> + * @DRM_DP_DUAL_MODE_TYPE2_LSPCON: Level shifter /protocol converter
>> */
>> enum drm_dp_dual_mode_type {
>> DRM_DP_DUAL_MODE_NONE,
>> @@ -78,6 +99,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_LSPCON,
>> };
>>
>> enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(struct i2c_adapter *adapter);
>> @@ -89,4 +111,8 @@ int drm_dp_dual_mode_set_tmds_output(enum drm_dp_dual_mode_type type,
>> struct i2c_adapter *adapter, bool enable);
>> const char *drm_dp_get_dual_mode_type_name(enum drm_dp_dual_mode_type type);
>>
>> +int drm_lspcon_get_mode(struct i2c_adapter *adapter,
>> + enum drm_lspcon_mode *current_mode);
>> +int drm_lspcon_set_mode(struct i2c_adapter *adapter,
>> + enum drm_lspcon_mode reqd_mode);
> more indent fails.
Same query as previous
Regards
Shashank
>> #endif
>> --
>> 1.9.1
More information about the Intel-gfx
mailing list