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.
