[PATCH 3/5] drivers:usb:fsl: Replace macros with enumerated type

[PATCH 3/5] drivers:usb:fsl: Replace macros with enumerated type

[Nikhil Badola]

Replace macros with enumerated type to represent usb ip
controller version

Signed-off-by: Nikhil Badola <nikhil.badola@freescale.com>
---
 include/linux/fsl_devices.h | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h
index 2a2f56b..6447b7b 100644
--- a/include/linux/fsl_devices.h
+++ b/include/linux/fsl_devices.h
@@ -20,11 +20,6 @@
 #define FSL_UTMI_PHY_DLY	10	/*As per P1010RM, delay for UTMI
 				PHY CLK to become stable - 10ms*/
 #define FSL_USB_PHY_CLK_TIMEOUT	10000	/* uSec */
-#define FSL_USB_VER_OLD		0
-#define FSL_USB_VER_1_6		1
-#define FSL_USB_VER_2_2		2
-#define FSL_USB_VER_2_4		3
-#define FSL_USB_VER_2_5		4
 
 #include <linux/types.h>
 
@@ -52,6 +47,14 @@
  *
  */
 
+enum fsl_usb2_controller_ver {
+	FSL_USB_VER_OLD = 0,
+	FSL_USB_VER_1_6,
+	FSL_USB_VER_2_2,
+	FSL_USB_VER_2_4,
+	FSL_USB_VER_2_5,
+};
+
 enum fsl_usb2_operating_modes {
 	FSL_USB2_MPH_HOST,
 	FSL_USB2_DR_HOST,
-- 
2.1.0

Re: [PATCH 3/5] drivers:usb:fsl: Replace macros with enumerated type

[Greg KH]

On Tue, May 26, 2015 at 05:15:48PM +0530, Nikhil Badola wrote:
> Replace macros with enumerated type to represent usb ip
> controller version
> 
> Signed-off-by: Nikhil Badola <nikhil.badola@freescale.com>
> ---
>  include/linux/fsl_devices.h | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h
> index 2a2f56b..6447b7b 100644
> --- a/include/linux/fsl_devices.h
> +++ b/include/linux/fsl_devices.h
> @@ -20,11 +20,6 @@
>  #define FSL_UTMI_PHY_DLY	10	/*As per P1010RM, delay for UTMI
>  				PHY CLK to become stable - 10ms*/
>  #define FSL_USB_PHY_CLK_TIMEOUT	10000	/* uSec */
> -#define FSL_USB_VER_OLD		0
> -#define FSL_USB_VER_1_6		1
> -#define FSL_USB_VER_2_2		2
> -#define FSL_USB_VER_2_4		3
> -#define FSL_USB_VER_2_5		4
>  
>  #include <linux/types.h>
>  
> @@ -52,6 +47,14 @@
>   *
>   */
>  
> +enum fsl_usb2_controller_ver {
> +	FSL_USB_VER_OLD = 0,
> +	FSL_USB_VER_1_6,
> +	FSL_USB_VER_2_2,
> +	FSL_USB_VER_2_4,
> +	FSL_USB_VER_2_5,
> +};

Don't you want to change that function that uses these to return the
enumerated type and not an integer?

Also, I don't think the C standard forces the non-numberd values to be
sequential, so I think the compiler could assign FSL_USB_VER_1_6 to be
45 and then FSL_USB_VER_2_2 to be 55 and so on.  So it's much better, if
you are going to be testing values, to be explicit about the values.
Please set them all here.

thanks,

greg k-h

Re: [PATCH 3/5] drivers:usb:fsl: Replace macros with enumerated type

[Joe Perches]

On Sun, 2015-05-31 at 15:50 +0900, Greg KH wrote:
> On Tue, May 26, 2015 at 05:15:48PM +0530, Nikhil Badola wrote:
> > Replace macros with enumerated type to represent usb ip
> > controller version
> > 
> > Signed-off-by: Nikhil Badola <nikhil.badola@freescale.com>
> > ---
> >  include/linux/fsl_devices.h | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h
[]
> > +enum fsl_usb2_controller_ver {
> > +	FSL_USB_VER_OLD = 0,
> > +	FSL_USB_VER_1_6,
> > +	FSL_USB_VER_2_2,
> > +	FSL_USB_VER_2_4,
> > +	FSL_USB_VER_2_5,
> > +};
> 
> Don't you want to change that function that uses these to return the
> enumerated type and not an integer?
> 
> Also, I don't think the C standard forces the non-numberd values to be
> sequential,

Then you haven't read the standard in awhile and you likely forgot.

6.7.2.2
If the first enumerator has no =, the value of its enumeration constant
is 0. Each subsequent enumerator with no = defines its enumeration
constant as the value of the constant expression obtained by adding 1 to
the value of the previous enumeration constant.

Re: [PATCH 3/5] drivers:usb:fsl: Replace macros with enumerated type

[Greg KH]

On Sun, May 31, 2015 at 12:11:18AM -0700, Joe Perches wrote:
> On Sun, 2015-05-31 at 15:50 +0900, Greg KH wrote:
> > On Tue, May 26, 2015 at 05:15:48PM +0530, Nikhil Badola wrote:
> > > Replace macros with enumerated type to represent usb ip
> > > controller version
> > > 
> > > Signed-off-by: Nikhil Badola <nikhil.badola@freescale.com>
> > > ---
> > >  include/linux/fsl_devices.h | 13 ++++++++-----
> > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h
> []
> > > +enum fsl_usb2_controller_ver {
> > > +	FSL_USB_VER_OLD = 0,
> > > +	FSL_USB_VER_1_6,
> > > +	FSL_USB_VER_2_2,
> > > +	FSL_USB_VER_2_4,
> > > +	FSL_USB_VER_2_5,
> > > +};
> > 
> > Don't you want to change that function that uses these to return the
> > enumerated type and not an integer?
> > 
> > Also, I don't think the C standard forces the non-numberd values to be
> > sequential,
> 
> Then you haven't read the standard in awhile and you likely forgot.
> 
> 6.7.2.2
> If the first enumerator has no =, the value of its enumeration constant
> is 0. Each subsequent enumerator with no = defines its enumeration
> constant as the value of the constant expression obtained by adding 1 to
> the value of the previous enumeration constant.

Ah, you are right, I did forget.

But if you are wanting to use the "numerical" value of these, please be
explicit when defining it.  Otherwise someone might think it was just
fine to put a new entry in the middle of the list.

thanks,

greg k-h

RE: [PATCH 3/5] drivers:usb:fsl: Replace macros with enumerated type

[Badola Nikhil]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Monday, June 01, 2015 2:32 AM
> To: Joe Perches
> Cc: Badola Nikhil-B46172; linux-kernel@vger.kernel.org;
> stern@rowland.harvard.edu; linux-usb@vger.kernel.org
> Subject: Re: [PATCH 3/5] drivers:usb:fsl: Replace macros with enumerated
> type
> 
> On Sun, May 31, 2015 at 12:11:18AM -0700, Joe Perches wrote:
> > On Sun, 2015-05-31 at 15:50 +0900, Greg KH wrote:
> > > On Tue, May 26, 2015 at 05:15:48PM +0530, Nikhil Badola wrote:
> > > > Replace macros with enumerated type to represent usb ip controller
> > > > version
> > > >
> > > > Signed-off-by: Nikhil Badola <nikhil.badola@freescale.com>
> > > > ---
> > > >  include/linux/fsl_devices.h | 13 ++++++++-----
> > > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/include/linux/fsl_devices.h
> > > > b/include/linux/fsl_devices.h
> > []
> > > > +enum fsl_usb2_controller_ver {
> > > > +	FSL_USB_VER_OLD = 0,
> > > > +	FSL_USB_VER_1_6,
> > > > +	FSL_USB_VER_2_2,
> > > > +	FSL_USB_VER_2_4,
> > > > +	FSL_USB_VER_2_5,
> > > > +};
> > >
> > > Don't you want to change that function that uses these to return the
> > > enumerated type and not an integer?
> > >

I missed that. I will do this change in the next patch version.

> > > Also, I don't think the C standard forces the non-numbered values to
> > > be sequential,
> >
> > Then you haven't read the standard in awhile and you likely forgot.
> >
> > 6.7.2.2
> > If the first enumerator has no =, the value of its enumeration
> > constant is 0. Each subsequent enumerator with no = defines its
> > enumeration constant as the value of the constant expression obtained
> > by adding 1 to the value of the previous enumeration constant.
> 
> Ah, you are right, I did forget.
> 
> But if you are wanting to use the "numerical" value of these, please be
> explicit when defining it.  Otherwise someone might think it was just fine to
> put a new entry in the middle of the list.
> 
We are only relying on the controller version to be less (or greater) than a specific version
and we are not using exact "numerical" value. Refer drivers/usb/host/ehci-fsl.c .
In addition, the enum entries defined are specific to controller version i.e 1.6, 2.2, 2.4 and 2.5.
In future if anyone needs to add an entry; supposedly 2.6; he would do so as per the 
sequence maintained.

RE: [PATCH 3/5] drivers:usb:fsl: Replace macros with enumerated type

[Badola Nikhil]

> -----Original Message-----
> From: Badola Nikhil-B46172
> Sent: Monday, June 01, 2015 11:50 AM
> To: 'Greg KH'
> Cc: linux-kernel@vger.kernel.org; stern@rowland.harvard.edu; linux-
> usb@vger.kernel.org; Joe Perches
> Subject: RE: [PATCH 3/5] drivers:usb:fsl: Replace macros with enumerated
> type
> 
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Monday, June 01, 2015 2:32 AM
> > To: Joe Perches
> > Cc: Badola Nikhil-B46172; linux-kernel@vger.kernel.org;
> > stern@rowland.harvard.edu; linux-usb@vger.kernel.org
> > Subject: Re: [PATCH 3/5] drivers:usb:fsl: Replace macros with
> > enumerated type
> >
> > On Sun, May 31, 2015 at 12:11:18AM -0700, Joe Perches wrote:
> > > On Sun, 2015-05-31 at 15:50 +0900, Greg KH wrote:
> > > > On Tue, May 26, 2015 at 05:15:48PM +0530, Nikhil Badola wrote:
> > > > > Replace macros with enumerated type to represent usb ip
> > > > > controller version
> > > > >
> > > > > Signed-off-by: Nikhil Badola <nikhil.badola@freescale.com>
> > > > > ---
> > > > >  include/linux/fsl_devices.h | 13 ++++++++-----
> > > > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/fsl_devices.h
> > > > > b/include/linux/fsl_devices.h
> > > []
> > > > > +enum fsl_usb2_controller_ver {
> > > > > +	FSL_USB_VER_OLD = 0,
> > > > > +	FSL_USB_VER_1_6,
> > > > > +	FSL_USB_VER_2_2,
> > > > > +	FSL_USB_VER_2_4,
> > > > > +	FSL_USB_VER_2_5,
> > > > > +};
> > > >
> > > > Don't you want to change that function that uses these to return
> > > > the enumerated type and not an integer?
> > > >
> 
> I missed that. I will do this change in the next patch version.
> 
> > > > Also, I don't think the C standard forces the non-numbered values
> > > > to be sequential,
> > >
> > > Then you haven't read the standard in awhile and you likely forgot.
> > >
> > > 6.7.2.2
> > > If the first enumerator has no =, the value of its enumeration
> > > constant is 0. Each subsequent enumerator with no = defines its
> > > enumeration constant as the value of the constant expression
> > > obtained by adding 1 to the value of the previous enumeration constant.
> >
> > Ah, you are right, I did forget.
> >
> > But if you are wanting to use the "numerical" value of these, please
> > be explicit when defining it.  Otherwise someone might think it was
> > just fine to put a new entry in the middle of the list.
> >
> We are only relying on the controller version to be less (or greater) than a
> specific version and we are not using exact "numerical" value. Refer
> drivers/usb/host/ehci-fsl.c .
> In addition, the enum entries defined are specific to controller version i.e 1.6,
> 2.2, 2.4 and 2.5.
> In future if anyone needs to add an entry; supposedly 2.6; he would do so as
> per the sequence maintained.

Is it fine by above explanation to keep value for only first entry as done in this patch?

Should I move ahead with next patch version changing the return type to enum for
function which is returning usb controller version?

Re: [PATCH 3/5] drivers:usb:fsl: Replace macros with enumerated type

[Greg KH]

On Tue, May 26, 2015 at 05:15:48PM +0530, Nikhil Badola wrote:
> Replace macros with enumerated type to represent usb ip
> controller version
> 
> Signed-off-by: Nikhil Badola <nikhil.badola@freescale.com>
> ---
>  include/linux/fsl_devices.h | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h
> index 2a2f56b..6447b7b 100644
> --- a/include/linux/fsl_devices.h
> +++ b/include/linux/fsl_devices.h
> @@ -20,11 +20,6 @@
>  #define FSL_UTMI_PHY_DLY	10	/*As per P1010RM, delay for UTMI
>  				PHY CLK to become stable - 10ms*/
>  #define FSL_USB_PHY_CLK_TIMEOUT	10000	/* uSec */
> -#define FSL_USB_VER_OLD		0
> -#define FSL_USB_VER_1_6		1
> -#define FSL_USB_VER_2_2		2
> -#define FSL_USB_VER_2_4		3
> -#define FSL_USB_VER_2_5		4
>  
>  #include <linux/types.h>
>  
> @@ -52,6 +47,14 @@
>   *
>   */
>  
> +enum fsl_usb2_controller_ver {
> +	FSL_USB_VER_OLD = 0,
> +	FSL_USB_VER_1_6,
> +	FSL_USB_VER_2_2,
> +	FSL_USB_VER_2_4,
> +	FSL_USB_VER_2_5,
> +};

I applied the first 2 patches here, can you please fix this up and
resend the rest?

thanks,

greg k-h