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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3628 bytes --]

On Tue, 28 Jan 2025, Joshua Grisham wrote:

> Thank you Kurt!
> 
> Den lör 25 jan. 2025 kl 16:06 skrev Kurt Borja <kuurtb@gmail.com>:
> >
> > 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>
> 
> Regarding using this positive error code internally within the module,
> I thought about maybe adding a comment to galaxybook_probe() before
> all of the inits which describe this a bit -- do you all think this
> will be helpful or is it clear enough / does not matter and can be
> skipped?
> 
> I also realized that maybe it is worth to describe that a specific
> sequence is needed for doing these "enable feature" + init calls to
> the ACPI methods otherwise some devices were reported as starting to
> reject the payloads if the sequence was not followed.
> 
> Based on these two then I have drafted a comment sort of like this to
> put in galaxybook_probe() before the init() calls:
> 
> /*
> * Features must be enabled and initialized in the following order to
> * avoid failures seen on certain devices:
> * - GB_SASB_POWER_MANAGEMENT (including performance mode)
> * - GB_SASB_KBD_BACKLIGHT
> * - GB_SASB_BLOCK_RECORDING (as part of fw_attrs init)
> *
> * The init function for features which are not supported on all devices
> * will return EOPNOTSUPP (positive to differentiate it from upstream
> * error codes) if the feature is not working and should be ignored.
> */
> 
> Does adding something like this seem like it would help make
> everything more clear (especially thinking when new refactoring comes
> by other maintainers in X months/years/decades, it would probably help
> them to know these subtleties, right?)?
> 
> If this comment (you all are welcome to suggest wording tweaks as
> well, of course!) plus the few other small tweaks make sense then I
> can prep this to send as a new version. But I am holding a bit in
> hopes that the 6.14 stuff gets merged to pdx86 for-next so that I can
> go ahead with implementing Thomas's new power supply extension
> interface at the same time.

Hi Joshua,

In general, you don't need to wait for me to act because any commit in 
Linus' tree during merge window will be fast-forwardable to 6.14-rc1 which 
is the commit I'll be basing for-next for 6.15. So you can just pick the 
latest commit on Linus' tree and rebase on top of it.

If there ever are some commits applied already during the merge window 
into for-next, those would get rebased on top of rc1 once it's released.

> Because there are multiple variations to these devices, and there were
> some small issues that users with other devices found, I was
> thinking/hoping once all looks good for all reviewers, including
> implementing the power supply extension, that this could be merged in
> to for-next and then I can ask a few people with other supported
> devices to test this revamped (and in some ways completely refactored)
> driver directly from the branch so that we can try to catch any other
> issues that I did not see on my device before it is proposed as a
> candidate for mainline -- does that sound reasonable?
> 
> Thanks again!
> 
> Best regards,
> Joshua
> 

-- 
 i.