Re: [PATCH v2 2/4] platform: arm64: dell-xps-ec: new driver

[Bjorn Andersson]

On Sun, Apr 05, 2026 at 08:48:25PM +0000, Aleksandrs Vinarskis wrote:
> On Sunday, April 5th, 2026 at 02:29, Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx> wrote:
> 
> > On 04/04/2026 13:55, Aleksandrs Vinarskis wrote:
> > > Introduce EC driver for Dell XPS 13 9345 (codename 'tributo') which may
> > > partially of fully compatible with Snapdragon-based Dell Latitude,
> > > Inspiron ('thena'). Primary function of this driver is unblock EC's
> > > thermal management, specifically to provide it with necessary
> > > information to control device fans, peripherals power.
> > >
> > > The driver was developed primarily by analyzing ACPI DSDT's _DSM and
> > > i2c dumps of communication between SoC and EC. Changes to Windows
> > > driver's behavior include increasing temperature feed loop from ~50ms
> > > to 100ms here.
> > >
> > > While Xps's EC is rather complex and controls practically all device
> > > peripherals including touch row's brightness and special keys such as
> > > mic mute, these do not go over this particular i2c interface.
> > >
> > > Not yet implemented features:
> > > - On lid-close IRQ event is registered. Windows performs what to
> > >    appears to be thermistor constants readout, though its not obvious
> > >    what it used for.
> > > - According to ACPI's _DSM there is a method to readout fans' RPM.
> > > - Initial thermistor constants were sniffed from Windows, these can be
> > >    likely fine tuned for better cooling performance.
> > > - There is additional temperature reading that Windows sents to EC but
> > >    more rare than others, likely SoC T_j / TZ98 or TZ4. This is the only
> > >    thermal zone who's reading can exceed 115C without triggering thermal
> > >    shutdown.
> > > - Given similarities between 'tributo' and 'thena' platforms, including
> > >    EC i2c address, driver can be potentially extended to support both.
> > >
> > > Signed-off-by: Aleksandrs Vinarskis <alex@xxxxxxxxxxxxx>
> > > ---
> > >   MAINTAINERS                          |   1 +
> > >   drivers/platform/arm64/Kconfig       |  12 ++
> > >   drivers/platform/arm64/Makefile      |   1 +
> > >   drivers/platform/arm64/dell-xps-ec.c | 267 +++++++++++++++++++++++++++++++++++
> > >   4 files changed, 281 insertions(+)
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index a5d175559f4468dfe363b319a1b08d3425f4d712..c150f57b60706224e5b24b0dfb3d8a9b81f36398 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -7240,6 +7240,7 @@ DELL XPS EMBEDDED CONTROLLER DRIVER
> > >   M:	Aleksandrs Vinarskis <alex@xxxxxxxxxxxxx>
> > >   S:	Maintained
> > >   F:	Documentation/devicetree/bindings/embedded-controller/dell,xps13-9345-ec.yaml
> > > +F:	drivers/platform/arm64/dell-xps-ec.c
> > >
> > >   DELTA AHE-50DC FAN CONTROL MODULE DRIVER
> > >   M:	Zev Weiss <zev@xxxxxxxxxxxxxxxxx>
> > > diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
> > > index 10f905d7d6bfa5fad30a0689d3a20481268c781e..0bc8f016032bb05cb3a7cc50bdf1092da04153bc 100644
> > > --- a/drivers/platform/arm64/Kconfig
> > > +++ b/drivers/platform/arm64/Kconfig
> > > @@ -33,6 +33,18 @@ config EC_ACER_ASPIRE1
> > >   	  laptop where this information is not properly exposed via the
> > >   	  standard ACPI devices.
> > >
> > > +config EC_DELL_XPS
> > > +	tristate "Dell XPS 9345 Embedded Controller driver"
> > > +	depends on ARCH_QCOM || COMPILE_TEST
> > > +	depends on I2C
> > > +	depends on IIO
> > > +	help
> > > +	  Driver for the Embedded Controller in the Qualcomm Snapdragon-based
> > > +	  Dell XPS 13 9345, which handles thermal management and fan speed
> > > +	  control.
> > > +
> > > +	  Say M or Y here to include this support.
> > > +
> > >   config EC_HUAWEI_GAOKUN
> > >   	tristate "Huawei Matebook E Go Embedded Controller driver"
> > >   	depends on ARCH_QCOM || COMPILE_TEST
> > > diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile
> > > index 60c131cff6a15bb51a49c9edab95badf513ee0f6..6768dc6c2310837374e67381cfc729bed1fdaaef 100644
> > > --- a/drivers/platform/arm64/Makefile
> > > +++ b/drivers/platform/arm64/Makefile
> > > @@ -6,6 +6,7 @@
> > >   #
> > >
> > >   obj-$(CONFIG_EC_ACER_ASPIRE1)	+= acer-aspire1-ec.o
> > > +obj-$(CONFIG_EC_DELL_XPS)	+= dell-xps-ec.o
> > >   obj-$(CONFIG_EC_HUAWEI_GAOKUN)	+= huawei-gaokun-ec.o
> > >   obj-$(CONFIG_EC_LENOVO_YOGA_C630) += lenovo-yoga-c630.o
> > >   obj-$(CONFIG_EC_LENOVO_THINKPAD_T14S) += lenovo-thinkpad-t14s.o
> > > diff --git a/drivers/platform/arm64/dell-xps-ec.c b/drivers/platform/arm64/dell-xps-ec.c
> > > new file mode 100644
> > > index 0000000000000000000000000000000000000000..bf1495fbe473ccdb82b95a66b56e8525f782cc8e
> > > --- /dev/null
> > > +++ b/drivers/platform/arm64/dell-xps-ec.c
> > > @@ -0,0 +1,267 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (c) 2026, Aleksandrs Vinarskis <alex@xxxxxxxxxxxxx>
> > > + */
> > > +
> > > +#include <linux/array_size.h>
> > > +#include <linux/dev_printk.h>
> > > +#include <linux/device.h>
> > > +#include <linux/devm-helpers.h>
> > > +#include <linux/err.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/iio/consumer.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/jiffies.h>
> > > +#include <linux/module.h>
> > > +#include <linux/pm.h>
> > > +#include <linux/unaligned.h>
> > > +#include <linux/workqueue.h>
> > > +
> > > +#define DELL_XPS_EC_SUSPEND_CMD		0xb9
> > > +#define DELL_XPS_EC_SUSPEND_MSG_LEN	64
> > > +
> > > +#define DELL_XPS_EC_TEMP_CMD0		0xfb
> > > +#define DELL_XPS_EC_TEMP_CMD1		0x20
> > > +#define DELL_XPS_EC_TEMP_CMD3		0x02
> > > +#define DELL_XPS_EC_TEMP_MSG_LEN	6
> > > +#define DELL_XPS_EC_TEMP_POLL_JIFFIES	msecs_to_jiffies(100)
> > > +
> > > +/*
> > > + * Format:
> > > + * - header/unknown (2 bytes)
> > > + * - per-thermistor entries (3 bytes): thermistor_id, param1, param2
> > > + */
> > > +static const u8 dell_xps_ec_thermistor_profile[] = {
> > > +	0xff, 0x54,
> > > +	0x01, 0x00, 0x2b,	/* sys_therm0 */
> > > +	0x02, 0x44, 0x2a,	/* sys_therm1 */
> > > +	0x03, 0x44, 0x2b,	/* sys_therm2 */
> > > +	0x04, 0x44, 0x28,	/* sys_therm3 */
> > > +	0x05, 0x55, 0x2a,	/* sys_therm4 */
> > > +	0x06, 0x44, 0x26,	/* sys_therm5 */
> > > +	0x07, 0x44, 0x2b,	/* sys_therm6 */
> > > +};
> > > +
> > > +/*
> > > + * Mapping from IIO channel name to EC command byte
> > > + */
> > > +static const struct {
> > > +	const char *name;
> > > +	u8 cmd;
> > > +} dell_xps_ec_therms[] = {
> > > +	/* TODO: 0x01 is sent only occasionally, likely TZ98 or TZ4 */
> > > +	{ "sys_therm0", 0x02 },
> > > +	{ "sys_therm1", 0x03 },
> > > +	{ "sys_therm2", 0x04 },
> > > +	{ "sys_therm3", 0x05 },
> > > +	{ "sys_therm4", 0x06 },
> > > +	{ "sys_therm5", 0x07 },
> > > +	{ "sys_therm6", 0x08 },
> > > +};
> >
> > You could probably retrieve these strings from the dt if you really need
> > them.
> >
> > I don't think you need static consts in your driver though you could
> > just as easily do `sprintf("sys_therm%d\n", i) where you use
> > ec_therms[i].name - the name is only used to print errors and you have
> > the index of the channel when you do.
> >
> > It would be nicer to get the strings from DT - certainly make the string
> > names mandatory but, then let the DT specify those names.
> >
> > Either that or just do the sprintf("sys_therm%d\n", i); for the index,
> > whichever you wish yourself.
> 
> Hi Bryan,
> 
> Will answer here to all three comments about `sys_thermX`.
> 
> The reason I have added them as static consts here, and defined them in
> the schema is because the order of the channels matters:
> 1. On my XPS (UEFI v2.11.0) changes in sys_therm2 immediately result in
>    changes in fan speeds. Other channels seemingly have no affect, at
>    least when spoofed one by one, implying that EC cares which value
>    is which.
> 2. As I do not know internals of the EC firmware, even if today the other
>    thermistor channels ordering is seemingly not relevant, we cannot be
>    sure it will not change with EC firmware upgrade.
> 
> I have reconstructed the order of channels by comparing i2c data dumps
> and real-time temps on Windows, eg. sys_therm0 is sent to EC under id 0x02
> and represents the TZ71 (around dram on XPS). There is no other reason to
> have the names of the channels in this driver except for enforcing the
> channel mapping, so `sprintf("sys_therm%d\n", i)` wouldn't be useful.
> 
> By allowing source and sink to define the names and not enforcing it in
> schema we lose ability to force the correct order, there is no way of
> knowing whether "lpddr5-therm" or "ssd-therm" goes first. By forcing
> "sys_thermX" convention, one would need to figure which one is which,
> for example by referring to laptop schematics. I assume, "thena"'s
> schematics has thermistors labeled as "sys_thermX"?
> 
> I do agree that labels of the ADC nodes could be more useful for the
> user. So far I followed the example of sc8280xp platforms that define
> ADC channels with "sys_thermX". Perhaps, we could separate the
> io-channel-names and ADC node labels then? eg:
> 

The general guidance for such naming questions is to follow naming from
the schematics, whenever available.

> + io-channel-names = "sys_therm0",
> + 		     "sys_therm1",
> ...
> 
> + &pmk8550_vadc {
> +	sys_therm0: channel@14c {
> + 		reg = <PM8350_ADC7_GPIO3_100K_PU(1)>;
> + 		qcom,hw-settle-time = <200>;
> + 		qcom,ratiometric;
> + 		label = "lpddr5x-therm";
> 
> Though not sure if such approach is 'legal'?

I might be missing something, but that does look legal. Your node's
label follows (what I assume to be) the naming in the schematics and you
provide a human-friendly label.


PS. Once we have these adc channels in place, I presume there's also a
TM that would allow us to wire them up as cooling-maps, to throttle the
CPUs? Similar to "skin-temp-thermal" in x13s.

Regards,
Bjorn