Re: [PATCH 5/9 v3] usb: musb: Using omap_device_build for musb device registration

[Kevin Hilman <khilman@deeprootsystems.com>]

Felipe Balbi <balbi@ti.com> writes:

> Hi,
>
> On Wed, Sep 22, 2010 at 07:29:10PM -0500, Kalliguddi, Hema wrote:
>>+#define MAX_OMAP_MUSB_HWMOD_NAME_LEN	16
>
> this isn't used anywhere.
>
>>@@ -75,31 +62,30 @@ static struct musb_hdrc_platform_data mu
>>
>> static u64 musb_dmamask = DMA_BIT_MASK(32);
>>
>>-static struct platform_device musb_device = {
>>-	.name		= "musb_hdrc",
>>-	.id		= -1,
>>-	.dev = {
>>-		.dma_mask		= &musb_dmamask,
>>-		.coherent_dma_mask	= DMA_BIT_MASK(32),
>>-		.platform_data		= &musb_plat,
>>+static struct omap_device_pm_latency omap_musb_latency[] = {
>>+	{
>>+		.deactivate_func = omap_device_idle_hwmods,
>>+		.activate_func   = omap_device_enable_hwmods,
>>+		.flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
>> 	},
>>-	.num_resources	= ARRAY_SIZE(musb_resources),
>>-	.resource	= musb_resources,
>> };
>>
>> void __init usb_musb_init(struct omap_musb_board_data *board_data)
>> {
>>-	if (cpu_is_omap243x()) {
>>-		musb_resources[0].start = OMAP243X_HS_BASE;
>>-	} else if (cpu_is_omap34xx()) {
>>-		musb_resources[0].start = OMAP34XX_HSUSB_OTG_BASE;
>>-	} else if (cpu_is_omap44xx()) {
>>-		musb_resources[0].start = OMAP44XX_HSUSB_OTG_BASE;
>>-		musb_resources[1].start = OMAP44XX_IRQ_HS_USB_MC_N;
>>-		musb_resources[2].start = OMAP44XX_IRQ_HS_USB_DMA_N;
>>+	struct omap_hwmod *oh;
>>+	struct omap_device *od;
>>+	struct platform_device *pdev;
>>+	struct device	*dev;
>>+	int bus_id = -1;
>>+	const char *oh_name = "usb_otg_hs";
>>+	struct musb_hdrc_platform_data *pdata;
>>+
>>+	oh = omap_hwmod_lookup(oh_name);
>>+
>>+	if (!oh) {
>>+		pr_err("Could not look up %s\n", oh_name);
>>+		return;
>> 	}
>
> Paul, Kevin, to me it looks like a duplication that all devices will
> have to:
>
> oh = omap_hwmod_lookup("my_hwmod_name");
> omap_device_build("my_device_name", bus_id, oh, pdata, sizeof(*pdata));
>
> could the omap_hwmod_lookup() part be moved to omap_device_build ? Or
> maybe create a omap_hwmod_lookup_and_build(oh_name, dev_name, bus_id,
> pdata, sizeof(*pdata)) ??

I don't think that this is too much extra work.

Also, many drivers are not doing a single hwmod lookup, they are using
an iterator to iterate over a bunch of hwmods in a given class (c.f.
omap_hwmod_for_each_by_class)

So, IMO, keeping the lookup and build separate is better.

>>@@ -110,8 +96,23 @@ void __init usb_musb_init(struct omap_mu
>> 	musb_plat.mode = board_data->mode;
>> 	musb_plat.extvbus = board_data->extvbus;
>>
>>-	if (platform_device_register(&musb_device) < 0)
>>-		printk(KERN_ERR "Unable to register HS-USB (MUSB) device\n");
>>+	pdata = &musb_plat;
>>+
>>+	od = omap_device_build(name, bus_id, oh, pdata,
>>+			       sizeof(struct musb_hdrc_platform_data),
>
> use sizeof(*pdata), if we change the name of that structure (very
> unlikely, but still) it'll avoid unwanted compile breakage.
>
>>+	pdev = &od->pdev;
>>+	dev = &pdev->dev;
>>+	get_device(dev);
>>+	dev->dma_mask = &musb_dmamask;
>>+	dev->coherent_dma_mask = musb_dmamask;
>>+	put_device(dev);
>
> I think this is also a duplication, it's gonna on all hwmod device
> registration, no ?

Only for devices setting DMA masks.

This is no more duplication than we have today for all platform_devices.

Kevin