linux-kernel - Re: [PATCH v7 2/3] x86: add support for Huawei WMI hotkeys.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <30a2b3bea95199ffc1a81f374deabbfadaeb2c22.camel@gmail.com>
Date:   Tue, 27 Nov 2018 19:58:24 -0500
From:   ayman.bagabas@...il.com
To:     Takashi Iwai <tiwai@...e.de>
Cc:     alsa-devel@...a-project.org, Hui Wang <hui.wang@...onical.com>,
        Andy Shevchenko <andy@...radead.org>,
        Darren Hart <dvhart@...radead.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Kailang Yang <kailang@...ltek.com>,
        linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v7 2/3] x86: add support for Huawei WMI hotkeys.

On Tue, 2018-11-27 at 10:45 +0100, Takashi Iwai wrote:
> On Tue, 27 Nov 2018 03:57:48 +0100,
> Ayman Bagabas wrote:
> > +static const struct key_entry huawei_wmi_keymap[] __initconst = {
> > +		{ KE_KEY,    0x281, { KEY_BRIGHTNESSDOWN } },
> > +		{ KE_KEY,    0x282, { KEY_BRIGHTNESSUP } },
> > +		{ KE_KEY,    0x284, { KEY_MUTE } },
> > +		{ KE_KEY,    0x285, { KEY_VOLUMEDOWN } },
> > +		{ KE_KEY,    0x286, { KEY_VOLUMEUP } },
> > +		{ KE_KEY,    0x287, { KEY_MICMUTE } },
> > +		{ KE_KEY,    0x289, { KEY_WLAN } },
> > +		// Huawei |M| button
> > +		{ KE_KEY,    0x28a, { KEY_CONFIG } },
> > +		// Keyboard light
> > +		{ KE_IGNORE, 0x293, { KEY_KBDILLUMTOGGLE } },
> > +		{ KE_IGNORE, 0x294, { KEY_KBDILLUMUP } },
> > +		{ KE_IGNORE, 0x295, { KEY_KBDILLUMUP } },
> > +		{ KE_END,    0 }
> 
> The indentation looks too deep here.
> 
> > +static int huawei_wmi_micmute_led_set(bool on)
> > +{
> > +	acpi_handle handle;
> > +	char *method;
> > +	union acpi_object args[3];
> > +	struct acpi_object_list arg_list = {
> > +		.pointer = args,
> > +		.count = ARRAY_SIZE(args),
> > +	};
> > +
> > +	handle = ACPI_HANDLE(&inputdev->dev);
> 
> Just wondering whether the ACPI handle is assigned properly for this
> device...

It is, but it returns a handle at "\".

> 
> > +	args[0].type = args[1].type = args[2].type = ACPI_TYPE_INTEGER;
> > +	args[1].integer.value = 0x04;
> > +
> > +	if (acpi_has_method(handle, method =
> > "\\_SB.PCI0.LPCB.EC0.SPIN")) {
> > +		args[0].integer.value = 0;
> > +		args[2].integer.value = on ? 1 : 0;
> > +	} else if (acpi_has_method(handle, method =
> > "\\_SB.PCI0.LPCB.EC0.WPIN")) {
> > +		args[0].integer.value = 1;
> > +		args[2].integer.value = on ? 0 : 1;
> > +	} else {
> > +		dev_err(&inputdev->dev, "Unable to find ACPI
> > method\n");
> > +		return -ENOSYS;
> > +	}
> > +
> > +	acpi_evaluate_object(handle, method, &arg_list, NULL);
> > +
> > +	return 0;
> > +}
> > +
> > +static int micmute_led_set(struct led_classdev *led_cdev,
> > +		enum led_brightness brightness)
> > +{
> > +	int state = brightness != LED_OFF;
> > +	int err;
> > +
> > +	err = huawei_wmi_micmute_led_set(state);
> > +	return err < 0 ? err : 0;
> 
> No need for checking err here, you can return
> huawei_wmi_mute_led_set() directly.
> 
> Or even you can fold two functions into one.

I think folding them make more sense.

> 
> > +static int __init huawei_wmi_init(void)
> > +{
> > +	int err;
> > +
> > +	if (wmi_has_guid(MBX_EVENT_GUID)) {
> > +		event_guid = MBX_EVENT_GUID;
> > +	} else if (wmi_has_guid(MBXP_EVENT_GUID)) {
> > +		event_guid = MBXP_EVENT_GUID;
> > +	} else {
> > +		pr_warn("Compatible WMI GUID not found\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	err = huawei_wmi_input_init();
> > +	if (err)
> > +		goto err_input;
> > +
> > +	micmute_led_cdev.brightness =
> > ledtrig_audio_get(LED_AUDIO_MICMUTE);
> > +	err = led_classdev_register(&inputdev->dev, &micmute_led_cdev);
> > +	if (err)
> > +		goto err_leds;
> > +
> > +	return 0;
> 
> Might it be cleaner to implement on top of wmi_driver?
> Then you can create both input and led devices on wmi device.
> 

Done! Now led devices get registered when wmi probes a new device.
Thanks.

> 
> thanks,
> 
> Takashi