linux-kernel - Re: [PATCH v2 4/4] ARM: PWM: add allwinner sun8i pwm support.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJeuY79ObkssY_=SLG==7xZri10P3yWFhXC3=CUVrdQ+7cBoHA@mail.gmail.com>
Date:   Thu, 17 May 2018 22:48:58 +0800
From:   Hao Zhang <hao5781286@...il.com>
To:     Maxime Ripard <maxime.ripard@...tlin.com>
Cc:     Thierry Reding <thierry.reding@...il.com>, robh+dt@...nel.org,
        Mark Rutland <mark.rutland@....com>, linux@...linux.org.uk,
        Chen-Yu Tsai <wens@...e.org>,
        Claudiu Beznea <Claudiu.Beznea@...rochip.com>,
        linux-gpio@...r.kernel.org,
        open list <linux-kernel@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        "moderated list:ARM/Allwinner sunXi SoC support" 
        <linux-arm-kernel@...ts.infradead.org>, linux-pwm@...r.kernel.org,
        linux-sunxi@...glegroups.com
Subject: Re: [PATCH v2 4/4] ARM: PWM: add allwinner sun8i pwm support.

2018-05-15 19:17 GMT+08:00 Maxime Ripard <maxime.ripard@...tlin.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@...tlin.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