On Tue, Dec 18, 2012 at 6:47 AM, chao bi chao...@intel.com wrote:
Thanks for your kind comments. Seems you were viewing the 1st version, I've
submitted 2nd version and to deliver the 3rd version soon, will include you
for review.
OK I'll look over it as it comes along...
+ WARN(i 0, %d words flush occured\n, i);
WARN really? Why not dev_warn()?
It's suppose that SPI FIFO is empty after each transfer, so call flush()
before each time of transfer, if any remain data in SPI FIFO,
It shows some kinds of Error must be happened in last transfer and
deserves a WARN() to record it.
dev_warning() is recorded, I don't get it.
+static int null_writer(struct ssp_driver_context *drv_context)
+static int null_reader(struct ssp_driver_context *drv_context)
+static int u8_writer(struct ssp_driver_context *drv_context)
+static int u8_reader(struct ssp_driver_context *drv_context)
+static int u16_writer(struct ssp_driver_context *drv_context)
+static int u16_reader(struct ssp_driver_context *drv_context)
+static int u32_writer(struct ssp_driver_context *drv_context)
+static int u32_reader(struct ssp_driver_context *drv_context)
These seem to all be designed to return 0 or 1 and should then be
bool. It seems strange actually, you would expect that such a
function returns the number of bytes or words read/written.
these functions are only used by PIO transfer, its name shows how many
bytes to be written/read, and to return whether write/read is success.
If returns bytes of read/written seems make no sense for PIO transfer,
so I think return type of boot is enough, what do you think?
If the return value is succes/error it should return zero for
success and 0 for error. IIRC these returned 1 on error which
is then wrong. Keep them as int but return something negative
when the function fails please.
+ length = TRUNCATE(drv_context-len,
+ drv_context-rx_fifo_threshold *
drv_context-n_bytes);
(...)
It looks very strange.
Isn't this simply an arithmetic soup construction to say:
length = drv_context-len / (drv_context-rx_fifo_threshold *
drv_context-n_bytes);
I think TRUNCATE() is different:
#define TRUNCATE(x, a) ((x) ~((a)-1))
Yes I got it all wrong. I have some problem with this
because of the name of the function I think.
Example for 32bit arithmetic:
TRUNCATE(0x1010, 0x100) ==
(0x1010 ~(0x0100-1)) ==
(0x1010 ~(0x00FF)) ==
(0x1010 0x00) ==
0x1000
It's quite unintuitive to call something that preserve the
upper bits and discards the lower bits to truncate,
usually we use that word for things like removing
trailing zeroes like writing 0x10 instead of 0x0010.
Should it not be named HIGH_N_BITS()
or something like that?
I have considered addin this to linux/bitops.h:
#define BITS(_start, _end) ((BIT(_end) - BIT(_start)) + BIT(_end))
So in this case you would instead of
y = TRUNCATE(x, a);
Use something like:
y = x BITS(a, 31);
And in that case it's also obvious what happens (IMO)
Uusally likely() / unlikely() micro-optimization is discouraged,
do you have specific performance numbers for using it so
much here?
I haven't done any sort of performance test for likely/unlikely, but per
our test on Medfield Platform, it meets our performance request. By the
way, would you please illustrate why likely/unlikely is discouraged. If
it impacts on performance, maybe we could consider to propose another
enhancement patch to optimize performance, but this should based on
test.
Please consult the following:
http://lwn.net/Articles/420019/
http://lwn.net/Articles/70476/
Quoting Rusty Russell:
Sometimes, unlikely()/likely() help code readability. But generally it
should be considered the register keyword of the 2000's: if the case isn't
ABSOLUTELY CRYSTAL CLEAR, or doesn't show up on benchmarks, distain is
appropriate.
+ /* We can fall here when not using DMA mode */
fall? fail?
I think it's fall
Do you mean fall through?
+ if (drv_context-quirks QUIRKS_BIT_BANGING) {
+ /* Bit banging on the clock is done through */
+ /* DFT which is available through I2C. */
+ /* get base address of I2C_Serbus registers */
+ drv_context-I2C_paddr = 0xff12b000;
What on earth is this?
Note the comment says get base address, you're not getting it at
all, you're hardcoding it. Resources like this should be passed in from
the outside.
Ok, the address is fixed for current platform, I'll define the address
at beginning and here just refer to the macro, if other platform, this
address should be defined as other value.
That really does not answer the question why it is not passed as
a resource from the outside.
+#define MAX_TRAILING_BYTE_RETRY 16
+#define MAX_TRAILING_BYTE_LOOP 100
Max iterations?
+#define DELAY_TO_GET_A_WORD 3
+#define DFLT_TIMEOUT_VAL 500
milliseconds?
It depends on
