
Dear Dirk,
In message 49AA9B19.6060907@googlemail.com you wrote:
Will it get an ACK if we change
...
and then call enable/disable from MUSB code at appropriate places?
Any hint if changing the patch doing something like above as proposed some days ago [1] would get an ack?
I'd like to see the patch, but so far I don;t see any problems with that.
Umm.... but this posting triggered me to look up what this function sr32() might be doing...
I think this could need some cleanup...
"cpu/arm_cortexa8/omap3/syslib.c":
43 /***************************************************************** 44 * sr32 - clear & set a value in a bit range for a 32 bit address 45 *****************************************************************/ 46 void sr32(void *addr, u32 start_bit, u32 num_bits, u32 value) 47 { 48 u32 tmp, msk = 0; 49 msk = 1 << num_bits;
It makes no sense to initialize msk with 0 when you set another value right in the next statement.
50 --msk;
I think the code would be clearer if you wrote:
msk = (1 << num_bits) - 1; 51 tmp = readl((u32)addr) & ~(msk << start_bit); 52 tmp |= value << start_bit;
Wouldn't it be better to make sure that "value" does not exceed the bit range? Like that:
tmp |= (value & ~msk) << start_bit;
?
Also, I find code that is based on bit numbers, bit widths and values *very* difficult to read. Do you really think that for example
sr32(&prcm_base->iclken1_core, 4, 1, 0x1);
is easier to read than
clrsetbits(u32, &prcm_base->iclken1_core, 0x10, 0x10);
?
And actually, just to set or clear values we don't even need that, a plain
setbits(u32, &prcm_base->iclken1_core, 0x10);
would do as well.
[Look up the definitions in include/asm-ppc/io.h if needed].
And probably 0x10 could be even replaced by some MUSB_INTERFACE_CLK #define from some header file?
I am aware that this is work for a future release, but I think it would *greatly* improve the code. It would, most of all, make it readable.
Best regards,
Wolfgang Denk