| Message ID | 20240804230832.247852-1-luzmaximilian@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] hwmon: Add thermal sensor driver for Surface Aggregator Module | expand |
Hi Maximilian, kernel test robot noticed the following build errors: [auto build test ERROR on groeck-staging/hwmon-next] [also build test ERROR on linus/master v6.11-rc2 next-20240806] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Maximilian-Luz/hwmon-Add-thermal-sensor-driver-for-Surface-Aggregator-Module/20240805-071237 base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next patch link: https://lore.kernel.org/r/20240804230832.247852-1-luzmaximilian%40gmail.com patch subject: [PATCH v2] hwmon: Add thermal sensor driver for Surface Aggregator Module config: x86_64-randconfig-103-20240806 (https://download.01.org/0day-ci/archive/20240807/202408070251.EcevLWaJ-lkp@intel.com/config) compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240807/202408070251.EcevLWaJ-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202408070251.EcevLWaJ-lkp@intel.com/ All errors (new ones prefixed by >>): >> ld.lld: error: undefined symbol: __ssam_device_driver_register >>> referenced by surface_temp.c:231 (drivers/hwmon/surface_temp.c:231) >>> drivers/hwmon/surface_temp.o:(ssam_temp_init) in archive vmlinux.a -- >> ld.lld: error: undefined symbol: ssam_device_driver_unregister >>> referenced by surface_temp.c:231 (drivers/hwmon/surface_temp.c:231) >>> drivers/hwmon/surface_temp.o:(ssam_temp_exit) in archive vmlinux.a
On 8/4/24 16:08, Maximilian Luz wrote: > Some of the newer Microsoft Surface devices (such as the Surface Book > 3 and Pro 9) have thermal sensors connected via the Surface Aggregator > Module (the embedded controller on those devices). Add a basic driver > to read out the temperature values of those sensors. > > The EC can have up to 16 thermal sensors connected via a single > sub-device, each providing temperature readings and a label string. > > Link: https://github.com/linux-surface/surface-aggregator-module/issues/59 > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > Co-developed-by: Ivor Wanders <ivor@iwanders.net> > Signed-off-by: Ivor Wanders <ivor@iwanders.net> > Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com> > > --- > > Links: > - v1: https://lore.kernel.org/linux-kernel/ee8c39ab-d47a-481d-a19c-1d656519e66d@gmail.com > > Changes in v2: > - Drop patch 0003 ("platform/surface: aggregator_registry: Add support > for thermal sensors on the Surface Pro 9") as it has already been > applied. > - Squash patches 0001 ("hwmon: Add thermal sensor driver for Surface > Aggregator Module") and 0002 ("hwmon: surface_temp: Add support for > sensor names") into a single patch. > - Replace usage of WARN_ON() with dev_err(). > - Fix formatting and (strict) checkpatch complaints. > > --- > MAINTAINERS | 6 + > drivers/hwmon/Kconfig | 10 ++ > drivers/hwmon/Makefile | 1 + > drivers/hwmon/surface_temp.c | 235 +++++++++++++++++++++++++++++++++++ > 4 files changed, 252 insertions(+) > create mode 100644 drivers/hwmon/surface_temp.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 42decde38320..39c61db0169c 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -15200,6 +15200,12 @@ S: Maintained > F: Documentation/hwmon/surface_fan.rst > F: drivers/hwmon/surface_fan.c > > +MICROSOFT SURFACE SENSOR THERMAL DRIVER > +M: Maximilian Luz <luzmaximilian@gmail.com> > +L: linux-hwmon@vger.kernel.org > +S: Maintained > +F: drivers/hwmon/surface_temp.c > + > MICROSOFT SURFACE GPE LID SUPPORT DRIVER > M: Maximilian Luz <luzmaximilian@gmail.com> > L: platform-driver-x86@vger.kernel.org > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index b60fe2e58ad6..70c6385f0ed6 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -2080,6 +2080,16 @@ config SENSORS_SURFACE_FAN > > Select M or Y here, if you want to be able to read the fan's speed. > > +config SENSORS_SURFACE_TEMP > + tristate "Microsoft Surface Thermal Sensor Driver" > + depends on SURFACE_AGGREGATOR As the kernel test robot points out, this dependency is wrong. __ssam_device_driver_register() is only available if SURFACE_AGGREGATOR_BUS is enabled. Guenter > + help > + Driver for monitoring thermal sensors connected via the Surface > + Aggregator Module (embedded controller) on Microsoft Surface devices. > + > + This driver can also be built as a module. If so, the module > + will be called surface_temp. > + > config SENSORS_ADC128D818 > tristate "Texas Instruments ADC128D818" > depends on I2C > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index b1c7056c37db..3ce8d6a9202e 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -209,6 +209,7 @@ obj-$(CONFIG_SENSORS_SPARX5) += sparx5-temp.o > obj-$(CONFIG_SENSORS_SPD5118) += spd5118.o > obj-$(CONFIG_SENSORS_STTS751) += stts751.o > obj-$(CONFIG_SENSORS_SURFACE_FAN)+= surface_fan.o > +obj-$(CONFIG_SENSORS_SURFACE_TEMP)+= surface_temp.o > obj-$(CONFIG_SENSORS_SY7636A) += sy7636a-hwmon.o > obj-$(CONFIG_SENSORS_AMC6821) += amc6821.o > obj-$(CONFIG_SENSORS_TC74) += tc74.o > diff --git a/drivers/hwmon/surface_temp.c b/drivers/hwmon/surface_temp.c > new file mode 100644 > index 000000000000..cd21f331f157 > --- /dev/null > +++ b/drivers/hwmon/surface_temp.c > @@ -0,0 +1,235 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Thermal sensor subsystem driver for Surface System Aggregator Module (SSAM). > + * > + * Copyright (C) 2022-2023 Maximilian Luz <luzmaximilian@gmail.com> > + */ > + > +#include <linux/bitops.h> > +#include <linux/hwmon.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/types.h> > + > +#include <linux/surface_aggregator/controller.h> > +#include <linux/surface_aggregator/device.h> > + > +/* -- SAM interface. -------------------------------------------------------- */ > + > +/* > + * Available sensors are indicated by a 16-bit bitfield, where a 1 marks the > + * presence of a sensor. So we have at most 16 possible sensors/channels. > + */ > +#define SSAM_TMP_SENSOR_MAX_COUNT 16 > + > +/* > + * All names observed so far are 6 characters long, but there's only > + * zeros after the name, so perhaps they can be longer. This number reflects > + * the maximum zero-padded space observed in the returned buffer. > + */ > +#define SSAM_TMP_SENSOR_NAME_LENGTH 18 > + > +struct ssam_tmp_get_name_rsp { > + __le16 unknown1; > + char unknown2; > + char name[SSAM_TMP_SENSOR_NAME_LENGTH]; > +} __packed; > + > +static_assert(sizeof(struct ssam_tmp_get_name_rsp) == 21); > + > +SSAM_DEFINE_SYNC_REQUEST_CL_R(__ssam_tmp_get_available_sensors, __le16, { > + .target_category = SSAM_SSH_TC_TMP, > + .command_id = 0x04, > +}); > + > +SSAM_DEFINE_SYNC_REQUEST_MD_R(__ssam_tmp_get_temperature, __le16, { > + .target_category = SSAM_SSH_TC_TMP, > + .command_id = 0x01, > +}); > + > +SSAM_DEFINE_SYNC_REQUEST_MD_R(__ssam_tmp_get_name, struct ssam_tmp_get_name_rsp, { > + .target_category = SSAM_SSH_TC_TMP, > + .command_id = 0x0e, > +}); > + > +static int ssam_tmp_get_available_sensors(struct ssam_device *sdev, s16 *sensors) > +{ > + __le16 sensors_le; > + int status; > + > + status = __ssam_tmp_get_available_sensors(sdev, &sensors_le); > + if (status) > + return status; > + > + *sensors = le16_to_cpu(sensors_le); > + return 0; > +} > + > +static int ssam_tmp_get_temperature(struct ssam_device *sdev, u8 iid, long *temperature) > +{ > + __le16 temp_le; > + int status; > + > + status = __ssam_tmp_get_temperature(sdev->ctrl, sdev->uid.target, iid, &temp_le); > + if (status) > + return status; > + > + /* Convert 1/10 °K to 1/1000 °C */ > + *temperature = (le16_to_cpu(temp_le) - 2731) * 100L; > + return 0; > +} > + > +static int ssam_tmp_get_name(struct ssam_device *sdev, u8 iid, char *buf, size_t buf_len) > +{ > + struct ssam_tmp_get_name_rsp name_rsp; > + int status; > + > + status = __ssam_tmp_get_name(sdev->ctrl, sdev->uid.target, iid, &name_rsp); > + if (status) > + return status; > + > + /* > + * This should not fail unless the name in the returned struct is not > + * null-terminated or someone changed something in the struct > + * definitions above, since our buffer and struct have the same > + * capacity by design. So if this fails, log an error message. Since > + * the more likely cause is that the returned string isn't > + * null-terminated, we might have received garbage (as opposed to just > + * an incomplete string), so also fail the function. > + */ > + status = strscpy(buf, name_rsp.name, buf_len); > + if (status < 0) { > + dev_err(&sdev->dev, "received non-null-terminated sensor name string\n"); > + return status; > + } > + > + return 0; > +} > + > +/* -- Driver.---------------------------------------------------------------- */ > + > +struct ssam_temp { > + struct ssam_device *sdev; > + s16 sensors; > + char names[SSAM_TMP_SENSOR_MAX_COUNT][SSAM_TMP_SENSOR_NAME_LENGTH]; > +}; > + > +static umode_t ssam_temp_hwmon_is_visible(const void *data, > + enum hwmon_sensor_types type, > + u32 attr, int channel) > +{ > + const struct ssam_temp *ssam_temp = data; > + > + if (!(ssam_temp->sensors & BIT(channel))) > + return 0; > + > + return 0444; > +} > + > +static int ssam_temp_hwmon_read(struct device *dev, > + enum hwmon_sensor_types type, > + u32 attr, int channel, long *value) > +{ > + const struct ssam_temp *ssam_temp = dev_get_drvdata(dev); > + > + return ssam_tmp_get_temperature(ssam_temp->sdev, channel + 1, value); > +} > + > +static int ssam_temp_hwmon_read_string(struct device *dev, > + enum hwmon_sensor_types type, > + u32 attr, int channel, const char **str) > +{ > + const struct ssam_temp *ssam_temp = dev_get_drvdata(dev); > + > + *str = ssam_temp->names[channel]; > + return 0; > +} > + > +static const struct hwmon_channel_info * const ssam_temp_hwmon_info[] = { > + HWMON_CHANNEL_INFO(chip, > + HWMON_C_REGISTER_TZ), > + HWMON_CHANNEL_INFO(temp, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL), > + NULL > +}; > + > +static const struct hwmon_ops ssam_temp_hwmon_ops = { > + .is_visible = ssam_temp_hwmon_is_visible, > + .read = ssam_temp_hwmon_read, > + .read_string = ssam_temp_hwmon_read_string, > +}; > + > +static const struct hwmon_chip_info ssam_temp_hwmon_chip_info = { > + .ops = &ssam_temp_hwmon_ops, > + .info = ssam_temp_hwmon_info, > +}; > + > +static int ssam_temp_probe(struct ssam_device *sdev) > +{ > + struct ssam_temp *ssam_temp; > + struct device *hwmon_dev; > + s16 sensors; > + int channel; > + int status; > + > + status = ssam_tmp_get_available_sensors(sdev, &sensors); > + if (status) > + return status; > + > + ssam_temp = devm_kzalloc(&sdev->dev, sizeof(*ssam_temp), GFP_KERNEL); > + if (!ssam_temp) > + return -ENOMEM; > + > + ssam_temp->sdev = sdev; > + ssam_temp->sensors = sensors; > + > + /* Retrieve the name for each available sensor. */ > + for (channel = 0; channel < SSAM_TMP_SENSOR_MAX_COUNT; channel++) { > + if (!(sensors & BIT(channel))) > + continue; > + > + status = ssam_tmp_get_name(sdev, channel + 1, ssam_temp->names[channel], > + SSAM_TMP_SENSOR_NAME_LENGTH); > + if (status) > + return status; > + } > + > + hwmon_dev = devm_hwmon_device_register_with_info(&sdev->dev, "surface_thermal", ssam_temp, > + &ssam_temp_hwmon_chip_info, NULL); > + return PTR_ERR_OR_ZERO(hwmon_dev); > +} > + > +static const struct ssam_device_id ssam_temp_match[] = { > + { SSAM_SDEV(TMP, SAM, 0x00, 0x02) }, > + { }, > +}; > +MODULE_DEVICE_TABLE(ssam, ssam_temp_match); > + > +static struct ssam_device_driver ssam_temp = { > + .probe = ssam_temp_probe, > + .match_table = ssam_temp_match, > + .driver = { > + .name = "surface_temp", > + .probe_type = PROBE_PREFER_ASYNCHRONOUS, > + }, > +}; > +module_ssam_device_driver(ssam_temp); > + > +MODULE_AUTHOR("Maximilian Luz <luzmaximilian@gmail.com>"); > +MODULE_DESCRIPTION("Thermal sensor subsystem driver for Surface System Aggregator Module"); > +MODULE_LICENSE("GPL");
On 8/7/24 2:32 AM, Guenter Roeck wrote: > On 8/4/24 16:08, Maximilian Luz wrote: [...] >> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig >> index b60fe2e58ad6..70c6385f0ed6 100644 >> --- a/drivers/hwmon/Kconfig >> +++ b/drivers/hwmon/Kconfig >> @@ -2080,6 +2080,16 @@ config SENSORS_SURFACE_FAN >> Select M or Y here, if you want to be able to read the fan's speed. >> +config SENSORS_SURFACE_TEMP >> + tristate "Microsoft Surface Thermal Sensor Driver" >> + depends on SURFACE_AGGREGATOR > > As the kernel test robot points out, this dependency is wrong. > __ssam_device_driver_register() is only available > if SURFACE_AGGREGATOR_BUS is enabled. Right, I should have spotted this before submission, sorry. This should be depends on SURFACE_AGGREGATOR depends on SURFACE_AGGREGATOR_BUS I'll fix that for v3 and likely re-spin this weekend. Anything else I should address for that? Regards, Max
On 8/7/24 12:25, Maximilian Luz wrote: > On 8/7/24 2:32 AM, Guenter Roeck wrote: >> On 8/4/24 16:08, Maximilian Luz wrote: > > [...] > >>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig >>> index b60fe2e58ad6..70c6385f0ed6 100644 >>> --- a/drivers/hwmon/Kconfig >>> +++ b/drivers/hwmon/Kconfig >>> @@ -2080,6 +2080,16 @@ config SENSORS_SURFACE_FAN >>> Select M or Y here, if you want to be able to read the fan's speed. >>> +config SENSORS_SURFACE_TEMP >>> + tristate "Microsoft Surface Thermal Sensor Driver" >>> + depends on SURFACE_AGGREGATOR >> >> As the kernel test robot points out, this dependency is wrong. >> __ssam_device_driver_register() is only available >> if SURFACE_AGGREGATOR_BUS is enabled. > > Right, I should have spotted this before submission, sorry. This should > be > > depends on SURFACE_AGGREGATOR > depends on SURFACE_AGGREGATOR_BUS > SURFACE_AGGREGATOR_BUS already depends on SURFACE_AGGREGATOR, so the extra dependency is not needed. > I'll fix that for v3 and likely re-spin this weekend. Anything else I > should address for that? > I didn't notice anything, but then I didn't try to build the code myself, or try to run checkpatch. My tools run checkpatch --strict, as necessary for hwmon submissions, so you might want to make sure that it passes. Guenter
On 8/7/24 9:50 PM, Guenter Roeck wrote: > On 8/7/24 12:25, Maximilian Luz wrote: >> On 8/7/24 2:32 AM, Guenter Roeck wrote: >>> On 8/4/24 16:08, Maximilian Luz wrote: >> >> [...] >> >>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig >>>> index b60fe2e58ad6..70c6385f0ed6 100644 >>>> --- a/drivers/hwmon/Kconfig >>>> +++ b/drivers/hwmon/Kconfig >>>> @@ -2080,6 +2080,16 @@ config SENSORS_SURFACE_FAN >>>> Select M or Y here, if you want to be able to read the fan's speed. >>>> +config SENSORS_SURFACE_TEMP >>>> + tristate "Microsoft Surface Thermal Sensor Driver" >>>> + depends on SURFACE_AGGREGATOR >>> >>> As the kernel test robot points out, this dependency is wrong. >>> __ssam_device_driver_register() is only available >>> if SURFACE_AGGREGATOR_BUS is enabled. >> >> Right, I should have spotted this before submission, sorry. This should >> be >> >> depends on SURFACE_AGGREGATOR >> depends on SURFACE_AGGREGATOR_BUS >> > > SURFACE_AGGREGATOR_BUS already depends on SURFACE_AGGREGATOR, so the extra > dependency is not needed. Unfortunately, SURFACE_AGGREGATOR_BUS is a bool and SURFACE_AGGREGATOR tri-state, and the inference of whether SURFACE_AGGREGATOR needs to be built in or not breaks because of that. Meaning we could have something like SENSORS_SURFACE_TEMP=y (tri-state, module) SURFACE_AGGREGATOR_BUS=y (bool, optional-code-flag) SURFACE_AGGREGATOR=m (tri-state, module) because SURFACE_AGGREGATOR_BUS is fine with either m or y. But in reality, SENSORS_SURFACE_TEMP=y would require SURFACE_AGGREGATOR=y. Maybe there's a better way to solve this? I guess it should be possible to convert SURFACE_AGGREGATOR_BUS into a tri-state (even though it's really just a flag to build a certain part of code into SURFACE_AGGREGATOR). I think at this point we could also consider removing that flag entirely and just build the bus support in unconditionally... but that discussion is outside of the scope here. >> I'll fix that for v3 and likely re-spin this weekend. Anything else I >> should address for that? >> > > I didn't notice anything, but then I didn't try to build the code myself, > or try to run checkpatch. My tools run checkpatch --strict, as necessary > for hwmon submissions, so you might want to make sure that it passes. Perfect, I already checked that. Thank you! Best regards, Max
On 8/7/24 13:11, Maximilian Luz wrote: > On 8/7/24 9:50 PM, Guenter Roeck wrote: >> On 8/7/24 12:25, Maximilian Luz wrote: >>> On 8/7/24 2:32 AM, Guenter Roeck wrote: >>>> On 8/4/24 16:08, Maximilian Luz wrote: >>> >>> [...] >>> >>>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig >>>>> index b60fe2e58ad6..70c6385f0ed6 100644 >>>>> --- a/drivers/hwmon/Kconfig >>>>> +++ b/drivers/hwmon/Kconfig >>>>> @@ -2080,6 +2080,16 @@ config SENSORS_SURFACE_FAN >>>>> Select M or Y here, if you want to be able to read the fan's speed. >>>>> +config SENSORS_SURFACE_TEMP >>>>> + tristate "Microsoft Surface Thermal Sensor Driver" >>>>> + depends on SURFACE_AGGREGATOR >>>> >>>> As the kernel test robot points out, this dependency is wrong. >>>> __ssam_device_driver_register() is only available >>>> if SURFACE_AGGREGATOR_BUS is enabled. >>> >>> Right, I should have spotted this before submission, sorry. This should >>> be >>> >>> depends on SURFACE_AGGREGATOR >>> depends on SURFACE_AGGREGATOR_BUS >>> >> >> SURFACE_AGGREGATOR_BUS already depends on SURFACE_AGGREGATOR, so the extra >> dependency is not needed. > > Unfortunately, SURFACE_AGGREGATOR_BUS is a bool and SURFACE_AGGREGATOR > tri-state, and the inference of whether SURFACE_AGGREGATOR needs to be > built in or not breaks because of that. Meaning we could have something > like > > SENSORS_SURFACE_TEMP=y (tri-state, module) > SURFACE_AGGREGATOR_BUS=y (bool, optional-code-flag) > SURFACE_AGGREGATOR=m (tri-state, module) > > because SURFACE_AGGREGATOR_BUS is fine with either m or y. But in > reality, SENSORS_SURFACE_TEMP=y would require SURFACE_AGGREGATOR=y. > Ah yes, I can see that the double dependency is there everywhere. Normally I'd have assumed that to be handled with SURFACE_AGGREGATOR_BUS as non-configurable option and its users selecting it, i.e., depends on SURFACE_AGGREGATOR select SURFACE_AGGREGATOR_BUS but, sure, it is your call to make SURFACE_AGGREGATOR_BUS a configurable (instead of selectable) option. I don't understand the benefit of being able to enable SURFACE_AGGREGATOR_BUS without any users, but then maybe I just don't have sufficient understanding of the context. Thanks, Guenter
On 5.08.2024 1:08 AM, Maximilian Luz wrote: > Some of the newer Microsoft Surface devices (such as the Surface Book > 3 and Pro 9) have thermal sensors connected via the Surface Aggregator > Module (the embedded controller on those devices). Add a basic driver > to read out the temperature values of those sensors. > > The EC can have up to 16 thermal sensors connected via a single > sub-device, each providing temperature readings and a label string. > > Link: https://github.com/linux-surface/surface-aggregator-module/issues/59 > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > Co-developed-by: Ivor Wanders <ivor@iwanders.net> > Signed-off-by: Ivor Wanders <ivor@iwanders.net> > Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com> > > --- Gave it a shot on SL7, some names are repeated and one sensor is totally busted /sys/class/hwmon/hwmon66/name:surface_thermal /sys/class/hwmon/hwmon66/temp10_input:32200 /sys/class/hwmon/hwmon66/temp10_label:I_RTS2 /sys/class/hwmon/hwmon66/temp11_input:31600 /sys/class/hwmon/hwmon66/temp11_label:I_RTS3 /sys/class/hwmon/hwmon66/temp12_input:38000 /sys/class/hwmon/hwmon66/temp12_label:I_RTS4 /sys/class/hwmon/hwmon66/temp1_input:43900 /sys/class/hwmon/hwmon66/temp1_label:I_RTS1 /sys/class/hwmon/hwmon66/temp2_input:44000 /sys/class/hwmon/hwmon66/temp2_label:I_RTS2 /sys/class/hwmon/hwmon66/temp3_input:47300 /sys/class/hwmon/hwmon66/temp3_label:I_RTS3 /sys/class/hwmon/hwmon66/temp4_input:-273100 /sys/class/hwmon/hwmon66/temp4_label:I_RTS4 /sys/class/hwmon/hwmon66/temp5_input:31300 /sys/class/hwmon/hwmon66/temp5_label:I_RTS5 /sys/class/hwmon/hwmon66/temp9_input:37100 /sys/class/hwmon/hwmon66/temp9_label:I_RTS1 Konrad
> Gave it a shot on SL7, some names are repeated and one sensor is totally busted
Interesting, thanks for testing. I'm not sure if this is the right place for discussing this, or
whether we should take this to the downstream thread (link in cover letter or in [2]).
I have duplicates for RTS{1..3} as well, so you're not alone there, for me it's also sensor 1 and 9
forming a duplicated name pair [1], this makes me wonder if the names are always of the form
`I_RTS#` where # is (id % 8 + 1), if that is the case for all surface models the names may not be
that much of a value add?
The surface diagnostics tool actually doesn't request these names: it has hardcoded names for just
three sensor ids that are part of the diagnostics [2], but I don't know if those three id's are
stable across the various devices though.
~Ivor
[1]: https://github.com/linux-surface/surface-aggregator-module/pull/68#issue-2054614428
[2]: https://github.com/linux-surface/surface-aggregator-module/issues/59#issuecomment-1974827016
On 8/10/24 1:35 AM, Konrad Dybcio wrote: > On 5.08.2024 1:08 AM, Maximilian Luz wrote: >> Some of the newer Microsoft Surface devices (such as the Surface Book >> 3 and Pro 9) have thermal sensors connected via the Surface Aggregator >> Module (the embedded controller on those devices). Add a basic driver >> to read out the temperature values of those sensors. >> >> The EC can have up to 16 thermal sensors connected via a single >> sub-device, each providing temperature readings and a label string. >> >> Link: https://github.com/linux-surface/surface-aggregator-module/issues/59 >> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >> Co-developed-by: Ivor Wanders <ivor@iwanders.net> >> Signed-off-by: Ivor Wanders <ivor@iwanders.net> >> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com> >> >> --- > > Gave it a shot on SL7, some names are repeated and one sensor is > totally busted > > /sys/class/hwmon/hwmon66/name:surface_thermal > /sys/class/hwmon/hwmon66/temp10_input:32200 > /sys/class/hwmon/hwmon66/temp10_label:I_RTS2 > /sys/class/hwmon/hwmon66/temp11_input:31600 > /sys/class/hwmon/hwmon66/temp11_label:I_RTS3 > /sys/class/hwmon/hwmon66/temp12_input:38000 > /sys/class/hwmon/hwmon66/temp12_label:I_RTS4 > /sys/class/hwmon/hwmon66/temp1_input:43900 > /sys/class/hwmon/hwmon66/temp1_label:I_RTS1 > /sys/class/hwmon/hwmon66/temp2_input:44000 > /sys/class/hwmon/hwmon66/temp2_label:I_RTS2 > /sys/class/hwmon/hwmon66/temp3_input:47300 > /sys/class/hwmon/hwmon66/temp3_label:I_RTS3 > /sys/class/hwmon/hwmon66/temp4_input:-273100 > /sys/class/hwmon/hwmon66/temp4_label:I_RTS4 > /sys/class/hwmon/hwmon66/temp5_input:31300 > /sys/class/hwmon/hwmon66/temp5_label:I_RTS5 > /sys/class/hwmon/hwmon66/temp9_input:37100 > /sys/class/hwmon/hwmon66/temp9_label:I_RTS1 Hmm, on the SPX it looks like this: I_RTS1: +31.9°C I_RTS2: +31.3°C I_RTS3: +31.4°C I_RTS4: +28.3°C I_RTS5: +29.3°C I_RTS6: +29.3°C I_RTS7: +29.3°C I_RTS8: +29.3°C VTS1: +30.2°C VTS2: +0.0°C VTS3: +0.0°C VTS4: +0.0°C VTS5: +0.0°C So VTS2-5 seem like they may not actually be connected, but the rest at least look somewhat sensible. I'd probably still keep the names as they at least might give an indication what the sensors could be for. But there's a good chance that we're missing something on how MS envisions these sensors to work exactly. Regards, Max
On 10.08.2024 3:04 AM, Maximilian Luz wrote: > On 8/10/24 1:35 AM, Konrad Dybcio wrote: >> On 5.08.2024 1:08 AM, Maximilian Luz wrote: >>> Some of the newer Microsoft Surface devices (such as the Surface Book >>> 3 and Pro 9) have thermal sensors connected via the Surface Aggregator >>> Module (the embedded controller on those devices). Add a basic driver >>> to read out the temperature values of those sensors. >>> >>> The EC can have up to 16 thermal sensors connected via a single >>> sub-device, each providing temperature readings and a label string. >>> >>> Link: https://github.com/linux-surface/surface-aggregator-module/issues/59 >>> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >>> Co-developed-by: Ivor Wanders <ivor@iwanders.net> >>> Signed-off-by: Ivor Wanders <ivor@iwanders.net> >>> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com> >>> >>> --- >> >> Gave it a shot on SL7, some names are repeated and one sensor is >> totally busted >> >> /sys/class/hwmon/hwmon66/name:surface_thermal >> /sys/class/hwmon/hwmon66/temp10_input:32200 >> /sys/class/hwmon/hwmon66/temp10_label:I_RTS2 >> /sys/class/hwmon/hwmon66/temp11_input:31600 >> /sys/class/hwmon/hwmon66/temp11_label:I_RTS3 >> /sys/class/hwmon/hwmon66/temp12_input:38000 >> /sys/class/hwmon/hwmon66/temp12_label:I_RTS4 >> /sys/class/hwmon/hwmon66/temp1_input:43900 >> /sys/class/hwmon/hwmon66/temp1_label:I_RTS1 >> /sys/class/hwmon/hwmon66/temp2_input:44000 >> /sys/class/hwmon/hwmon66/temp2_label:I_RTS2 >> /sys/class/hwmon/hwmon66/temp3_input:47300 >> /sys/class/hwmon/hwmon66/temp3_label:I_RTS3 >> /sys/class/hwmon/hwmon66/temp4_input:-273100 >> /sys/class/hwmon/hwmon66/temp4_label:I_RTS4 >> /sys/class/hwmon/hwmon66/temp5_input:31300 >> /sys/class/hwmon/hwmon66/temp5_label:I_RTS5 >> /sys/class/hwmon/hwmon66/temp9_input:37100 >> /sys/class/hwmon/hwmon66/temp9_label:I_RTS1 > > Hmm, on the SPX it looks like this: > > I_RTS1: +31.9°C > I_RTS2: +31.3°C > I_RTS3: +31.4°C > I_RTS4: +28.3°C > I_RTS5: +29.3°C > I_RTS6: +29.3°C > I_RTS7: +29.3°C > I_RTS8: +29.3°C > VTS1: +30.2°C > VTS2: +0.0°C > VTS3: +0.0°C > VTS4: +0.0°C > VTS5: +0.0°C > > So VTS2-5 seem like they may not actually be connected, but the rest at > least look somewhat sensible. I'd probably still keep the names as they > at least might give an indication what the sensors could be for. > > But there's a good chance that we're missing something on how MS > envisions these sensors to work exactly. I think the takeaway here is that I'll keep the sensors disabled for now on SL7 until we have a better idea and not block this driver Konrad
On 8/7/24 10:37 PM, Guenter Roeck wrote: > On 8/7/24 13:11, Maximilian Luz wrote: >> On 8/7/24 9:50 PM, Guenter Roeck wrote: >>> On 8/7/24 12:25, Maximilian Luz wrote: >>>> On 8/7/24 2:32 AM, Guenter Roeck wrote: >>>>> On 8/4/24 16:08, Maximilian Luz wrote: >>>> >>>> [...] >>>> >>>>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig >>>>>> index b60fe2e58ad6..70c6385f0ed6 100644 >>>>>> --- a/drivers/hwmon/Kconfig >>>>>> +++ b/drivers/hwmon/Kconfig >>>>>> @@ -2080,6 +2080,16 @@ config SENSORS_SURFACE_FAN >>>>>> Select M or Y here, if you want to be able to read the fan's speed. >>>>>> +config SENSORS_SURFACE_TEMP >>>>>> + tristate "Microsoft Surface Thermal Sensor Driver" >>>>>> + depends on SURFACE_AGGREGATOR >>>>> >>>>> As the kernel test robot points out, this dependency is wrong. >>>>> __ssam_device_driver_register() is only available >>>>> if SURFACE_AGGREGATOR_BUS is enabled. >>>> >>>> Right, I should have spotted this before submission, sorry. This should >>>> be >>>> >>>> depends on SURFACE_AGGREGATOR >>>> depends on SURFACE_AGGREGATOR_BUS >>>> >>> >>> SURFACE_AGGREGATOR_BUS already depends on SURFACE_AGGREGATOR, so the extra >>> dependency is not needed. >> >> Unfortunately, SURFACE_AGGREGATOR_BUS is a bool and SURFACE_AGGREGATOR >> tri-state, and the inference of whether SURFACE_AGGREGATOR needs to be >> built in or not breaks because of that. Meaning we could have something >> like >> >> SENSORS_SURFACE_TEMP=y (tri-state, module) >> SURFACE_AGGREGATOR_BUS=y (bool, optional-code-flag) >> SURFACE_AGGREGATOR=m (tri-state, module) >> >> because SURFACE_AGGREGATOR_BUS is fine with either m or y. But in >> reality, SENSORS_SURFACE_TEMP=y would require SURFACE_AGGREGATOR=y. >> > > Ah yes, I can see that the double dependency is there everywhere. Normally I'd > have assumed that to be handled with SURFACE_AGGREGATOR_BUS as non-configurable > option and its users selecting it, i.e., > > depends on SURFACE_AGGREGATOR > select SURFACE_AGGREGATOR_BUS > > but, sure, it is your call to make SURFACE_AGGREGATOR_BUS a configurable > (instead of selectable) option. I don't understand the benefit of being able > to enable SURFACE_AGGREGATOR_BUS without any users, but then maybe I just > don't have sufficient understanding of the context. No... you're completely right. "select" does make more sense. I'll add a patch to change that for surface_fan as well for v3 (and then I guess slowly update it for the other subsystems too). Best regards, Max
On 10.08.2024 3:14 AM, Konrad Dybcio wrote: > On 10.08.2024 3:04 AM, Maximilian Luz wrote: >> On 8/10/24 1:35 AM, Konrad Dybcio wrote: >>> On 5.08.2024 1:08 AM, Maximilian Luz wrote: >>>> Some of the newer Microsoft Surface devices (such as the Surface Book >>>> 3 and Pro 9) have thermal sensors connected via the Surface Aggregator >>>> Module (the embedded controller on those devices). Add a basic driver >>>> to read out the temperature values of those sensors. >>>> >>>> The EC can have up to 16 thermal sensors connected via a single >>>> sub-device, each providing temperature readings and a label string. >>>> >>>> Link: https://github.com/linux-surface/surface-aggregator-module/issues/59 >>>> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >>>> Co-developed-by: Ivor Wanders <ivor@iwanders.net> >>>> Signed-off-by: Ivor Wanders <ivor@iwanders.net> >>>> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com> >>>> >>>> --- >>> >>> Gave it a shot on SL7, some names are repeated and one sensor is >>> totally busted >>> >>> /sys/class/hwmon/hwmon66/name:surface_thermal >>> /sys/class/hwmon/hwmon66/temp10_input:32200 >>> /sys/class/hwmon/hwmon66/temp10_label:I_RTS2 >>> /sys/class/hwmon/hwmon66/temp11_input:31600 >>> /sys/class/hwmon/hwmon66/temp11_label:I_RTS3 >>> /sys/class/hwmon/hwmon66/temp12_input:38000 >>> /sys/class/hwmon/hwmon66/temp12_label:I_RTS4 >>> /sys/class/hwmon/hwmon66/temp1_input:43900 >>> /sys/class/hwmon/hwmon66/temp1_label:I_RTS1 >>> /sys/class/hwmon/hwmon66/temp2_input:44000 >>> /sys/class/hwmon/hwmon66/temp2_label:I_RTS2 >>> /sys/class/hwmon/hwmon66/temp3_input:47300 >>> /sys/class/hwmon/hwmon66/temp3_label:I_RTS3 >>> /sys/class/hwmon/hwmon66/temp4_input:-273100 >>> /sys/class/hwmon/hwmon66/temp4_label:I_RTS4 >>> /sys/class/hwmon/hwmon66/temp5_input:31300 >>> /sys/class/hwmon/hwmon66/temp5_label:I_RTS5 >>> /sys/class/hwmon/hwmon66/temp9_input:37100 >>> /sys/class/hwmon/hwmon66/temp9_label:I_RTS1 >> >> Hmm, on the SPX it looks like this: >> >> I_RTS1: +31.9°C >> I_RTS2: +31.3°C >> I_RTS3: +31.4°C >> I_RTS4: +28.3°C >> I_RTS5: +29.3°C >> I_RTS6: +29.3°C >> I_RTS7: +29.3°C >> I_RTS8: +29.3°C >> VTS1: +30.2°C >> VTS2: +0.0°C >> VTS3: +0.0°C >> VTS4: +0.0°C >> VTS5: +0.0°C >> >> So VTS2-5 seem like they may not actually be connected, but the rest at >> least look somewhat sensible. I'd probably still keep the names as they >> at least might give an indication what the sensors could be for. >> >> But there's a good chance that we're missing something on how MS >> envisions these sensors to work exactly. > > I think the takeaway here is that I'll keep the sensors disabled for > now on SL7 until we have a better idea and not block this driver I had an idea.. maybe the busted sensor is for something modem-related. My unit doesn't come with one. Konrad
On 8/19/24 12:26 PM, Konrad Dybcio wrote: > On 10.08.2024 3:14 AM, Konrad Dybcio wrote: >> On 10.08.2024 3:04 AM, Maximilian Luz wrote: >>> On 8/10/24 1:35 AM, Konrad Dybcio wrote: >>>> On 5.08.2024 1:08 AM, Maximilian Luz wrote: >>>>> Some of the newer Microsoft Surface devices (such as the Surface Book >>>>> 3 and Pro 9) have thermal sensors connected via the Surface Aggregator >>>>> Module (the embedded controller on those devices). Add a basic driver >>>>> to read out the temperature values of those sensors. >>>>> >>>>> The EC can have up to 16 thermal sensors connected via a single >>>>> sub-device, each providing temperature readings and a label string. >>>>> >>>>> Link: https://github.com/linux-surface/surface-aggregator-module/issues/59 >>>>> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >>>>> Co-developed-by: Ivor Wanders <ivor@iwanders.net> >>>>> Signed-off-by: Ivor Wanders <ivor@iwanders.net> >>>>> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com> >>>>> >>>>> --- >>>> >>>> Gave it a shot on SL7, some names are repeated and one sensor is >>>> totally busted >>>> >>>> /sys/class/hwmon/hwmon66/name:surface_thermal >>>> /sys/class/hwmon/hwmon66/temp10_input:32200 >>>> /sys/class/hwmon/hwmon66/temp10_label:I_RTS2 >>>> /sys/class/hwmon/hwmon66/temp11_input:31600 >>>> /sys/class/hwmon/hwmon66/temp11_label:I_RTS3 >>>> /sys/class/hwmon/hwmon66/temp12_input:38000 >>>> /sys/class/hwmon/hwmon66/temp12_label:I_RTS4 >>>> /sys/class/hwmon/hwmon66/temp1_input:43900 >>>> /sys/class/hwmon/hwmon66/temp1_label:I_RTS1 >>>> /sys/class/hwmon/hwmon66/temp2_input:44000 >>>> /sys/class/hwmon/hwmon66/temp2_label:I_RTS2 >>>> /sys/class/hwmon/hwmon66/temp3_input:47300 >>>> /sys/class/hwmon/hwmon66/temp3_label:I_RTS3 >>>> /sys/class/hwmon/hwmon66/temp4_input:-273100 >>>> /sys/class/hwmon/hwmon66/temp4_label:I_RTS4 >>>> /sys/class/hwmon/hwmon66/temp5_input:31300 >>>> /sys/class/hwmon/hwmon66/temp5_label:I_RTS5 >>>> /sys/class/hwmon/hwmon66/temp9_input:37100 >>>> /sys/class/hwmon/hwmon66/temp9_label:I_RTS1 >>> >>> Hmm, on the SPX it looks like this: >>> >>> I_RTS1: +31.9°C >>> I_RTS2: +31.3°C >>> I_RTS3: +31.4°C >>> I_RTS4: +28.3°C >>> I_RTS5: +29.3°C >>> I_RTS6: +29.3°C >>> I_RTS7: +29.3°C >>> I_RTS8: +29.3°C >>> VTS1: +30.2°C >>> VTS2: +0.0°C >>> VTS3: +0.0°C >>> VTS4: +0.0°C >>> VTS5: +0.0°C >>> >>> So VTS2-5 seem like they may not actually be connected, but the rest at >>> least look somewhat sensible. I'd probably still keep the names as they >>> at least might give an indication what the sensors could be for. >>> >>> But there's a good chance that we're missing something on how MS >>> envisions these sensors to work exactly. >> >> I think the takeaway here is that I'll keep the sensors disabled for >> now on SL7 until we have a better idea and not block this driver > > I had an idea.. maybe the busted sensor is for something modem-related. > My unit doesn't come with one. I think that could make sense. It is interesting though that SAM says it's present (there's a bit-field indicating which sensor is there), but I guess they just hard-code that the same way across all variants. Best regards, Max
diff --git a/MAINTAINERS b/MAINTAINERS index 42decde38320..39c61db0169c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -15200,6 +15200,12 @@ S: Maintained F: Documentation/hwmon/surface_fan.rst F: drivers/hwmon/surface_fan.c +MICROSOFT SURFACE SENSOR THERMAL DRIVER +M: Maximilian Luz <luzmaximilian@gmail.com> +L: linux-hwmon@vger.kernel.org +S: Maintained +F: drivers/hwmon/surface_temp.c + MICROSOFT SURFACE GPE LID SUPPORT DRIVER M: Maximilian Luz <luzmaximilian@gmail.com> L: platform-driver-x86@vger.kernel.org diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index b60fe2e58ad6..70c6385f0ed6 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -2080,6 +2080,16 @@ config SENSORS_SURFACE_FAN Select M or Y here, if you want to be able to read the fan's speed. +config SENSORS_SURFACE_TEMP + tristate "Microsoft Surface Thermal Sensor Driver" + depends on SURFACE_AGGREGATOR + help + Driver for monitoring thermal sensors connected via the Surface + Aggregator Module (embedded controller) on Microsoft Surface devices. + + This driver can also be built as a module. If so, the module + will be called surface_temp. + config SENSORS_ADC128D818 tristate "Texas Instruments ADC128D818" depends on I2C diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index b1c7056c37db..3ce8d6a9202e 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -209,6 +209,7 @@ obj-$(CONFIG_SENSORS_SPARX5) += sparx5-temp.o obj-$(CONFIG_SENSORS_SPD5118) += spd5118.o obj-$(CONFIG_SENSORS_STTS751) += stts751.o obj-$(CONFIG_SENSORS_SURFACE_FAN)+= surface_fan.o +obj-$(CONFIG_SENSORS_SURFACE_TEMP)+= surface_temp.o obj-$(CONFIG_SENSORS_SY7636A) += sy7636a-hwmon.o obj-$(CONFIG_SENSORS_AMC6821) += amc6821.o obj-$(CONFIG_SENSORS_TC74) += tc74.o diff --git a/drivers/hwmon/surface_temp.c b/drivers/hwmon/surface_temp.c new file mode 100644 index 000000000000..cd21f331f157 --- /dev/null +++ b/drivers/hwmon/surface_temp.c @@ -0,0 +1,235 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Thermal sensor subsystem driver for Surface System Aggregator Module (SSAM). + * + * Copyright (C) 2022-2023 Maximilian Luz <luzmaximilian@gmail.com> + */ + +#include <linux/bitops.h> +#include <linux/hwmon.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/types.h> + +#include <linux/surface_aggregator/controller.h> +#include <linux/surface_aggregator/device.h> + +/* -- SAM interface. -------------------------------------------------------- */ + +/* + * Available sensors are indicated by a 16-bit bitfield, where a 1 marks the + * presence of a sensor. So we have at most 16 possible sensors/channels. + */ +#define SSAM_TMP_SENSOR_MAX_COUNT 16 + +/* + * All names observed so far are 6 characters long, but there's only + * zeros after the name, so perhaps they can be longer. This number reflects + * the maximum zero-padded space observed in the returned buffer. + */ +#define SSAM_TMP_SENSOR_NAME_LENGTH 18 + +struct ssam_tmp_get_name_rsp { + __le16 unknown1; + char unknown2; + char name[SSAM_TMP_SENSOR_NAME_LENGTH]; +} __packed; + +static_assert(sizeof(struct ssam_tmp_get_name_rsp) == 21); + +SSAM_DEFINE_SYNC_REQUEST_CL_R(__ssam_tmp_get_available_sensors, __le16, { + .target_category = SSAM_SSH_TC_TMP, + .command_id = 0x04, +}); + +SSAM_DEFINE_SYNC_REQUEST_MD_R(__ssam_tmp_get_temperature, __le16, { + .target_category = SSAM_SSH_TC_TMP, + .command_id = 0x01, +}); + +SSAM_DEFINE_SYNC_REQUEST_MD_R(__ssam_tmp_get_name, struct ssam_tmp_get_name_rsp, { + .target_category = SSAM_SSH_TC_TMP, + .command_id = 0x0e, +}); + +static int ssam_tmp_get_available_sensors(struct ssam_device *sdev, s16 *sensors) +{ + __le16 sensors_le; + int status; + + status = __ssam_tmp_get_available_sensors(sdev, &sensors_le); + if (status) + return status; + + *sensors = le16_to_cpu(sensors_le); + return 0; +} + +static int ssam_tmp_get_temperature(struct ssam_device *sdev, u8 iid, long *temperature) +{ + __le16 temp_le; + int status; + + status = __ssam_tmp_get_temperature(sdev->ctrl, sdev->uid.target, iid, &temp_le); + if (status) + return status; + + /* Convert 1/10 °K to 1/1000 °C */ + *temperature = (le16_to_cpu(temp_le) - 2731) * 100L; + return 0; +} + +static int ssam_tmp_get_name(struct ssam_device *sdev, u8 iid, char *buf, size_t buf_len) +{ + struct ssam_tmp_get_name_rsp name_rsp; + int status; + + status = __ssam_tmp_get_name(sdev->ctrl, sdev->uid.target, iid, &name_rsp); + if (status) + return status; + + /* + * This should not fail unless the name in the returned struct is not + * null-terminated or someone changed something in the struct + * definitions above, since our buffer and struct have the same + * capacity by design. So if this fails, log an error message. Since + * the more likely cause is that the returned string isn't + * null-terminated, we might have received garbage (as opposed to just + * an incomplete string), so also fail the function. + */ + status = strscpy(buf, name_rsp.name, buf_len); + if (status < 0) { + dev_err(&sdev->dev, "received non-null-terminated sensor name string\n"); + return status; + } + + return 0; +} + +/* -- Driver.---------------------------------------------------------------- */ + +struct ssam_temp { + struct ssam_device *sdev; + s16 sensors; + char names[SSAM_TMP_SENSOR_MAX_COUNT][SSAM_TMP_SENSOR_NAME_LENGTH]; +}; + +static umode_t ssam_temp_hwmon_is_visible(const void *data, + enum hwmon_sensor_types type, + u32 attr, int channel) +{ + const struct ssam_temp *ssam_temp = data; + + if (!(ssam_temp->sensors & BIT(channel))) + return 0; + + return 0444; +} + +static int ssam_temp_hwmon_read(struct device *dev, + enum hwmon_sensor_types type, + u32 attr, int channel, long *value) +{ + const struct ssam_temp *ssam_temp = dev_get_drvdata(dev); + + return ssam_tmp_get_temperature(ssam_temp->sdev, channel + 1, value); +} + +static int ssam_temp_hwmon_read_string(struct device *dev, + enum hwmon_sensor_types type, + u32 attr, int channel, const char **str) +{ + const struct ssam_temp *ssam_temp = dev_get_drvdata(dev); + + *str = ssam_temp->names[channel]; + return 0; +} + +static const struct hwmon_channel_info * const ssam_temp_hwmon_info[] = { + HWMON_CHANNEL_INFO(chip, + HWMON_C_REGISTER_TZ), + HWMON_CHANNEL_INFO(temp, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL), + NULL +}; + +static const struct hwmon_ops ssam_temp_hwmon_ops = { + .is_visible = ssam_temp_hwmon_is_visible, + .read = ssam_temp_hwmon_read, + .read_string = ssam_temp_hwmon_read_string, +}; + +static const struct hwmon_chip_info ssam_temp_hwmon_chip_info = { + .ops = &ssam_temp_hwmon_ops, + .info = ssam_temp_hwmon_info, +}; + +static int ssam_temp_probe(struct ssam_device *sdev) +{ + struct ssam_temp *ssam_temp; + struct device *hwmon_dev; + s16 sensors; + int channel; + int status; + + status = ssam_tmp_get_available_sensors(sdev, &sensors); + if (status) + return status; + + ssam_temp = devm_kzalloc(&sdev->dev, sizeof(*ssam_temp), GFP_KERNEL); + if (!ssam_temp) + return -ENOMEM; + + ssam_temp->sdev = sdev; + ssam_temp->sensors = sensors; + + /* Retrieve the name for each available sensor. */ + for (channel = 0; channel < SSAM_TMP_SENSOR_MAX_COUNT; channel++) { + if (!(sensors & BIT(channel))) + continue; + + status = ssam_tmp_get_name(sdev, channel + 1, ssam_temp->names[channel], + SSAM_TMP_SENSOR_NAME_LENGTH); + if (status) + return status; + } + + hwmon_dev = devm_hwmon_device_register_with_info(&sdev->dev, "surface_thermal", ssam_temp, + &ssam_temp_hwmon_chip_info, NULL); + return PTR_ERR_OR_ZERO(hwmon_dev); +} + +static const struct ssam_device_id ssam_temp_match[] = { + { SSAM_SDEV(TMP, SAM, 0x00, 0x02) }, + { }, +}; +MODULE_DEVICE_TABLE(ssam, ssam_temp_match); + +static struct ssam_device_driver ssam_temp = { + .probe = ssam_temp_probe, + .match_table = ssam_temp_match, + .driver = { + .name = "surface_temp", + .probe_type = PROBE_PREFER_ASYNCHRONOUS, + }, +}; +module_ssam_device_driver(ssam_temp); + +MODULE_AUTHOR("Maximilian Luz <luzmaximilian@gmail.com>"); +MODULE_DESCRIPTION("Thermal sensor subsystem driver for Surface System Aggregator Module"); +MODULE_LICENSE("GPL");