Re: [PATCH v8 2/3] x86: add support for Huawei WMI hotkeys.

[Takashi Iwai]

On Mon, 03 Dec 2018 16:46:01 +0100,
ayman.bagabas@xxxxxxxxx wrote:
> 
> > > +	priv->cdev.name = "platform::micmute";
> > > +	priv->cdev.max_brightness = 1;
> > > +	priv->cdev.brightness_set_blocking =
> > > huawei_wmi_micmute_led_set;
> > > +	priv->cdev.default_trigger = "audio-micmute";
> > > +	priv->cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> > > +	priv->cdev.dev = &wdev->dev;
> > 
> > What about suspend/resume?
> > When the driver is bound wit HD-audio, the HD-audio will restore the
> > state at resume, so it would work.  But, by providing the LED class
> > device, it is supposed to work even without HD-audio, so it might
> > make
> > sense to pass LED_CORE_SUSPENDRESUME, too.
> 
> Besides that, is there anything needed for wmi_device suspend/resume?

AFAIK, the wmi_driver itself doesn't need anything.  The input also
doesn't need, so most likely only LED.

> > > +static int __init huawei_wmi_init(void)
> > > +{
> > > +	if (!(wmi_has_guid(WMI0_EVENT_GUID) ||
> > > wmi_has_guid(AMW0_EVENT_GUID))) {
> > > +		pr_debug("Compatible WMI GUID not found\n");
> > > +		return -ENODEV;
> > > +	}
> > 
> > This is superfluous when you implement with wmi_driver.
> > In theory, the supported GUID can be added dynamically via sysfs,
> > too.
> > 
> 
> I left it that way so it doesn't insert the module if these GUIDs were
> not found.

But they aren't loaded on such devices unless you do explicitly.
If they are done explicitly, there must be a reason to do so, hence
you don't need to block it :)

> Should I drop that and use module_wmi_driver instead?

Yes, that's cleaner.  Let's try to make it as simple as possible at
first.


thanks,

Takashi