* [PATCH] drm/amd/display: Change status's type in aux_reply_transaction_data
@ 2018-09-21 21:55 Nathan Chancellor
2018-09-24 22:07 ` Nick Desaulniers
2018-09-27 18:06 ` [PATCH v2] drm/amd/display: Use proper enums in process_channel_reply Nathan Chancellor
0 siblings, 2 replies; 8+ messages in thread
From: Nathan Chancellor @ 2018-09-21 21:55 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Alex Deucher, Christian König,
David (ChunMing) Zhou
Cc: amd-gfx, dri-devel, linux-kernel, Nick Desaulniers,
Nathan Chancellor
Clang warns when one enumerated type is implicitly converted to another.
drivers/gpu/drm/amd/amdgpu/../display/dc/dce/dce_aux.c:315:19: warning:
implicit conversion from enumeration type 'enum
aux_channel_operation_result' to different enumeration type 'enum
aux_transaction_reply' [-Wenum-conversion]
reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/amd/amdgpu/../display/dc/i2caux/dce110/aux_engine_dce110.c:349:19:
warning: implicit conversion from enumeration type 'enum
aux_channel_operation_result' to different enumeration type 'enum
aux_transaction_reply' [-Wenum-conversion]
reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Instead of implicitly or explicitly converting between types, just
change status to type uint8_t (since its max size is 255) which avoids
this construct altogether.
Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
drivers/gpu/drm/amd/display/dc/dc_ddc_types.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
index 05c8c31d8b31..97e1d4d19263 100644
--- a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
+++ b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
@@ -79,7 +79,7 @@ enum aux_transaction_reply {
};
struct aux_reply_transaction_data {
- enum aux_transaction_reply status;
+ uint8_t status;
uint32_t length;
uint8_t *data;
};
--
2.19.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] drm/amd/display: Change status's type in aux_reply_transaction_data
2018-09-21 21:55 [PATCH] drm/amd/display: Change status's type in aux_reply_transaction_data Nathan Chancellor
@ 2018-09-24 22:07 ` Nick Desaulniers
2018-09-24 22:22 ` Nathan Chancellor
2018-09-27 18:06 ` [PATCH v2] drm/amd/display: Use proper enums in process_channel_reply Nathan Chancellor
1 sibling, 1 reply; 8+ messages in thread
From: Nick Desaulniers @ 2018-09-24 22:07 UTC (permalink / raw)
To: Nathan Chancellor
Cc: harry.wentland, sunpeng.li, alexander.deucher, christian.koenig,
David1.Zhou, amd-gfx, dri-devel, LKML
On Fri, Sep 21, 2018 at 2:55 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> Clang warns when one enumerated type is implicitly converted to another.
>
> drivers/gpu/drm/amd/amdgpu/../display/dc/dce/dce_aux.c:315:19: warning:
> implicit conversion from enumeration type 'enum
> aux_channel_operation_result' to different enumeration type 'enum
> aux_transaction_reply' [-Wenum-conversion]
> reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/amd/amdgpu/../display/dc/i2caux/dce110/aux_engine_dce110.c:349:19:
> warning: implicit conversion from enumeration type 'enum
> aux_channel_operation_result' to different enumeration type 'enum
> aux_transaction_reply' [-Wenum-conversion]
> reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I think the enum is actually wrong here. I think the correct fix would be:
- reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
+ reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON;
The identifiers are so similar, my guess was that it was easy to mix
them up. This looks like an actual bug to me, since the identifiers
have different values between the 2 different enums.
>
> Instead of implicitly or explicitly converting between types, just
> change status to type uint8_t (since its max size is 255) which avoids
> this construct altogether.
>
> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> drivers/gpu/drm/amd/display/dc/dc_ddc_types.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
> index 05c8c31d8b31..97e1d4d19263 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
> @@ -79,7 +79,7 @@ enum aux_transaction_reply {
> };
>
> struct aux_reply_transaction_data {
> - enum aux_transaction_reply status;
> + uint8_t status;
> uint32_t length;
> uint8_t *data;
> };
> --
> 2.19.0
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm/amd/display: Change status's type in aux_reply_transaction_data
2018-09-24 22:07 ` Nick Desaulniers
@ 2018-09-24 22:22 ` Nathan Chancellor
2018-09-27 10:03 ` Harry Wentland
0 siblings, 1 reply; 8+ messages in thread
From: Nathan Chancellor @ 2018-09-24 22:22 UTC (permalink / raw)
To: Nick Desaulniers
Cc: harry.wentland, sunpeng.li, alexander.deucher, christian.koenig,
David1.Zhou, amd-gfx, dri-devel, LKML
On Mon, Sep 24, 2018 at 03:07:16PM -0700, Nick Desaulniers wrote:
> On Fri, Sep 21, 2018 at 2:55 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > Clang warns when one enumerated type is implicitly converted to another.
> >
> > drivers/gpu/drm/amd/amdgpu/../display/dc/dce/dce_aux.c:315:19: warning:
> > implicit conversion from enumeration type 'enum
> > aux_channel_operation_result' to different enumeration type 'enum
> > aux_transaction_reply' [-Wenum-conversion]
> > reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> > ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/gpu/drm/amd/amdgpu/../display/dc/i2caux/dce110/aux_engine_dce110.c:349:19:
> > warning: implicit conversion from enumeration type 'enum
> > aux_channel_operation_result' to different enumeration type 'enum
> > aux_transaction_reply' [-Wenum-conversion]
> > reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> > ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> I think the enum is actually wrong here. I think the correct fix would be:
>
> - reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> + reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON;
>
> The identifiers are so similar, my guess was that it was easy to mix
> them up. This looks like an actual bug to me, since the identifiers
> have different values between the 2 different enums.
>
Hmmm interesting... I will be happy to send a v2 with your suggestion if
one of the maintainers could confirm that to be the case (given DRM code
is rather dense).
Thanks for the review!
Nathan
> >
> > Instead of implicitly or explicitly converting between types, just
> > change status to type uint8_t (since its max size is 255) which avoids
> > this construct altogether.
> >
> > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> > drivers/gpu/drm/amd/display/dc/dc_ddc_types.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
> > index 05c8c31d8b31..97e1d4d19263 100644
> > --- a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
> > +++ b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
> > @@ -79,7 +79,7 @@ enum aux_transaction_reply {
> > };
> >
> > struct aux_reply_transaction_data {
> > - enum aux_transaction_reply status;
> > + uint8_t status;
> > uint32_t length;
> > uint8_t *data;
> > };
> > --
> > 2.19.0
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm/amd/display: Change status's type in aux_reply_transaction_data
2018-09-24 22:22 ` Nathan Chancellor
@ 2018-09-27 10:03 ` Harry Wentland
0 siblings, 0 replies; 8+ messages in thread
From: Harry Wentland @ 2018-09-27 10:03 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers
Cc: sunpeng.li, alexander.deucher, christian.koenig, David1.Zhou,
amd-gfx, dri-devel, LKML
On 2018-09-24 06:22 PM, Nathan Chancellor wrote:
> On Mon, Sep 24, 2018 at 03:07:16PM -0700, Nick Desaulniers wrote:
>> On Fri, Sep 21, 2018 at 2:55 PM Nathan Chancellor
>> <natechancellor@gmail.com> wrote:
>>>
>>> Clang warns when one enumerated type is implicitly converted to another.
>>>
>>> drivers/gpu/drm/amd/amdgpu/../display/dc/dce/dce_aux.c:315:19: warning:
>>> implicit conversion from enumeration type 'enum
>>> aux_channel_operation_result' to different enumeration type 'enum
>>> aux_transaction_reply' [-Wenum-conversion]
>>> reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
>>> ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> drivers/gpu/drm/amd/amdgpu/../display/dc/i2caux/dce110/aux_engine_dce110.c:349:19:
>>> warning: implicit conversion from enumeration type 'enum
>>> aux_channel_operation_result' to different enumeration type 'enum
>>> aux_transaction_reply' [-Wenum-conversion]
>>> reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
>>> ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> I think the enum is actually wrong here. I think the correct fix would be:
>>
>> - reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
>> + reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON;
>>
>> The identifiers are so similar, my guess was that it was easy to mix
>> them up. This looks like an actual bug to me, since the identifiers
>> have different values between the 2 different enums.
>>
>
> Hmmm interesting... I will be happy to send a v2 with your suggestion if
> one of the maintainers could confirm that to be the case (given DRM code
> is rather dense).
>
Nick is correct. We should keep the enum but assign AUX_TRANSACTION_REPLY_HPD_DISCON in dce_aux.c and aux_engine_dce110.c.
Thanks for spotting this.
Harry
> Thanks for the review!
> Nathan
>
>>>
>>> Instead of implicitly or explicitly converting between types, just
>>> change status to type uint8_t (since its max size is 255) which avoids
>>> this construct altogether.
>>>
>>> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
>>> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
>>> ---
>>> drivers/gpu/drm/amd/display/dc/dc_ddc_types.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
>>> index 05c8c31d8b31..97e1d4d19263 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
>>> +++ b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
>>> @@ -79,7 +79,7 @@ enum aux_transaction_reply {
>>> };
>>>
>>> struct aux_reply_transaction_data {
>>> - enum aux_transaction_reply status;
>>> + uint8_t status;
>>> uint32_t length;
>>> uint8_t *data;
>>> };
>>> --
>>> 2.19.0
>>>
>>
>>
>> --
>> Thanks,
>> ~Nick Desaulniers
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] drm/amd/display: Use proper enums in process_channel_reply
2018-09-21 21:55 [PATCH] drm/amd/display: Change status's type in aux_reply_transaction_data Nathan Chancellor
2018-09-24 22:07 ` Nick Desaulniers
@ 2018-09-27 18:06 ` Nathan Chancellor
2018-09-27 18:08 ` Nathan Chancellor
1 sibling, 1 reply; 8+ messages in thread
From: Nathan Chancellor @ 2018-09-27 18:06 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Alex Deucher, Christian König,
David (ChunMing) Zhou
Cc: amd-gfx, dri-devel, linux-kernel, Nick Desaulniers,
Nathan Chancellor
Clang warns when one enumerated type is implicitly converted to another.
drivers/gpu/drm/amd/amdgpu/../display/dc/dce/dce_aux.c:315:19: warning:
implicit conversion from enumeration type 'enum
aux_channel_operation_result' to different enumeration type 'enum
aux_transaction_reply' [-Wenum-conversion]
reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/amd/amdgpu/../display/dc/i2caux/dce110/aux_engine_dce110.c:349:19:
warning: implicit conversion from enumeration type 'enum
aux_channel_operation_result' to different enumeration type 'enum
aux_transaction_reply' [-Wenum-conversion]
reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The current enum is incorrect, it should be from aux_transaction_reply,
so use AUX_TRANSACTION_REPLY_HPD_DISCON.
Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
v1 -> v2:
* Rather than change status to an integer, use the proper enumerated
type from aux_transaction reply as suggested by Nick and confirmed
by Henry.
drivers/gpu/drm/amd/display/dc/dce/dce_aux.c | 2 +-
.../gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c b/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c
index 3f5b2e6f7553..aaeb7faac0c4 100644
--- a/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c
+++ b/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c
@@ -312,7 +312,7 @@ static void process_channel_reply(
/* in case HPD is LOW, exit AUX transaction */
if ((sw_status & AUX_SW_STATUS__AUX_SW_HPD_DISCON_MASK)) {
- reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
+ reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON;
return;
}
diff --git a/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c b/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c
index 8eee8ace1259..59c3ed43d609 100644
--- a/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c
+++ b/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c
@@ -346,7 +346,7 @@ static void process_channel_reply(
/* in case HPD is LOW, exit AUX transaction */
if ((sw_status & AUX_SW_STATUS__AUX_SW_HPD_DISCON_MASK)) {
- reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
+ reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON;
return;
}
--
2.19.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2] drm/amd/display: Use proper enums in process_channel_reply
2018-09-27 18:06 ` [PATCH v2] drm/amd/display: Use proper enums in process_channel_reply Nathan Chancellor
@ 2018-09-27 18:08 ` Nathan Chancellor
2018-09-27 18:11 ` Nick Desaulniers
0 siblings, 1 reply; 8+ messages in thread
From: Nathan Chancellor @ 2018-09-27 18:08 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Alex Deucher, Christian König,
David (ChunMing) Zhou
Cc: amd-gfx, dri-devel, linux-kernel, Nick Desaulniers
On Thu, Sep 27, 2018 at 11:06:33AM -0700, Nathan Chancellor wrote:
> Clang warns when one enumerated type is implicitly converted to another.
>
> drivers/gpu/drm/amd/amdgpu/../display/dc/dce/dce_aux.c:315:19: warning:
> implicit conversion from enumeration type 'enum
> aux_channel_operation_result' to different enumeration type 'enum
> aux_transaction_reply' [-Wenum-conversion]
> reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/amd/amdgpu/../display/dc/i2caux/dce110/aux_engine_dce110.c:349:19:
> warning: implicit conversion from enumeration type 'enum
> aux_channel_operation_result' to different enumeration type 'enum
> aux_transaction_reply' [-Wenum-conversion]
> reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> The current enum is incorrect, it should be from aux_transaction_reply,
> so use AUX_TRANSACTION_REPLY_HPD_DISCON.
>
> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>
> v1 -> v2:
>
> * Rather than change status to an integer, use the proper enumerated
> type from aux_transaction reply as suggested by Nick and confirmed
> by Henry.
Sigh Harry, sorry...
>
> drivers/gpu/drm/amd/display/dc/dce/dce_aux.c | 2 +-
> .../gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c b/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c
> index 3f5b2e6f7553..aaeb7faac0c4 100644
> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c
> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c
> @@ -312,7 +312,7 @@ static void process_channel_reply(
>
> /* in case HPD is LOW, exit AUX transaction */
> if ((sw_status & AUX_SW_STATUS__AUX_SW_HPD_DISCON_MASK)) {
> - reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> + reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON;
> return;
> }
>
> diff --git a/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c b/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c
> index 8eee8ace1259..59c3ed43d609 100644
> --- a/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c
> +++ b/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c
> @@ -346,7 +346,7 @@ static void process_channel_reply(
>
> /* in case HPD is LOW, exit AUX transaction */
> if ((sw_status & AUX_SW_STATUS__AUX_SW_HPD_DISCON_MASK)) {
> - reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> + reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON;
> return;
> }
>
> --
> 2.19.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] drm/amd/display: Use proper enums in process_channel_reply
2018-09-27 18:08 ` Nathan Chancellor
@ 2018-09-27 18:11 ` Nick Desaulniers
2018-10-02 14:55 ` Harry Wentland
0 siblings, 1 reply; 8+ messages in thread
From: Nick Desaulniers @ 2018-09-27 18:11 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Harry Wentland, sunpeng.li, alexander.deucher, christian.koenig,
David1.Zhou, amd-gfx, dri-devel, LKML
On Thu, Sep 27, 2018 at 11:08 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Thu, Sep 27, 2018 at 11:06:33AM -0700, Nathan Chancellor wrote:
> > Clang warns when one enumerated type is implicitly converted to another.
> >
> > drivers/gpu/drm/amd/amdgpu/../display/dc/dce/dce_aux.c:315:19: warning:
> > implicit conversion from enumeration type 'enum
> > aux_channel_operation_result' to different enumeration type 'enum
> > aux_transaction_reply' [-Wenum-conversion]
> > reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> > ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/gpu/drm/amd/amdgpu/../display/dc/i2caux/dce110/aux_engine_dce110.c:349:19:
> > warning: implicit conversion from enumeration type 'enum
> > aux_channel_operation_result' to different enumeration type 'enum
> > aux_transaction_reply' [-Wenum-conversion]
> > reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> > ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > The current enum is incorrect, it should be from aux_transaction_reply,
> > so use AUX_TRANSACTION_REPLY_HPD_DISCON.
> >
> > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> >
> > v1 -> v2:
> >
> > * Rather than change status to an integer, use the proper enumerated
> > type from aux_transaction reply as suggested by Nick and confirmed
> > by Henry.
>
> Sigh Harry, sorry...
Thanks for the patch, Nathan. Don't worry about sending a v3 over
this; its below the commit line so this detail wont get committed.
>
> >
> > drivers/gpu/drm/amd/display/dc/dce/dce_aux.c | 2 +-
> > .../gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c b/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c
> > index 3f5b2e6f7553..aaeb7faac0c4 100644
> > --- a/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c
> > +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c
> > @@ -312,7 +312,7 @@ static void process_channel_reply(
> >
> > /* in case HPD is LOW, exit AUX transaction */
> > if ((sw_status & AUX_SW_STATUS__AUX_SW_HPD_DISCON_MASK)) {
> > - reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> > + reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON;
> > return;
> > }
> >
> > diff --git a/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c b/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c
> > index 8eee8ace1259..59c3ed43d609 100644
> > --- a/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c
> > +++ b/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c
> > @@ -346,7 +346,7 @@ static void process_channel_reply(
> >
> > /* in case HPD is LOW, exit AUX transaction */
> > if ((sw_status & AUX_SW_STATUS__AUX_SW_HPD_DISCON_MASK)) {
> > - reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> > + reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON;
> > return;
> > }
> >
> > --
> > 2.19.0
> >
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] drm/amd/display: Use proper enums in process_channel_reply
2018-09-27 18:11 ` Nick Desaulniers
@ 2018-10-02 14:55 ` Harry Wentland
0 siblings, 0 replies; 8+ messages in thread
From: Harry Wentland @ 2018-10-02 14:55 UTC (permalink / raw)
To: Nick Desaulniers, Nathan Chancellor
Cc: sunpeng.li, alexander.deucher, christian.koenig, David1.Zhou,
amd-gfx, dri-devel, LKML
On 2018-09-27 02:11 PM, Nick Desaulniers wrote:
> On Thu, Sep 27, 2018 at 11:08 AM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
>>
>> On Thu, Sep 27, 2018 at 11:06:33AM -0700, Nathan Chancellor wrote:
>>> Clang warns when one enumerated type is implicitly converted to another.
>>>
>>> drivers/gpu/drm/amd/amdgpu/../display/dc/dce/dce_aux.c:315:19: warning:
>>> implicit conversion from enumeration type 'enum
>>> aux_channel_operation_result' to different enumeration type 'enum
>>> aux_transaction_reply' [-Wenum-conversion]
>>> reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
>>> ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> drivers/gpu/drm/amd/amdgpu/../display/dc/i2caux/dce110/aux_engine_dce110.c:349:19:
>>> warning: implicit conversion from enumeration type 'enum
>>> aux_channel_operation_result' to different enumeration type 'enum
>>> aux_transaction_reply' [-Wenum-conversion]
>>> reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
>>> ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> The current enum is incorrect, it should be from aux_transaction_reply,
>>> so use AUX_TRANSACTION_REPLY_HPD_DISCON.
>>>
>>> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
>>> Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
>>> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
Pulling it in through amd-staging-drm-next.
>
>>> ---
>>>
>>> v1 -> v2:
>>>
>>> * Rather than change status to an integer, use the proper enumerated
>>> type from aux_transaction reply as suggested by Nick and confirmed
>>> by Henry.
>>
>> Sigh Harry, sorry...
>
No worries. You're not the first to get that wrong. :)
Harry
> Thanks for the patch, Nathan. Don't worry about sending a v3 over
> this; its below the commit line so this detail wont get committed.
>
>>
>>>
>>> drivers/gpu/drm/amd/display/dc/dce/dce_aux.c | 2 +-
>>> .../gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c | 2 +-
>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c b/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c
>>> index 3f5b2e6f7553..aaeb7faac0c4 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c
>>> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c
>>> @@ -312,7 +312,7 @@ static void process_channel_reply(
>>>
>>> /* in case HPD is LOW, exit AUX transaction */
>>> if ((sw_status & AUX_SW_STATUS__AUX_SW_HPD_DISCON_MASK)) {
>>> - reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
>>> + reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON;
>>> return;
>>> }
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c b/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c
>>> index 8eee8ace1259..59c3ed43d609 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c
>>> +++ b/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c
>>> @@ -346,7 +346,7 @@ static void process_channel_reply(
>>>
>>> /* in case HPD is LOW, exit AUX transaction */
>>> if ((sw_status & AUX_SW_STATUS__AUX_SW_HPD_DISCON_MASK)) {
>>> - reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
>>> + reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON;
>>> return;
>>> }
>>>
>>> --
>>> 2.19.0
>>>
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-10-02 14:55 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-09-21 21:55 [PATCH] drm/amd/display: Change status's type in aux_reply_transaction_data Nathan Chancellor
2018-09-24 22:07 ` Nick Desaulniers
2018-09-24 22:22 ` Nathan Chancellor
2018-09-27 10:03 ` Harry Wentland
2018-09-27 18:06 ` [PATCH v2] drm/amd/display: Use proper enums in process_channel_reply Nathan Chancellor
2018-09-27 18:08 ` Nathan Chancellor
2018-09-27 18:11 ` Nick Desaulniers
2018-10-02 14:55 ` Harry Wentland
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox