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

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3428 bytes --]

On 01/14/2013 11:53 AM, Felipe Balbi wrote:
> Hi,
> 
> 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 wrote:
>>>>>> 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)
>>>>>>>>  
>>>>>>>>  	return fsl_udc_resume(NULL);
>>>>>>>>  }
>>>>>>>> -
>>>>>>>>  /*-------------------------------------------------------------------------
>>>>>>>>  	Register entry point for the peripheral controller driver
>>>>>>>>  --------------------------------------------------------------------------*/
>>>>>>>> -
>>>>>>>> +static const struct platform_device_id fsl_udc_devtype[] = {
>>>>>>>> +	{
>>>>>>>> +		.name = "imx-udc-mx25",
>>>>>>>> +		.driver_data = IMX25_UDC,
>>>>>>>> +	}, {
>>>>>>>> +		.name = "imx-udc-mx27",
>>>>>>>> +		.driver_data = IMX27_UDC,
>>>>>>>> +	}, {
>>>>>>>> +		.name = "imx-udc-mx31",
>>>>>>>> +		.driver_data = IMX31_UDC,
>>>>>>>> +	}, {
>>>>>>>> +		.name = "imx-udc-mx35",
>>>>>>>> +		.driver_data = IMX35_UDC,
>>>>>>>> +	}, {
>>>>>>>> +		.name = "imx-udc-mx51",
>>>>>>>> +		.driver_data = 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). You 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 space 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 instead
>> 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.
> 
> 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 actually
> different.

Usually there isn't any Changelog between IP cores used in the different
fsl processors (at least available outside of fsl), that makes it quite
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.

just my 2¢
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]