| Message ID | 20161208123618.28603-4-kernel@kempniu.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thursday 08 December 2016 13:36:14 Michał Kępień wrote: > --- a/sound/pci/hda/dell_wmi_helper.c > +++ b/sound/pci/hda/dell_wmi_helper.c > @@ -6,7 +6,7 @@ > #include <linux/dell-led.h> > > static int dell_led_value; > -static int (*dell_led_set_func)(int, int); > +static int (*dell_led_set_func)(int); Suggestion: what about changing name to dell_micmute_led_set_func to match real function name which is used after this patch?
> On Thursday 08 December 2016 13:36:14 Michał Kępień wrote: > > --- a/sound/pci/hda/dell_wmi_helper.c > > +++ b/sound/pci/hda/dell_wmi_helper.c > > @@ -6,7 +6,7 @@ > > #include <linux/dell-led.h> > > > > static int dell_led_value; > > -static int (*dell_led_set_func)(int, int); > > +static int (*dell_led_set_func)(int); > > Suggestion: what about changing name to dell_micmute_led_set_func to > match real function name which is used after this patch? While I like the idea itself, implementing it will double the number of lines that this patch changes (6 vs. 12), arguably making its intention less clear. Please let me know if you would really like this to happen (perhaps as a separate patch?), otherwise I will skip this idea in v2.
On Thu Dec 15 15:46:28 2016 Michał Kępień <kernel@kempniu.pl> wrote: > > On Thursday 08 December 2016 13:36:14 Michał Kępień wrote: > > > --- a/sound/pci/hda/dell_wmi_helper.c > > > +++ b/sound/pci/hda/dell_wmi_helper.c > > > @@ -6,7 +6,7 @@ > > > #include <linux/dell-led.h> > > > > > > static int dell_led_value; > > > -static int (*dell_led_set_func)(int, int); > > > +static int (*dell_led_set_func)(int); > > > > Suggestion: what about changing name to dell_micmute_led_set_func to > > match real function name which is used after this patch? > > While I like the idea itself, implementing it will double the number of > lines that this patch changes (6 vs. 12), arguably making its intention If some patch makes final result of code better then metric number of lines is not priority. > less clear. Please let me know if you would really like this to happen > (perhaps as a separate patch?), otherwise I will skip this idea in v2. > > -- > Best regards, > Michał Kępień If it will be in this or another following patch... I dot care. But this is sound subsystem, so sound maintainers need to decide what they prefer.
> On Thu Dec 15 15:46:28 2016 Michał Kępień <kernel@kempniu.pl> wrote: > > > On Thursday 08 December 2016 13:36:14 Michał Kępień wrote: > > > > --- a/sound/pci/hda/dell_wmi_helper.c > > > > +++ b/sound/pci/hda/dell_wmi_helper.c > > > > @@ -6,7 +6,7 @@ > > > > #include <linux/dell-led.h> > > > > > > > > static int dell_led_value; > > > > -static int (*dell_led_set_func)(int, int); > > > > +static int (*dell_led_set_func)(int); > > > > > > Suggestion: what about changing name to dell_micmute_led_set_func to > > > match real function name which is used after this patch? > > > > While I like the idea itself, implementing it will double the number of > > lines that this patch changes (6 vs. 12), arguably making its intention > > If some patch makes final result of code better then metric number of lines is not priority. > > > less clear. Please let me know if you would really like this to happen > > (perhaps as a separate patch?), otherwise I will skip this idea in v2. > > > > -- > > Best regards, > > Michał Kępień > > If it will be in this or another following patch... I dot care. But this is sound subsystem, so sound maintainers need to decide what they prefer. Good point. Jaroslav, Takashi, I will patiently wait for your opinion on this patch and Pali's suggestion quoted above before I post v2 of this series.
> > On Thu Dec 15 15:46:28 2016 Michał Kępień <kernel@kempniu.pl> wrote: > > > > On Thursday 08 December 2016 13:36:14 Michał Kępień wrote: > > > > > --- a/sound/pci/hda/dell_wmi_helper.c > > > > > +++ b/sound/pci/hda/dell_wmi_helper.c > > > > > @@ -6,7 +6,7 @@ > > > > > #include <linux/dell-led.h> > > > > > > > > > > static int dell_led_value; > > > > > -static int (*dell_led_set_func)(int, int); > > > > > +static int (*dell_led_set_func)(int); > > > > > > > > Suggestion: what about changing name to dell_micmute_led_set_func to > > > > match real function name which is used after this patch? > > > > > > While I like the idea itself, implementing it will double the number of > > > lines that this patch changes (6 vs. 12), arguably making its intention > > > > If some patch makes final result of code better then metric number of lines is not priority. > > > > > less clear. Please let me know if you would really like this to happen > > > (perhaps as a separate patch?), otherwise I will skip this idea in v2. > > > > > > -- > > > Best regards, > > > Michał Kępień > > > > If it will be in this or another following patch... I dot care. But this is sound subsystem, so sound maintainers need to decide what they prefer. > > Good point. Jaroslav, Takashi, I will patiently wait for your opinion > on this patch and Pali's suggestion quoted above before I post v2 of > this series. As it has already been a month since I posted this patch series, this is just a reminder that I am waiting for comments from the sound subsystem maintainers before I post v2. Jaroslav, Takashi, I could really use your input. Please let me know if something is unclear. Thanks,
On Mon, 09 Jan 2017 14:26:31 +0100, Michał Kępień wrote: > > > > On Thu Dec 15 15:46:28 2016 Michał Kępień <kernel@kempniu.pl> wrote: > > > > > On Thursday 08 December 2016 13:36:14 Michał Kępień wrote: > > > > > > --- a/sound/pci/hda/dell_wmi_helper.c > > > > > > +++ b/sound/pci/hda/dell_wmi_helper.c > > > > > > @@ -6,7 +6,7 @@ > > > > > > #include <linux/dell-led.h> > > > > > > > > > > > > static int dell_led_value; > > > > > > -static int (*dell_led_set_func)(int, int); > > > > > > +static int (*dell_led_set_func)(int); > > > > > > > > > > Suggestion: what about changing name to dell_micmute_led_set_func to > > > > > match real function name which is used after this patch? > > > > > > > > While I like the idea itself, implementing it will double the number of > > > > lines that this patch changes (6 vs. 12), arguably making its intention > > > > > > If some patch makes final result of code better then metric number of lines is not priority. > > > > > > > less clear. Please let me know if you would really like this to happen > > > > (perhaps as a separate patch?), otherwise I will skip this idea in v2. > > > > > > > > -- > > > > Best regards, > > > > Michał Kępień > > > > > > If it will be in this or another following patch... I dot care. But this is sound subsystem, so sound maintainers need to decide what they prefer. > > > > Good point. Jaroslav, Takashi, I will patiently wait for your opinion > > on this patch and Pali's suggestion quoted above before I post v2 of > > this series. > > As it has already been a month since I posted this patch series, this is > just a reminder that I am waiting for comments from the sound subsystem > maintainers before I post v2. > > Jaroslav, Takashi, I could really use your input. Please let me know if > something is unclear. Thanks, Sorry, the last post was overseen as I already went off to vacation. The rename can be done in another patch. Doing multiple things in a single patch often leads to more confusion and error-prone. Other than that, I'm fine with rename. thanks, Takashi
> On Mon, 09 Jan 2017 14:26:31 +0100, > Michał Kępień wrote: > > > > > > On Thu Dec 15 15:46:28 2016 Michał Kępień <kernel@kempniu.pl> wrote: > > > > > > On Thursday 08 December 2016 13:36:14 Michał Kępień wrote: > > > > > > > --- a/sound/pci/hda/dell_wmi_helper.c > > > > > > > +++ b/sound/pci/hda/dell_wmi_helper.c > > > > > > > @@ -6,7 +6,7 @@ > > > > > > > #include <linux/dell-led.h> > > > > > > > > > > > > > > static int dell_led_value; > > > > > > > -static int (*dell_led_set_func)(int, int); > > > > > > > +static int (*dell_led_set_func)(int); > > > > > > > > > > > > Suggestion: what about changing name to dell_micmute_led_set_func to > > > > > > match real function name which is used after this patch? > > > > > > > > > > While I like the idea itself, implementing it will double the number of > > > > > lines that this patch changes (6 vs. 12), arguably making its intention > > > > > > > > If some patch makes final result of code better then metric number of lines is not priority. > > > > > > > > > less clear. Please let me know if you would really like this to happen > > > > > (perhaps as a separate patch?), otherwise I will skip this idea in v2. > > > > > > > > > > -- > > > > > Best regards, > > > > > Michał Kępień > > > > > > > > If it will be in this or another following patch... I dot care. But this is sound subsystem, so sound maintainers need to decide what they prefer. > > > > > > Good point. Jaroslav, Takashi, I will patiently wait for your opinion > > > on this patch and Pali's suggestion quoted above before I post v2 of > > > this series. > > > > As it has already been a month since I posted this patch series, this is > > just a reminder that I am waiting for comments from the sound subsystem > > maintainers before I post v2. > > > > Jaroslav, Takashi, I could really use your input. Please let me know if > > something is unclear. Thanks, > > Sorry, the last post was overseen as I already went off to vacation. No worries, thank you for getting back to me. > The rename can be done in another patch. Doing multiple things in a > single patch often leads to more confusion and error-prone. > > Other than that, I'm fine with rename. Thank you for your comments, I will keep them in mind for v2.
diff --git a/sound/pci/hda/dell_wmi_helper.c b/sound/pci/hda/dell_wmi_helper.c index 19d41da..e128c80 100644 --- a/sound/pci/hda/dell_wmi_helper.c +++ b/sound/pci/hda/dell_wmi_helper.c @@ -6,7 +6,7 @@ #include <linux/dell-led.h> static int dell_led_value; -static int (*dell_led_set_func)(int, int); +static int (*dell_led_set_func)(int); static void (*dell_old_cap_hook)(struct hda_codec *, struct snd_kcontrol *, struct snd_ctl_elem_value *); @@ -27,7 +27,7 @@ static void update_dell_wmi_micmute_led(struct hda_codec *codec, return; dell_led_value = val; if (dell_led_set_func) - dell_led_set_func(DELL_LED_MICMUTE, dell_led_value); + dell_led_set_func(dell_led_value); } } @@ -40,14 +40,14 @@ static void alc_fixup_dell_wmi(struct hda_codec *codec, if (action == HDA_FIXUP_ACT_PROBE) { if (!dell_led_set_func) - dell_led_set_func = symbol_request(dell_app_wmi_led_set); + dell_led_set_func = symbol_request(dell_micmute_led_set); if (!dell_led_set_func) { - codec_warn(codec, "Failed to find dell wmi symbol dell_app_wmi_led_set\n"); + codec_warn(codec, "Failed to find dell wmi symbol dell_micmute_led_set\n"); return; } removefunc = true; - if (dell_led_set_func(DELL_LED_MICMUTE, false) >= 0) { + if (dell_led_set_func(false) >= 0) { dell_led_value = 0; if (spec->gen.num_adc_nids > 1 && !spec->gen.dyn_adc_switch) codec_dbg(codec, "Skipping micmute LED control due to several ADCs"); @@ -61,7 +61,7 @@ static void alc_fixup_dell_wmi(struct hda_codec *codec, } if (dell_led_set_func && (action == HDA_FIXUP_ACT_FREE || removefunc)) { - symbol_put(dell_app_wmi_led_set); + symbol_put(dell_micmute_led_set); dell_led_set_func = NULL; dell_old_cap_hook = NULL; }
When the dell_app_wmi_led_set() method was introduced in db6d8cc ("dell-led: add mic mute led interface"), it was implemented as an easily extensible entry point for other modules to set the state of various LEDs. However, almost three years later it is still only used to control the mic mute LED, so it will be replaced with direct calls to dell_micmute_led_set(). Signed-off-by: Michał Kępień <kernel@kempniu.pl> --- sound/pci/hda/dell_wmi_helper.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)