Hi,
On Saturday, April 15th, 2023 at 15:30, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> Hi,
>
> On 4/15/23 15:24, Liav Albani wrote:
>
> > On 4/15/23 13:12, Hans de Goede wrote:
> >
> > > Hi,
> > >
> > > On 4/14/23 20:02, Daniel Bertalan wrote:
> > >
> > > > On the X380 Yoga, the `ECRD` and `ECWR` ACPI objects cannot be used for
> > > > accessing the Embedded Controller: instead of a method that reads from
> > > > the EC's memory, `ECRD` is the name of a location in high memory. This
> > > > meant that trying to call them would fail with the following message:
> > > >
> > > > ACPI: \_SB.PCI0.LPCB.EC.ECRD: 1 arguments were passed to a non-method
> > > > ACPI object (RegionField)
> > > >
> > > > With this commit, it is now possible to access the EC and read
> > > > temperature and fan speed information. Note that while writes to the
> > > > HFSP register do go through (as indicated by subsequent reads showing
> > > > the new value), the fan does not actually change its speed.
> > > >
> > > > Signed-off-by: Daniel Bertalan dani@xxxxxxxxxxxxxxxxxx
> > > > Interestig, this looks like a pretty clean solution to me.
> >
> > Daniel and I have looked in the DSDT ASL code and found a bunch of registers in high physical memory location (which is an ACPI OpRegion),
> > and one of the registers had a bit called ECRD.
> > However, there were many other registers that might be interesting as well, the problem is the short names in the ASL code (so we only see abbreviations essentially).
> >
> > While I do agree that adding this code is indeed a clean solution, if you (people that are dealing with Thinkpad laptops) know about cleaner way to access the embedded controller, I think it's preferable, because this way Daniel might be able to trigger the fan on that laptop so it will actually spin and will dissipate-out heat from the system, without the relying on the embedded controller to get into some sort of thermal state and then to trigger the fan.
>
>
> Have you tried falling back to the ec_read() and ec_write() helpers
> exported by the ACPI EC code ?
>
> The "first_ec" pointer used by these functions is exported too,
> so you could try modifying thinkpad_acpi to use ec_read() and
> ec_write() as fallback when first_ec is set (and the quirk
> added by this patch triggers).
This is basically what my patch does. If the ECRD/ECWR method handles
are NULL, thinkpad_acpi's acpi_ec_{read,write} functions fall back to
the regular ACPI EC helpers you mentioned.
Speaking of which, does anyone know why these private helper functions
exist in the first place? The code seems to use them interchangeably.
Even in the fan control code, there are places where the regular EC
helpers are called directly. Can we get away with always doing that?
Back to the issue at hand, is there someone we could ask if the X380Y
even supports manual fan control in the first place? My debugging
efforts are starting to look like a wild goose chase.
The thermal sensors and fan PWM readings now work, which is better
than nothing, but it would be good to get to the bottom of this.
All the best,
Daniel
> Regards,
>
> Hans
>
>
> > > Mark Pearson, do you have any remarks on this ?
> > >
> > > Regards,
> > >
> > > Hans
> > >
> > > > ---
> > > > drivers/platform/x86/thinkpad_acpi.c | 16 ++++++++++++++++
> > > > 1 file changed, 16 insertions(+)
> > > >
> > > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> > > > index 7191ff2625b1..6fe82f805ea8 100644
> > > > --- a/drivers/platform/x86/thinkpad_acpi.c
> > > > +++ b/drivers/platform/x86/thinkpad_acpi.c
> > > > @@ -11699,6 +11699,7 @@ static int __init thinkpad_acpi_module_init(void)
> > > > {
> > > > const struct dmi_system_id *dmi_id;
> > > > int ret, i;
> > > > + acpi_object_type obj_type;
> > > >
> > > > tpacpi_lifecycle = TPACPI_LIFE_INIT;
> > > >
> > > > @@ -11724,6 +11725,21 @@ static int __init thinkpad_acpi_module_init(void)
> > > > TPACPI_ACPIHANDLE_INIT(ecrd);
> > > > TPACPI_ACPIHANDLE_INIT(ecwr);
> > > >
> > > > + /*
> > > > + * Quirk: in some models (e.g. X380 Yoga), an object named ECRD
> > > > + * exists, but it is a register, not a method.
> > > > + */
> > > > + if (ecrd_handle) {
> > > > + acpi_get_type(ecrd_handle, &obj_type);
> > > > + if (obj_type != ACPI_TYPE_METHOD)
> > > > + ecrd_handle = NULL;
> > > > + }
> > > > + if (ecwr_handle) {
> > > > + acpi_get_type(ecwr_handle, &obj_type);
> > > > + if (obj_type != ACPI_TYPE_METHOD)
> > > > + ecwr_handle = NULL;
> > > > + }
> > > > +
> > > > tpacpi_wq = create_singlethread_workqueue(TPACPI_WORKQUEUE_NAME);
> > > > if (!tpacpi_wq) {
> > > > thinkpad_acpi_module_exit();
