Re: [PATCH 0/2] IdeaPad platform profile support

[Rafael J. Wysocki]

On Mon, Jan 4, 2021 at 9:58 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi,
>
> On 1/4/21 9:33 PM, Rafael J. Wysocki wrote:
> > On Mon, Jan 4, 2021 at 3:36 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >>
> >> Hi,
> >>
> >> On 1/1/21 1:56 PM, Jiaxun Yang wrote:
> >>> Tested on Lenovo Yoga-14SARE Chinese Edition.
> >>>
> >>> Jiaxun Yang (2):
> >>>   ACPI: platform-profile: Introduce data parameter to handler
> >>>   platform/x86: ideapad-laptop: DYTC Platform profile support
> >>>
> >>>  drivers/acpi/platform_profile.c       |   4 +-
> >>>  drivers/platform/x86/Kconfig          |   1 +
> >>>  drivers/platform/x86/ideapad-laptop.c | 281 ++++++++++++++++++++++++++
> >>>  include/linux/platform_profile.h      |   5 +-
> >>>  4 files changed, 287 insertions(+), 4 deletions(-)
> >>
> >>
> >> Thank you for your series, unfortunately the
> >> "ACPI: platform-profile: Introduce data parameter to handler"
> >> patch causes a conflict with the pending:
> >> "[PATCH v8 3/3] platform/x86: thinkpad_acpi: Add platform profile support"
> >> patch.
> >>
> >> But I do agree that adding that data parameter makes sense, so
> >> it might be best to merge:
> >>
> >> "ACPI: platform-profile: Introduce data parameter to handler"
> >>
> >> First and then rebase the thinkpad_acpi patch on top.
> >>
> >> Rafael, do you think you could add:
> >>
> >> "ACPI: platform-profile: Introduce data parameter to handler"
> >>
> >> To the 2 ACPI: platform-profile patches which you already have pending for 5.11-rc# ?
> >
> > I'm not sure why that patch is needed at all, because whoever
> > registers a platform profile handler needs to have access to the
> > original handler object anyway.
>
> True, I was actually thinking that instead of the data argument, we might
> pass a pointer to the original handler object like this:
>
> @@ -64,7 +64,7 @@ static ssize_t platform_profile_show(struct device *dev,
>                 return -ENODEV;
>         }
>
> -       err = cur_profile->profile_get(&profile);
> +       err = cur_profile->profile_get(cur_profile, &profile);
>         mutex_unlock(&profile_lock);
>         if (err)
>                 return err;

I would prefer this approach.

>
> And then the driver which has registered the cur_profile, can get to
> its own data by using container of on the cur_profile pointer.
>
> With the code currently in your bleeding-edge branch, there is no way
> for any driver-code to get to its own (possibly/likely dynamically
> allocated) driver-data struct.
>
> E.g. a typical driver using only dynamic data tied to device_get_drvdata,
> might have this:
>
> struct driver_data {
>         ...
>         struct platform_profile_handler profile_handler;
>         ...
> };
>
> int probe(...) {
>         struct driver_data *my_data;
>
>         my_data = devm_kzalloc(dev, sizeof(*my_data), GFP_KERNEL);
>
>         ...
>
>         ret = platform_profile_register(&my_data->profile_handler);
>         ...
> }
>
> And with the change which I suggest above would then be able to
> get the struct driver_data *my_data back from the profile_get callback by
> using container_of on the struct platform_profile_handler *profile_handler
> argument added to the profile_get callback.

OK, fair enough.

> I know that the platform_profile stuff is intended to only have a
> single provider, so this could use global variables, but some
> drivers which may be a provider use 0 global variables (other then
> module_params) atm and it would be a lot cleaner from the pov
> of the design of these drivers to be able to do something like the
> pseudo code above. Which is why I added my Reviewed-by to patch 1/2
> of the series from this thread.
>
> Patch 1/2 does use a slightly different approach then I suggest above,
> thinking more about this it would be cleaner IMHO to just pass the
> cur_profile pointer to the callbacks as the pseudo-code patch which I
> wrote above does. Drivers which use globals can then just ignore
> the extra argument (and keep the platform_profile_handler struct const)
> where as drivers which use dynamic allocation can embed the struct in
> their driver's data-struct.

Agreed.

> > Also, on a somewhat related note, I'm afraid that it may not be a good
> > idea to push this series for 5.11-rc in the face of recent objections
> > against new material going in after the merge window.
>
> That is fine with me, since this did not make rc1 (nor rc2) I'm not entirely
> comfortable with sending out a late pull-req for the pdx86 side of this
> either, so lets postpone this to 5.12 (sorry Mark).
>
> Rafael, once we have the discussion with the passing a pointer back to
> the drivers data thing resolved (and a patch merged for that if we go
> that route) can you provide me with an immutable branch to merge into
> pdx86/for-next so that I can then merge the pdx86 bits on top ?

Sure, no problem.

> Note this does not need to be done right now around say rc4 would be fine,
> so that we have some time for the patches currently in bleeding-edge to
> settle a bit.

OK

Cheers!