Re: [PATCH v3 1/2] ohci-platform: Add support for devicetree instantiation

On Fri, 10 Jan 2014, Hans de Goede wrote:

quoted

quoted

+static struct usb_ohci_pdata ohci_platform_defaults = {
+	.power_on =		ohci_platform_power_on,
+	.power_suspend =	ohci_platform_power_off,
+	.power_off =		ohci_platform_power_off,
  };

I wonder if these routines couldn't be shared with non-DT setups.  We
could add an array of 3 struct clk pointers to the usb_ohci_pdata
structure.  Then if any of them are set, store these routines in the
power_* pointers.  (And likewise for ehci-platform.)  Something to
consider for a future patch...

I don't like automatically setting the power_ platform_data members much.

Me either.

But I've already been thinking about moving the clks member to platform_data,
and simply exporting ohci_platform_power_* so that other drivers can use them
and put them in the platform_data members themselves. That is indeed something
for another patch though.

It would require building ohci-platform into the kernel instead of 
making it a separate module.  That shouldn't matter much for 
embedded/SoC systems, though.  It seems like a good approach.

quoted

quoted

  	if (!pdata) {
-		WARN_ON(1);
-		return -ENODEV;
+		pdata = dev->dev.platform_data = &ohci_platform_defaults;

This has a subtle bug.  Consider what will happen if ohci-platform is
loaded, then unloaded and loaded again.

Right, so we need to add a "dev->dev.platform_data == &ohci_platform_defaults"
check on remove an then set platform_data to NULL, good point, will fix in the
next version.

Exactly.

quoted

The existing ehci-platform driver has this same bug.  I definitely
remember discussing at once or twice in the past, but evidently it has
never been fixed.

If you agree with the above fix I'll also throw it into the next revision
of the ehci patch.

Thank you.

Before I do a v4, one question:

Ma Haijun does not like the use of OHCI_MAX_CLKS:

 >> +#define OHCI_MAX_CLKS 3
 >
 > I still recommend using of_count_phandle_with_args(np, "clocks",
 > "#clock-cells") to determine the number of clocks or the existence of clocks
 > property (-ENOENT)

His suggestion would mean dynamically allocating the clks array and having
clks_count variable. I still think this is a bit overkill, but I can do that for
v4 if you want, so what do you prefer ?

Space isn't a big consideration, because we're not likely to see a 
system with more a few controllers.  I say keep it simple.  If someone 
comes up with a controller using more than 3 clocks, we can change to 
dynamic allocation then.

Alan Stern