| Message ID | 1464686746-5099-2-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 31, 2016 at 02:55:42PM +0530, Shashank Sharma wrote: > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/drm_dp_dual_mode_helper.c | 85 +++++++++++++++++++++++++++++++ > include/drm/drm_dp_dual_mode_helper.h | 29 +++++++++++ > 2 files changed, 114 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..11cab25 100644 > --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c > +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c > @@ -148,6 +148,12 @@ 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 == 0xa8); I'd probably make this something like #define DP_DUAL_MODE_RESERVED_LSPCON 0x08 return adaptor_id == (DP_DUAL_MODE_TYPE_TYPE2 | DP_DUAL_MODE_RESERVED_LSPCON | DP_DUAL_MODE_REV_TYPE2); > +} > + > /** > * drm_dp_dual_mode_detect - Identify the DP dual mode adaptor > * @adapter: I2C adapter for the DDC bus > @@ -203,6 +209,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 +372,80 @@ 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 > + * > + * Returns: > + * Enum representing current mode of operation > + */ > +enum drm_lspcon_mode > +drm_lspcon_get_current_mode(struct i2c_adapter *adapter) To match with drm_dp_dual_mode_get_tmds_output() this may want to return the error status, and return the actual mode via a function argument. As for the name drm_dp_dual_mode_get_lspcon_mode() and drm_dp_dual_mode_set_lspcon_mode() perhaps? Or drm_lspcon_get_mode()/drm_lspcon_set_mode() perhaps if we want to keep things shorter? > +{ > + u8 data; > + int err = 0; I used 'ret' in the rest of the code. This is a bit inconsistent. > + > + /* Read Status: i2c over aux */ > + err = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE, > + (void *)&data, sizeof(data)); Pointless cast. > + if (err < 0) { if (ret) is what I've used elsewhere. > + DRM_ERROR("LSPCON read(0x80, 0x41) failed\n"); I'm not sure we want and ERROR from a helper. I used DRM_DEBUG_KMS for the other error paths. > + return DRM_LSPCON_MODE_INVALID; > + } > + > + return data & DP_DUAL_MODE_LSPCON_MODE_MASK ? > + DRM_LSPCON_MODE_PCON : DRM_LSPCON_MODE_LS; > +} > +EXPORT_SYMBOL(drm_lspcon_get_current_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_change_mode(struct i2c_adapter *adapter, > + enum drm_lspcon_mode reqd_mode) > +{ > + u8 data; > + int err; > + int time_out = 200; > + > + if (reqd_mode == DRM_LSPCON_MODE_LS) > + data = ~DP_DUAL_MODE_LSPCON_MODE_MASK; Is that really correct? > + else > + data = DP_DUAL_MODE_LSPCON_MODE_MASK; > + > + /* Change mode */ > + err = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_LSPCON_MODE_CHANGE, > + &data, sizeof(data)); > + if (err < 0) { > + DRM_ERROR("LSPCON mode change failed\n"); > + return err; > + } > + > + /* > + * 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. > + */ indent fail > + while (time_out) { > + if (reqd_mode != drm_lspcon_get_current_mode(adapter)) { > + mdelay(10); mdelay() is bad. > + time_out -= 10; > + } else { > + DRM_DEBUG_KMS("LSPCON mode changed to %s\n", > + reqd_mode == DRM_LSPCON_MODE_LS ? "LS" : "PCON"); > + return 0; > + } > + } > + > + DRM_ERROR("LSPCON mode change timed out\n"); > + return -EFAULT; EFAULT doens't seem like a sensible error value here. ETIMEDOUT I suppose seems like what we should return. > +} > +EXPORT_SYMBOL(drm_lspcon_change_mode); > diff --git a/include/drm/drm_dp_dual_mode_helper.h b/include/drm/drm_dp_dual_mode_helper.h > index e8a9dfd..f335df5 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 > @@ -54,6 +55,9 @@ > #define DP_DUAL_MODE_HDMI_PIN_CTRL 0x21 > #define DP_DUAL_MODE_CEC_ENABLE 0x01 > #define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22 Maybe leave an empty line here to separate the standard regs from the vendor specific ones. Hmm. Actually the vendor register space is 0x30-0x3f, whereas 0x40+ is reserved. But anyway empty line would seem warranted. > +#define DP_DUAL_MODE_LSPCON_MODE_CHANGE 0x40 > +#define DP_DUAL_MODE_LSPCON_CURRENT_MODE 0x41 > +#define DP_DUAL_MODE_LSPCON_MODE_MASK 0x1 Please indent the bit defnition by one space to make it stand out from the reg defines a little. > struct i2c_adapter; > > @@ -63,6 +67,20 @@ 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_invalid: No idea what's going on > +* @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. > +*/ > +enum drm_lspcon_mode { > + DRM_LSPCON_MODE_INVALID, > + DRM_LSPCON_MODE_LS, > + DRM_LSPCON_MODE_PCON, > +}; If we change the _get_mode() thing to return the error status separately from the mode, we won't need the INVALID value. > + > +/** > * 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 +88,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 +97,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 +109,13 @@ 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_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); > +enum drm_lspcon_mode drm_lspcon_get_current_mode(struct i2c_adapter *adapter); > +int drm_lspcon_change_mode(struct i2c_adapter *adapter, > + enum drm_lspcon_mode reqd_mode); > #endif > -- > 1.9.1
Thanks for the review, Ville. My comments, inline. Regards Shashank On 5/31/2016 3:22 PM, Ville Syrjälä wrote: > On Tue, May 31, 2016 at 02:55:42PM +0530, Shashank Sharma wrote: >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> drivers/gpu/drm/drm_dp_dual_mode_helper.c | 85 +++++++++++++++++++++++++++++++ >> include/drm/drm_dp_dual_mode_helper.h | 29 +++++++++++ >> 2 files changed, 114 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..11cab25 100644 >> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c >> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c >> @@ -148,6 +148,12 @@ 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 == 0xa8); > > > I'd probably make this something like > > #define DP_DUAL_MODE_RESERVED_LSPCON 0x08 > > return adaptor_id == (DP_DUAL_MODE_TYPE_TYPE2 | > DP_DUAL_MODE_RESERVED_LSPCON | > DP_DUAL_MODE_REV_TYPE2); > Thanks, this is a good suggestion, I will do so. > >> +} >> + >> /** >> * drm_dp_dual_mode_detect - Identify the DP dual mode adaptor >> * @adapter: I2C adapter for the DDC bus >> @@ -203,6 +209,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 +372,80 @@ 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 >> + * >> + * Returns: >> + * Enum representing current mode of operation >> + */ >> +enum drm_lspcon_mode >> +drm_lspcon_get_current_mode(struct i2c_adapter *adapter) > > To match with drm_dp_dual_mode_get_tmds_output() this may want to > return the error status, and return the actual mode via a function > argument. > sure, can be done. > As for the name drm_dp_dual_mode_get_lspcon_mode() and > drm_dp_dual_mode_set_lspcon_mode() perhaps? > Or drm_lspcon_get_mode()/drm_lspcon_set_mode() perhaps if we want to > keep things shorter? > Yep, initially I made the same names as you suggested, but drm_dp_dual_mode_get_lspcon_mode dint sound good to me :). drm_lspcon_get/set_mode sounds good. >> +{ >> + u8 data; >> + int err = 0; > > I used 'ret' in the rest of the code. This is a bit inconsistent. got it. > >> + >> + /* Read Status: i2c over aux */ >> + err = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE, >> + (void *)&data, sizeof(data)); > > Pointless cast. > good catch, this is unnecessary. >> + if (err < 0) { > > if (ret) is what I've used elsewhere. got it. > >> + DRM_ERROR("LSPCON read(0x80, 0x41) failed\n"); > > I'm not sure we want and ERROR from a helper. I used DRM_DEBUG_KMS for > the other error paths. > IMHO, This is a failure which needs notice, as it can point out to a HW level problem, or a problem in LSPCON FW, so I will prefer to keep it as error. >> + return DRM_LSPCON_MODE_INVALID; >> + } >> + >> + return data & DP_DUAL_MODE_LSPCON_MODE_MASK ? >> + DRM_LSPCON_MODE_PCON : DRM_LSPCON_MODE_LS; >> +} >> +EXPORT_SYMBOL(drm_lspcon_get_current_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_change_mode(struct i2c_adapter *adapter, >> + enum drm_lspcon_mode reqd_mode) >> +{ >> + u8 data; >> + int err; >> + int time_out = 200; >> + >> + if (reqd_mode == DRM_LSPCON_MODE_LS) >> + data = ~DP_DUAL_MODE_LSPCON_MODE_MASK; > > Is that really correct? > Oops, it was a blunder, but fortunately that register only bothers about bit 0, and that's why it worked, I will fix this. >> + else >> + data = DP_DUAL_MODE_LSPCON_MODE_MASK; >> + >> + /* Change mode */ >> + err = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_LSPCON_MODE_CHANGE, >> + &data, sizeof(data)); >> + if (err < 0) { >> + DRM_ERROR("LSPCON mode change failed\n"); >> + return err; >> + } >> + >> + /* >> + * 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. >> + */ > > indent fail OK. > >> + while (time_out) { >> + if (reqd_mode != drm_lspcon_get_current_mode(adapter)) { >> + mdelay(10); > > mdelay() is bad. Do you think I should replace with some of the non-busy wait (like msleep or so ?) > >> + time_out -= 10; >> + } else { >> + DRM_DEBUG_KMS("LSPCON mode changed to %s\n", >> + reqd_mode == DRM_LSPCON_MODE_LS ? "LS" : "PCON"); >> + return 0; >> + } >> + } >> + >> + DRM_ERROR("LSPCON mode change timed out\n"); >> + return -EFAULT; > > EFAULT doens't seem like a sensible error value here. ETIMEDOUT I > suppose seems like what we should return. Sure. > >> +} >> +EXPORT_SYMBOL(drm_lspcon_change_mode); >> diff --git a/include/drm/drm_dp_dual_mode_helper.h b/include/drm/drm_dp_dual_mode_helper.h >> index e8a9dfd..f335df5 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 >> @@ -54,6 +55,9 @@ >> #define DP_DUAL_MODE_HDMI_PIN_CTRL 0x21 >> #define DP_DUAL_MODE_CEC_ENABLE 0x01 >> #define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22 > > Maybe leave an empty line here to separate the standard regs from the > vendor specific ones. Hmm. Actually the vendor register space is > 0x30-0x3f, whereas 0x40+ is reserved. But anyway empty line would > seem warranted. > Got it. >> +#define DP_DUAL_MODE_LSPCON_MODE_CHANGE 0x40 >> +#define DP_DUAL_MODE_LSPCON_CURRENT_MODE 0x41 >> +#define DP_DUAL_MODE_LSPCON_MODE_MASK 0x1 > > Please indent the bit defnition by one space to make it stand out from > the reg defines a little. > Ok, the first two are registers only, I guess this is for DP_DUAL_MODE_LSPCON_MODE_MASK. I will align it. >> struct i2c_adapter; >> >> @@ -63,6 +67,20 @@ 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_invalid: No idea what's going on >> +* @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. >> +*/ >> +enum drm_lspcon_mode { >> + DRM_LSPCON_MODE_INVALID, >> + DRM_LSPCON_MODE_LS, >> + DRM_LSPCON_MODE_PCON, >> +}; > > If we change the _get_mode() thing to return the error status separately > from the mode, we won't need the INVALID value. > Agree. >> + >> +/** >> * 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 +88,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 +97,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 +109,13 @@ 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_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); >> +enum drm_lspcon_mode drm_lspcon_get_current_mode(struct i2c_adapter *adapter); >> +int drm_lspcon_change_mode(struct i2c_adapter *adapter, >> + enum drm_lspcon_mode reqd_mode); >> #endif >> -- >> 1.9.1 >
On Tue, May 31, 2016 at 04:22:08PM +0530, Sharma, Shashank wrote: > Thanks for the review, Ville. > My comments, inline. > > Regards > Shashank > On 5/31/2016 3:22 PM, Ville Syrjälä wrote: > > On Tue, May 31, 2016 at 02:55:42PM +0530, Shashank Sharma wrote: > >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > >> --- > >> drivers/gpu/drm/drm_dp_dual_mode_helper.c | 85 +++++++++++++++++++++++++++++++ > >> include/drm/drm_dp_dual_mode_helper.h | 29 +++++++++++ > >> 2 files changed, 114 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..11cab25 100644 > >> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c > >> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c > >> @@ -148,6 +148,12 @@ 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 == 0xa8); > > > > > > I'd probably make this something like > > > > #define DP_DUAL_MODE_RESERVED_LSPCON 0x08 > > > > return adaptor_id == (DP_DUAL_MODE_TYPE_TYPE2 | > > DP_DUAL_MODE_RESERVED_LSPCON | > > DP_DUAL_MODE_REV_TYPE2); > > > Thanks, this is a good suggestion, I will do so. > > > >> +} > >> + > >> /** > >> * drm_dp_dual_mode_detect - Identify the DP dual mode adaptor > >> * @adapter: I2C adapter for the DDC bus > >> @@ -203,6 +209,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 +372,80 @@ 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 > >> + * > >> + * Returns: > >> + * Enum representing current mode of operation > >> + */ > >> +enum drm_lspcon_mode > >> +drm_lspcon_get_current_mode(struct i2c_adapter *adapter) > > > > To match with drm_dp_dual_mode_get_tmds_output() this may want to > > return the error status, and return the actual mode via a function > > argument. > > > sure, can be done. > > As for the name drm_dp_dual_mode_get_lspcon_mode() and > > drm_dp_dual_mode_set_lspcon_mode() perhaps? > > Or drm_lspcon_get_mode()/drm_lspcon_set_mode() perhaps if we want to > > keep things shorter? > > > Yep, initially I made the same names as you suggested, but > drm_dp_dual_mode_get_lspcon_mode dint sound good to me :). > drm_lspcon_get/set_mode sounds good. > >> +{ > >> + u8 data; > >> + int err = 0; > > > > I used 'ret' in the rest of the code. This is a bit inconsistent. > got it. > > > >> + > >> + /* Read Status: i2c over aux */ > >> + err = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE, > >> + (void *)&data, sizeof(data)); > > > > Pointless cast. > > > good catch, this is unnecessary. > >> + if (err < 0) { > > > > if (ret) is what I've used elsewhere. > got it. > > > >> + DRM_ERROR("LSPCON read(0x80, 0x41) failed\n"); > > > > I'm not sure we want and ERROR from a helper. I used DRM_DEBUG_KMS for > > the other error paths. > > > IMHO, This is a failure which needs notice, as it can point out to a HW > level problem, or a problem in LSPCON FW, so I will prefer to keep it as > error. Fair enough. > >> + return DRM_LSPCON_MODE_INVALID; > >> + } > >> + > >> + return data & DP_DUAL_MODE_LSPCON_MODE_MASK ? > >> + DRM_LSPCON_MODE_PCON : DRM_LSPCON_MODE_LS; > >> +} > >> +EXPORT_SYMBOL(drm_lspcon_get_current_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_change_mode(struct i2c_adapter *adapter, > >> + enum drm_lspcon_mode reqd_mode) > >> +{ > >> + u8 data; > >> + int err; > >> + int time_out = 200; > >> + > >> + if (reqd_mode == DRM_LSPCON_MODE_LS) > >> + data = ~DP_DUAL_MODE_LSPCON_MODE_MASK; > > > > Is that really correct? > > > Oops, it was a blunder, but fortunately that register only bothers about > bit 0, and that's why it worked, I will fix this. > >> + else > >> + data = DP_DUAL_MODE_LSPCON_MODE_MASK; > >> + > >> + /* Change mode */ > >> + err = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_LSPCON_MODE_CHANGE, > >> + &data, sizeof(data)); > >> + if (err < 0) { > >> + DRM_ERROR("LSPCON mode change failed\n"); > >> + return err; > >> + } > >> + > >> + /* > >> + * 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. > >> + */ > > > > indent fail > OK. > > > >> + while (time_out) { > >> + if (reqd_mode != drm_lspcon_get_current_mode(adapter)) { > >> + mdelay(10); > > > > mdelay() is bad. > Do you think I should replace with some of the non-busy wait (like > msleep or so ?) How long would we typically have to wait for the mode to change? > > > >> + time_out -= 10; > >> + } else { > >> + DRM_DEBUG_KMS("LSPCON mode changed to %s\n", > >> + reqd_mode == DRM_LSPCON_MODE_LS ? "LS" : "PCON"); > >> + return 0; > >> + } > >> + } > >> + > >> + DRM_ERROR("LSPCON mode change timed out\n"); > >> + return -EFAULT; > > > > EFAULT doens't seem like a sensible error value here. ETIMEDOUT I > > suppose seems like what we should return. > Sure. > > > >> +} > >> +EXPORT_SYMBOL(drm_lspcon_change_mode); > >> diff --git a/include/drm/drm_dp_dual_mode_helper.h b/include/drm/drm_dp_dual_mode_helper.h > >> index e8a9dfd..f335df5 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 > >> @@ -54,6 +55,9 @@ > >> #define DP_DUAL_MODE_HDMI_PIN_CTRL 0x21 > >> #define DP_DUAL_MODE_CEC_ENABLE 0x01 > >> #define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22 > > > > Maybe leave an empty line here to separate the standard regs from the > > vendor specific ones. Hmm. Actually the vendor register space is > > 0x30-0x3f, whereas 0x40+ is reserved. But anyway empty line would > > seem warranted. > > > Got it. > >> +#define DP_DUAL_MODE_LSPCON_MODE_CHANGE 0x40 > >> +#define DP_DUAL_MODE_LSPCON_CURRENT_MODE 0x41 > >> +#define DP_DUAL_MODE_LSPCON_MODE_MASK 0x1 > > > > Please indent the bit defnition by one space to make it stand out from > > the reg defines a little. > > > Ok, the first two are registers only, I guess this is for > DP_DUAL_MODE_LSPCON_MODE_MASK. I will align it. > >> struct i2c_adapter; > >> > >> @@ -63,6 +67,20 @@ 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_invalid: No idea what's going on > >> +* @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. > >> +*/ > >> +enum drm_lspcon_mode { > >> + DRM_LSPCON_MODE_INVALID, > >> + DRM_LSPCON_MODE_LS, > >> + DRM_LSPCON_MODE_PCON, > >> +}; > > > > If we change the _get_mode() thing to return the error status separately > > from the mode, we won't need the INVALID value. > > > Agree. > >> + > >> +/** > >> * 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 +88,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 +97,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 +109,13 @@ 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_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); > >> +enum drm_lspcon_mode drm_lspcon_get_current_mode(struct i2c_adapter *adapter); > >> +int drm_lspcon_change_mode(struct i2c_adapter *adapter, > >> + enum drm_lspcon_mode reqd_mode); > >> #endif > >> -- > >> 1.9.1 > >
Regards Shashank On 5/31/2016 5:40 PM, Ville Syrjälä wrote: > On Tue, May 31, 2016 at 04:22:08PM +0530, Sharma, Shashank wrote: >> Thanks for the review, Ville. >> My comments, inline. >> >> Regards >> Shashank >> On 5/31/2016 3:22 PM, Ville Syrjälä wrote: >>> On Tue, May 31, 2016 at 02:55:42PM +0530, Shashank Sharma wrote: >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>>> --- >>>> drivers/gpu/drm/drm_dp_dual_mode_helper.c | 85 +++++++++++++++++++++++++++++++ >>>> include/drm/drm_dp_dual_mode_helper.h | 29 +++++++++++ >>>> 2 files changed, 114 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..11cab25 100644 >>>> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c >>>> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c >>>> @@ -148,6 +148,12 @@ 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 == 0xa8); >>> >>> >>> I'd probably make this something like >>> >>> #define DP_DUAL_MODE_RESERVED_LSPCON 0x08 >>> >>> return adaptor_id == (DP_DUAL_MODE_TYPE_TYPE2 | >>> DP_DUAL_MODE_RESERVED_LSPCON | >>> DP_DUAL_MODE_REV_TYPE2); >>> >> Thanks, this is a good suggestion, I will do so. >>> >>>> +} >>>> + >>>> /** >>>> * drm_dp_dual_mode_detect - Identify the DP dual mode adaptor >>>> * @adapter: I2C adapter for the DDC bus >>>> @@ -203,6 +209,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 +372,80 @@ 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 >>>> + * >>>> + * Returns: >>>> + * Enum representing current mode of operation >>>> + */ >>>> +enum drm_lspcon_mode >>>> +drm_lspcon_get_current_mode(struct i2c_adapter *adapter) >>> >>> To match with drm_dp_dual_mode_get_tmds_output() this may want to >>> return the error status, and return the actual mode via a function >>> argument. >>> >> sure, can be done. >>> As for the name drm_dp_dual_mode_get_lspcon_mode() and >>> drm_dp_dual_mode_set_lspcon_mode() perhaps? >>> Or drm_lspcon_get_mode()/drm_lspcon_set_mode() perhaps if we want to >>> keep things shorter? >>> >> Yep, initially I made the same names as you suggested, but >> drm_dp_dual_mode_get_lspcon_mode dint sound good to me :). >> drm_lspcon_get/set_mode sounds good. >>>> +{ >>>> + u8 data; >>>> + int err = 0; >>> >>> I used 'ret' in the rest of the code. This is a bit inconsistent. >> got it. >>> >>>> + >>>> + /* Read Status: i2c over aux */ >>>> + err = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE, >>>> + (void *)&data, sizeof(data)); >>> >>> Pointless cast. >>> >> good catch, this is unnecessary. >>>> + if (err < 0) { >>> >>> if (ret) is what I've used elsewhere. >> got it. >>> >>>> + DRM_ERROR("LSPCON read(0x80, 0x41) failed\n"); >>> >>> I'm not sure we want and ERROR from a helper. I used DRM_DEBUG_KMS for >>> the other error paths. >>> >> IMHO, This is a failure which needs notice, as it can point out to a HW >> level problem, or a problem in LSPCON FW, so I will prefer to keep it as >> error. > > Fair enough. > >>>> + return DRM_LSPCON_MODE_INVALID; >>>> + } >>>> + >>>> + return data & DP_DUAL_MODE_LSPCON_MODE_MASK ? >>>> + DRM_LSPCON_MODE_PCON : DRM_LSPCON_MODE_LS; >>>> +} >>>> +EXPORT_SYMBOL(drm_lspcon_get_current_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_change_mode(struct i2c_adapter *adapter, >>>> + enum drm_lspcon_mode reqd_mode) >>>> +{ >>>> + u8 data; >>>> + int err; >>>> + int time_out = 200; >>>> + >>>> + if (reqd_mode == DRM_LSPCON_MODE_LS) >>>> + data = ~DP_DUAL_MODE_LSPCON_MODE_MASK; >>> >>> Is that really correct? >>> >> Oops, it was a blunder, but fortunately that register only bothers about >> bit 0, and that's why it worked, I will fix this. >>>> + else >>>> + data = DP_DUAL_MODE_LSPCON_MODE_MASK; >>>> + >>>> + /* Change mode */ >>>> + err = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_LSPCON_MODE_CHANGE, >>>> + &data, sizeof(data)); >>>> + if (err < 0) { >>>> + DRM_ERROR("LSPCON mode change failed\n"); >>>> + return err; >>>> + } >>>> + >>>> + /* >>>> + * 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. >>>> + */ >>> >>> indent fail >> OK. >>> >>>> + while (time_out) { >>>> + if (reqd_mode != drm_lspcon_get_current_mode(adapter)) { >>>> + mdelay(10); >>> >>> mdelay() is bad. >> Do you think I should replace with some of the non-busy wait (like >> msleep or so ?) > > How long would we typically have to wait for the mode to change? > In a normal scenario, it takes max 1-2 iterations (max 20ms) but on Android trees, We had initially observed it taking as much as 300ms, (but thats once in a blue moon). MCA has made some changes in the latest LSPCON FW, to make it better. But 200ms is just to be in the safer side. >>> >>>> + time_out -= 10; >>>> + } else { >>>> + DRM_DEBUG_KMS("LSPCON mode changed to %s\n", >>>> + reqd_mode == DRM_LSPCON_MODE_LS ? "LS" : "PCON"); >>>> + return 0; >>>> + } >>>> + } >>>> + >>>> + DRM_ERROR("LSPCON mode change timed out\n"); >>>> + return -EFAULT; >>> >>> EFAULT doens't seem like a sensible error value here. ETIMEDOUT I >>> suppose seems like what we should return. >> Sure. >>> >>>> +} >>>> +EXPORT_SYMBOL(drm_lspcon_change_mode); >>>> diff --git a/include/drm/drm_dp_dual_mode_helper.h b/include/drm/drm_dp_dual_mode_helper.h >>>> index e8a9dfd..f335df5 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 >>>> @@ -54,6 +55,9 @@ >>>> #define DP_DUAL_MODE_HDMI_PIN_CTRL 0x21 >>>> #define DP_DUAL_MODE_CEC_ENABLE 0x01 >>>> #define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22 >>> >>> Maybe leave an empty line here to separate the standard regs from the >>> vendor specific ones. Hmm. Actually the vendor register space is >>> 0x30-0x3f, whereas 0x40+ is reserved. But anyway empty line would >>> seem warranted. >>> >> Got it. >>>> +#define DP_DUAL_MODE_LSPCON_MODE_CHANGE 0x40 >>>> +#define DP_DUAL_MODE_LSPCON_CURRENT_MODE 0x41 >>>> +#define DP_DUAL_MODE_LSPCON_MODE_MASK 0x1 >>> >>> Please indent the bit defnition by one space to make it stand out from >>> the reg defines a little. >>> >> Ok, the first two are registers only, I guess this is for >> DP_DUAL_MODE_LSPCON_MODE_MASK. I will align it. >>>> struct i2c_adapter; >>>> >>>> @@ -63,6 +67,20 @@ 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_invalid: No idea what's going on >>>> +* @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. >>>> +*/ >>>> +enum drm_lspcon_mode { >>>> + DRM_LSPCON_MODE_INVALID, >>>> + DRM_LSPCON_MODE_LS, >>>> + DRM_LSPCON_MODE_PCON, >>>> +}; >>> >>> If we change the _get_mode() thing to return the error status separately >>> from the mode, we won't need the INVALID value. >>> >> Agree. >>>> + >>>> +/** >>>> * 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 +88,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 +97,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 +109,13 @@ 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_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); >>>> +enum drm_lspcon_mode drm_lspcon_get_current_mode(struct i2c_adapter *adapter); >>>> +int drm_lspcon_change_mode(struct i2c_adapter *adapter, >>>> + enum drm_lspcon_mode reqd_mode); >>>> #endif >>>> -- >>>> 1.9.1 >>> >
On Tue, May 31, 2016 at 02:55:42PM +0530, Shashank Sharma wrote: > @@ -78,6 +97,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 +109,13 @@ 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_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); These look like leftovers from an earlier attempt. > +enum drm_lspcon_mode drm_lspcon_get_current_mode(struct i2c_adapter *adapter); > +int drm_lspcon_change_mode(struct i2c_adapter *adapter, > + enum drm_lspcon_mode reqd_mode); > #endif > -- > 1.9.1
Regards Shashank On 5/31/2016 9:35 PM, Ville Syrjälä wrote: > On Tue, May 31, 2016 at 02:55:42PM +0530, Shashank Sharma wrote: >> @@ -78,6 +97,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 +109,13 @@ 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_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); > > These look like leftovers from an earlier attempt. Yes, I noticed I missed them during cleanup. Will remove these. > >> +enum drm_lspcon_mode drm_lspcon_get_current_mode(struct i2c_adapter *adapter); >> +int drm_lspcon_change_mode(struct i2c_adapter *adapter, >> + enum drm_lspcon_mode reqd_mode); >> #endif >> -- >> 1.9.1 >
diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c index a7b2a75..11cab25 100644 --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c @@ -148,6 +148,12 @@ 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 == 0xa8); +} + /** * drm_dp_dual_mode_detect - Identify the DP dual mode adaptor * @adapter: I2C adapter for the DDC bus @@ -203,6 +209,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 +372,80 @@ 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 + * + * Returns: + * Enum representing current mode of operation + */ +enum drm_lspcon_mode +drm_lspcon_get_current_mode(struct i2c_adapter *adapter) +{ + u8 data; + int err = 0; + + /* Read Status: i2c over aux */ + err = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE, + (void *)&data, sizeof(data)); + if (err < 0) { + DRM_ERROR("LSPCON read(0x80, 0x41) failed\n"); + return DRM_LSPCON_MODE_INVALID; + } + + return data & DP_DUAL_MODE_LSPCON_MODE_MASK ? + DRM_LSPCON_MODE_PCON : DRM_LSPCON_MODE_LS; +} +EXPORT_SYMBOL(drm_lspcon_get_current_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_change_mode(struct i2c_adapter *adapter, + enum drm_lspcon_mode reqd_mode) +{ + u8 data; + int err; + int time_out = 200; + + if (reqd_mode == DRM_LSPCON_MODE_LS) + data = ~DP_DUAL_MODE_LSPCON_MODE_MASK; + else + data = DP_DUAL_MODE_LSPCON_MODE_MASK; + + /* Change mode */ + err = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_LSPCON_MODE_CHANGE, + &data, sizeof(data)); + if (err < 0) { + DRM_ERROR("LSPCON mode change failed\n"); + return err; + } + + /* + * 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. + */ + while (time_out) { + if (reqd_mode != drm_lspcon_get_current_mode(adapter)) { + mdelay(10); + time_out -= 10; + } else { + DRM_DEBUG_KMS("LSPCON mode changed to %s\n", + reqd_mode == DRM_LSPCON_MODE_LS ? "LS" : "PCON"); + return 0; + } + } + + DRM_ERROR("LSPCON mode change timed out\n"); + return -EFAULT; +} +EXPORT_SYMBOL(drm_lspcon_change_mode); diff --git a/include/drm/drm_dp_dual_mode_helper.h b/include/drm/drm_dp_dual_mode_helper.h index e8a9dfd..f335df5 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 @@ -54,6 +55,9 @@ #define DP_DUAL_MODE_HDMI_PIN_CTRL 0x21 #define DP_DUAL_MODE_CEC_ENABLE 0x01 #define DP_DUAL_MODE_I2C_SPEED_CTRL 0x22 +#define DP_DUAL_MODE_LSPCON_MODE_CHANGE 0x40 +#define DP_DUAL_MODE_LSPCON_CURRENT_MODE 0x41 +#define DP_DUAL_MODE_LSPCON_MODE_MASK 0x1 struct i2c_adapter; @@ -63,6 +67,20 @@ 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_invalid: No idea what's going on +* @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. +*/ +enum drm_lspcon_mode { + DRM_LSPCON_MODE_INVALID, + 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 +88,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 +97,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 +109,13 @@ 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_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); +enum drm_lspcon_mode drm_lspcon_get_current_mode(struct i2c_adapter *adapter); +int drm_lspcon_change_mode(struct i2c_adapter *adapter, + enum drm_lspcon_mode reqd_mode); #endif
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> --- drivers/gpu/drm/drm_dp_dual_mode_helper.c | 85 +++++++++++++++++++++++++++++++ include/drm/drm_dp_dual_mode_helper.h | 29 +++++++++++ 2 files changed, 114 insertions(+)