Re: [PATCH v2 4/4] platform/x86: Add Lenovo Other Mode WMI Driver

[Derek John Clark]

On Fri, Jan 10, 2025 at 4:10 PM Armin Wolf <W_Armin@xxxxxx> wrote:

> >> Would it make sense to do something similar with each attribute, so that each attribute
> >> can use container_of() to access lenovo_wmi_om_priv without having to use a list lookup?
> >>
> >> This would of course mean that each attribute as to be allocated dynamically.
> >>
> >> Heep in mind that we are currently working on a new API for registering firmware-atrtibute class
> >> devices which should fix this.
> >>
> > I'm not sure I understand what you mean exactly. I think what you're
> > saying is, instead of an attr_group, allocate each attribute as a
> > struct in priv?
>
> Kind of. I envisioned something like this (pseudo code):
>
>         struct firmware_attribute {
>                 struct kobj_attribute attr;
>                 struct lenovo_wmi_om_priv;
>         }
>
> This would allow you to use container_of() to access priv, but would force you to allocate each attribute separately.

Ah, I see. Since we have the kobj in the functions we're accessing the
list in, we could get the firmware_attribute struct instead which
gives the pointer to priv. This will take a bit of refactoring for the
probe & macro sections but I agree that it would be worth it.

> Maybe you can wait with the lenovo-wmi-other driver until the improved firmware-attribute class device API has landed.
> Meanwhile we can focus on the lenovo-wmi-gamezone driver.

I'm not opposed to that. The API update seems to be progressing
quickly and with the multiple other changes I need to figure out v3
might come after that is in for_next anyway. I'll play it by ear and
work on the gamezone changes first.

> >> Is there a reason why this needs to be put inside the header? If no then please put this
> >> inside the driver.
> > To clarify, you mean the macros? I was under the impression they
> > belonged in headers but I can move them. I will move some of the
> > enums/structs as well which are referenced here and the driver only.
>
> I mean both the macros and the show functions. They are only used inside lenovo-wmi-other, so there
> is no reason to expose them inside the public header.
>
> Thanks,
> Armin Wolf

Make sense. I'll move those as well.

Thanks again Armin,
Derek

> >
> >> Thanks,
> >> Armin Wolf
> >>
> >>> +
> >>>    #endif /* !_LENOVO_WMI_H_ */