Re: [Intel-gfx] [PATCH] drm/i915: protect macro parameters in SWING_SEL_{UPP, LO}WER

[Jani Nikula]

On Fri, 23 Mar 2018, Paulo Zanoni  wrote:
> Protect the macro parameters with parens in order to avoid priority
> issues on macro evaluation when the macro argument is not a single
> operand.
>
> This is not a problem today, but it could be in the future. I found
> this while reviewing a patch that introduces new callers for the
> macros.
>
> Reference: commit 04416108ccea ("drm/i915/cnl: Add registers related to 
> voltage swing sequences.")
> Cc: Rodrigo Vivi 
> Signed-off-by: Paulo Zanoni 
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index da2f6c623ab2..49c90e1aa796 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1729,9 +1729,9 @@ enum i915_power_well_id {
>  
>  #define CNL_PORT_TX_DW2_GRP(port)_MMIO(_CNL_PORT_TX_DW_GRP((port), 2))
>  #define CNL_PORT_TX_DW2_LN0(port)_MMIO(_CNL_PORT_TX_DW_LN0((port), 2))
> -#define   SWING_SEL_UPPER(x) ((x >> 3) << 15)
> +#define   SWING_SEL_UPPER(x) (((x) >> 3) << 15)
>  #define   SWING_SEL_UPPER_MASK   (1 << 15)
> -#define   SWING_SEL_LOWER(x) ((x & 0x7) << 11)
> +#define   SWING_SEL_LOWER(x) (((x) & 0x7) << 11)

Unrelated to the patch at hand, but why do we have >> 3 and & 7 here
like this? We could have a single SWING_SEL_MASK() and SWING_SEL() that
would do the split to two parts. For future follow-up...

BR,
Jani.


>  #define   SWING_SEL_LOWER_MASK   (0x7 << 11)
>  #define   RCOMP_SCALAR(x)((x) << 0)
>  #define   RCOMP_SCALAR_MASK  (0xFF << 0)

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: protect macro parameters in SWING_SEL_{UPP, LO}WER

[Rodrigo Vivi]

On Fri, Mar 23, 2018 at 12:58:53PM -0700, Paulo Zanoni wrote:
> Protect the macro parameters with parens in order to avoid priority
> issues on macro evaluation when the macro argument is not a single
> operand.
> 
> This is not a problem today, but it could be in the future. I found
> this while reviewing a patch that introduces new callers for the
> macros.
> 
> Reference: commit 04416108ccea ("drm/i915/cnl: Add registers related to 
> voltage swing sequences.")
> Cc: Rodrigo Vivi 
> Signed-off-by: Paulo Zanoni 

Reviewed-by: Rodrigo Vivi 

> ---
>  drivers/gpu/drm/i915/i915_reg.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index da2f6c623ab2..49c90e1aa796 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1729,9 +1729,9 @@ enum i915_power_well_id {
>  
>  #define CNL_PORT_TX_DW2_GRP(port)_MMIO(_CNL_PORT_TX_DW_GRP((port), 2))
>  #define CNL_PORT_TX_DW2_LN0(port)_MMIO(_CNL_PORT_TX_DW_LN0((port), 2))
> -#define   SWING_SEL_UPPER(x) ((x >> 3) << 15)
> +#define   SWING_SEL_UPPER(x) (((x) >> 3) << 15)
>  #define   SWING_SEL_UPPER_MASK   (1 << 15)
> -#define   SWING_SEL_LOWER(x) ((x & 0x7) << 11)
> +#define   SWING_SEL_LOWER(x) (((x) & 0x7) << 11)
>  #define   SWING_SEL_LOWER_MASK   (0x7 << 11)
>  #define   RCOMP_SCALAR(x)((x) << 0)
>  #define   RCOMP_SCALAR_MASK  (0xFF << 0)
> -- 
> 2.14.3
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: protect macro parameters in SWING_SEL_{UPP, LO}WER

[Paulo Zanoni]

Protect the macro parameters with parens in order to avoid priority
issues on macro evaluation when the macro argument is not a single
operand.

This is not a problem today, but it could be in the future. I found
this while reviewing a patch that introduces new callers for the
macros.

Reference: commit 04416108ccea ("drm/i915/cnl: Add registers related to voltage 
swing sequences.")
Cc: Rodrigo Vivi 
Signed-off-by: Paulo Zanoni 
---
 drivers/gpu/drm/i915/i915_reg.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index da2f6c623ab2..49c90e1aa796 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1729,9 +1729,9 @@ enum i915_power_well_id {
 
 #define CNL_PORT_TX_DW2_GRP(port)  _MMIO(_CNL_PORT_TX_DW_GRP((port), 2))
 #define CNL_PORT_TX_DW2_LN0(port)  _MMIO(_CNL_PORT_TX_DW_LN0((port), 2))
-#define   SWING_SEL_UPPER(x)   ((x >> 3) << 15)
+#define   SWING_SEL_UPPER(x)   (((x) >> 3) << 15)
 #define   SWING_SEL_UPPER_MASK (1 << 15)
-#define   SWING_SEL_LOWER(x)   ((x & 0x7) << 11)
+#define   SWING_SEL_LOWER(x)   (((x) & 0x7) << 11)
 #define   SWING_SEL_LOWER_MASK (0x7 << 11)
 #define   RCOMP_SCALAR(x)  ((x) << 0)
 #define   RCOMP_SCALAR_MASK(0xFF << 0)
-- 
2.14.3

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx