Hello Sergei,
On 10.02.2016 15:05, Sergei Shtylyov 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.
Do you think 2008, 2016, 2005 makes more sense?
+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?
Apart of arch specific macros like e.g. CLOCK_12M in arch/mips/lantiq/clk.h
I can't find any such macros in the include/ directory.
+ 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 :-)
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.
+ if (ret)
+ power_ma = 0;
+
+ /* the "mentor,power" value is in milli-amps, whereas
milli-Ampers, no?
Wrong. Correct is "milliamperes" - if the comment is worth the time
spent arguing about it.
However "milliamps" is also commonly used :-)
+ /* 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.
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.
+static const struct musb_hdrc_config da830_config = {
+ .ram_bits = 10,
+ .num_eps = 5,
+ .multipoint = 1,
+
+static const struct of_device_id da8xx_id_table[] = {
+ {
+ .compatible = "ti,da830-musb",
+ .data = &da830_config,
I don't think you need to init. 'data' at this stage, keep it simple.
OK.
+enum musb_mode musb_get_dr_mode(struct device *dev)
I'd call it just musb_get_mode()...
If you like :-)
And please add this function in a separate (preceding) patch.
OK.
Petr
--
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
