Re: [PATCH 2/2] USB: musb: DA8xx: Add DT support for the DA8xx driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello.

On 02/10/2016 07:01 PM, Petr Kulhavy wrote:

@@ -1,6 +1,9 @@
  /*
   * Texas Instruments DA8xx/OMAP-L1x "glue layer"
   *
+ * DT support
+ * Copyright (c) 2016 Petr Kulhavy, Barix AG <petr@xxxxxxxxx>
+ *

   Could you place this after MV's copyright?

I was trying to preserve the descending date order in the file.

   The TI's copyright was copied from the davinci.c.

Do you think 2008, 2016, 2005 makes more sense?

   Yes.

+static inline int phy_refclk_cfg(int frequency)
+{
+    switch (frequency) {
+    case 12000000:
+        return 0x01;

   There's a macro for this.

[...]
   And for this.
[...]
   And for this.

Would you mind pointing me to those macros?

   See CFGCHIP2_REFFREQ_*MHZ in include/linux/platfrom_data/usb-davinci.h.

+        if (!data) {
+            ret = -ENOMEM;
+            goto err5;
+        }
+
+        config = devm_kzalloc(&pdev->dev, sizeof(*config), GFP_KERNEL);

   Why not just use the static config?!

I would say bad example, again :-)

   You're supposed to think a bit, not just copy&paste. ;-)

BTW the config pointer in struct musb_hdrc_platform_data should be const to
make sure the musb core doesn't alter it.
I'll clean up that section.

+        /* optional parameter reference clock frequency */
+        if (!of_property_read_u32(np, "ti,phy20-refclock-frequency",

   Actually, this one smells like mandatory prop... U-Boot doesn't program
CFGCHIP2, so REFFREQ is left 0.

   And 0 is reserved.

Yes, you might be right. I thought this parameter is valid only if the
external clock is selected.
But reading the clock section in the manual again, it seems the value must be
set in both cases.
I'll make it a mandatory parameter.

   Yes, please.

+enum musb_mode musb_get_dr_mode(struct device *dev)

   I'd call it just musb_get_mode()...

If you like :-)

Note that there's MUSB_PORT_MODE_* in musb_core.h and musb_dsps.c uses those in such function (incorrectly). I'll look into this once the generic function is there.

   And please add this function in a separate (preceding) patch.

OK.

   Unless Felipe objects, that is.

Petr

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux