| Message ID | 1510701276.9469.19.camel@k3saltt0 (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
On 11/14/2017 03:14 PM, Simranjit Singh wrote: > From: Simranjit Singh <simranjit.singh@corebaby.org> > > Using the checkpatch.pl script, there were 8 errors in thinkpad_acpi.c. I fixed them by changing permissions to octal, > and by adding parenthesis. > On the current tree, if you use checkpatch.pl, thinkpad_acpi.c will have 8 errors. With my changes, there are 0 errors. > However, I have added 3 new warnings. > I'm very new to kernel development, so if I messed up somewhere, please do tell me. > > Changed binary permissions to octal in accordance with checkpatch.pl in thinkpad_acpi.c > > Signed-off-by: Simranjit Singh <simranjit.singh@corebaby.org> a. You should have [PATCH 1/2] and [PATCH 2/2]. b. Subject should be something like [PATCH 1/2] thinkpad_acpi: fix octal permissions (if they need to be fixed) > --- > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index 2242d60..9ad1f5b 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -9574,7 +9574,7 @@ static int __init set_ibm_param(const char *val, struct kernel_param *kp) > MODULE_PARM_DESC(experimental, > "Enables experimental features when non-zero"); > > -module_param_named(debug, dbg_level, uint, 0); Is 0 not octal? > +module_param_named(debug, dbg_level, uint, 0444; missing closing ')'. So you did not compile this, right? boo. > MODULE_PARM_DESC(debug, "Sets debug level bit-mask"); > > module_param(force_load, bool, 0444); > @@ -9638,25 +9638,25 @@ static int __init set_ibm_param(const char *val, struct kernel_param *kp) > #ifdef CONFIG_THINKPAD_ACPI_DEBUGFACILITIES > module_param(dbg_wlswemul, uint, 0444); > MODULE_PARM_DESC(dbg_wlswemul, "Enables WLSW emulation"); > -module_param_named(wlsw_state, tpacpi_wlsw_emulstate, bool, 0); > +module_param_named(wlsw_state, tpacpi_wlsw_emulstate, 044); module_param_named() still takes 4 parameters. > MODULE_PARM_DESC(wlsw_state, > "Initial state of the emulated WLSW switch"); > > module_param(dbg_bluetoothemul, uint, 0444); > MODULE_PARM_DESC(dbg_bluetoothemul, "Enables bluetooth switch emulation"); > -module_param_named(bluetooth_state, tpacpi_bluetooth_emulstate, bool, 0); > +module_param_named(bluetooth_state, tpacpi_bluetooth_emulstate, 0444); ' ditto. > MODULE_PARM_DESC(bluetooth_state, > "Initial state of the emulated bluetooth switch"); > > module_param(dbg_wwanemul, uint, 0444); > MODULE_PARM_DESC(dbg_wwanemul, "Enables WWAN switch emulation"); > -module_param_named(wwan_state, tpacpi_wwan_emulstate, bool, 0); > +module_param_named(wwan_state, tpacpi_wwan_emulstate, 0444); ditto. > MODULE_PARM_DESC(wwan_state, > "Initial state of the emulated WWAN switch"); > > module_param(dbg_uwbemul, uint, 0444); > MODULE_PARM_DESC(dbg_uwbemul, "Enables UWB switch emulation"); > -module_param_named(uwb_state, tpacpi_uwb_emulstate, bool, 0); > +module_param_named(uwb_state, tpacpi_uwb_emulstate, 0444); ditto. > MODULE_PARM_DESC(uwb_state, > "Initial state of the emulated UWB switch"); > #endif > > >
On Wed, Nov 15, 2017 at 1:14 AM, Simranjit Singh <simranjit.singh@corebaby.org> wrote: > From: Simranjit Singh <simranjit.singh@corebaby.org> > > Using the checkpatch.pl script, there were 8 errors in thinkpad_acpi.c. I fixed them by changing permissions to octal, > and by adding parenthesis. > On the current tree, if you use checkpatch.pl, thinkpad_acpi.c will have 8 errors. With my changes, there are 0 errors. > However, I have added 3 new warnings. > I'm very new to kernel development, so if I messed up somewhere, please do tell me. > > Changed binary permissions to octal in accordance with checkpatch.pl in thinkpad_acpi.c Besides comments you got already, keep in mind that patch numbering starts from 1, 0 is a special dedicated value for cover letter which you apparently miss. Of course, in case of this "series" you are not supposed to make it as a series. And one more time, it's a contributor's responsibility to test (*) changes before sending _by default _. *) sometimes compilation test is enough, though most of the times the real run would be required. As per change 0 to 0444 for the cases you did, you obviously didn't read the code and thus may not understand the ABI of this module. So, obvious NAK for this patch.
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index 2242d60..9ad1f5b 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -9574,7 +9574,7 @@ static int __init set_ibm_param(const char *val, struct kernel_param *kp) MODULE_PARM_DESC(experimental, "Enables experimental features when non-zero"); -module_param_named(debug, dbg_level, uint, 0); +module_param_named(debug, dbg_level, uint, 0444; MODULE_PARM_DESC(debug, "Sets debug level bit-mask"); module_param(force_load, bool, 0444); @@ -9638,25 +9638,25 @@ static int __init set_ibm_param(const char *val, struct kernel_param *kp) #ifdef CONFIG_THINKPAD_ACPI_DEBUGFACILITIES module_param(dbg_wlswemul, uint, 0444); MODULE_PARM_DESC(dbg_wlswemul, "Enables WLSW emulation"); -module_param_named(wlsw_state, tpacpi_wlsw_emulstate, bool, 0); +module_param_named(wlsw_state, tpacpi_wlsw_emulstate, 044); MODULE_PARM_DESC(wlsw_state, "Initial state of the emulated WLSW switch"); module_param(dbg_bluetoothemul, uint, 0444); MODULE_PARM_DESC(dbg_bluetoothemul, "Enables bluetooth switch emulation"); -module_param_named(bluetooth_state, tpacpi_bluetooth_emulstate, bool, 0); +module_param_named(bluetooth_state, tpacpi_bluetooth_emulstate, 0444); MODULE_PARM_DESC(bluetooth_state, "Initial state of the emulated bluetooth switch"); module_param(dbg_wwanemul, uint, 0444); MODULE_PARM_DESC(dbg_wwanemul, "Enables WWAN switch emulation"); -module_param_named(wwan_state, tpacpi_wwan_emulstate, bool, 0); +module_param_named(wwan_state, tpacpi_wwan_emulstate, 0444); MODULE_PARM_DESC(wwan_state, "Initial state of the emulated WWAN switch"); module_param(dbg_uwbemul, uint, 0444); MODULE_PARM_DESC(dbg_uwbemul, "Enables UWB switch emulation"); -module_param_named(uwb_state, tpacpi_uwb_emulstate, bool, 0); +module_param_named(uwb_state, tpacpi_uwb_emulstate, 0444); MODULE_PARM_DESC(uwb_state, "Initial state of the emulated UWB switch"); #endif