Re: [PATCH v3] platform/x86: samsung-galaxybook: Add samsung-galaxybook driver

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 16-Dec-24 5:46 PM, Thomas Weißschuh wrote:
> Hi Joshua!
> 
> (disclaimer: I didn't read all the previous reviews)
> 

Disclaimer: I also did not read all of the previous reviews.

<snip>

>> +Parameters
>> +==========
>> +
>> +The driver includes a list of boolean parameters that allow for manually
>> +enabling or disabling various features:
> 
> Can you explain *why* these are configurable? In general the addition of
> kernel parameters is frowned upon.

Ack, Joshua if posssible just remove the kernel module parameters.

We can always add them back later if there is a good reason for them.
removing them later OTOH can be more tricky.

<snip>


>> +.. _settings-attributes:
>> +
>> +Settings Attributes
>> +===================
>> +
>> +Various hardware settings can be controlled by the following sysfs attributes:
>> +
>> +- ``allow_recording`` (allows or blocks usage of built-in camera and microphone)
>> +- ``start_on_lid_open`` (power on automatically when opening the lid)
>> +- ``usb_charge`` (allows USB ports to provide power even when device is off)
> 
> Non-standard sysfs attributes should be avoided where possible.
> Userspace will need bespoke code to handle them.
> This looks like it could be handled by the standard firmware_attributes
> interface.
> This would standardize discovery and usage.

Ack this really feels like firmware-attributes. I would not be surprised
if there are matching BIOS settings and if changing those also changes
the sysfs files and likewise if the sysfs settings persist over reboot.

<snip>

>> +Keyboard hotkey actions (i8042 filter)
>> +======================================
>> +
>> +Controlled by parameter: ``i8042_filter``
>> +
>> +The i8042 filter will swallow the keyboard events for the Fn+F9 hotkey (Multi-
>> +level keyboard backlight toggle) and Fn+F10 hotkey (Allow/block recording
>> +toggle) and instead execute their actions within the driver itself.
>> +
>> +Fn+F9 will cycle through the brightness levels of the keyboard backlight. A
>> +notification will be sent using ``led_classdev_notify_brightness_hw_changed``
>> +so that the userspace can be aware of the change. This mimics the behavior of
>> +other existing devices where the brightness level is cycled internally by the
>> +embedded controller and then reported via a notification.
>> +
>> +Fn+F10 will toggle the value of the "Allow recording" setting.
> 
> Personally I'm not a big fan to implement policy this way in the kernel.
> But others may disagree.

The keyboard backlight cycling ws already discussed and handling this in
the driver is ok.

The allow-recording setting toggle is new to me. So this is changeable
at runtime, interesting.

Joshua above you write this toggle both the microphone mute and
disables the camera?

It would be good to report the camera state to userspace using
a separate input/evdev device which reports SW_CAMERA_LENS_COVER
as discussed here:

https://lore.kernel.org/linux-media/CANiDSCtjpPG3XzaEOEeczZWO5gL-V_sj_Fv5=w82D6zKC9hnpw@mail.gmail.com/

the plan is to make that the canonical API to reported "muted"
cameras.

What happens with the camera when recording is disallowed,
dus it drop of the USB bus or does it only produce black
frames ?

It is a bit unexpected that this one button controls both
microphone and camera mute. But given that unique behavior
I guess that handling this in the kernel is probably best.

The alsamixer should send some events for the mic mute/unmute
I hope and we can use SW_CAMERA_LENS_COVER to report the camera
state.

<snip>

>> +static struct platform_driver galaxybook_platform_driver = {
> 
> Could this be a 'struct acpi_driver'?

The use of acpi_driver is deprecated. AFAIK the plan it to
move the remaining ones to platform-drivers and eventually
remove the whole ACPI bus concept turning ACPI companion
nodes into "normal" fwnodes like the current software and
openfirmware fwnodes.

Regards,

Hans