Message-ID: <20160803075157.GD5243@dell>
Date:	Wed, 3 Aug 2016 08:51:57 +0100
From:	Lee Jones <lee.jones@...aro.org>
To:	Baole Ni <baolex.ni@...el.com>
Cc:	chaitra.basappa@...adcom.com,
	suganath-prabu.subramani@...adcom.com, mchehab@...hat.com,
	m.chehab@...sung.com, pawel@...iak.com, m.szyprowski@...sung.com,
	kyungmin.park@...sung.com, k.kozlowski@...sung.com,
	linux-kernel@...r.kernel.org, chuansheng.liu@...el.com
Subject: Re: [PATCH 0677/1285] Replace numeric parameter like 0444 with macro

On Tue, 02 Aug 2016, Baole Ni wrote:
> I find that the developers often just specified the numeric value
> when calling a macro which is defined with a parameter for access permission.
> As we know, these numeric value for access permission have had the corresponding macro,
> and that using macro can improve the robustness and readability of the code,
> thus, I suggest replacing the numeric parameter with the macro.
> 
> Signed-off-by: Chuansheng Liu <chuansheng.liu@...el.com>
> Signed-off-by: Baole Ni <baolex.ni@...el.com>
> ---
>  drivers/mfd/sm501.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I can tell you now that this patch-set is going to cause some upset.

First and foremost, the change doesn't necessarily improve anything.
Many of us actually prefer the numeric representation for file
permissions -- I know that I do!  The description is wrong too, since
they are simple defines, not macros.

More importantly, you're breaking many of our official (and unwritten)
submitting patches rules.  For instance, this is taken directly from
Documentation/SubmittingPatches:

  "Do not send more than 15 patches at once to the vger mailing lists!!!"

Arguably, you have been too aggressive with your Cc: list.  People who
only made simple changes to a file X months ago, do not need to
receive/review your patch, and certainly don't expect to have their
inbox filled by someone who wasn't thinking clearly or using a script
to spam kernel contributors.  The output of get_maintainers.pl should
be taken objectively and a certain amount of common sense applied.  By
writing and submitting your patch-set using a script, you've removed
the possibility of doing that.

IMO, the patch-set is too granular.  If I were do make a one line
change in many files across the kernel, I would have probably done so
one subsystem at a time, or at the very least, one patch per
subsystem.  The only reason for submitting using your chosen method
would be to artificially increase your upstream creds (patch count).
Unfortunately, by submitting this way, I fear this may have achieved
this exact opposite.

I don't seem to be able to locate a 0th patch.  This is normally
submitted to everyone, so if any person wished to share their views
with the remainder of the recipients (which I'm sure many do), they
would do so there.  It also provides the full-diff, which is very
useful when debugging and an opportunity to offer your reasoning for
any unusual patch submitting behaviour.  This too would have been
useful.

Anyway, as you're probably already aware, for the MFD and Backlight
patches, it's a NACK I'm afraid.

> diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
> index 65cd0d2..6837aa3 100644
> --- a/drivers/mfd/sm501.c
> +++ b/drivers/mfd/sm501.c
> @@ -1216,7 +1216,7 @@ static ssize_t sm501_dbg_regs(struct device *dev,
>  }
>  
>  
> -static DEVICE_ATTR(dbg_regs, 0444, sm501_dbg_regs, NULL);
> +static DEVICE_ATTR(dbg_regs, S_IRUSR | S_IRGRP | S_IROTH, sm501_dbg_regs, NULL);
>  
>  /* sm501_init_reg
>   *

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Powered by blists - more mailing lists