[PATCH v7 00/14] platform/x86: alienware-wmi driver rework

Kurt Borja posted 14 patches 10 months, 2 weeks ago
There is a newer version of this series
MAINTAINERS                                   |    4 +-
drivers/platform/x86/dell/Kconfig             |   30 +-
drivers/platform/x86/dell/Makefile            |   45 +-
.../platform/x86/dell/alienware-wmi-base.c    |  496 +++++++
.../platform/x86/dell/alienware-wmi-legacy.c  |   95 ++
.../platform/x86/dell/alienware-wmi-wmax.c    |  790 +++++++++++
drivers/platform/x86/dell/alienware-wmi.c     | 1249 -----------------
drivers/platform/x86/dell/alienware-wmi.h     |  101 ++
8 files changed, 1534 insertions(+), 1276 deletions(-)
create mode 100644 drivers/platform/x86/dell/alienware-wmi-base.c
create mode 100644 drivers/platform/x86/dell/alienware-wmi-legacy.c
create mode 100644 drivers/platform/x86/dell/alienware-wmi-wmax.c
delete mode 100644 drivers/platform/x86/dell/alienware-wmi.c
create mode 100644 drivers/platform/x86/dell/alienware-wmi.h
[PATCH v7 00/14] platform/x86: alienware-wmi driver rework
Posted by Kurt Borja 10 months, 2 weeks ago
Hi!

I bring some last minute modifications.

I found commit

	8d8fc146dd7a ("nvmem: core: switch to use device_add_groups()")

which states that it's unnecesary to call device_remove_groups() when
the device is removed, so I dropped it to simplify things.

I also found commit

	957961b6dcc8 ("hwmon: (oxp-sensors) Move tt_toggle attribute to dev_groups")

which states that no driver should add sysfs groups while probing the
device as it races with userspace, so I re-added PROBE_FORCE_SYNCHRONOUS
to the platform driver, so groups are added only after the device has
finished probing.

I'm not 100% sure that the second commit message applies here, but it is
revd-by Greg K-H so I added it just in case.

Aside from that, I added .pprof to awcc_quirks because I'm going to add
support for new features after this series, and it makes sense that
force_platform_profile only forces the pprof and not other upcoming
features.

~ Kurt
---
[02/14]
  - In alienware_alienfx_setup() add a devm action to remove the created
    platform device
  - Drop device_remove_groups() in WMAX .remove callback
  - Add PROBE_FORCE_SYNCHRONOUS to the platform driver
  - Drop .remove callbacks on both WMI drivers

[03/14]
  - Add awcc_platform_profile_init() to create the platform_profile
    device on quirks->thermal == true condition

[07/14]
  - Add .pprof to awcc_quirks

[10/14]
  - Drop unused member `quirks` on `alienfx_priv` (remnant of another
    version)

v6: https://lore.kernel.org/platform-driver-x86/20250127040406.17112-1-kuurtb@gmail.com/

Kurt Borja (14):
  platform/x86: alienware-wmi: Add a state container for LED control
    feature
  platform/x86: alienware-wmi: Add WMI Drivers
  platform/x86: alienware-wmi: Add a state container for thermal control
    methods
  platform/x86: alienware-wmi: Refactor LED control methods
  platform/x86: alienware-wmi: Refactor hdmi, amplifier, deepslp methods
  platform/x86: alienware-wmi: Refactor thermal control methods
  platform/x86: alienware-wmi: Split DMI table
  MAINTAINERS: Update ALIENWARE WMI DRIVER entry
  platform/x86: Rename alienware-wmi.c
  platform/x86: Add alienware-wmi.h
  platform/x86: Split the alienware-wmi driver
  platform/x86: dell: Modify Makefile alignment
  platform/x86: Update alienware-wmi config entries
  platform/x86: alienware-wmi: Update header and module information

 MAINTAINERS                                   |    4 +-
 drivers/platform/x86/dell/Kconfig             |   30 +-
 drivers/platform/x86/dell/Makefile            |   45 +-
 .../platform/x86/dell/alienware-wmi-base.c    |  496 +++++++
 .../platform/x86/dell/alienware-wmi-legacy.c  |   95 ++
 .../platform/x86/dell/alienware-wmi-wmax.c    |  790 +++++++++++
 drivers/platform/x86/dell/alienware-wmi.c     | 1249 -----------------
 drivers/platform/x86/dell/alienware-wmi.h     |  101 ++
 8 files changed, 1534 insertions(+), 1276 deletions(-)
 create mode 100644 drivers/platform/x86/dell/alienware-wmi-base.c
 create mode 100644 drivers/platform/x86/dell/alienware-wmi-legacy.c
 create mode 100644 drivers/platform/x86/dell/alienware-wmi-wmax.c
 delete mode 100644 drivers/platform/x86/dell/alienware-wmi.c
 create mode 100644 drivers/platform/x86/dell/alienware-wmi.h


base-commit: 05dbaf8dd8bf537d4b4eb3115ab42a5fb40ff1f5
-- 
2.48.1
Re: [PATCH v7 00/14] platform/x86: alienware-wmi driver rework
Posted by Ilpo Järvinen 10 months, 2 weeks ago
On Mon, 3 Feb 2025, Kurt Borja wrote:

> Hi!
>
> I bring some last minute modifications.
> 
> I found commit
> 
> 	8d8fc146dd7a ("nvmem: core: switch to use device_add_groups()")
> 
> which states that it's unnecesary to call device_remove_groups() when
> the device is removed, so I dropped it to simplify things.

Hi Kurt,

> I also found commit
> 
> 	957961b6dcc8 ("hwmon: (oxp-sensors) Move tt_toggle attribute to dev_groups")
> 
> which states that no driver should add sysfs groups while probing the
> device as it races with userspace, so I re-added PROBE_FORCE_SYNCHRONOUS
> to the platform driver, so groups are added only after the device has
> finished probing.
>
> I'm not 100% sure that the second commit message applies here, but it is
> revd-by Greg K-H so I added it just in case.

Which is why .dev_groups should be used as it is able to avoid those 
races on driver core level.

Why you call device_add_groups() at all? Can't you just insert it into 
.dev_groups member in alienware_wmax_wmi_driver?

-- 
 i.

> Aside from that, I added .pprof to awcc_quirks because I'm going to add
> support for new features after this series, and it makes sense that
> force_platform_profile only forces the pprof and not other upcoming
> features.
> 
> ~ Kurt
> ---
> [02/14]
>   - In alienware_alienfx_setup() add a devm action to remove the created
>     platform device
>   - Drop device_remove_groups() in WMAX .remove callback
>   - Add PROBE_FORCE_SYNCHRONOUS to the platform driver
>   - Drop .remove callbacks on both WMI drivers
> 
> [03/14]
>   - Add awcc_platform_profile_init() to create the platform_profile
>     device on quirks->thermal == true condition
> 
> [07/14]
>   - Add .pprof to awcc_quirks
> 
> [10/14]
>   - Drop unused member `quirks` on `alienfx_priv` (remnant of another
>     version)
> 
> v6: https://lore.kernel.org/platform-driver-x86/20250127040406.17112-1-kuurtb@gmail.com/
> 
> Kurt Borja (14):
>   platform/x86: alienware-wmi: Add a state container for LED control
>     feature
>   platform/x86: alienware-wmi: Add WMI Drivers
>   platform/x86: alienware-wmi: Add a state container for thermal control
>     methods
>   platform/x86: alienware-wmi: Refactor LED control methods
>   platform/x86: alienware-wmi: Refactor hdmi, amplifier, deepslp methods
>   platform/x86: alienware-wmi: Refactor thermal control methods
>   platform/x86: alienware-wmi: Split DMI table
>   MAINTAINERS: Update ALIENWARE WMI DRIVER entry
>   platform/x86: Rename alienware-wmi.c
>   platform/x86: Add alienware-wmi.h
>   platform/x86: Split the alienware-wmi driver
>   platform/x86: dell: Modify Makefile alignment
>   platform/x86: Update alienware-wmi config entries
>   platform/x86: alienware-wmi: Update header and module information
> 
>  MAINTAINERS                                   |    4 +-
>  drivers/platform/x86/dell/Kconfig             |   30 +-
>  drivers/platform/x86/dell/Makefile            |   45 +-
>  .../platform/x86/dell/alienware-wmi-base.c    |  496 +++++++
>  .../platform/x86/dell/alienware-wmi-legacy.c  |   95 ++
>  .../platform/x86/dell/alienware-wmi-wmax.c    |  790 +++++++++++
>  drivers/platform/x86/dell/alienware-wmi.c     | 1249 -----------------
>  drivers/platform/x86/dell/alienware-wmi.h     |  101 ++
>  8 files changed, 1534 insertions(+), 1276 deletions(-)
>  create mode 100644 drivers/platform/x86/dell/alienware-wmi-base.c
>  create mode 100644 drivers/platform/x86/dell/alienware-wmi-legacy.c
>  create mode 100644 drivers/platform/x86/dell/alienware-wmi-wmax.c
>  delete mode 100644 drivers/platform/x86/dell/alienware-wmi.c
>  create mode 100644 drivers/platform/x86/dell/alienware-wmi.h
> 
> 
> base-commit: 05dbaf8dd8bf537d4b4eb3115ab42a5fb40ff1f5
>
Re: [PATCH v7 00/14] platform/x86: alienware-wmi driver rework
Posted by Kurt Borja 10 months, 2 weeks ago
On Mon Feb 3, 2025 at 4:20 AM -05, Ilpo Järvinen wrote:
> On Mon, 3 Feb 2025, Kurt Borja wrote:
>
>> Hi!
>>
>> I bring some last minute modifications.
>> 
>> I found commit
>> 
>> 	8d8fc146dd7a ("nvmem: core: switch to use device_add_groups()")
>> 
>> which states that it's unnecesary to call device_remove_groups() when
>> the device is removed, so I dropped it to simplify things.
>
> Hi Kurt,

Hi Ilpo,

>
>> I also found commit
>> 
>> 	957961b6dcc8 ("hwmon: (oxp-sensors) Move tt_toggle attribute to dev_groups")
>> 
>> which states that no driver should add sysfs groups while probing the
>> device as it races with userspace, so I re-added PROBE_FORCE_SYNCHRONOUS
>> to the platform driver, so groups are added only after the device has
>> finished probing.
>>
>> I'm not 100% sure that the second commit message applies here, but it is
>> revd-by Greg K-H so I added it just in case.
>
> Which is why .dev_groups should be used as it is able to avoid those 
> races on driver core level.

In previous discussions with Armin we agreed it made more sense to move
WMAX-only groups from alienware-wmi-base.c to alienware-wmi-wmax.c when
splitting.

I have no problem in moving them back to .dev_groups though.

>
> Why you call device_add_groups() at all? Can't you just insert it into 
> .dev_groups member in alienware_wmax_wmi_driver?

I'd love to do this as it would simplify things a LOT, but some
user-space tools might expect this attributes to be exposed by the
"fake" platform device located at

/sys/devices/platform/alienware-wmi

If it were not for this, I would expose every attribute in the WMI
device.

~ Kurt
Re: [PATCH v7 00/14] platform/x86: alienware-wmi driver rework
Posted by Ilpo Järvinen 10 months, 2 weeks ago
On Mon, 3 Feb 2025, Kurt Borja wrote:

> On Mon Feb 3, 2025 at 4:20 AM -05, Ilpo Järvinen wrote:
> > On Mon, 3 Feb 2025, Kurt Borja wrote:
> >
> >> Hi!
> >>
> >> I bring some last minute modifications.
> >> 
> >> I found commit
> >> 
> >> 	8d8fc146dd7a ("nvmem: core: switch to use device_add_groups()")
> >> 
> >> which states that it's unnecesary to call device_remove_groups() when
> >> the device is removed, so I dropped it to simplify things.
> >
> > Hi Kurt,
> 
> Hi Ilpo,
> 
> >
> >> I also found commit
> >> 
> >> 	957961b6dcc8 ("hwmon: (oxp-sensors) Move tt_toggle attribute to dev_groups")
> >> 
> >> which states that no driver should add sysfs groups while probing the
> >> device as it races with userspace, so I re-added PROBE_FORCE_SYNCHRONOUS
> >> to the platform driver, so groups are added only after the device has
> >> finished probing.
> >>
> >> I'm not 100% sure that the second commit message applies here, but it is
> >> revd-by Greg K-H so I added it just in case.
> >
> > Which is why .dev_groups should be used as it is able to avoid those 
> > races on driver core level.
> 
> In previous discussions with Armin we agreed it made more sense to move
> WMAX-only groups from alienware-wmi-base.c to alienware-wmi-wmax.c when
> splitting.
> 
> I have no problem in moving them back to .dev_groups though.
> 
> >
> > Why you call device_add_groups() at all? Can't you just insert it into 
> > .dev_groups member in alienware_wmax_wmi_driver?
> 
> I'd love to do this as it would simplify things a LOT, but some
> user-space tools might expect this attributes to be exposed by the
> "fake" platform device located at
> 
> /sys/devices/platform/alienware-wmi
> 
> If it were not for this, I would expose every attribute in the WMI
> device.

Ah, sorry, I didn't pay attention where they were added to. I vaguely 
recall that discussion.

But still, you could make the groups available through .h and just add 
them directly into alienfx_groups (with an #ifdef/#else in .h), or is 
there again something I don't see?

Obviously, .is_visible functions need to be extended slightly to filter 
out by interface but that should be relatively easy too. Also, the group 
variable names should be properly prefixed when making them cross file 
boundary like that.

-- 
 i.
Re: [PATCH v7 00/14] platform/x86: alienware-wmi driver rework
Posted by Kurt Borja 10 months, 2 weeks ago
On Mon Feb 3, 2025 at 7:55 AM -05, Ilpo Järvinen wrote:
> On Mon, 3 Feb 2025, Kurt Borja wrote:
>
>> On Mon Feb 3, 2025 at 4:20 AM -05, Ilpo Järvinen wrote:
>> > On Mon, 3 Feb 2025, Kurt Borja wrote:
>> >
>> >> Hi!
>> >>
>> >> I bring some last minute modifications.
>> >> 
>> >> I found commit
>> >> 
>> >> 	8d8fc146dd7a ("nvmem: core: switch to use device_add_groups()")
>> >> 
>> >> which states that it's unnecesary to call device_remove_groups() when
>> >> the device is removed, so I dropped it to simplify things.
>> >
>> > Hi Kurt,
>> 
>> Hi Ilpo,
>> 
>> >
>> >> I also found commit
>> >> 
>> >> 	957961b6dcc8 ("hwmon: (oxp-sensors) Move tt_toggle attribute to dev_groups")
>> >> 
>> >> which states that no driver should add sysfs groups while probing the
>> >> device as it races with userspace, so I re-added PROBE_FORCE_SYNCHRONOUS
>> >> to the platform driver, so groups are added only after the device has
>> >> finished probing.
>> >>
>> >> I'm not 100% sure that the second commit message applies here, but it is
>> >> revd-by Greg K-H so I added it just in case.
>> >
>> > Which is why .dev_groups should be used as it is able to avoid those 
>> > races on driver core level.
>> 
>> In previous discussions with Armin we agreed it made more sense to move
>> WMAX-only groups from alienware-wmi-base.c to alienware-wmi-wmax.c when
>> splitting.
>> 
>> I have no problem in moving them back to .dev_groups though.
>> 
>> >
>> > Why you call device_add_groups() at all? Can't you just insert it into 
>> > .dev_groups member in alienware_wmax_wmi_driver?
>> 
>> I'd love to do this as it would simplify things a LOT, but some
>> user-space tools might expect this attributes to be exposed by the
>> "fake" platform device located at
>> 
>> /sys/devices/platform/alienware-wmi
>> 
>> If it were not for this, I would expose every attribute in the WMI
>> device.
>
> Ah, sorry, I didn't pay attention where they were added to. I vaguely 
> recall that discussion.
>
> But still, you could make the groups available through .h and just add 
> them directly into alienfx_groups (with an #ifdef/#else in .h), or is 
> there again something I don't see?

What do you think about something like:

alienware-wmi.h
---------------

#if IS_ENABLED(CONFIG_ALIENWARE_WMI_WMAX)
#define WMAX_ONLY_GROUP(name)		(wmax_##name)

extern const struct attribute_group wmax_hdmi_attribute_group;
...
#else
#define WMAX_ONLY_GROUP(name)		NULL
#endif

alienware-wmi-base.c
--------------------
...
static const struct attribute_group *alienfx_groups[] = {
	&zone_attribute_group,
	WMAX_ONLY_GROUP(hdmi_attribute_group),
	WMAX_ONLY_GROUP(amplifier_attribute_group),
	WMAX_ONLY_GROUP(deepsleep_attribute_group),
	NULL
...

};

>
> Obviously, .is_visible functions need to be extended slightly to filter 
> out by interface but that should be relatively easy too. Also, the group 
> variable names should be properly prefixed when making them cross file 
> boundary like that.
Re: [PATCH v7 00/14] platform/x86: alienware-wmi driver rework
Posted by Ilpo Järvinen 10 months, 2 weeks ago
On Mon, 3 Feb 2025, Kurt Borja wrote:

> On Mon Feb 3, 2025 at 7:55 AM -05, Ilpo Järvinen wrote:
> > On Mon, 3 Feb 2025, Kurt Borja wrote:
> >
> >> On Mon Feb 3, 2025 at 4:20 AM -05, Ilpo Järvinen wrote:
> >> > On Mon, 3 Feb 2025, Kurt Borja wrote:
> >> >
> >> >> Hi!
> >> >>
> >> >> I bring some last minute modifications.
> >> >> 
> >> >> I found commit
> >> >> 
> >> >> 	8d8fc146dd7a ("nvmem: core: switch to use device_add_groups()")
> >> >> 
> >> >> which states that it's unnecesary to call device_remove_groups() when
> >> >> the device is removed, so I dropped it to simplify things.
> >> >
> >> > Hi Kurt,
> >> 
> >> Hi Ilpo,
> >> 
> >> >
> >> >> I also found commit
> >> >> 
> >> >> 	957961b6dcc8 ("hwmon: (oxp-sensors) Move tt_toggle attribute to dev_groups")
> >> >> 
> >> >> which states that no driver should add sysfs groups while probing the
> >> >> device as it races with userspace, so I re-added PROBE_FORCE_SYNCHRONOUS
> >> >> to the platform driver, so groups are added only after the device has
> >> >> finished probing.
> >> >>
> >> >> I'm not 100% sure that the second commit message applies here, but it is
> >> >> revd-by Greg K-H so I added it just in case.
> >> >
> >> > Which is why .dev_groups should be used as it is able to avoid those 
> >> > races on driver core level.
> >> 
> >> In previous discussions with Armin we agreed it made more sense to move
> >> WMAX-only groups from alienware-wmi-base.c to alienware-wmi-wmax.c when
> >> splitting.
> >> 
> >> I have no problem in moving them back to .dev_groups though.
> >> 
> >> >
> >> > Why you call device_add_groups() at all? Can't you just insert it into 
> >> > .dev_groups member in alienware_wmax_wmi_driver?
> >> 
> >> I'd love to do this as it would simplify things a LOT, but some
> >> user-space tools might expect this attributes to be exposed by the
> >> "fake" platform device located at
> >> 
> >> /sys/devices/platform/alienware-wmi
> >> 
> >> If it were not for this, I would expose every attribute in the WMI
> >> device.
> >
> > Ah, sorry, I didn't pay attention where they were added to. I vaguely 
> > recall that discussion.
> >
> > But still, you could make the groups available through .h and just add 
> > them directly into alienfx_groups (with an #ifdef/#else in .h), or is 
> > there again something I don't see?
> 
> What do you think about something like:
> 
> alienware-wmi.h
> ---------------
> 
> #if IS_ENABLED(CONFIG_ALIENWARE_WMI_WMAX)
> #define WMAX_ONLY_GROUP(name)		(wmax_##name)
> 
> extern const struct attribute_group wmax_hdmi_attribute_group;
> ...
> #else
> #define WMAX_ONLY_GROUP(name)		NULL
> #endif
> 
> alienware-wmi-base.c
> --------------------
> ...
> static const struct attribute_group *alienfx_groups[] = {
> 	&zone_attribute_group,
> 	WMAX_ONLY_GROUP(hdmi_attribute_group),
> 	WMAX_ONLY_GROUP(amplifier_attribute_group),
> 	WMAX_ONLY_GROUP(deepsleep_attribute_group),

IMHO, just define WMAX_GROUPS in the header and use it here.

Similar to e.g. ARCH_PCI_DEV_GROUPS in drivers/pci/pci-sysfs.c.

> 	NULL
> ...
> 
> };
> 
> >
> > Obviously, .is_visible functions need to be extended slightly to filter 
> > out by interface but that should be relatively easy too. Also, the group 
> > variable names should be properly prefixed when making them cross file 
> > boundary like that.
> 

-- 
 i.
Re: [PATCH v7 00/14] platform/x86: alienware-wmi driver rework
Posted by Kurt Borja 10 months, 2 weeks ago
On Mon Feb 3, 2025 at 9:09 AM -05, Ilpo Järvinen wrote:
> On Mon, 3 Feb 2025, Kurt Borja wrote:
>
>> On Mon Feb 3, 2025 at 7:55 AM -05, Ilpo Järvinen wrote:
>> > On Mon, 3 Feb 2025, Kurt Borja wrote:
>> >
>> >> On Mon Feb 3, 2025 at 4:20 AM -05, Ilpo Järvinen wrote:
>> >> > On Mon, 3 Feb 2025, Kurt Borja wrote:
>> >> >
>> >> >> Hi!
>> >> >>
>> >> >> I bring some last minute modifications.
>> >> >> 
>> >> >> I found commit
>> >> >> 
>> >> >> 	8d8fc146dd7a ("nvmem: core: switch to use device_add_groups()")
>> >> >> 
>> >> >> which states that it's unnecesary to call device_remove_groups() when
>> >> >> the device is removed, so I dropped it to simplify things.
>> >> >
>> >> > Hi Kurt,
>> >> 
>> >> Hi Ilpo,
>> >> 
>> >> >
>> >> >> I also found commit
>> >> >> 
>> >> >> 	957961b6dcc8 ("hwmon: (oxp-sensors) Move tt_toggle attribute to dev_groups")
>> >> >> 
>> >> >> which states that no driver should add sysfs groups while probing the
>> >> >> device as it races with userspace, so I re-added PROBE_FORCE_SYNCHRONOUS
>> >> >> to the platform driver, so groups are added only after the device has
>> >> >> finished probing.
>> >> >>
>> >> >> I'm not 100% sure that the second commit message applies here, but it is
>> >> >> revd-by Greg K-H so I added it just in case.
>> >> >
>> >> > Which is why .dev_groups should be used as it is able to avoid those 
>> >> > races on driver core level.
>> >> 
>> >> In previous discussions with Armin we agreed it made more sense to move
>> >> WMAX-only groups from alienware-wmi-base.c to alienware-wmi-wmax.c when
>> >> splitting.
>> >> 
>> >> I have no problem in moving them back to .dev_groups though.
>> >> 
>> >> >
>> >> > Why you call device_add_groups() at all? Can't you just insert it into 
>> >> > .dev_groups member in alienware_wmax_wmi_driver?
>> >> 
>> >> I'd love to do this as it would simplify things a LOT, but some
>> >> user-space tools might expect this attributes to be exposed by the
>> >> "fake" platform device located at
>> >> 
>> >> /sys/devices/platform/alienware-wmi
>> >> 
>> >> If it were not for this, I would expose every attribute in the WMI
>> >> device.
>> >
>> > Ah, sorry, I didn't pay attention where they were added to. I vaguely 
>> > recall that discussion.
>> >
>> > But still, you could make the groups available through .h and just add 
>> > them directly into alienfx_groups (with an #ifdef/#else in .h), or is 
>> > there again something I don't see?
>> 
>> What do you think about something like:
>> 
>> alienware-wmi.h
>> ---------------
>> 
>> #if IS_ENABLED(CONFIG_ALIENWARE_WMI_WMAX)
>> #define WMAX_ONLY_GROUP(name)		(wmax_##name)
>> 
>> extern const struct attribute_group wmax_hdmi_attribute_group;
>> ...
>> #else
>> #define WMAX_ONLY_GROUP(name)		NULL
>> #endif
>> 
>> alienware-wmi-base.c
>> --------------------
>> ...
>> static const struct attribute_group *alienfx_groups[] = {
>> 	&zone_attribute_group,
>> 	WMAX_ONLY_GROUP(hdmi_attribute_group),
>> 	WMAX_ONLY_GROUP(amplifier_attribute_group),
>> 	WMAX_ONLY_GROUP(deepsleep_attribute_group),
>
> IMHO, just define WMAX_GROUPS in the header and use it here.
>
> Similar to e.g. ARCH_PCI_DEV_GROUPS in drivers/pci/pci-sysfs.c.

Thanks for the example, I was overthinking it. I'll send a v8 with this
approach!

~ Kurt

>
>> 	NULL
>> ...
>> 
>> };
>> 
>> >
>> > Obviously, .is_visible functions need to be extended slightly to filter 
>> > out by interface but that should be relatively easy too. Also, the group 
>> > variable names should be properly prefixed when making them cross file 
>> > boundary like that.
>> 
Re: [PATCH v7 00/14] platform/x86: alienware-wmi driver rework
Posted by Armin Wolf 10 months, 2 weeks ago
Am 03.02.25 um 07:20 schrieb Kurt Borja:

> Hi!
>
> I bring some last minute modifications.
>
> I found commit
>
> 	8d8fc146dd7a ("nvmem: core: switch to use device_add_groups()")
>
> which states that it's unnecesary to call device_remove_groups() when
> the device is removed, so I dropped it to simplify things.

What? That sound quite strange to me. I CCed Greg because i am curious how this
should work.

>
> I also found commit
>
> 	957961b6dcc8 ("hwmon: (oxp-sensors) Move tt_toggle attribute to dev_groups")
>
> which states that no driver should add sysfs groups while probing the
> device as it races with userspace, so I re-added PROBE_FORCE_SYNCHRONOUS
> to the platform driver, so groups are added only after the device has
> finished probing.

Using PROBE_FORCE_SYNCHRONOUS is not going to solve this problem here, please remove it.

Thanks,
Armin Wolf

> I'm not 100% sure that the second commit message applies here, but it is
> revd-by Greg K-H so I added it just in case.
>
> Aside from that, I added .pprof to awcc_quirks because I'm going to add
> support for new features after this series, and it makes sense that
> force_platform_profile only forces the pprof and not other upcoming
> features.
>
> ~ Kurt
> ---
> [02/14]
>    - In alienware_alienfx_setup() add a devm action to remove the created
>      platform device
>    - Drop device_remove_groups() in WMAX .remove callback
>    - Add PROBE_FORCE_SYNCHRONOUS to the platform driver
>    - Drop .remove callbacks on both WMI drivers
>
> [03/14]
>    - Add awcc_platform_profile_init() to create the platform_profile
>      device on quirks->thermal == true condition
>
> [07/14]
>    - Add .pprof to awcc_quirks
>
> [10/14]
>    - Drop unused member `quirks` on `alienfx_priv` (remnant of another
>      version)
>
> v6: https://lore.kernel.org/platform-driver-x86/20250127040406.17112-1-kuurtb@gmail.com/
>
> Kurt Borja (14):
>    platform/x86: alienware-wmi: Add a state container for LED control
>      feature
>    platform/x86: alienware-wmi: Add WMI Drivers
>    platform/x86: alienware-wmi: Add a state container for thermal control
>      methods
>    platform/x86: alienware-wmi: Refactor LED control methods
>    platform/x86: alienware-wmi: Refactor hdmi, amplifier, deepslp methods
>    platform/x86: alienware-wmi: Refactor thermal control methods
>    platform/x86: alienware-wmi: Split DMI table
>    MAINTAINERS: Update ALIENWARE WMI DRIVER entry
>    platform/x86: Rename alienware-wmi.c
>    platform/x86: Add alienware-wmi.h
>    platform/x86: Split the alienware-wmi driver
>    platform/x86: dell: Modify Makefile alignment
>    platform/x86: Update alienware-wmi config entries
>    platform/x86: alienware-wmi: Update header and module information
>
>   MAINTAINERS                                   |    4 +-
>   drivers/platform/x86/dell/Kconfig             |   30 +-
>   drivers/platform/x86/dell/Makefile            |   45 +-
>   .../platform/x86/dell/alienware-wmi-base.c    |  496 +++++++
>   .../platform/x86/dell/alienware-wmi-legacy.c  |   95 ++
>   .../platform/x86/dell/alienware-wmi-wmax.c    |  790 +++++++++++
>   drivers/platform/x86/dell/alienware-wmi.c     | 1249 -----------------
>   drivers/platform/x86/dell/alienware-wmi.h     |  101 ++
>   8 files changed, 1534 insertions(+), 1276 deletions(-)
>   create mode 100644 drivers/platform/x86/dell/alienware-wmi-base.c
>   create mode 100644 drivers/platform/x86/dell/alienware-wmi-legacy.c
>   create mode 100644 drivers/platform/x86/dell/alienware-wmi-wmax.c
>   delete mode 100644 drivers/platform/x86/dell/alienware-wmi.c
>   create mode 100644 drivers/platform/x86/dell/alienware-wmi.h
>
>
> base-commit: 05dbaf8dd8bf537d4b4eb3115ab42a5fb40ff1f5
Re: [PATCH v7 00/14] platform/x86: alienware-wmi driver rework
Posted by Kurt Borja 10 months, 2 weeks ago
On Mon Feb 3, 2025 at 10:34 AM -05, Armin Wolf wrote:
> Am 03.02.25 um 07:20 schrieb Kurt Borja:
>
>> Hi!
>>
>> I bring some last minute modifications.
>>
>> I found commit
>>
>> 	8d8fc146dd7a ("nvmem: core: switch to use device_add_groups()")
>>
>> which states that it's unnecesary to call device_remove_groups() when
>> the device is removed, so I dropped it to simplify things.
>
> What? That sound quite strange to me. I CCed Greg because i am curious how this
> should work.

I'm curious too! 

I found it while searching for something like devm_device_add_groups
which it turns out, was removed from the kernel.

>
>>
>> I also found commit
>>
>> 	957961b6dcc8 ("hwmon: (oxp-sensors) Move tt_toggle attribute to dev_groups")
>>
>> which states that no driver should add sysfs groups while probing the
>> device as it races with userspace, so I re-added PROBE_FORCE_SYNCHRONOUS
>> to the platform driver, so groups are added only after the device has
>> finished probing.
>
> Using PROBE_FORCE_SYNCHRONOUS is not going to solve this problem here, please remove it.

Yes, I'm going for Ilpo's .dev_groups approach.

~ Kurt

>
> Thanks,
> Armin Wolf
>
>> I'm not 100% sure that the second commit message applies here, but it is
>> revd-by Greg K-H so I added it just in case.
>>
>> Aside from that, I added .pprof to awcc_quirks because I'm going to add
>> support for new features after this series, and it makes sense that
>> force_platform_profile only forces the pprof and not other upcoming
>> features.
>>
>> ~ Kurt
>> ---
>> [02/14]
>>    - In alienware_alienfx_setup() add a devm action to remove the created
>>      platform device
>>    - Drop device_remove_groups() in WMAX .remove callback
>>    - Add PROBE_FORCE_SYNCHRONOUS to the platform driver
>>    - Drop .remove callbacks on both WMI drivers
>>
>> [03/14]
>>    - Add awcc_platform_profile_init() to create the platform_profile
>>      device on quirks->thermal == true condition
>>
>> [07/14]
>>    - Add .pprof to awcc_quirks
>>
>> [10/14]
>>    - Drop unused member `quirks` on `alienfx_priv` (remnant of another
>>      version)
>>
>> v6: https://lore.kernel.org/platform-driver-x86/20250127040406.17112-1-kuurtb@gmail.com/
>>
>> Kurt Borja (14):
>>    platform/x86: alienware-wmi: Add a state container for LED control
>>      feature
>>    platform/x86: alienware-wmi: Add WMI Drivers
>>    platform/x86: alienware-wmi: Add a state container for thermal control
>>      methods
>>    platform/x86: alienware-wmi: Refactor LED control methods
>>    platform/x86: alienware-wmi: Refactor hdmi, amplifier, deepslp methods
>>    platform/x86: alienware-wmi: Refactor thermal control methods
>>    platform/x86: alienware-wmi: Split DMI table
>>    MAINTAINERS: Update ALIENWARE WMI DRIVER entry
>>    platform/x86: Rename alienware-wmi.c
>>    platform/x86: Add alienware-wmi.h
>>    platform/x86: Split the alienware-wmi driver
>>    platform/x86: dell: Modify Makefile alignment
>>    platform/x86: Update alienware-wmi config entries
>>    platform/x86: alienware-wmi: Update header and module information
>>
>>   MAINTAINERS                                   |    4 +-
>>   drivers/platform/x86/dell/Kconfig             |   30 +-
>>   drivers/platform/x86/dell/Makefile            |   45 +-
>>   .../platform/x86/dell/alienware-wmi-base.c    |  496 +++++++
>>   .../platform/x86/dell/alienware-wmi-legacy.c  |   95 ++
>>   .../platform/x86/dell/alienware-wmi-wmax.c    |  790 +++++++++++
>>   drivers/platform/x86/dell/alienware-wmi.c     | 1249 -----------------
>>   drivers/platform/x86/dell/alienware-wmi.h     |  101 ++
>>   8 files changed, 1534 insertions(+), 1276 deletions(-)
>>   create mode 100644 drivers/platform/x86/dell/alienware-wmi-base.c
>>   create mode 100644 drivers/platform/x86/dell/alienware-wmi-legacy.c
>>   create mode 100644 drivers/platform/x86/dell/alienware-wmi-wmax.c
>>   delete mode 100644 drivers/platform/x86/dell/alienware-wmi.c
>>   create mode 100644 drivers/platform/x86/dell/alienware-wmi.h
>>
>>
>> base-commit: 05dbaf8dd8bf537d4b4eb3115ab42a5fb40ff1f5