Hi Ai Chao, On 3/26/24 3:54 AM, 艾超 wrote: > Hi > > > > WMI > >> > The Camera button is a GPIO device. This driver receives ACPI notifyi >> > when the camera button is switched on/off. This driver is used in >> > Lenovo A70, it is a Computer integrated machine. > >> > +config LENOVO_WMI_CAMERA >> > + tristate "Lenovo WMI Camera Button driver" >> > + depends on ACPI_WMI >> > + depends on INPUT > >> No COMPILE_TEST? > > > > I compile this driver and used Evtest tool to test it on lenovo A70. > > > ... > >> > + /* obj->buffer.pointer[0] is camera mode: >> > + * 0 camera close >> > + * 1 camera open >> > + */ > >> /* >> * The correct multi-line comment style >> * is depicted here. >> */ > > > > Thanks, I will modify it. > ... > >> > + keycode = (camera_mode == SW_CAMERA_ON ? >> > + KEY_CAMERA_ACCESS_ENABLE : KEY_CAMERA_ACCESS_DISABLE); > >> Useless parentheses. > > > > I think the parentheses is a good programming style and beneficial for reading. > > > > ... > >> > + ret = input_register_device(priv->idev); >> > + if (ret) >> > + return ret; > >> > + mutex_init(&priv->notify_lock); > >> Your mutex should be initialized before use. Have you tested that? > > > > Yes, I tested it. > > > ... > >> > +static struct wmi_driver lenovo_wmi_driver = { >> > + .driver = { >> > + .name = "lenovo-wmi-camera", >> > + .probe_type = PROBE_PREFER_ASYNCHRONOUS, >> > + }, >> > + .id_table = lenovo_wmi_id_table, >> > + .no_singleton = true, >> > + .probe = lenovo_wmi_probe, >> > + .notify = lenovo_wmi_notify, >> > + .remove = lenovo_wmi_remove, >> > +}; >> > + > >> Unneeded blank line. > > > > Thanks, I will modify it. > > >> > +module_wmi_driver(lenovo_wmi_driver); > > ... > >> > +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_id_table); > >> Please, move it closer to the respective table. > > > > Thanks, I will modify it. I have already merged this. I'll squash in fixes for the few small code style remarks from Andy, so there is no need to send a new version. Regards, Hans