Re: [PATCH] Fix the bug of axp20x chip driver

[Chen-Yu Tsai]

Hi,

On Sat, Nov 21, 2020 at 10:00 PM dinghua.ma <dinghua.ma.sz@xxxxxxxxx> wrote:
>
> From: "dinghua.ma" <dinghua.ma.sz@xxxxxxxxx>

First of all, the subject should follow existing conventions, with
proper subsystem and driver prefixes, which in this case would be
"regulator: axp20x: ".

Second, the subject should be more precise; "Fix the bug" could mean
anything. "Fix DLDO2 voltage control register mask for AXP22x" says
what is fixed.

So for this patch, the subject should read:

    regulator: axp20x: Fix DLDO2 voltage control register mask for AXP22x

> Modified the mask parameter of the voltage data register of the
> axp20x power chip of the PM system, and its port is DLDO2. My test
> environment is under Allwinner A40I of arm architecture, and the
> test kernel version is 5.4.

Third,

Your commit message should state why you did this change. You already
covered what you changed, but that is also plainly visible from the
patch body.

You should include how you encountered the bug. In your case this would
probably be the regulator output not changing voltage as it should. And
if possible, include why the bug existed (which in this case it was likely
a copy-paste error in the macro conversion patch). The latter is not
required, but is helpful for others looking at the change.

You also included your test platform. But you should include the test
result of the "fixed" system, as a before-and-after comparison. This
could be as simple as "Now the regulator output voltage changes as
the system requests it".

Fourth, please add the following tags so that the patch gets backported:

    Fixes: db4a555f7c4c ("regulator: axp20x: use defines for masks")
    Cc: <stable@xxxxxxxxxxxxxxx>

You can read more about stable kernel rules in general in
Documentation/process/stable-kernel-rules.rst in the kernel tree.

Last, and I believe this is more superficial, could you write your
name in a slightly more standard way, such as Ding-Hua Ma, or
DingHua Ma? Apologies if I got that wrong.


Regards
ChenYu


> Signed-off-by: dinghua.ma <dinghua.ma.sz@xxxxxxxxx>
> ---
>  drivers/regulator/axp20x-regulator.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> index cd1224182ad7..90cb8445f721 100644
> --- a/drivers/regulator/axp20x-regulator.c
> +++ b/drivers/regulator/axp20x-regulator.c
> @@ -594,7 +594,7 @@ static const struct regulator_desc axp22x_regulators[] = {
>                  AXP22X_DLDO1_V_OUT, AXP22X_DLDO1_V_OUT_MASK,
>                  AXP22X_PWR_OUT_CTRL2, AXP22X_PWR_OUT_DLDO1_MASK),
>         AXP_DESC(AXP22X, DLDO2, "dldo2", "dldoin", 700, 3300, 100,
> -                AXP22X_DLDO2_V_OUT, AXP22X_PWR_OUT_DLDO2_MASK,
> +                AXP22X_DLDO2_V_OUT, AXP22X_DLDO2_V_OUT_MASK,
>                  AXP22X_PWR_OUT_CTRL2, AXP22X_PWR_OUT_DLDO2_MASK),
>         AXP_DESC(AXP22X, DLDO3, "dldo3", "dldoin", 700, 3300, 100,
>                  AXP22X_DLDO3_V_OUT, AXP22X_DLDO3_V_OUT_MASK,
> --
> 2.25.1
>