Re: [PATCH 3/4] platform/x86: ideapad-laptop: move ymc_trigger_ec from lenovo-ymc

[Gergo Koteles <soyer@irl.hu>]

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@irl.hu>
> > ---
> >  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