linux-kernel - Re: [PATCH v3 5/6] pwm: mediatek: add PWM_CLK_DIV

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: <20170821075801.GO18996@ulmo>
Date:   Mon, 21 Aug 2017 09:58:01 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Zhi Mao <zhi.mao@...iatek.com>
Cc:     john@...ozen.org, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        linux-pwm@...r.kernel.org, srv_heupstream@...iatek.com,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, yingjoe.chen@...iatek.com,
        yt.shen@...iatek.com, sean.wang@...iatek.com,
        zhenbao.liu@...iatek.com
Subject: Re: [PATCH v3 5/6] pwm: mediatek: add PWM_CLK_DIV_MAX

On Fri, Jun 30, 2017 at 02:05:20PM +0800, Zhi Mao wrote:
> 1. Replace "7" with "PWM_CLK_DIV_MAX" in function:mtk_pwm_config()
> to improve the code readablity.
> 2. add pwm clk disable in function:mtk_pwm_config()
> for error parameter checking case.
> 
> Signed-off-by: Zhi Mao <zhi.mao@...iatek.com>
> ---
>  drivers/pwm/pwm-mediatek.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Same comment as before. You've got two logical, unrelated changes in
this one commit. In this case you're mixing a cosmetic change with an
actual bug fix. And to make things worse, the commit subject mentions
only the cosmetic change, while the more important changes is only
described in a drive-by fashion.

You get another free pass this time, but please be more conscious about
these things in the future.

I've applied this to for-4.14/drivers with the following commit message:

--- >8 ---
pwm: mediatek: Disable clock on PWM configuration failure

Make sure to disable the PWM clock if the PWM cannot be configured due
to the clock divider exceeding the maximum value.

While at it, replace the hardcoded maximum clock divider with a defined
constant to improve code readability.

Signed-off-by: Zhi Mao <zhi.mao@...iatek.com>
Acked-by: John Crispin <john@...ozen.org>
Signed-off-by: Thierry Reding <thierry.reding@...il.com>
--- 8< ---

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)