2018-05-15 19:17 GMT+08:00 Maxime Ripard [off-list ref]:

Hi,

On Mon, May 14, 2018 at 10:45:44PM +0800, Hao Zhang wrote:

quoted

2018-02-26 17:00 GMT+08:00 Maxime Ripard [off-list ref]:

quoted

Thanks for respinning this serie. It looks mostly good, but you still
have a quite significant number of checkpatch (--strict) warnings that
you should address.

Thanks for reviews :) ,i'm sorry for that, it will be fixed next
time.  and, besides, in what situation were the checkpatch warning
can be ignore？

The only one that can be reasonably be ignored is the long line
warning, and only if complying to the limit would make it less easy to
understand.

quoted

quoted

On Sun, Feb 25, 2018 at 09:53:08PM +0800, hao_zhang wrote:

quoted

+#define CAPTURE_IRQ_ENABLE_REG       0x0010
+#define CFIE(ch)     BIT(ch << 1 + 1)
+#define CRIE(ch)     BIT(ch << 1)

You should also put your argument between parentheses here (and in all
your other macros).

Do you mean like this ?
#define CFIE(ch)     BIT((ch) << 1 + 1)
#define CRIE(ch)     BIT((ch) << 1)

Yep, exactly. Otherwise, if you do something like CRIE(1 + 1), the
result will be BIT(1 + 1 << 1), which will expand to 3, instead of 4.

Also, CFIE looks a bit weird here, is it the offset that is
incremented, or the value? You should probably have parentheses to
make it explicit.

The vallue,
BIT(((ch) << 1) + 1) It seem not very nice...

uhmm...
In CAPTURE_IRQ_ENABLE_REG odd number is CFIE, even number is CRIE
each channel has one CFIE and CRIE.

we can also describe like this:
#define CFIE(ch)     BIT((ch) * 2 + 1)
#define CRIE(ch)     BIT((ch) * 2)

quoted

quoted

quoted

+static const u16 div_m_table[] = {
+     1,
+     2,
+     4,
+     8,
+     16,
+     32,
+     64,
+     128,
+     256
+};

If this is just a power of two, you can use either the power of two /
ilog2 to switch back and forth, instead of using that table.

I think using table is more explicit and extended...

If you didn't have a simple mapping between the register values and
the divider value, then yeah, sure. But it's not the case here.

Thanks!
Maxime

--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.