Hi Ilpo,
Thanks for the review.
On Thu, 2024-07-11 at 11:18 +0300, Ilpo Järvinen wrote:
> On Tue, 9 Jul 2024, Gergo Koteles wrote:
>
> > Some models need to trigger the EC for the yoga mode control to work
> > properly. EC triggering consists of a VPC call that needs to be
> > synchronized. The vpc_mutex is in the ideapad-laptop module, so
> > synchronization is easier there.
> >
> > Move the ymc_trigger_ec function to the ideapad-laptop module.
> >
> > Signed-off-by: Gergo Koteles <soyer@xxxxxx>
> > ---
> > drivers/platform/x86/Kconfig | 1 +
> > drivers/platform/x86/ideapad-laptop.c | 18 ++++++++++++++
> > drivers/platform/x86/ideapad-laptop.h | 2 ++
> > drivers/platform/x86/lenovo-ymc.c | 34 ++++-----------------------
> > 4 files changed, 26 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 665fa9524986..ddfccc226751 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -477,6 +477,7 @@ config LENOVO_YMC
> > tristate "Lenovo Yoga Tablet Mode Control"
> > depends on ACPI_WMI
> > depends on INPUT
> > + depends on IDEAPAD_LAPTOP
> > select INPUT_SPARSEKMAP
> > help
> > This driver maps the Tablet Mode Control switch to SW_TABLET_MODE input
> > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> > index 464cc5fd0088..4dcdbb153dda 100644
> > --- a/drivers/platform/x86/ideapad-laptop.c
> > +++ b/drivers/platform/x86/ideapad-laptop.c
> > @@ -1615,6 +1615,24 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_
> > priv->r_touchpad_val = value;
> > }
> >
> > +void ideapad_ymc_trigger_ec(void)
> > +{
> > + struct ideapad_private *priv;
> > + int ret;
> > +
> > + guard(mutex)(&ideapad_shared_mutex);
> > +
> > + priv = ideapad_shared;
> > + if (!priv)
> > + return;
> > +
> > + scoped_guard(mutex, &priv->vpc_mutex)
>
> It feels the ordering in this series is off because patch 2 add the
> mutex and here you add it into yet another function. If this patch
> would be before the other, the mutex changes could all go into the same
> patch.
>
I (wrongly) thought it would be easier to explain why a new dependency
is needed.
Mutex changes go into one patch in the v2.
> > + ret = write_ec_cmd(priv->adev->handle, VPCCMD_W_YMC, 1);
> > + if (ret)
> > + dev_warn(&priv->platform_device->dev, "Could not write YMC: %d\n", ret);
> > +}
> > +EXPORT_SYMBOL_GPL(ideapad_ymc_trigger_ec);
>
> This export should be namespaced.
It will be in the v2.
>
> I'll have to say I can accept this approach only grudgingly... it feels
> off even if I understand why you have to do this cross driver thing.
>
Based on the WMI driver development guide, I will add a notification
chain in v2 soon. Looks better.
Best Regards,
Gergo Koteles
