lkml.org 
[lkml]   [2018]   [May]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2 4/4] ARM: PWM: add allwinner sun8i pwm support.
2018-05-15 19:17 GMT+08:00 Maxime Ripard <maxime.ripard@bootlin.com>:
> Hi,
>
> On Mon, May 14, 2018 at 10:45:44PM +0800, Hao Zhang wrote:
>> 2018-02-26 17:00 GMT+08:00 Maxime Ripard <maxime.ripard@bootlin.com>:
>> > 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.
>
>> >
>> > On Sun, Feb 25, 2018 at 09:53:08PM +0800, hao_zhang wrote:
>> >> +#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)

>
>> >
>> >> +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

\
 
 \ /
  Last update: 2018-05-17 16:50    [W:0.043 / U:10.452 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site