RE: [PATCH] drivers: usb :fsl: Add support for USB controller version-2.5

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: Greg KH [mailto:greg@xxxxxxxxx]
> Sent: Monday, September 29, 2014 11:56 AM
> To: Badola Nikhil-B46172
> Cc: linux-usb@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] drivers: usb :fsl: Add support for USB controller version-
> 2.5
> 
> On Mon, Sep 29, 2014 at 03:46:02AM +0000, nikhil.badola@xxxxxxxxxxxxx
> wrote:
> > >-----Original Message-----
> > >From: Greg KH [mailto:greg@xxxxxxxxx]
> > >Sent: Wednesday, September 24, 2014 10:19 AM
> > >To: Badola Nikhil-B46172
> > >Cc: linux-usb@xxxxxxxxxxxxxxx
> > >Subject: Re: [PATCH] drivers: usb :fsl: Add support for USB
> > >controller version-2.5
> > >
> > >On Thu, Aug 21, 2014 at 12:56:22PM +0530, Nikhil Badola wrote:
> > >> Add support for USB controller version-2.5 used in
> > >> T4240 rev2.0, T1024, B3421, T1040, T2080, LS1021A.
> > >>
> > >> Signed-off-by: Nikhil Badola <nikhil.badola@xxxxxxxxxxxxx>
> > >> ---
> > >> 	- Depends on commit 990c2c7829d98517228f2b2ff14919c83b75e124
> > >> 	  drivers: usb: fsl: Check IP version 2.4 for mph USB controller
> > >>
> > >>  drivers/usb/host/fsl-mph-dr-of.c | 5 +++++
> > >>  include/linux/fsl_devices.h      | 1 +
> > >>  2 files changed, 6 insertions(+)
> > >>
> > >> diff --git a/drivers/usb/host/fsl-mph-dr-of.c
> > >> b/drivers/usb/host/fsl-mph-dr-of.c
> > >> index e0315de..45b9e36 100644
> > >> --- a/drivers/usb/host/fsl-mph-dr-of.c
> > >> +++ b/drivers/usb/host/fsl-mph-dr-of.c
> > >> @@ -127,6 +127,7 @@ static int usb_get_ver_info(struct device_node
> *np)
> > >>  	 * returns 1 for usb controller version 1.6
> > >>  	 * returns 2 for usb controller version 2.2
> > >>  	 * returns 3 for usb controller version 2.4
> > >> +	 * returns 4 for usb controller version 2.5
> > >>  	 * returns 0 otherwise
> > >>  	 */
> > >>  	if (of_device_is_compatible(np, "fsl-usb2-dr")) { @@ -136,6
> > >> +137,8 @@ static int usb_get_ver_info(struct device_node *np)
> > >>  			ver = FSL_USB_VER_2_2;
> > >>  		else if (of_device_is_compatible(np, "fsl-usb2-dr-v2.4"))
> > >>  			ver = FSL_USB_VER_2_4;
> > >> +		else if (of_device_is_compatible(np, "fsl-usb2-dr-v2.5"))
> > >> +			ver = FSL_USB_VER_2_5;
> > >>  		else /* for previous controller versions */
> > >>  			ver = FSL_USB_VER_OLD;
> > >>
> > >> @@ -153,6 +156,8 @@ static int usb_get_ver_info(struct device_node
> *np)
> > >>  			ver = FSL_USB_VER_2_2;
> > >>  		else if (of_device_is_compatible(np, "fsl-usb2-mph-v2.4"))
> > >>  			ver = FSL_USB_VER_2_4;
> > >> +		else if (of_device_is_compatible(np, "fsl-usb2-mph-v2.5"))
> > >> +			ver = FSL_USB_VER_2_5;
> > >>  		else /* for previous controller versions */
> > >>  			ver = FSL_USB_VER_OLD;
> > >>  	}
> > >> diff --git a/include/linux/fsl_devices.h
> > >> b/include/linux/fsl_devices.h index a82296a..2a2f56b 100644
> > >> --- a/include/linux/fsl_devices.h
> > >> +++ b/include/linux/fsl_devices.h
> > >> @@ -24,6 +24,7 @@
> > >>  #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
> > >
> > >Why not just keep going and add the rest of the numbers while you are at
> it?
> >
> > This is the last controller version of this family hence there would
> > not be any requirement to add further numbers.
> 
> People always bring products back, you never know this :)
> 
> > >Seriously, what are these two patches doing?  You just set the value,
> > >never do anything with it, what good is it?
> >
> > We indeed use these macros for controller version specific code, for
> > example in following snippet from ehci-fsl.c if (pdata->have_sysif_regs &&
> >             pdata->controller_ver > FSL_USB_VER_1_6 &&
> >             (phy_mode == FSL_USB2_PHY_ULPI)) {
> >                 /* check PHY_CLK_VALID to get phy clk valid */ .
> > .
> > If we don't set the macros then by default FSL_USB_VER_OLD, which is
> > 0, is assigned as controller version and in that case phy_clk_valid bit would
> not be checked for controller version 2.4 and 2.5.
> 
> You are relying on a define to be > a specific value, which seems wrong as it's
> impossible to figure this type of thing out.  Please use an enumerated type at
> the very least if you want to test this type of thing.

Shall I float the patch for replacing macros with enum type on top of this patch?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux