Message-ID: <CAMF+KeaM2DhOX3h+HyUBDXp9xNtFAuw+7ooZp1XEJQxdp6CVKg@mail.gmail.com>
Date: Thu, 30 Jan 2025 18:17:47 +0100
From: Joshua Grisham <josh@...huagrisham.com>
To: Thomas Weißschuh <linux@...ssschuh.net>
Cc: Kurt Borja <kuurtb@...il.com>, W_Armin@....de, 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

Den tis 28 jan. 2025 kl 22:17 skrev Thomas Weißschuh <linux@...ssschuh.net>:
>
> Hi Joshua,
>
> sorry for the late reply.
>
> On 2025-01-28 20:17:53+0100, Joshua Grisham wrote:
> > Thank you Kurt!
> >
> > Den lör 25 jan. 2025 kl 16:06 skrev Kurt Borja <kuurtb@...il.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?
>
> To me that sounds reasonable.
>
> <snip>
>
> > 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.
>
> Nice :-)
>
> <snip>

Hi Thomas! I have been looking into this now and it seems I have
gotten it working using the new power_supply_register_extension
without too much fuss. It seems like a nice API but especially helpful
when there are multiple attributes (in this case there is only 1, but
still feels "nicer" than manually creating sysfs files!).

One thing I noticed was that, as I still needed a pointer to the
battery's struct power_supply, then it seemed to still work best if I
left in the existing battery hook (devm_battery_hook_register) and
then within the add_battery callback I could take the pointer to the
struct power_supply to hang my new power_supply_ext onto.  Essentially
much of the code for the "init" + using a battery hook that I had from
before is still the same, I have just replaced manually creating the
sysfs file (and its show/store callbacks) with the extension and then
implemented the callbacks to get/set the value from power_supply_ext
instead. Does this sound basically as you would have expected?

Regarding the possibility for the module to be loaded multiple times
and/or if one of these devices suddenly had multiple battery devices,
is there anything within power supply extension framework that will
handle multiple instances (e.g. auto-appending a number to the name or
something like with LED classes) or would this case need to be somehow
covered in each individual implementation (e.g. samsung-galaxybook)?

I had not really covered this so well either when just manually
creating the sysfs attribute (working assumption is that
device_create_file() would have failed if trying to create the same
file more than once under the same battery device, which would have
lead to a probe fail and the module unloaded for this "second"
instance ? ) and not sure what exactly the best approach would be
without giving it a bit more thought.. but maybe you can think about
this a bit when I send v9 of the patch up in just a few minutes :)
This is again a super "corner case" and it would certainly be very
weird if one of these devices suddenly had multiple instances of one
of the supporting ACPI device IDs so I am not sure if it would ever
really be a reasonable possibility. More than one battery in the same
device seems more likely, but not super reasonable either given that
one of the hallmarks of these devices is "thin and light" then I would
guess it is not likely there would ever be one with multiple
batteries?

Thanks again!

Joshua

Powered by blists - more mailing lists