From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1794E1B424A for ; Wed, 18 Dec 2024 14:24:16 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734531859; cv=none; b=sL29NR5yBY3bk8hWnj8HtjW3jURowcPNvb+Pvb/M7dzzW3eBrf4mZoJ1nl6g8pH0gdsSIiXsOh0N9dEU77Z6ESTBszUj6FcWO5kW/KPW8tfIi3rt9ApJzFsDg0NaRAyzgv8/dJSRwvpmsa9wIfx+YUZP0e2Gy/x7sNo1lt+RayI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734531859; c=relaxed/simple; bh=fxq5WPhXvFyAK+buBZe3FD3w5xOl2nrTCisD3TZj8AA=; h=Message-ID:Date:MIME-Version:Subject:To:References:From: In-Reply-To:Content-Type; b=HShbcehljcozRXCils8OyjgjyDlQDGkUmKD3z90Qrg00iOr69oEHcw2u/DeM6sldswbSQEssMMB3Ty+AQIh+EEwTxqya50RIgAfRRBTWOgZvFhozR6ehdONeFAVRel7uJIKDylGXtvEZ7RTtbu9hDZz0K8Q9i1fKYsDXmo/5/YU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=QhWN3/71; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="QhWN3/71" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1734531856; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=wYDfdm4rHVwC2HrRooT1qxFxAJepkEO0lOVD/zIJr0U=; b=QhWN3/71J+XphbAAHN5DdrzgiNCyb/fXS4SdkvYkGjJJKeazurzZmqfTVp+CQVdDBMSQSp +L0Dat5GBJMiXWtpO9Xbzcy8FNTDmFmVH777qn+N6Cyim5AQFcPRZpfidIyB06eCTuVKyk 4xhXikNRWnOoPViZipyJ6FzSTImlgg8= Received: from mail-ed1-f71.google.com (mail-ed1-f71.google.com [209.85.208.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-158-PQkfYUFoN_Ou0FbbIqCVfg-1; Wed, 18 Dec 2024 09:24:14 -0500 X-MC-Unique: PQkfYUFoN_Ou0FbbIqCVfg-1 X-Mimecast-MFC-AGG-ID: PQkfYUFoN_Ou0FbbIqCVfg Received: by mail-ed1-f71.google.com with SMTP id 4fb4d7f45d1cf-5d43ec75bc4so3860001a12.2 for ; Wed, 18 Dec 2024 06:24:13 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1734531853; x=1735136653; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=wYDfdm4rHVwC2HrRooT1qxFxAJepkEO0lOVD/zIJr0U=; b=go623hwokCWl7IlKgjL2rBuW/H+yMsB8bVLooV2xnceGfLPV/A1/rknyjTcuTgOtKU q3xdF4TfA1JYqCKComHJqs4Vy/YSfSaVnmBI0naDg3bMI3mZLl6ChQg3IJZNkcIreKlE iODx3uaFjEl0cEww6SAvWcV2e5lqTFil3G/rdEOmUTMoW10m/uI5+Ys+nYVKFSA2+Cgp nyx38SRrRji5543l80nmBoXZ7eZAvTfyxZhS90//jvfjmMY+N79ww/8Nis+BX2DGUf5c waIw8bIGk2Q618vg/5ozXUCjTgSGm/aaXpeJeYgghkGzCjjoiUv1rYJDBn/NPEFO8yWV C+EQ== X-Forwarded-Encrypted: i=1; AJvYcCXRhzlBxxujGV79jbxM2DQ/BiaYfPn2oQ14a33J2I+IG1HXXBkir8/Ph8OfSgCZeVWbJ07f860DSyLqrRU=@vger.kernel.org X-Gm-Message-State: AOJu0Yy2mDnJeafSgGy8a2y6v+gQSVk5aaQIJAnuzg2KgOP2Lnd624Ie RJhP62hiDGpmn/FlJ+OQVQr6q+s5H4b2FSXhrjqjpq827G97+ETnnGzf4t2SlOG5oM5G0jWh5IS clfRYXYI17gTG5D917db8+BAgLJXcZgT52MRA1Jl8eQ2udRFDWQQWQ4BWsQT7CQ== X-Gm-Gg: ASbGncsYubtGitTh2pDap7TAoajuky2da+tJFil4CRbtGsHiMe5ruVq7NY4pmCV+s95 Rwl6UyeoKR8Y+LCy1/Yzwn2zT2rrIRXdv3i6SiGghATg3BsTB8b/h7Y7y0Xj8cdX+mWzi/h6Fp+ vOIh+FucjxpcjSlv0ZWfiQCW6Lz6RpWewZNs5Z1kmnVq3omxnkaEWIzGiec0D3M562M6x7H3C6u I1lqGn+VOQwZOaF8yJHqq0kbGsAq78ovkGQGTFanxbo3pa2aJ1pz33hCA/wD8AcCvOdIInLWaja E5XTEu7WJFMxIi1jZ9WoTG0hACfHZUckxfDHlYer2LKmi2X/ePh6+sxnziULhf+MyPhtcVl+6Dx NmVORZUzlGfzTxEXUXvPa5+R3+C1aWIU= X-Received: by 2002:a05:6402:34cb:b0:5d0:e826:f0da with SMTP id 4fb4d7f45d1cf-5d7ee3b4ce0mr2712549a12.16.1734531852730; Wed, 18 Dec 2024 06:24:12 -0800 (PST) X-Google-Smtp-Source: AGHT+IHFtRdU6T9fjczyDU36xPulMKDITuG4yX5oo84uMCHGFDo5XudrAQybG+dnV4bd1kStWZRLnQ== X-Received: by 2002:a05:6402:34cb:b0:5d0:e826:f0da with SMTP id 4fb4d7f45d1cf-5d7ee3b4ce0mr2712521a12.16.1734531852288; Wed, 18 Dec 2024 06:24:12 -0800 (PST) Received: from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec? (2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl. [2001:1c00:c32:7800:5bfa:a036:83f0:f9ec]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-5d652ab5109sm5407494a12.16.2024.12.18.06.24.11 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 18 Dec 2024 06:24:11 -0800 (PST) Message-ID: <6d2e175f-0a84-4a6f-9484-c2c299bcc3c5@redhat.com> Date: Wed, 18 Dec 2024 15:24:10 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3] platform/x86: samsung-galaxybook: Add samsung-galaxybook driver To: Armin Wolf , Joshua Grisham , ilpo.jarvinen@linux.intel.com, platform-driver-x86@vger.kernel.org, corbet@lwn.net, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, rostedt@goodmis.org, mhiramat@kernel.org, mathieu.desnoyers@efficios.com, linux-trace-kernel@vger.kernel.org References: <20241216103855.18457-1-josh@joshuagrisham.com> Content-Language: en-US, nl From: Hans de Goede In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hi Armin, Thank you for reviewing this new driver. On 17-Dec-24 10:31 PM, Armin Wolf wrote: > Am 16.12.24 um 11:38 schrieb Joshua Grisham: >> +/* >> + * Hotkey work and filters >> + */ >> + >> +static void galaxybook_kbd_backlight_hotkey_work(struct work_struct *work) >> +{ >> +    struct samsung_galaxybook *galaxybook = >> +        container_of(work, struct samsung_galaxybook, kbd_backlight_hotkey_work); >> + >> +    if (galaxybook->kbd_backlight.brightness < galaxybook->kbd_backlight.max_brightness) >> +        kbd_backlight_acpi_set(galaxybook, galaxybook->kbd_backlight.brightness + 1); >> +    else >> +        kbd_backlight_acpi_set(galaxybook, 0); >> + >> +    led_classdev_notify_brightness_hw_changed( >> +        &galaxybook->kbd_backlight, >> +        galaxybook->kbd_backlight.brightness); > > This is the exact reason why i think this should be done in userspace. You can replace this code > with a simple input event submission using the KBDILLUM* key codes. This would also allow you to > avoid any special locking in this case. > > This would also allow userspace to configure the step with of the brightness changes. As discussed in an earlier thread, there is really no good way to let userspace handle this atm. KEY_KBDILLUMTOGGLE gets mapped to XF86KbdLightOnOff while we really want Cycle, as we have in e.g. XF86MonBrightnessCycle. I just checked gnome-settings-daemon and it does handle XF86KbdLightOnOff but it only toggles on/off. In most laptops the cycle key is handled by the embedded controller and this simply emulates that common setup to match what userspace currently expects. Handling this in userspace would require adding a new KEY_KBDILLUMCYCLE and then adding support for that to xkb and maybe also upower and gnome-settings-daemon and KDE and then wait for that all to land before things start to work. As for your argument about allowing to set the percentage to step, note that this in kernel handling only increaeses the level by 1 on the hotkey press that make sense because typically these kbds backlights only have 2 - 4 levels. So IMHO handling this in the kernel is an acceptable compromise, (yes I do realize that this is a compromise). If some need arises later to do move this to userspace we can always add a module parameter to completely disable the in kernel handling and instead send out a new KEY_KBDILLUMCYCLE key-code when this now module parameter is set. Regards, Hans