Hi
2021. február 12., péntek 9:40 keltezéssel, Hans de Goede írta:
> [...]
> I like the series but I would like to see the 3th patch to go even
> further wrt removing duplication between thinkpad_acpi and ideapad-laptop.
>
> The big difference between the 2 drivers is thinkpad_acpi relying on global
> variables while ideapad-laptop stores everything in a driver data-struct.
>
> What you can do is add a struct which holds all the necessary data for the
> dytc code, struct ideapad_dytc_priv is a start for this I guess. Lets say
> we rename struct ideapad_dytc_priv to struct dytc_priv, then thinkpad_acpi.c
> can have a:
>
> static struct dytc_priv *dytc_priv;
>
> And there can be a shared dytc probe function like this:
>
> static int dytc_profile_init(struct dytc_priv **dytc_priv_ret)
> {
> struct dytc_priv *dytc_priv;
>
> ...
>
> *dytc_priv_ret = dytc_priv;
> return NULL;
>
> error:
> // clean stuff
> *dytc_priv_ret = NULL;
> return err;
> }
>
> Which thinkpad_acpi can then call on its global dytc_priv pointer:
>
> err = dytc_profile_init(&dytc_priv);
>
> Where as ideapad_laptop would use the pointer inside its data struct:
>
> err = dytc_profile_init(&priv->dytc);
>
>
> I think this way we should be able to share almost all of the dytc code
> not just the 2 convert functions.
>
How exactly do you imagine that? In separate (e.g. "lenovo-dytc") kernel module?
And I assume platform profile registration/etc. should be done by shared code as well?
> One difference between the 2 which I'm aware of is that ideapad_laptop caches
> the handle, where as thinkpad_acpi looks it up everytime.
>
> Caching obviously is better, so the shared code should just cache it
> (add a copy of the handle pointer to the dytc_priv struct).
>
> I guess you may hit some other issues but those should all be fixable.
> So over all I think sharing most of the dytc code should be doable and
> we really should move in that direction.
>
> Note it would be best to do the further duplication removal in a
> third patch, or even in multiple further patches to make reviewing this
> easier.
> [...]
Regards,
Barnabás Pőcze
