Re: [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id

[Peter Chen <peter.chen@freescale.com>]

On Mon, Jan 14, 2013 at 01:06:00PM +0200, Felipe Balbi wrote:
> On Mon, Jan 14, 2013 at 12:03:04PM +0100, Marc Kleine-Budde wrote:
> > On 01/14/2013 11:53 AM, Felipe Balbi wrote:
> > > Hi,
> > >=20
> > > On Mon, Jan 14, 2013 at 11:50:41AM +0100, Marc Kleine-Budde wrote:
> > >> On 01/14/2013 11:39 AM, Felipe Balbi wrote:
> > >>> On Mon, Jan 14, 2013 at 11:34:05AM +0100, Marc Kleine-Budde wrote=
:
> > >>>> On 01/14/2013 11:24 AM, Felipe Balbi wrote:
> > >>>>> On Mon, Jan 14, 2013 at 11:18:53AM +0100, Marc Kleine-Budde wro=
te:
> > >>>>>> On 01/14/2013 11:16 AM, Felipe Balbi wrote:
> > >>>>>>> Hi,
> > >>>>>>>
> > >>>>>>> On Mon, Jan 14, 2013 at 06:12:39PM +0800, Peter Chen wrote:
> > >>>>>>>> @@ -2756,22 +2753,41 @@ static int fsl_udc_otg_resume(struct=
 device *dev)
> > >>>>>>>> =20
> > >>>>>>>>  	return fsl_udc_resume(NULL);
> > >>>>>>>>  }
> > >>>>>>>> -
> > >>>>>>>>  /*---------------------------------------------------------=
----------------
> > >>>>>>>>  	Register entry point for the peripheral controller driver
> > >>>>>>>>  -----------------------------------------------------------=
---------------*/
> > >>>>>>>> -
> > >>>>>>>> +static const struct platform_device_id fsl_udc_devtype[] =3D=
 {
> > >>>>>>>> +	{
> > >>>>>>>> +		.name =3D "imx-udc-mx25",
> > >>>>>>>> +		.driver_data =3D IMX25_UDC,
> > >>>>>>>> +	}, {
> > >>>>>>>> +		.name =3D "imx-udc-mx27",
> > >>>>>>>> +		.driver_data =3D IMX27_UDC,
> > >>>>>>>> +	}, {
> > >>>>>>>> +		.name =3D "imx-udc-mx31",
> > >>>>>>>> +		.driver_data =3D IMX31_UDC,
> > >>>>>>>> +	}, {
> > >>>>>>>> +		.name =3D "imx-udc-mx35",
> > >>>>>>>> +		.driver_data =3D IMX35_UDC,
> > >>>>>>>> +	}, {
> > >>>>>>>> +		.name =3D "imx-udc-mx51",
> > >>>>>>>> +		.driver_data =3D IMX51_UDC,
> > >>>>>>>> +	}
> > >>>>>>>> +};
> > >>>>>>>
> > >>>>>>> I wonder if your driver-data is actually needed since you can=
 use string
> > >>>>>>> comparisson to achieve the exact same outcome.
> > >>>>>>
> > >>>>>> Why use a string compare, if the kernel infrastructure already=
 does this
> > >>>>>> for you?
> > >>>>>
> > >>>>> what do you mean ? What kernel infrastructure is doing waht for=
 me ?
> > >>>>
> > >>>> The kernel infrastructure is doing the string compare for you to=
 match
> > >>>> the device against the driver (via platform_device_id->name). Yo=
u get
> > >>>> the a pointer to the driver_data for free. So you don't need any=
 string
> > >>>> compare in the driver later.
> > >>>
> > >>> but current driver data is just duplicating name with an integer,=
 it's
> > >>> pretty useless driver data.
> > >>
> > >> I don't think so - another argument:
> > >> Less code. As struct platform_device_id is a static array the spac=
e is
> > >> allocated anyway. So it doesn't make any difference if driver_data=
 is
> > >> NULL or not. Later you just need to make an integer comparison ins=
tead
> > >> of a call to a strcmp(), if you have a switch/case and IMX*_UDC is=
 an
> > >> enum, the compiler will warn you if you've missed an IMX variant.
> > >=20
> > > fair enough, but then don't create a different enum value for each =
imx
> > > instance if they're mostly the same. Differentiate only what's actu=
ally
> > > different.
> >=20
> > Usually there isn't any Changelog between IP cores used in the differ=
ent
> > fsl processors (at least available outside of fsl), that makes it qui=
te
> > difficult to say if something found on one imx is really the same as =
on
> > the other one. And they (usually) don't provide any versioning
> > information in a register or the documentation.
> >=20
> > just my 2=A2
>=20
> $SUBJECT is trying to differentiate a single feature (or maybe two) to
> replace cpu_is_xxx(), then expose that on driver_data without creating
> one enum value for each release from fsl.

Felipe, every one or two SoCs may have their special operations for
integrate PHY interface, clk operation, or workaround for IC limitation.

Maybe, it will add more future or SoCs (maybe not for this driver)
in the future, using enum is easier than string comparison for expanding =
something.
>=20
> --=20
> balbi



--=20

Best Regards,
Peter Chen