Re: [PATCH] HP: wmi: added support for 4 zone keyboard rgb

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2347 bytes --]

On Wed, 27 Mar 2024, Carlos Ferreira wrote:

> On 3/27/24 1:05 PM, Ilpo Järvinen wrote:
> 
> > On Tue, 26 Mar 2024, Carlos Ferreira wrote:
> > 
> >> Hi, i have changed some of the code. How does it look now?
> >>
> >> Signed-off-by: Carlos Ferreira <carlosmiguelferreira.2003@gmail.com>
> >> ---
> > 
> > First of all, you need to make a proper submission with versioning, that 
> > is:
> > 
> > - Put version into the subject: PATCH v2
> > - Don't put extra stuff into changelog like the above question, if you 
> > need to ask something, put your question underneath the first --- line.
> > - List the changes you made underneath the first --- line (see ML 
> > archives for examples about formatting)
> 
> Should i submit the v2 patch with the current state or with the changes 
> suggested such as the use of the leds API? 

Please send v3 once you've made the changes, there's no need to send 
patches which we know to become obsolete.

> >>  drivers/platform/x86/hp/hp-wmi.c | 251 +++++++++++++++++++++++++++++--
> >>  1 file changed, 241 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> >> index e53660422..8108ca7e9 100644
> >> --- a/drivers/platform/x86/hp/hp-wmi.c
> >> +++ b/drivers/platform/x86/hp/hp-wmi.c
> >> @@ -27,6 +27,7 @@
> >>  #include <linux/rfkill.h>
> >>  #include <linux/string.h>
> >>  #include <linux/dmi.h>
> >> +#include <linux/bitfield.h>
> > 
> > Try to put it earlier, these should eventually be in alphabetic order 
> > (again, ordered by a separate patch, not this one).
> 
> You mean organizing all the imports like this?
> #include <linux/acpi.h>
> #include <linux/bitfield.h>
> #include <linux/dmi.h>
> #include <linux/hwmon.h>
> #include <linux/init.h>
> #include <linux/input.h>
> #include <linux/input/sparse-keymap.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> #include <linux/platform_profile.h>
> #include <linux/rfkill.h>
> #include <linux/slab.h>
> #include <linux/string.h>
> #include <linux/types.h>

Yes.

If adding to a non-sorted list, the include should be placed before any 
entries that are alphabetically after it but providing another patch 
to sort them is of course even better than that.

-- 
 i.