Message-ID: <CAMF+Keb5UzEUeim=33JR=Vv8qK7xqGn_jjNdtZMQTFtrpKrgSA@mail.gmail.com>
Date: Sat, 25 Jan 2025 12:45:02 +0100
From: Joshua Grisham <josh@...huagrisham.com>
To: Thomas Weißschuh <linux@...ssschuh.net>
Cc: W_Armin@....de, kuurtb@...il.com, ilpo.jarvinen@...ux.intel.com, 
	hdegoede@...hat.com, platform-driver-x86@...r.kernel.org, corbet@....net, 
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8] platform/x86: samsung-galaxybook: Add
 samsung-galaxybook driver

Hi Thomas, thank you for the review and taking the time to go through it again!

Den fre 24 jan. 2025 kl 00:42 skrev Thomas Weißschuh <linux@...ssschuh.net>:
>
> Hi Joshua,
>
> looks good to me.
> I have some nitpicks inline, but even for the current state:
>
> Reviewed-by: Thomas Weißschuh <linux@...ssschuh.net>
>
> > +static ssize_t charge_control_end_threshold_show(struct device *dev, struct device_attribute *attr,
> > +                                              char *buf)
> > +{
> > +     struct samsung_galaxybook *galaxybook =
> > +             container_of(attr, struct samsung_galaxybook, charge_control_end_threshold_attr);
> > +     u8 value;
> > +     int err;
> > +
> > +     err = charge_control_end_threshold_acpi_get(galaxybook, &value);
> > +     if (err)
> > +             return err;
> > +
> > +     /*
> > +      * device stores "no end threshold" as 0 instead of 100;
> > +      * if device has 0, report 100
> > +      */
> > +     if (value == 0)
> > +             value = 100;
> > +
> > +     return sysfs_emit(buf, "%u\n", value);
> > +}
>
> For the next revision you should be able to use the power supply
> extension framework.
>

I looked around a bit in the mailing lists and saw some of the
proposed patches now which add power_supply_sysfs_add_extension() and
similar functions, but do not see them yet in for-next of the pdx86
repository. Do you think it makes more sense to wait on
samsung-galaxybook and then add these changes from the start, or go
ahead with samsung-galaxybook and then update it after with using the
new framework?

> > +
> > +#define gb_pfmode(profile) galaxybook->profile_performance_modes[profile]
>
> The usage sites of this macro don't look like regular C syntax.
> This is iffy and can confuse some code parsers.
> Any chance it could be reworked to look more regular?
>

Good point, and to be honest the only reason for this was to give me a
way to keep all of the lines below 100 characters :) Now I have just
made it a local pointer within galaxybook_platform_profile_probe in
order to achieve the same effect, so hopefully it looks and feels more
"standard" now, but please take a look when I eventually send this
later as v9 !

> > +static const struct platform_profile_ops galaxybook_platform_profile_ops = {
> > +     .probe = galaxybook_platform_profile_probe,
> > +     .profile_get = galaxybook_platform_profile_get,
> > +     .profile_set = galaxybook_platform_profile_set,
> > +};
> > +
> > +static int galaxybook_platform_profile_init(struct samsung_galaxybook *galaxybook)
> > +{
> > +     struct device *platform_profile_dev;
> > +     u8 performance_mode;
> > +     int err;
> > +
> > +     /* check that performance mode appears to be supported on this device */
> > +     err = performance_mode_acpi_get(galaxybook, &performance_mode);
> > +     if (err) {
> > +             dev_dbg(&galaxybook->platform->dev,
> > +                     "failed to get initial performance mode, error %d\n", err);
> > +             return 0;
> > +     }
> > +
> > +     galaxybook->has_performance_mode = true;
>
> This should be set *after* devm_platform_profile_register() succeeded, no?
> I would prefer it slightly if the flags where set by galaxybook_probe()
> instead of the _init() functions.
>

Here it gets a bit tricky. Originally, I had much of the logic from
galaxybook_platform_profile_probe in this
galaxybook_platform_profile_init function, as I really wanted to
evaluate if all of the ACPI methods were working and it was possible
to map at least one Samsung "performance mode" to a profile, but
feedback from Kurt (which I agree with) is that it is within the probe
that should really be handling this kind of logic.

At that point I decided that it was ONLY success of
performance_mode_acpi_get that I am now using to determine
has_performance_mode, so I set it immediately after more from a
"self-documenting" perspective.

Now the code works so that if galaxybook_platform_profile_probe fails,
then that failure will bubble up to galaxybook_probe which will then
cause the entire driver to unload ... so it will not matter anyway if
or where the value was set, the module will no longer even be loaded
:)

Regarding setting all of these "feature flags" in galaxybook_probe, I
think this will be even more tricky now that I have refactored to
actually fail the galaxybook_probe for "valid failures" (e.g. I have
detected that the device does seem to support kbd_backlight, for
example (the ACPI method to get the brightness gave an expected
result) but there was some kind of failure when registering the LED
class... which, yes, in this case I would guess we DO want that it
should fail and the driver should be unloaded, because something that
definitely SHOULD work has failed ---- that is the thinking, now,
anyway)

These flags are mostly being used to control behavior after the driver
has probed and the user is interacting with various things from the
userspace (e.g. pressing hotkeys) -- we don't want the driver to try
and use features that we detected during the probe are not supported
on the particular device the driver is running on.

So essentially I want that the various init() functions called from
galaxybook_probe will return 0 (success) even if the feature is not
supported, but internally within that respective init() function I may
have disabled the feature (has_kbd_backlight=false for example).
Because of this then I think it would be a bit tricky to try and
implement setting the flags back in galaxybook_probe (e.g. need to
create some kind of custom return facility that indicates if there was
a "real error" vs if the feature is not supported but the probe should
continue?). To me it sounds a bit more complicated and potentially
"hacky" but if you have a good suggestion on how this could be done in
a better way then I would most definitely welcome it!

The only thing that comes immediately to mind is that I could pass a
pointer to the flags in the init method (e.g. "err =
galaxybook_kbd_backlight_init(galaxybook,
&galaxybook->has_kdb_backlight);" within galaxybook_probe) but it
feels more confusing and kind of same-same result compared to what
there is now (that the logic for setting the value would still be
within the init functions anyway....)

Past ALL of that I do not have any strong opinions on if setting
has_performance_mode should come before or after
devm_platform_profile_register and am fine to change it if it should
be changed. As I mentioned before, I did it this way as it felt more
"self-documenting" and also keeping in mind that if
devm_platform_profile_register returns nonzero then the whole driver
will be unloaded anyway so it would not matter :)

> > +static ssize_t current_value_store(struct kobject *kobj, struct kobj_attribute *attr,
> > +                                const char *buf, size_t count)
> > +{
> > +     struct galaxybook_fw_attr *fw_attr =
> > +             container_of(attr, struct galaxybook_fw_attr, current_value);
> > +     struct samsung_galaxybook *galaxybook = fw_attr->galaxybook;
> > +     bool value;
> > +     int err;
> > +
> > +     if (!count)
> > +             return -EINVAL;
> > +
> > +     err = kstrtobool(buf, &value);
> > +     if (err)
> > +             return err;
> > +
> > +     guard(mutex)(&galaxybook->fw_attr_lock);
> > +
> > +     err = fw_attr->set_value(galaxybook, value);
> > +     if (err)
> > +             return err;
> > +
> > +     return count;
> > +}
> > +
> > +static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > +{
> > +     struct galaxybook_fw_attr *fw_attr =
> > +             container_of(attr, struct galaxybook_fw_attr, current_value);
> > +     bool value;
> > +     int err;
> > +
> > +     err = fw_attr->get_value(fw_attr->galaxybook, &value);
> > +     if (err)
> > +             return err;
> > +
> > +     return sysfs_emit(buf, "%u\n", value);
> > +}
>
> _show() is normally defined before _store().
>

Thanks for catching this! I have now gone through the entire driver
and tried to match this pattern ("get" before "set" for the ACPI
method functions, and "show" before "store" for all of the sysfs attr
functions) -- this update will come in v9 of the patch.

> > +     err = sysfs_create_group(&galaxybook->fw_attrs_kset->kobj, &fw_attr->attr_group);
> > +     if (err)
> > +             return err;
> > +
> > +     return devm_add_action_or_reset(&galaxybook->platform->dev,
> > +                                     galaxybook_fw_attr_remove, fw_attr);
>
> I think it is unnecessary to manually clean up the group.
> When the kset gets unregistered all its children and their attributes
> should be cleaned up automatically.
>

Thank you, good catch! I tested this and you are absolutely right, it
works exactly as you say. So I have removed this for the coming v9 :)

> > +static int galaxybook_probe(struct platform_device *pdev)
> > +{
> > +     struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> > +     struct samsung_galaxybook *galaxybook;
> > +     int err;
> > +
> > +     if (!adev)
> > +             return -ENODEV;
> > +
> > +     galaxybook = devm_kzalloc(&pdev->dev, sizeof(*galaxybook), GFP_KERNEL);
> > +     if (!galaxybook)
> > +             return -ENOMEM;
> > +
> > +     galaxybook->platform = pdev;
> > +     galaxybook->acpi = adev;
> > +     galaxybook->has_kbd_backlight = false;
> > +     galaxybook->has_block_recording = false;
> > +     galaxybook->has_performance_mode = false;
>
> Nit: These are already initialized to false due to kzalloc() above.
>

Thank you, good catch again. I think I got stuck in my head when I
fixed an uninitialized local variable somewhere else and was thinking
more in terms of malloc here.. I have removed these :)


I will wait another day or two in case there are any other comments
(or answer to the above question on timing for implementing the power
supply extension framework); if I don't hear anything by then then I
will go ahead and send what I have now (per comments above) as v9.

Thank you again!

Best regards,
Joshua

Powered by blists - more mailing lists