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

["Kurt Borja" <kuurtb@gmail.com>]

On Sat Jan 25, 2025 at 6:45 AM -05, Joshua Grisham wrote:
> 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@weissschuh.net>:
>>
>> Hi Joshua,
>>
>> looks good to me.
>> I have some nitpicks inline, but even for the current state:
>>
>> Reviewed-by: Thomas Weißschuh <linux@weissschuh.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
> :)

Now I understand the original problem better. I didn't consider this
possibility when designing the callback.

While this is a fine solution I believe Thomas' EOPNOTSUPP solution is
the way to go. I think positive err value would be the safest but you
should wait for the advice of someone with more experience.

Aside from that I really like how the whole platform profile sections
works now. Good design choices :)

~ Kurt

> <snip>