LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] soc: qcom: geni: Make version macros simpler
@ 2018-05-18 22:47 Stephen Boyd
  2018-05-21 19:31 ` Doug Anderson
  2018-06-22 16:54 ` Bjorn Andersson
  0 siblings, 2 replies; 4+ messages in thread
From: Stephen Boyd @ 2018-05-18 22:47 UTC (permalink / raw)
  To: Andy Gross
  Cc: linux-arm-msm, linux-kernel, Karthikeyan Ramasubramanian,
	Sagar Dharia, Girish Mahadevan

This macro doesn't work, because it hides a local variable inside of the
macro to hold the version and that variable name is called 'ver' and
'version' sometimes.

Let's change this to be more explicit. Introduce three macros for the
major, minor, and step of the version, and require callers to pass the
version in to get the part of the version out. This way we don't hide
local variables inside macros and things are less evil overall.

Cc: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
Cc: Sagar Dharia <sdharia@codeaurora.org>
Cc: Girish Mahadevan <girishm@codeaurora.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 include/linux/qcom-geni-se.h | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
index 5d6144977828..3bcd67fd5548 100644
--- a/include/linux/qcom-geni-se.h
+++ b/include/linux/qcom-geni-se.h
@@ -225,19 +225,14 @@ struct geni_se {
 #define HW_VER_MINOR_SHFT		16
 #define HW_VER_STEP_MASK		GENMASK(15, 0)
 
+#define GENI_SE_VERSION_MAJOR(ver) ((ver & HW_VER_MAJOR_MASK) >> HW_VER_MAJOR_SHFT)
+#define GENI_SE_VERSION_MINOR(ver) ((ver & HW_VER_MINOR_MASK) >> HW_VER_MINOR_SHFT)
+#define GENI_SE_VERSION_STEP(ver) (ver & HW_VER_STEP_MASK)
+
 #if IS_ENABLED(CONFIG_QCOM_GENI_SE)
 
 u32 geni_se_get_qup_hw_version(struct geni_se *se);
 
-#define geni_se_get_wrapper_version(se, major, minor, step) do { \
-	u32 ver; \
-\
-	ver = geni_se_get_qup_hw_version(se); \
-	major = (ver & HW_VER_MAJOR_MASK) >> HW_VER_MAJOR_SHFT; \
-	minor = (ver & HW_VER_MINOR_MASK) >> HW_VER_MINOR_SHFT; \
-	step = version & HW_VER_STEP_MASK; \
-} while (0)
-
 /**
  * geni_se_read_proto() - Read the protocol configured for a serial engine
  * @se:		Pointer to the concerned serial engine.
-- 
Sent by a computer through tubes

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] soc: qcom: geni: Make version macros simpler
  2018-05-18 22:47 [PATCH] soc: qcom: geni: Make version macros simpler Stephen Boyd
@ 2018-05-21 19:31 ` Doug Anderson
  2018-06-22 16:54 ` Bjorn Andersson
  1 sibling, 0 replies; 4+ messages in thread
From: Doug Anderson @ 2018-05-21 19:31 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Andy Gross, linux-arm-msm, LKML, Karthikeyan Ramasubramanian,
	Sagar Dharia, Girish Mahadevan

Hi,

On Fri, May 18, 2018 at 3:47 PM, Stephen Boyd <swboyd@chromium.org> wrote:
> This macro doesn't work, because it hides a local variable inside of the
> macro to hold the version and that variable name is called 'ver' and
> 'version' sometimes.
>
> Let's change this to be more explicit. Introduce three macros for the
> major, minor, and step of the version, and require callers to pass the
> version in to get the part of the version out. This way we don't hide
> local variables inside macros and things are less evil overall.
>
> Cc: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
> Cc: Sagar Dharia <sdharia@codeaurora.org>
> Cc: Girish Mahadevan <girishm@codeaurora.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  include/linux/qcom-geni-se.h | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)

FWIW I agree that Stephen's patch makes things more readable.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] soc: qcom: geni: Make version macros simpler
  2018-05-18 22:47 [PATCH] soc: qcom: geni: Make version macros simpler Stephen Boyd
  2018-05-21 19:31 ` Doug Anderson
@ 2018-06-22 16:54 ` Bjorn Andersson
  2018-06-22 17:39   ` Stephen Boyd
  1 sibling, 1 reply; 4+ messages in thread
From: Bjorn Andersson @ 2018-06-22 16:54 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Andy Gross, linux-arm-msm, linux-kernel,
	Karthikeyan Ramasubramanian, Sagar Dharia, Girish Mahadevan

On Fri 18 May 15:47 PDT 2018, Stephen Boyd wrote:

> This macro doesn't work, because it hides a local variable inside of the
> macro to hold the version and that variable name is called 'ver' and
> 'version' sometimes.
> 
> Let's change this to be more explicit. Introduce three macros for the
> major, minor, and step of the version, and require callers to pass the
> version in to get the part of the version out. This way we don't hide
> local variables inside macros and things are less evil overall.
> 
> Cc: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
> Cc: Sagar Dharia <sdharia@codeaurora.org>
> Cc: Girish Mahadevan <girishm@codeaurora.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

I was expecting that someone referenced this macro, but I guess that's
coming?

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  include/linux/qcom-geni-se.h | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
> index 5d6144977828..3bcd67fd5548 100644
> --- a/include/linux/qcom-geni-se.h
> +++ b/include/linux/qcom-geni-se.h
> @@ -225,19 +225,14 @@ struct geni_se {
>  #define HW_VER_MINOR_SHFT		16
>  #define HW_VER_STEP_MASK		GENMASK(15, 0)
>  
> +#define GENI_SE_VERSION_MAJOR(ver) ((ver & HW_VER_MAJOR_MASK) >> HW_VER_MAJOR_SHFT)
> +#define GENI_SE_VERSION_MINOR(ver) ((ver & HW_VER_MINOR_MASK) >> HW_VER_MINOR_SHFT)
> +#define GENI_SE_VERSION_STEP(ver) (ver & HW_VER_STEP_MASK)
> +
>  #if IS_ENABLED(CONFIG_QCOM_GENI_SE)
>  
>  u32 geni_se_get_qup_hw_version(struct geni_se *se);
>  
> -#define geni_se_get_wrapper_version(se, major, minor, step) do { \
> -	u32 ver; \
> -\
> -	ver = geni_se_get_qup_hw_version(se); \
> -	major = (ver & HW_VER_MAJOR_MASK) >> HW_VER_MAJOR_SHFT; \
> -	minor = (ver & HW_VER_MINOR_MASK) >> HW_VER_MINOR_SHFT; \
> -	step = version & HW_VER_STEP_MASK; \
> -} while (0)
> -
>  /**
>   * geni_se_read_proto() - Read the protocol configured for a serial engine
>   * @se:		Pointer to the concerned serial engine.
> -- 
> Sent by a computer through tubes
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] soc: qcom: geni: Make version macros simpler
  2018-06-22 16:54 ` Bjorn Andersson
@ 2018-06-22 17:39   ` Stephen Boyd
  0 siblings, 0 replies; 4+ messages in thread
From: Stephen Boyd @ 2018-06-22 17:39 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Andy Gross, linux-arm-msm, linux-kernel,
	Karthikeyan Ramasubramanian, Sagar Dharia, Girish Mahadevan

Quoting Bjorn Andersson (2018-06-22 09:54:54)
> On Fri 18 May 15:47 PDT 2018, Stephen Boyd wrote:
> 
> > This macro doesn't work, because it hides a local variable inside of the
> > macro to hold the version and that variable name is called 'ver' and
> > 'version' sometimes.
> > 
> > Let's change this to be more explicit. Introduce three macros for the
> > major, minor, and step of the version, and require callers to pass the
> > version in to get the part of the version out. This way we don't hide
> > local variables inside macros and things are less evil overall.
> > 
> > Cc: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
> > Cc: Sagar Dharia <sdharia@codeaurora.org>
> > Cc: Girish Mahadevan <girishm@codeaurora.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> 
> I was expecting that someone referenced this macro, but I guess that's
> coming?

The SPI driver does.


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-06-22 17:39 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-18 22:47 [PATCH] soc: qcom: geni: Make version macros simpler Stephen Boyd
2018-05-21 19:31 ` Doug Anderson
2018-06-22 16:54 ` Bjorn Andersson
2018-06-22 17:39   ` Stephen Boyd

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).