On Mon, Jun 30, 2014 at 03:33:13PM +0200, Antoine T?nart wrote:

Peter,

On Fri, Jun 27, 2014 at 11:25:07AM +0800, Peter Chen wrote:

quoted

On Tue, Jun 24, 2014 at 12:35:16PM +0200, Antoine T?nart wrote:

quoted

 
 ifneq ($(CONFIG_OF),)
 	obj-$(CONFIG_USB_CHIPIDEA)	+= usbmisc_imx.o ci_hdrc_imx.o
+	obj-$(CONFIG_USB_CHIPIDEA)	+= ci_hdrc_generic.o
 endif

As a generic driver, you may need to support both dt and non-dt
solution.

Since the dt is now the best practice and since there is no need (yet)
for a non-dt usage of this driver shouldn't we let anyone needing it
implement it when the time comes?

No, at least your code structure should support both dt and non-dt,
and let the compile pass for non-dt platform if you don't have one.
Then, someone with non-dt platform can change few to support it. 
A good example is: drivers/usb/host/ehci-platform.c

quoted

quoted

+static int ci_hdrc_generic_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ci_hdrc_generic_priv *priv;
+	struct ci_hdrc_platform_data ci_pdata = {
+		.name		= "ci_hdrc",

How about this using dev_name(&pdev->dev) for name?

Yes, why not. I don't have a strong preference.

quoted

quoted

+
+clk_err:
+	clk_disable_unprepare(priv->clk);

You may need to add "if (!IS_ERR(priv->clk))"

Right! I'll update this.

quoted

quoted

+
+static const struct of_device_id ci_hdrc_generic_of_match[] = {
+	{ .compatible = "chipidea-usb" },
+	{ }
+};

Even as a generic driver, you can also use your own compatible string.

Well, there is nothing specific about the Berlin CI. Some subsystems
use the 'generic' keyword in these cases. Do you see a particular reason
I should use some Berlin related compatible here?

Not must, one suggestion is: can you change the compatible string
to "chipidea-usb-generic"?

-- 

Best Regards,
Peter Chen