* Re: [PATCH v2 02/11] dt-bindings: hwmon: Add Apple System Management Controller hwmon schema
From: James Calligeros @ 2025-09-28 0:36 UTC (permalink / raw)
To: Janne Grunau, Rob Herring
Cc: Sven Peter, Alyssa Rosenzweig, Neal Gompa, Lee Jones,
Krzysztof Kozlowski, Conor Dooley, Alexandre Belloni,
Jean Delvare, Guenter Roeck, Dmitry Torokhov, asahi,
linux-arm-kernel, devicetree, linux-kernel, linux-rtc,
linux-hwmon, linux-input
In-Reply-To: <CAL_JsqK-9n3_H6vS80bZuZiSPi9UNuMzHEPFL_EzYTeyNS1cYg@mail.gmail.com>
Hi Rob,
On Friday, 26 September 2025 7:43:23 am Australian Eastern Standard Time Rob
Herring wrote:
> On Thu, Sep 25, 2025 at 3:49 PM Janne Grunau <j@jannau.net> wrote:
> > On Fri, Aug 29, 2025 at 11:40:57AM -0500, Rob Herring wrote:
> > >
> > > This should be something like this:
> > >
> > > "^current-[A-Za-z0-9]{4}$":
> > > $ref: "#/$defs/sensor"
> > > unevaluatedProperties: false
> > >
> > > With the $defs/sensor being:
> > >
> > > $defs:
> > > sensor:
> > > type: object
> > >
> > > properties:
> > > apple,key-id:
> > > $ref: /schemas/types.yaml#/definitions/string
> > > pattern: "^[A-Za-z0-9]{4}$"
> > >
> > > description:
> > > The SMC FourCC key of the desired sensor. Must match the
> > > node's suffix.
> > >
> > > label:
> > > description: Human-readable name for the sensor
> > >
> > > required:
> > > - apple,key-id
> > > - label
> > >
> > > Though in general, 'label' should never be required being just for human
> > > convenience.
> >
> > That does not sound as it would be compatible with skipping nodes in the
> > driver if the node misses label. The driver could of course fall back
> > to create a hwmon sensors without labels.
>
> The driver absolutely should.
The original submission (and our downstream version) do this, but I changed
it for v2 per Sven's feedback [1]. Outside of development/experimentation,
we will (should) never have a sensor in the Devicetree of uknown utility.
If we know what a sensor is for, then we should have a label for it.
> > I looks to me it would be a
> > stretch to call the presence of the labels human convenience.
>
> Then it is an abuse of 'label". "label" is supposed to be literally
> that. Matching a sticker on a port of a device.
>
> If you need to associate a sensor with some other piece of h/w, then
> that should be via a phandle or something.
I don't think doing so is particularly useful for this platform. Few of
the sensors that we know about are directly related to any one piece of
hardware.
It's pretty much just the CPU cores and Broadcom module. The rest are things
like fans, palm rest area temperature sensors, ammeters and voltmeters for
entire
rails, etc.
Even where we can reliably associate a sensor to a piece of hardware,
(e.g. the WiFi/BT board), doing so does not by itself do anything useful. We
still need to write a human-readable label for the sensor.
I was trying to avoid yet another vendor property, but would something like
'apple,sensor-label' work here?
> Rob
James
[1]: https://lore.kernel.org/asahi/4a95cbf3-b3ae-4b26-8db2-dd5cf14a4c0c@kernel.org/
^ permalink raw reply
* [PATCH v5] HID: logitech-dj: Add support for a new lightspeed receiver iteration
From: Mavroudis Chatzilazaridis @ 2025-09-27 23:49 UTC (permalink / raw)
To: jikos, stuart.a.hayhurst
Cc: linux-input, benjamin.tissoires, hadess, lains, mavchatz
This lightspeed receiver uses 13 byte mouse reports without an ID.
There are 5 additional vendor defined bytes appended to the report.
The workaround for such cases has been adjusted to handle these larger
reports.
The keyboard reports have a distinct layout with differing minimums and
maximums. Additionally, the LED reports now require a report ID of 1.
New keyboard and mouse descriptors have been added to reflect these
changes.
The devices attached to this receiver now report the status of their
battery in wireless mode, libratbag communicates with them and they can
be configured with Piper.
Fixes: 9d1bd9346241 ("HID: logitech-dj: Add support for a new lightspeed receiver iteration")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218172
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218094
Co-developed-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Mavroudis Chatzilazaridis <mavchatz@protonmail.com>
---
V4 -> V5: Added report ID 1 to LED reports, corrected MAX_RDESC_SIZE
V3 -> V4: Rebased and renamed dev id define from 1_2 to 1_3,
gaming_kbd_descriptor to kbd_lightspeed_1_3_descriptor
V2 -> V3: Fixed regression which broke keyboard related input
V1 -> V2: Addressed review comment for commit message
I investigated "hidpp_root_get_protocol_version: received protocol error
0x08" and it occurs only when the device is in a low power state. If the
mouse is being moved constantly while the receiver is plugged in, this
message is not printed. When comparing the behaviour with a previous
receiver (046d:c539) with a G403, it behaves the exact same way, but it
returns 0x09 (HIDPP_ERROR_RESOURCE_ERROR), disconnecting the device
silently, instead of 0x08 (HIDPP_ERROR_UNKNOWN_DEVICE).
I will be sending a patch for that in the near future, but for now it
does not cause any issues as the end result is the same.
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-logitech-dj.c | 118 ++++++++++++++++++++++++++++++++--
2 files changed, 112 insertions(+), 7 deletions(-)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 149798754570..25d52024f729 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -911,6 +911,7 @@
#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1 0xc539
#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_1 0xc53f
#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_2 0xc543
+#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_3 0xc547
#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_POWERPLAY 0xc53a
#define USB_DEVICE_ID_LOGITECH_BOLT_RECEIVER 0xc548
#define USB_DEVICE_ID_SPACETRAVELLER 0xc623
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index cce54dd9884a..d66f4807311a 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -116,6 +116,7 @@ enum recvr_type {
recvr_type_dj,
recvr_type_hidpp,
recvr_type_gaming_hidpp,
+ recvr_type_gaming_hidpp_ls_1_3,
recvr_type_mouse_only,
recvr_type_27mhz,
recvr_type_bluetooth,
@@ -211,6 +212,44 @@ static const char kbd_descriptor[] = {
0xC0
};
+/* Gaming Keyboard descriptor (1) */
+static const char kbd_lightspeed_1_3_descriptor[] = {
+ 0x05, 0x01, /* Usage Page (Generic Desktop) */
+ 0x09, 0x06, /* Usage (Keyboard) */
+ 0xA1, 0x01, /* Collection (Application) */
+ 0x85, 0x01, /* Report ID (1) */
+ 0x05, 0x07, /* Usage Page (Kbrd/Keypad) */
+ 0x19, 0xE0, /* Usage Minimum (0xE0) */
+ 0x29, 0xE7, /* Usage Maximum (0xE7) */
+ 0x15, 0x00, /* Logical Minimum (0) */
+ 0x25, 0x01, /* Logical Maximum (1) */
+ 0x75, 0x01, /* Report Size (1) */
+ 0x95, 0x08, /* Report Count (8) */
+ 0x81, 0x02, /* Input (Data,Var) */
+ 0x95, 0x70, /* Report Count (112) */
+ 0x19, 0x04, /* Usage Minimum (0x04) */
+ 0x29, 0x73, /* Usage Maximum (0x73) */
+ 0x81, 0x02, /* Input (Data,Var,Abs) */
+ 0x95, 0x05, /* Report Count (5) */
+ 0x19, 0x87, /* Usage Minimum (0x87) */
+ 0x29, 0x8B, /* Usage Maximum (0x8B) */
+ 0x81, 0x02, /* Input (Data,Var,Abs) */
+ 0x95, 0x03, /* Report Count (3) */
+ 0x19, 0x90, /* Usage Minimum (0x90) */
+ 0x29, 0x92, /* Usage Maximum (0x92) */
+ 0x81, 0x02, /* Input (Data,Var,Abs) */
+ 0x95, 0x05, /* Report Count (5) */
+ 0x85, 0x0E, /* Report ID (14) */
+ 0x05, 0x08, /* Usage Page (LEDs) */
+ 0x19, 0x01, /* Usage Minimum (Num Lock) */
+ 0x29, 0x05, /* Usage Maximum (Kana) */
+ 0x91, 0x02, /* Output (Data,Var,Abs) */
+ 0x95, 0x01, /* Report Count (1) */
+ 0x75, 0x03, /* Report Size (3) */
+ 0x91, 0x03, /* Output (Const,Var,Abs) */
+ 0xC0, /* End Collection */
+};
+
/* Mouse descriptor (2) */
static const char mse_descriptor[] = {
0x05, 0x01, /* USAGE_PAGE (Generic Desktop) */
@@ -415,6 +454,51 @@ static const char mse_high_res_descriptor[] = {
0xC0, /* END_COLLECTION */
};
+/* Gaming Mouse descriptor with vendor data (2) */
+static const char mse_high_res_ls_1_3_descriptor[] = {
+ 0x05, 0x01, /* Usage Page (Generic Desktop) */
+ 0x09, 0x02, /* Usage (Mouse) */
+ 0xA1, 0x01, /* Collection (Application) */
+ 0x85, 0x02, /* Report ID (2) */
+ 0x09, 0x01, /* Usage (Pointer) */
+ 0xA1, 0x00, /* Collection (Physical) */
+ 0x95, 0x10, /* Report Count (16) */
+ 0x75, 0x01, /* Report Size (1) */
+ 0x15, 0x00, /* Logical Minimum (0) */
+ 0x25, 0x01, /* Logical Maximum (1) */
+ 0x05, 0x09, /* Usage Page (Button) */
+ 0x19, 0x01, /* Usage Minimum (0x01) */
+ 0x29, 0x10, /* Usage Maximum (0x10) */
+ 0x81, 0x02, /* Input (Data,Var,Abs) */
+ 0x95, 0x02, /* Report Count (2) */
+ 0x75, 0x10, /* Report Size (16) */
+ 0x16, 0x01, 0x80, /* Logical Minimum (-32767) */
+ 0x26, 0xFF, 0x7F, /* Logical Maximum (32767) */
+ 0x05, 0x01, /* Usage Page (Generic Desktop) */
+ 0x09, 0x30, /* Usage (X) */
+ 0x09, 0x31, /* Usage (Y) */
+ 0x81, 0x06, /* Input (Data,Var,Rel) */
+ 0x95, 0x01, /* Report Count (1) */
+ 0x75, 0x08, /* Report Size (8) */
+ 0x15, 0x81, /* Logical Minimum (-127) */
+ 0x25, 0x7F, /* Logical Maximum (127) */
+ 0x09, 0x38, /* Usage (Wheel) */
+ 0x81, 0x06, /* Input (Data,Var,Rel) */
+ 0x95, 0x01, /* Report Count (1) */
+ 0x05, 0x0C, /* Usage Page (Consumer) */
+ 0x0A, 0x38, 0x02, /* Usage (AC Pan) */
+ 0x81, 0x06, /* Input (Data,Var,Rel) */
+ 0xC0, /* End Collection */
+ 0x06, 0x00, 0xFF, /* Usage Page (Vendor Defined 0xFF00) */
+ 0x09, 0xF1, /* Usage (0xF1) */
+ 0x75, 0x08, /* Report Size (8) */
+ 0x95, 0x05, /* Report Count (5) */
+ 0x15, 0x00, /* Logical Minimum (0) */
+ 0x26, 0xFF, 0x00, /* Logical Maximum (255) */
+ 0x81, 0x00, /* Input (Data,Array,Abs) */
+ 0xC0, /* End Collection */
+};
+
/* Consumer Control descriptor (3) */
static const char consumer_descriptor[] = {
0x05, 0x0C, /* USAGE_PAGE (Consumer Devices) */
@@ -520,9 +604,9 @@ static const char hidpp_descriptor[] = {
/* Maximum size of all defined hid reports in bytes (including report id) */
#define MAX_REPORT_SIZE 8
-/* Make sure all descriptors are present here */
+/* Make sure the largest of each descriptor type is present here */
#define MAX_RDESC_SIZE \
- (sizeof(kbd_descriptor) + \
+ (sizeof(kbd_lightspeed_1_3_descriptor) +\
sizeof(mse_bluetooth_descriptor) + \
sizeof(mse5_bluetooth_descriptor) + \
sizeof(consumer_descriptor) + \
@@ -1374,12 +1458,19 @@ static int logi_dj_ll_raw_request(struct hid_device *hid,
return -EINVAL;
if (djrcv_dev->type != recvr_type_dj && count >= 2) {
+ unsigned char led_report_id = 0;
+
if (!djrcv_dev->keyboard) {
hid_warn(hid, "Received REPORT_TYPE_LEDS request before the keyboard interface was enumerated\n");
return 0;
}
+
+ /* This Lightspeed receiver expects LED reports with report ID 1 */
+ if (djrcv_dev->type == recvr_type_gaming_hidpp_ls_1_3)
+ led_report_id = 1;
+
/* usbhid overrides the report ID and ignores the first byte */
- return hid_hw_raw_request(djrcv_dev->keyboard, 0, buf, count,
+ return hid_hw_raw_request(djrcv_dev->keyboard, led_report_id, buf, count,
report_type, reqtype);
}
@@ -1426,7 +1517,11 @@ static int logi_dj_ll_parse(struct hid_device *hid)
if (djdev->reports_supported & STD_KEYBOARD) {
dbg_hid("%s: sending a kbd descriptor, reports_supported: %llx\n",
__func__, djdev->reports_supported);
- rdcat(rdesc, &rsize, kbd_descriptor, sizeof(kbd_descriptor));
+ if (djdev->dj_receiver_dev->type == recvr_type_gaming_hidpp_ls_1_3)
+ rdcat(rdesc, &rsize, kbd_lightspeed_1_3_descriptor,
+ sizeof(kbd_lightspeed_1_3_descriptor));
+ else
+ rdcat(rdesc, &rsize, kbd_descriptor, sizeof(kbd_descriptor));
}
if (djdev->reports_supported & STD_MOUSE) {
@@ -1436,6 +1531,9 @@ static int logi_dj_ll_parse(struct hid_device *hid)
djdev->dj_receiver_dev->type == recvr_type_mouse_only)
rdcat(rdesc, &rsize, mse_high_res_descriptor,
sizeof(mse_high_res_descriptor));
+ else if (djdev->dj_receiver_dev->type == recvr_type_gaming_hidpp_ls_1_3)
+ rdcat(rdesc, &rsize, mse_high_res_ls_1_3_descriptor,
+ sizeof(mse_high_res_ls_1_3_descriptor));
else if (djdev->dj_receiver_dev->type == recvr_type_27mhz)
rdcat(rdesc, &rsize, mse_27mhz_descriptor,
sizeof(mse_27mhz_descriptor));
@@ -1695,11 +1793,12 @@ static int logi_dj_raw_event(struct hid_device *hdev,
}
/*
* Mouse-only receivers send unnumbered mouse data. The 27 MHz
- * receiver uses 6 byte packets, the nano receiver 8 bytes.
+ * receiver uses 6 byte packets, the nano receiver 8 bytes,
+ * the lightspeed receiver (Pro X Superlight) 13 bytes.
*/
if (djrcv_dev->unnumbered_application == HID_GD_MOUSE &&
- size <= 8) {
- u8 mouse_report[9];
+ size <= 13){
+ u8 mouse_report[14];
/* Prepend report id */
mouse_report[0] = REPORT_TYPE_MOUSE;
@@ -1776,6 +1875,7 @@ static int logi_dj_probe(struct hid_device *hdev,
case recvr_type_dj: no_dj_interfaces = 3; break;
case recvr_type_hidpp: no_dj_interfaces = 2; break;
case recvr_type_gaming_hidpp: no_dj_interfaces = 3; break;
+ case recvr_type_gaming_hidpp_ls_1_3: no_dj_interfaces = 3; break;
case recvr_type_mouse_only: no_dj_interfaces = 2; break;
case recvr_type_27mhz: no_dj_interfaces = 2; break;
case recvr_type_bluetooth: no_dj_interfaces = 2; break;
@@ -1987,6 +2087,10 @@ static const struct hid_device_id logi_dj_receivers[] = {
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_2),
.driver_data = recvr_type_gaming_hidpp},
+ { /* Logitech lightspeed receiver (0xc547) */
+ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
+ USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_3),
+ .driver_data = recvr_type_gaming_hidpp_ls_1_3},
{ /* Logitech 27 MHz HID++ 1.0 receiver (0xc513) */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER),
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v4] HID: logitech-dj: Add support for a new lightspeed receiver iteration
From: Stuart @ 2025-09-27 16:58 UTC (permalink / raw)
To: Mavroudis Chatzilazaridis
Cc: jikos, linux-input, hadess, lains, benjamin.tissoires
In-Reply-To: <848a3c8d-5f0b-409c-b394-1cce6a4c1e62@protonmail.com>
Hey, I've been keeping an eye out for the v5, did I miss it?
Thanks,
Stuart
^ permalink raw reply
* Re: [PATCH v1 2/2] Input: add support for the STM FTS2BA61Y touchscreen
From: Eric Gonçalves @ 2025-09-27 2:42 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Henrik Rydberg, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Ivaylo Ivanov, devicetree, linux-input, linux-kernel
In-Reply-To: <cxnqffq3vj22nk4nukhqb2m3gqeomajdusrhaaq77v2rkfxnup@g4mahnify2ke>
On 9/20/25 21:13, Dmitry Torokhov wrote:
> Hi Eric,
>
> On Sat, Sep 20, 2025 at 01:44:50AM +0000, Eric Gonçalves wrote:
>> The ST-Microelectronics FTS2BA61Y touchscreen is a capacitive multi-touch
>> controller connected through SPI at 0x0, the touchscreen is typically
>> used in mobile devices (like the Galaxy S22 series)
>>
>> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
>> Signed-off-by: Eric Gonçalves <ghatto404@gmail.com>
> Thank you for the patch. A few comments below.
>
>> ---
>> drivers/input/touchscreen/Kconfig | 11 +
>> drivers/input/touchscreen/Makefile | 1 +
>> drivers/input/touchscreen/fts2ba61y.c | 588 ++++++++++++++++++++++++++
>> 3 files changed, 600 insertions(+)
>> create mode 100644 drivers/input/touchscreen/fts2ba61y.c
>>
>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> index 196905162945..1e199191f527 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -370,6 +370,17 @@ config TOUCHSCREEN_EXC3000
>> To compile this driver as a module, choose M here: the
>> module will be called exc3000.
>>
>> +config TOUCHSCREEN_FTS2BA61Y
>> + tristate "ST-Microelectronics FTS2BA61Y touchscreen"
>> + depends on SPI
>> + help
>> + Say Y here if you have the ST-Microelectronics FTS2BA61Y touchscreen
>> +
>> + If unsure, say N.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called fts2ba61y.
>> +
>> config TOUCHSCREEN_FUJITSU
>> tristate "Fujitsu serial touchscreen"
>> select SERIO
>> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>> index 97a025c6a377..408a9fd5bd35 100644
>> --- a/drivers/input/touchscreen/Makefile
>> +++ b/drivers/input/touchscreen/Makefile
>> @@ -43,6 +43,7 @@ obj-$(CONFIG_TOUCHSCREEN_ELO) += elo.o
>> obj-$(CONFIG_TOUCHSCREEN_EGALAX) += egalax_ts.o
>> obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL) += egalax_ts_serial.o
>> obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
>> +obj-$(CONFIG_TOUCHSCREEN_FTS2BA61Y) += fts2ba61y.o
>> obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
>> obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
>> obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
>> diff --git a/drivers/input/touchscreen/fts2ba61y.c b/drivers/input/touchscreen/fts2ba61y.c
>> new file mode 100644
>> index 000000000000..b3b3abca5404
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/fts2ba61y.c
>> @@ -0,0 +1,588 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Based loosely on s6sy761.c
>> +
>> +#include <linux/delay.h>
>> +#include <linux/input.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/input/mt.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/input/touchscreen.h>
>> +#include <linux/unaligned.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +/* commands */
>> +#define FTS2BA61Y_CMD_SENSE_ON 0x10
>> +#define FTS2BA61Y_CMD_SENSE_OFF 0x11
>> +#define FTS2BA61Y_CMD_READ_PANEL_INFO 0x23
>> +#define FTS2BA61Y_CMD_READ_FW_VER 0x24
>> +#define FTS2BA61Y_CMD_TOUCHTYPE 0x30 /* R/W for get/set */
>> +#define FTS2BA61Y_CMD_CLEAR_EVENTS 0x62
>> +#define FTS2BA61Y_CMD_READ_EVENT 0x87
>> +#define FTS2BA61Y_CMD_CUSTOM_W 0xC0
>> +#define FTS2BA61Y_CMD_CUSTOM_R 0xD1
>> +#define FTS2BA61Y_CMD_REG_W 0xFA
>> +#define FTS2BA61Y_CMD_REG_R 0xFB
>> +
>> +/* touch type masks */
>> +#define FTS2BA61Y_MASK_TOUCH BIT(0)
>> +#define FTS2BA61Y_MASK_HOVER BIT(1)
>> +#define FTS2BA61Y_MASK_COVER BIT(2)
>> +#define FTS2BA61Y_MASK_GLOVE BIT(3)
>> +#define FTS2BA61Y_MASK_STYLUS BIT(4)
>> +#define FTS2BA61Y_MASK_PALM BIT(5)
>> +#define FTS2BA61Y_MASK_WET BIT(6)
>> +#define FTS2BA61Y_TOUCHTYPE_DEFAULT (FTS2BA61Y_MASK_TOUCH | \
>> + FTS2BA61Y_MASK_PALM | \
>> + FTS2BA61Y_MASK_WET)
>> +
>> +/* event status masks */
>> +#define FTS2BA61Y_MASK_STYPE GENMASK(5, 2)
>> +#define FTS2BA61Y_MASK_EVENT_ID GENMASK(1, 0)
>> +
>> +/* event coordinate masks */
>> +#define FTS2BA61Y_MASK_TCHSTA GENMASK(7, 6)
>> +#define FTS2BA61Y_MASK_TID GENMASK(5, 2)
>> +#define FTS2BA61Y_MASK_X_3_0 GENMASK(7, 4)
>> +#define FTS2BA61Y_MASK_Y_3_0 GENMASK(3, 0)
>> +#define FTS2BA61Y_MASK_Z GENMASK(5, 0)
>> +#define FTS2BA61Y_MASK_TTYPE_3_2 GENMASK(7, 6)
>> +#define FTS2BA61Y_MASK_TTYPE_1_0 GENMASK(1, 0)
>> +#define FTS2BA61Y_MASK_LEFT_EVENTS GENMASK(4, 0)
>> +
>> +/* event error status */
>> +#define FTS2BA61Y_EVENT_STATUSTYPE_INFO 0x2
>> +
>> +/* information report */
>> +#define FTS2BA61Y_INFO_READY_STATUS 0x0
>> +
>> +/* event status */
>> +#define FTS2BA61Y_COORDINATE_EVENT 0x0
>> +
>> +/* touch types */
>> +#define FTS2BA61Y_TOUCHTYPE_NORMAL 0x0
>> +#define FTS2BA61Y_TOUCHTYPE_HOVER 0x1
>> +#define FTS2BA61Y_TOUCHTYPE_FLIPCOVER 0x2
>> +#define FTS2BA61Y_TOUCHTYPE_GLOVE 0x3
>> +#define FTS2BA61Y_TOUCHTYPE_STYLUS 0x4
>> +#define FTS2BA61Y_TOUCHTYPE_PALM 0x5
>> +#define FTS2BA61Y_TOUCHTYPE_WET 0x6
>> +#define FTS2BA61Y_TOUCHTYPE_PROXIMITY 0x7
>> +#define FTS2BA61Y_TOUCHTYPE_JIG 0x8
>> +
>> +#define FTS2BA61Y_COORDINATE_ACTION_NONE 0x0
>> +#define FTS2BA61Y_COORDINATE_ACTION_PRESS 0x1
>> +#define FTS2BA61Y_COORDINATE_ACTION_MOVE 0x2
>> +#define FTS2BA61Y_COORDINATE_ACTION_RELEASE 0x3
>> +
>> +#define FTS2BA61Y_DEV_NAME "fts2ba61y"
>> +#define FTS2BA61Y_EVENT_BUFF_SIZE 16
>> +#define FTS2BA61Y_PANEL_INFO_SIZE 11
>> +#define FTS2BA61Y_RESET_CMD_SIZE 5
>> +#define FTS2BA61Y_EVENT_COUNT 31
>> +#define MAX_TRANSFER_SIZE 256
>> +
>> +enum fts2ba61y_regulators {
>> + FTS2BA61Y_REGULATOR_VDD,
>> + FTS2BA61Y_REGULATOR_AVDD,
>> +};
>> +
>> +struct fts2ba61y_data {
>> + struct spi_device *spi;
>> + struct regulator_bulk_data regulators[2];
>> + struct input_dev *input_dev;
>> + struct mutex mutex;
>> + struct touchscreen_properties prop;
>> +
>> + u8 tx_count;
>> +
>> + unsigned int max_x;
>> + unsigned int max_y;
>> +};
>> +
>> +static int fts2ba61y_write(struct fts2ba61y_data *ts,
>> + u8 *reg, int cmd_len, u8 *data, int data_len)
>> +{
>> + struct spi_message msg;
>> + struct spi_transfer xfers;
>> + char *tx_buf;
>> + int len;
>> + int ret;
> Please use "error" for variables that only contain error codes or 0.
Okay
>
>> +
>> + tx_buf = kzalloc(cmd_len + data_len + 1, GFP_KERNEL);
>> + if (!tx_buf) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
> Instead of allocating and freeing memory on each transfer consider
> allocating tx and rx scratch buffers in fts2ba61y_data structure (either
> as ____cacheline_aligned or as separate allocations).
>
> If you absolutely need a per transfer allocations then use
>
> u8 *tx_buf __free(kfree) = kzalloc(...);
Will do
>
>> +
>> + memset(&xfers, 0, sizeof(xfers));
>> + spi_message_init(&msg);
>> +
>> + memcpy(&tx_buf[0], reg, cmd_len);
>> + if (data_len && data)
>> + memcpy(&tx_buf[cmd_len], data, data_len);
>> +
>> + len = cmd_len + data_len;
>> +
>> + /* custom write cmd */
>> + if (reg[0] != FTS2BA61Y_CMD_REG_W &&
>> + reg[0] != FTS2BA61Y_CMD_REG_R) {
>> + memmove(tx_buf + 1, tx_buf, len);
>> + tx_buf[0] = FTS2BA61Y_CMD_CUSTOM_W;
>> + len++;
>> + }
>> +
>> + xfers.len = len;
>> + xfers.tx_buf = tx_buf;
>> +
>> + spi_message_add_tail(&xfers, &msg);
>> +
>> + mutex_lock(&ts->mutex);
> Why is this mutex needed? spi_sync() does the bus lock already, what
> else needs protection. Even with shared scratch buffers I believe the
> driver at any one point would only have one read or write operation in
> progress...
Yeah you're right, the mutex was kept because it was in downstream code.
Will drop
>> + ret = spi_sync(ts->spi, &msg);
>> + if (ret)
>> + dev_err(&ts->spi->dev, "spi transfer error, %d", ret);
>> + mutex_unlock(&ts->mutex);
>> +
>> +out:
>> + kfree(tx_buf);
>> + return ret;
>> +}
>> +
>> +static int fts2ba61y_spi_raw_read(struct fts2ba61y_data *ts,
>> + u8 *tx_buf, u8 *rx_buf, int len)
>> +{
>> + struct spi_message msg;
>> + struct spi_transfer xfer;
>> + int ret;
>> +
>> + memset(&xfer, 0, sizeof(xfer));
>> + spi_message_init(&msg);
>> +
>> + xfer.len = len;
>> + xfer.tx_buf = tx_buf;
>> + xfer.rx_buf = rx_buf;
>> + spi_message_add_tail(&xfer, &msg);
>> +
>> + mutex_lock(&ts->mutex);
>> + ret = spi_sync(ts->spi, &msg);
>> + if (ret)
>> + dev_err(&ts->spi->dev, "spi transfer error, %d", ret);
>> + mutex_unlock(&ts->mutex);
>> +
>> + return ret;
>> +}
>> +
>> +/*
>> + * higher-level wrapper that prepares the buffers for a read.
>> + */
>> +static int fts2ba61y_read(struct fts2ba61y_data *ts,
>> + u8 reg[], int tx_len, u8 buf[], int rx_len)
> As far as I can see fts2ba61y_read() is always used with a single byte
> command. Why not make it "u8 cmd" or "u8 reg" and drop tx_len.
>
> Same goes for fts2ba61y_write(). Also the read buffer might make sense
> as void * instead of u8 *, so that you do not have to cast.
Alright
>
>> +{
>> + char *tx_buf, *rx_buf;
>> + int ret, mem_len;
>> + u16 reg_val;
>> +
>> + if (tx_len > 3)
>> + mem_len = rx_len + 1 + tx_len;
>> + else
>> + mem_len = rx_len + 4;
> A commend why we need this "+ 4" would be useful.
Will do
>
>> +
>> + tx_buf = kzalloc(mem_len, GFP_KERNEL);
>> + rx_buf = kzalloc(mem_len, GFP_KERNEL);
>> + if (!tx_buf || !rx_buf) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + switch (reg[0]) {
>> + case FTS2BA61Y_CMD_READ_EVENT:
>> + case FTS2BA61Y_CMD_REG_W:
>> + case FTS2BA61Y_CMD_REG_R:
>> + memcpy(tx_buf, reg, tx_len);
>> + break;
>> +
>> + default:
>> + tx_buf[0] = FTS2BA61Y_CMD_CUSTOM_R;
>> +
>> + if (tx_len == 1)
>> + reg_val = 0;
>> + else if (tx_len == 2)
>> + reg_val = reg[0];
>> + else if (tx_len == 3)
>> + reg_val = reg[0] | (reg[1] << 8);
>> + else {
> If one branch has braces all of them have to have braces.
>
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + tx_len = 3;
>> + put_unaligned_be16(reg_val, &tx_buf[1]);
>> +
>> + ret = fts2ba61y_write(ts, reg, 1, NULL, 0);
>> + if (ret < 0)
>> + goto out;
>> + break;
>> + }
>> +
>> + ret = fts2ba61y_spi_raw_read(ts, tx_buf, rx_buf, rx_len + 1 + tx_len);
>> + if (ret < 0)
>> + goto out;
>> +
>> + memcpy(buf, &rx_buf[1 + tx_len], rx_len);
>> +
>> +out:
>> + kfree(tx_buf);
>> + kfree(rx_buf);
>> + return ret;
>> +}
>> +
>> +static int fts2ba61y_wait_for_ready(struct fts2ba61y_data *ts)
>> +{
>> + u8 buffer[FTS2BA61Y_EVENT_BUFF_SIZE];
>> + u8 cmd = FTS2BA61Y_CMD_READ_EVENT;
>> + u8 status_id, stype;
>> + int ret;
>> +
>> + for (int retries = 5; retries > 0; retries--) {
>> + ret = fts2ba61y_read(ts, &cmd, 1, buffer, FTS2BA61Y_EVENT_BUFF_SIZE);
>> +
>> + stype = FIELD_GET(FTS2BA61Y_MASK_STYPE, buffer[0]);
>> + status_id = buffer[1];
>> +
>> + if (stype == FTS2BA61Y_EVENT_STATUSTYPE_INFO &&
>> + status_id == FTS2BA61Y_INFO_READY_STATUS) {
>> + ret = 0;
>> + break;
>> + } else
>> + ret = -ENODEV;
> "else" needs braces as well.
>
>> +
>> + msleep(20);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int fts2ba61y_reset(struct fts2ba61y_data *ts)
>> +{
>> + u8 cmd = FTS2BA61Y_CMD_REG_W;
>> + /* the following sequence is undocumented */
>> + u8 reset[FTS2BA61Y_RESET_CMD_SIZE] = { 0x20, 0x00,
>> + 0x00, 0x24, 0x81 };
>> + int ret;
>> +
>> + disable_irq(ts->spi->irq);
>> +
>> + ret = fts2ba61y_write(ts, &cmd, 1, &reset[0], FTS2BA61Y_RESET_CMD_SIZE);
>> + if (ret)
>> + return ret;
> You end up with interrupts disabled on error which may be unexpected.
> Better use
>
> guard(disable_irq)(&ts->spi->irq);
>
>> + msleep(30);
>> +
>> + ret = fts2ba61y_wait_for_ready(ts);
>> + if (ret)
>> + return ret;
>> +
>> + enable_irq(ts->spi->irq);
>> +
>> + return 0;
>> +}
>> +
>> +static int fts2ba61y_set_channels(struct fts2ba61y_data *ts)
>> +{
>> + int ret;
>> + u8 cmd = FTS2BA61Y_CMD_READ_PANEL_INFO;
>> + u8 data[FTS2BA61Y_PANEL_INFO_SIZE];
>> +
>> + ret = fts2ba61y_read(ts, &cmd, 1, data, FTS2BA61Y_PANEL_INFO_SIZE);
>> + if (ret)
>> + return ret;
>> +
>> + ts->max_x = get_unaligned_be16(data);
>> + ts->max_y = get_unaligned_be16(data + 2);
>> +
>> + /* if no tx channels defined, at least keep one */
>> + ts->tx_count = max_t(u8, data[8], 1);
>> +
>> + return 0;
>> +}
>> +
>> +static int fts2ba61y_set_touch_func(struct fts2ba61y_data *ts)
>> +{
>> + u8 cmd = FTS2BA61Y_CMD_TOUCHTYPE;
>> + u16 touchtype = cpu_to_le16(FTS2BA61Y_TOUCHTYPE_DEFAULT);
>> +
>> + return fts2ba61y_write(ts, &cmd, 1, (u8 *)&touchtype, 2);
>> +}
>> +
>> +static int fts2ba61y_hw_init(struct fts2ba61y_data *ts)
>> +{
>> + int ret;
>> +
>> + ret = regulator_bulk_enable(ARRAY_SIZE(ts->regulators),
>> + ts->regulators);
>> + if (ret)
>> + return ret;
>> +
>> + msleep(140);
>> +
>> + ret = fts2ba61y_reset(ts);
>> + if (ret)
>> + return ret;
> You need to disable regulators on error.
>
>> +
>> + ret = fts2ba61y_set_channels(ts);
>> + if (ret)
>> + return ret;
>> +
>> + return fts2ba61y_set_touch_func(ts);
> In functions with multiple points of failure do not end with "return
> function_call();". Use standard
>
> error = operation(...);
> if (error)
> return error;
>
> return 0;
>
>> +}
>> +
>> +static int fts2ba61y_get_event(struct fts2ba61y_data *ts, u8 *data, int *n_events)
>> +{
>> + int ret;
>> + u8 cmd = FTS2BA61Y_CMD_READ_EVENT;
>> +
>> + ret = fts2ba61y_read(ts, &cmd, 1, data, FTS2BA61Y_EVENT_BUFF_SIZE);
>> + if (ret < 0)
> fts2ba61y_read() does not return positive success values, so:
>
> if (error)
> return error;
>
>> + return ret;
>> +
>> + if (!data[0]) {
>> + *n_events = 0;
>> + return 0;
>> + }
>> +
>> + *n_events = FIELD_GET(FTS2BA61Y_MASK_LEFT_EVENTS, data[7]);
>> + if (unlikely(*n_events >= FTS2BA61Y_EVENT_COUNT)) {
>> + cmd = FTS2BA61Y_CMD_CLEAR_EVENTS;
>> + fts2ba61y_write(ts, &cmd, 1, NULL, 0);
>> + *n_events = 0;
>> + return -EINVAL;
>> + }
>> +
>> + if (*n_events > 0) {
>> + ret = fts2ba61y_read(ts, &cmd, 1,
>> + &data[1 * FTS2BA61Y_EVENT_BUFF_SIZE],
>> + FTS2BA61Y_EVENT_BUFF_SIZE * (*n_events));
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void fts2ba61y_report_coordinates(struct fts2ba61y_data *ts,
>> + u8 *event, u8 tid)
>> +{
>> + u8 major = event[4];
>> + u8 minor = event[5];
>> + u8 z = FIELD_GET(FTS2BA61Y_MASK_Z, event[6]);
>> +
>> + u16 x = (event[1] << 4) |
>> + FIELD_GET(FTS2BA61Y_MASK_X_3_0, event[3]);
>> + u16 y = (event[2] << 4) |
>> + FIELD_GET(FTS2BA61Y_MASK_Y_3_0, event[3]);
>> + u16 ttype = (FIELD_GET(FTS2BA61Y_MASK_TTYPE_3_2, event[6]) << 2) |
>> + (FIELD_GET(FTS2BA61Y_MASK_TTYPE_1_0, event[7]) << 0);
>> +
>> + if (ttype != FTS2BA61Y_TOUCHTYPE_NORMAL &&
>> + ttype != FTS2BA61Y_TOUCHTYPE_PALM &&
>> + ttype != FTS2BA61Y_TOUCHTYPE_WET &&
>> + ttype != FTS2BA61Y_TOUCHTYPE_GLOVE)
>> + return;
>> +
>> + input_mt_slot(ts->input_dev, tid);
>> + input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER, true);
>> + input_report_abs(ts->input_dev, ABS_MT_POSITION_X, x);
>> + input_report_abs(ts->input_dev, ABS_MT_POSITION_Y, y);
>> + input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR, major);
>> + input_report_abs(ts->input_dev, ABS_MT_TOUCH_MINOR, minor);
>> + input_report_abs(ts->input_dev, ABS_MT_PRESSURE, z);
>> +
>> + input_mt_sync_frame(ts->input_dev);
>> + input_sync(ts->input_dev);
>> +}
>> +
>> +static void fts2ba61y_report_release(struct fts2ba61y_data *ts, u8 tid)
>> +{
>> + input_mt_slot(ts->input_dev, tid);
>> + input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER, false);
>> +
>> + input_mt_sync_frame(ts->input_dev);
>> + input_sync(ts->input_dev);
>> +}
>> +
>> +static void fts2ba61y_handle_coordinates(struct fts2ba61y_data *ts, u8 *event)
>> +{
>> + u8 t_id = FIELD_GET(FTS2BA61Y_MASK_TID, event[0]);
>> + u8 action = FIELD_GET(FTS2BA61Y_MASK_TCHSTA, event[0]);
>> +
>> + if (t_id > ts->tx_count)
>> + return;
>> +
>> + switch (action) {
>> + case FTS2BA61Y_COORDINATE_ACTION_PRESS:
>> + case FTS2BA61Y_COORDINATE_ACTION_MOVE:
>> + fts2ba61y_report_coordinates(ts, event, t_id);
>> + break;
>> +
>> + case FTS2BA61Y_COORDINATE_ACTION_RELEASE:
>> + fts2ba61y_report_release(ts, t_id);
>> + break;
>> + }
>> +}
>> +
>> +static irqreturn_t fts2ba61y_irq_handler(int irq, void *handle)
>> +{
>> + struct fts2ba61y_data *ts = handle;
>> + u8 buffer[FTS2BA61Y_EVENT_COUNT * FTS2BA61Y_EVENT_BUFF_SIZE];
>> + u8 *event;
>> + u8 event_id;
>> + int n_events = 0;
>> + int ret;
>> +
>> + usleep(1);
> Why?
Must've been added by mistake.
>
>> +
>> + ret = fts2ba61y_get_event(ts, buffer, &n_events);
>> + if (ret < 0) {
>> + dev_dbg(&ts->spi->dev, "failed to get event: %d", ret);
>> + return IRQ_HANDLED;
>> + }
>> +
>> + for (int i = 0; i <= n_events; i++) {
>> + event = &buffer[i * FTS2BA61Y_EVENT_BUFF_SIZE];
>> + event_id = FIELD_GET(FTS2BA61Y_MASK_EVENT_ID, event[0]);
>> +
>> + if (event_id == FTS2BA61Y_COORDINATE_EVENT)
>> + fts2ba61y_handle_coordinates(ts, event);
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int fts2ba61y_input_open(struct input_dev *dev)
>> +{
>> + struct fts2ba61y_data *ts = input_get_drvdata(dev);
>> + u8 cmd = FTS2BA61Y_CMD_SENSE_ON;
>> +
>> + return fts2ba61y_write(ts, &cmd, 1, NULL, 0);
>> +}
>> +
>> +static void fts2ba61y_input_close(struct input_dev *dev)
>> +{
>> + struct fts2ba61y_data *ts = input_get_drvdata(dev);
>> + int ret;
>> + u8 cmd = FTS2BA61Y_CMD_SENSE_OFF;
>> +
>> + ret = fts2ba61y_write(ts, &cmd, 1, NULL, 0);
>> + if (ret)
>> + dev_err(&ts->spi->dev, "failed to turn off sensing\n");
>> +}
>> +
>> +static void fts2ba61y_power_off(void *data)
>> +{
>> + struct fts2ba61y_data *ts = data;
>> +
>> + disable_irq(ts->spi->irq);
> This may get called before interrupt is requested. Why does it need to
> be here?
To ensure the interrupt doesn't fire during shutdown.
>
>> + regulator_bulk_disable(ARRAY_SIZE(ts->regulators),
>> + ts->regulators);
>> +}
>> +
>> +static int fts2ba61y_probe(struct spi_device *spi) {
>> + struct fts2ba61y_data *ts;
>> + struct input_dev *input_dev;
>> + int error;
>> +
>> + ts = devm_kzalloc(&spi->dev, sizeof(*ts), GFP_KERNEL);
>> + if (!ts)
>> + return -ENOMEM;
>> +
>> + ts->spi = spi;
>> + mutex_init(&ts->mutex);
>> +
>> + spi->mode = SPI_MODE_0;
>> + spi->bits_per_word = 8;
>> +
>> + error = spi_setup(spi);
>> + if (error)
>> + return error;
>> +
>> + ts->regulators[FTS2BA61Y_REGULATOR_VDD].supply = "vdd";
>> + ts->regulators[FTS2BA61Y_REGULATOR_AVDD].supply = "avdd";
>> + error = devm_regulator_bulk_get(&spi->dev,
>> + ARRAY_SIZE(ts->regulators),
>> + ts->regulators);
>> + if (error)
>> + return error;
>> +
>> + error = fts2ba61y_hw_init(ts);
>> + if (error)
>> + return error;
>> +
>> + error = devm_add_action_or_reset(&ts->spi->dev, fts2ba61y_power_off, ts);
>> + if (error)
>> + return error;
>> +
>> + input_dev = devm_input_allocate_device(&spi->dev);
>> + if (!input_dev)
>> + return -ENOMEM;
>> +
>> + ts->input_dev = input_dev;
>> +
>> + input_dev->name = FTS2BA61Y_DEV_NAME;
>> + input_dev->id.bustype = BUS_SPI;
>> + input_dev->open = fts2ba61y_input_open;
>> + input_dev->close = fts2ba61y_input_close;
>> +
>> + input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0, ts->max_x, 0, 0);
>> + input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0, ts->max_y, 0, 0);
>> + input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
>> + input_set_abs_params(input_dev, ABS_MT_TOUCH_MINOR, 0, 255, 0, 0);
>> + input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
>> +
>> + touchscreen_parse_properties(input_dev, true, &ts->prop);
>> +
>> + spi_set_drvdata(spi, ts);
>> + input_set_drvdata(input_dev, ts);
>> +
>> + error = input_mt_init_slots(input_dev, ts->tx_count, INPUT_MT_DIRECT);
>> + if (error)
>> + return error;
>> +
>> + error = input_register_device(input_dev);
>> + if (error)
>> + return error;
>> +
>> + error = devm_request_threaded_irq(&spi->dev, spi->irq, NULL,
>> + fts2ba61y_irq_handler,
>> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> Do not encode interrupt polarity, let device tree specify it according
> to system design. So just IRQF_ONESHOT.
Okay
>
>> + "fts2ba61y_irq", ts);
>> + return error;
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id spi_touchscreen_dt_ids[] = {
>> + { .compatible = "st,fts2ba61y" },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, spi_touchscreen_dt_ids);
>> +#endif
>> +
>> +static const struct spi_device_id fts2ba61y_spi_ids[] = {
>> + { "fts2ba61y" },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(spi, fts2ba61y_spi_ids);
>> +
>> +static struct spi_driver spi_touchscreen_driver = {
>> + .driver = {
>> + .name = FTS2BA61Y_DEV_NAME,
>> + .of_match_table = of_match_ptr(spi_touchscreen_dt_ids),
>> + },
>> + .probe = fts2ba61y_probe,
>> + .id_table = fts2ba61y_spi_ids,
>> +};
>> +
>> +module_spi_driver(spi_touchscreen_driver);
>> +
>> +MODULE_AUTHOR("Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>");
>> +MODULE_DESCRIPTION("ST-Microelectronics FTS2BA61Y Touch Screen");
>> +MODULE_LICENSE("GPL");
> Thanks.
Thank you for the review! I will send a v2 soon.
^ permalink raw reply
* Re: [PATCH v6 0/2] Add support for sound profile switching and leverage for OnePlus 6 slider
From: David Heidelberg @ 2025-09-26 21:31 UTC (permalink / raw)
To: Dmitry Torokhov, Jonathan Corbet, Jiri Kosina, Benjamin Tissoires,
Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: linux-input, linux-doc, linux-kernel, linux-arm-msm, devicetree,
Gergo Koteles, Casey Connolly
In-Reply-To: <20250731-op6-tri-state-v6-0-569c25cbc8c2@ixit.cz>
Hello!
Small reminder about this great patchset opening a way for handling
slider switches.
Thank you
David
On 31/07/2025 23:17, David Heidelberg via B4 Relay wrote:
> This code was tested for two years within the downstream Snapdragon 845 tree.
> It is now perfectly integrated with feedbackd in the Phosh environment.
>
> Changes in v6:
> - Rebased again next-20250731, otherwise just a resent.
> - Link to v5: https://lore.kernel.org/r/20250419-op6-tri-state-v5-0-443127078517@ixit.cz
>
> Changes in v5:
> - Dropped merged
> "Input: gpio-keys - add support for linux,input-value DTS property"
> - Link to v4: https://lore.kernel.org/all/cover.1677022414.git.soyer@irl.hu/
>
> Changes in v4:
> - DTS: use default debounce-interval, order alphabetically
> - Link to v3: https://lore.kernel.org/lkml/cover.1676850819.git.soyer@irl.hu/
>
> Changes in v3:
> - rename tri-state-key to alert-slider, fix DTS warnings,
>
> Changes in v2:
> - rebase to qcom/for-next
> add SND_PROFILE_* identifiers to input-event-codes.h
>
> base-commit: 02ac8d2a011b630481d959298a1cc76ca0717f3e
> ---
> Gergo Koteles (2):
> Input: add ABS_SND_PROFILE
> arm64: dts: qcom: sdm845-oneplus: Add alert-slider
>
> Documentation/input/event-codes.rst | 6 ++++
> .../arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi | 39 ++++++++++++++++++++--
> drivers/hid/hid-debug.c | 1 +
> include/uapi/linux/input-event-codes.h | 9 +++++
> 4 files changed, 53 insertions(+), 2 deletions(-)
> ---
> base-commit: 84b92a499e7eca54ba1df6f6c6e01766025943f1
> change-id: 20250419-op6-tri-state-ed1a05a11125
>
> Best regards,
--
David Heidelberg
^ permalink raw reply
* [PATCH v2 2/2] dt-bindings: touchscreen: move ar1021.txt to trivial-touch.yaml
From: Frank Li @ 2025-09-26 18:47 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)...,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
Cc: imx
In-Reply-To: <20250926184720.391335-1-Frank.Li@nxp.com>
ar1021 have only reg and interrupts property beside touch common
properties. So move context of ar1021.txt into trivial-touch.yaml.
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change in v2
move to trivial-touch.yaml
previous discussion
https://lore.kernel.org/imx/20250925-swimming-overspend-ddf7ab4a252c@spud/T/#t
---
.../bindings/input/touchscreen/ar1021.txt | 15 ---------------
.../bindings/input/touchscreen/trivial-touch.yaml | 2 ++
2 files changed, 2 insertions(+), 15 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/input/touchscreen/ar1021.txt
diff --git a/Documentation/devicetree/bindings/input/touchscreen/ar1021.txt b/Documentation/devicetree/bindings/input/touchscreen/ar1021.txt
deleted file mode 100644
index 82019bd6094ee..0000000000000
--- a/Documentation/devicetree/bindings/input/touchscreen/ar1021.txt
+++ /dev/null
@@ -1,15 +0,0 @@
-* Microchip AR1020 and AR1021 touchscreen interface (I2C)
-
-Required properties:
-- compatible : "microchip,ar1021-i2c"
-- reg : I2C slave address
-- interrupts : touch controller interrupt
-
-Example:
-
- touchscreen@4d {
- compatible = "microchip,ar1021-i2c";
- reg = <0x4d>;
- interrupt-parent = <&gpio3>;
- interrupts = <11 IRQ_TYPE_LEVEL_HIGH>;
- };
diff --git a/Documentation/devicetree/bindings/input/touchscreen/trivial-touch.yaml b/Documentation/devicetree/bindings/input/touchscreen/trivial-touch.yaml
index c393cce273c5b..d6aed3afd4acb 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/trivial-touch.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/trivial-touch.yaml
@@ -14,6 +14,8 @@ properties:
enum:
# MAXI MAX11801 Resistive touch screen controller with i2c interface
- maxim,max11801
+ # Microchip AR1020 and AR1021 touchscreen interface (I2C)
+ - microchip,ar1021-i2c
reg:
maxItems: 1
--
2.34.1
^ permalink raw reply related
* [PATCH v2 1/2] dt-bindings: touchscreen: rename maxim,max11801.yaml to trivial-touch.yaml
From: Frank Li @ 2025-09-26 18:47 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)...,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
Cc: imx
Rename maxim,max11801.yaml to trivial-touch.yaml for simple i2c touch
controller with one interrupt and common touch properties.
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change in v2
new patch
previous discussion
https://lore.kernel.org/imx/20250925-swimming-overspend-ddf7ab4a252c@spud/T/#t
---
.../{maxim,max11801.yaml => trivial-touch.yaml} | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
rename Documentation/devicetree/bindings/input/touchscreen/{maxim,max11801.yaml => trivial-touch.yaml} (74%)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/maxim,max11801.yaml b/Documentation/devicetree/bindings/input/touchscreen/trivial-touch.yaml
similarity index 74%
rename from Documentation/devicetree/bindings/input/touchscreen/maxim,max11801.yaml
rename to Documentation/devicetree/bindings/input/touchscreen/trivial-touch.yaml
index 4f528d2201992..c393cce273c5b 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/maxim,max11801.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/trivial-touch.yaml
@@ -1,17 +1,19 @@
# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
%YAML 1.2
---
-$id: http://devicetree.org/schemas/input/touchscreen/maxim,max11801.yaml#
+$id: http://devicetree.org/schemas/input/touchscreen/trivial-touch.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#
-title: MAXI MAX11801 Resistive touch screen controller with i2c interface
+title: Trivial touch screen controller with i2c interface
maintainers:
- Frank Li <Frank.Li@nxp.com>
properties:
compatible:
- const: maxim,max11801
+ enum:
+ # MAXI MAX11801 Resistive touch screen controller with i2c interface
+ - maxim,max11801
reg:
maxItems: 1
--
2.34.1
^ permalink raw reply related
* [PATCH v3] HID: cp2112: Add parameter validation to data length
From: Deepak Sharma @ 2025-09-26 14:58 UTC (permalink / raw)
To: jikos, bentiss
Cc: linux-input, linux-kernel, linux-kernel-mentees, skhan,
david.hunter.linux, Deepak Sharma, syzbot+7617e19c8a59edfbd879
Syzkaller reported a stack OOB access in cp2112_write_req caused by lack
of parameter validation for the user input in I2C SMBUS ioctl in cp2112
driver
Add the parameter validation for the data->block[0] to be bounded by
I2C_SMBUS_BLOCK_MAX + the additional compatibility padding
Reported-by: syzbot+7617e19c8a59edfbd879@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=7617e19c8a59edfbd879
Tested-by: syzbot+7617e19c8a59edfbd879@syzkaller.appspotmail.com
Signed-off-by: Deepak Sharma <deepak.sharma.472935@gmail.com>
---
v3:
- Fix the commit message to not include the changelog
v2:
- Get rid of magic numbers
- Improve the bounds check
v1:
- Add bound checks on the input parameters at the driver
drivers/hid/hid-cp2112.c | 27 ++++++++++++++++++++++++---
1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 482f62a78c41..13dcd2470d92 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -689,7 +689,14 @@ static int cp2112_xfer(struct i2c_adapter *adap, u16 addr,
count = cp2112_write_read_req(buf, addr, read_length,
command, NULL, 0);
} else {
- count = cp2112_write_req(buf, addr, command,
+ /* Copy starts from data->block[1] so the length can
+ * be at max I2C_SMBUS_CLOCK_MAX + 1
+ */
+
+ if (data->block[0] > I2C_SMBUS_BLOCK_MAX + 1)
+ count = -EINVAL;
+ else
+ count = cp2112_write_req(buf, addr, command,
data->block + 1,
data->block[0]);
}
@@ -700,7 +707,14 @@ static int cp2112_xfer(struct i2c_adapter *adap, u16 addr,
I2C_SMBUS_BLOCK_MAX,
command, NULL, 0);
} else {
- count = cp2112_write_req(buf, addr, command,
+ /* data_length here is data->block[0] + 1
+ * so make sure that the data->block[0] is
+ * less than or equals I2C_SMBUS_BLOCK_MAX + 1
+ */
+ if (data->block[0] > I2C_SMBUS_BLOCK_MAX + 1)
+ count = -EINVAL;
+ else
+ count = cp2112_write_req(buf, addr, command,
data->block,
data->block[0] + 1);
}
@@ -709,7 +723,14 @@ static int cp2112_xfer(struct i2c_adapter *adap, u16 addr,
size = I2C_SMBUS_BLOCK_DATA;
read_write = I2C_SMBUS_READ;
- count = cp2112_write_read_req(buf, addr, I2C_SMBUS_BLOCK_MAX,
+ /* data_length is data->block[0] + 1, so
+ * so data->block[0] should be less than or
+ * equal to the I2C_SMBUS_BLOCK_MAX + 1
+ */
+ if (data->block[0] > I2C_SMBUS_BLOCK_MAX + 1)
+ count = -EINVAL;
+ else
+ count = cp2112_write_read_req(buf, addr, I2C_SMBUS_BLOCK_MAX,
command, data->block,
data->block[0] + 1);
break;
--
2.51.0
^ permalink raw reply related
* Re: [PATCH RESEND v2] HID: cp2112: Add parameter validation to data length
From: Deepak Sharma @ 2025-09-26 14:17 UTC (permalink / raw)
To: jikos, bentiss
Cc: linux-input, linux-kernel, linux-kernel-mentees, skhan,
david.hunter.linux, syzbot+7617e19c8a59edfbd879
In-Reply-To: <20250926140729.267766-1-deepak.sharma.472935@gmail.com>
Please ignore this patch. I did this patch quite a while ago and put
the change log mistakenly into the commit message. Will send a v3
fixing it. Sorry for being clumsy with that
Thanks,
Deepak Sharma
On Fri, Sep 26, 2025 at 7:39 PM Deepak Sharma
<deepak.sharma.472935@gmail.com> wrote:
>
> This is v2 for the earlier patch, where a few bounds check were
> unnecessarily strict. This patch also removes the use of magic numbers
>
> Syzkaller reported a stack OOB access in cp2112_write_req caused by lack
> of parameter validation for the user input in I2C SMBUS ioctl codeflow
> in the report
>
> I2C device drivers are "responsible for checking all the parameters that
> come from user-space for validity" as specified at Documentation/i2c/dev-interface
>
> Add the parameter validation for the data->block[0] to be bounded by
> I2C_SMBUS_BLOCK_MAX + the additional compatibility padding
>
> Reported-by: syzbot+7617e19c8a59edfbd879@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=7617e19c8a59edfbd879
> Tested-by: syzbot+7617e19c8a59edfbd879@syzkaller.appspotmail.com
> Signed-off-by: Deepak Sharma <deepak.sharma.472935@gmail.com>
> ---
> drivers/hid/hid-cp2112.c | 27 ++++++++++++++++++++++++---
> 1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> index 482f62a78c41..13dcd2470d92 100644
> --- a/drivers/hid/hid-cp2112.c
> +++ b/drivers/hid/hid-cp2112.c
> @@ -689,7 +689,14 @@ static int cp2112_xfer(struct i2c_adapter *adap, u16 addr,
> count = cp2112_write_read_req(buf, addr, read_length,
> command, NULL, 0);
> } else {
> - count = cp2112_write_req(buf, addr, command,
> + /* Copy starts from data->block[1] so the length can
> + * be at max I2C_SMBUS_CLOCK_MAX + 1
> + */
> +
> + if (data->block[0] > I2C_SMBUS_BLOCK_MAX + 1)
> + count = -EINVAL;
> + else
> + count = cp2112_write_req(buf, addr, command,
> data->block + 1,
> data->block[0]);
> }
> @@ -700,7 +707,14 @@ static int cp2112_xfer(struct i2c_adapter *adap, u16 addr,
> I2C_SMBUS_BLOCK_MAX,
> command, NULL, 0);
> } else {
> - count = cp2112_write_req(buf, addr, command,
> + /* data_length here is data->block[0] + 1
> + * so make sure that the data->block[0] is
> + * less than or equals I2C_SMBUS_BLOCK_MAX + 1
> + */
> + if (data->block[0] > I2C_SMBUS_BLOCK_MAX + 1)
> + count = -EINVAL;
> + else
> + count = cp2112_write_req(buf, addr, command,
> data->block,
> data->block[0] + 1);
> }
> @@ -709,7 +723,14 @@ static int cp2112_xfer(struct i2c_adapter *adap, u16 addr,
> size = I2C_SMBUS_BLOCK_DATA;
> read_write = I2C_SMBUS_READ;
>
> - count = cp2112_write_read_req(buf, addr, I2C_SMBUS_BLOCK_MAX,
> + /* data_length is data->block[0] + 1, so
> + * so data->block[0] should be less than or
> + * equal to the I2C_SMBUS_BLOCK_MAX + 1
> + */
> + if (data->block[0] > I2C_SMBUS_BLOCK_MAX + 1)
> + count = -EINVAL;
> + else
> + count = cp2112_write_read_req(buf, addr, I2C_SMBUS_BLOCK_MAX,
> command, data->block,
> data->block[0] + 1);
> break;
> --
> 2.51.0
>
^ permalink raw reply
* [PATCH RESEND v2] HID: cp2112: Add parameter validation to data length
From: Deepak Sharma @ 2025-09-26 14:07 UTC (permalink / raw)
To: jikos, bentiss
Cc: linux-input, linux-kernel, linux-kernel-mentees, skhan,
david.hunter.linux, Deepak Sharma, syzbot+7617e19c8a59edfbd879
This is v2 for the earlier patch, where a few bounds check were
unnecessarily strict. This patch also removes the use of magic numbers
Syzkaller reported a stack OOB access in cp2112_write_req caused by lack
of parameter validation for the user input in I2C SMBUS ioctl codeflow
in the report
I2C device drivers are "responsible for checking all the parameters that
come from user-space for validity" as specified at Documentation/i2c/dev-interface
Add the parameter validation for the data->block[0] to be bounded by
I2C_SMBUS_BLOCK_MAX + the additional compatibility padding
Reported-by: syzbot+7617e19c8a59edfbd879@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=7617e19c8a59edfbd879
Tested-by: syzbot+7617e19c8a59edfbd879@syzkaller.appspotmail.com
Signed-off-by: Deepak Sharma <deepak.sharma.472935@gmail.com>
---
drivers/hid/hid-cp2112.c | 27 ++++++++++++++++++++++++---
1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 482f62a78c41..13dcd2470d92 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -689,7 +689,14 @@ static int cp2112_xfer(struct i2c_adapter *adap, u16 addr,
count = cp2112_write_read_req(buf, addr, read_length,
command, NULL, 0);
} else {
- count = cp2112_write_req(buf, addr, command,
+ /* Copy starts from data->block[1] so the length can
+ * be at max I2C_SMBUS_CLOCK_MAX + 1
+ */
+
+ if (data->block[0] > I2C_SMBUS_BLOCK_MAX + 1)
+ count = -EINVAL;
+ else
+ count = cp2112_write_req(buf, addr, command,
data->block + 1,
data->block[0]);
}
@@ -700,7 +707,14 @@ static int cp2112_xfer(struct i2c_adapter *adap, u16 addr,
I2C_SMBUS_BLOCK_MAX,
command, NULL, 0);
} else {
- count = cp2112_write_req(buf, addr, command,
+ /* data_length here is data->block[0] + 1
+ * so make sure that the data->block[0] is
+ * less than or equals I2C_SMBUS_BLOCK_MAX + 1
+ */
+ if (data->block[0] > I2C_SMBUS_BLOCK_MAX + 1)
+ count = -EINVAL;
+ else
+ count = cp2112_write_req(buf, addr, command,
data->block,
data->block[0] + 1);
}
@@ -709,7 +723,14 @@ static int cp2112_xfer(struct i2c_adapter *adap, u16 addr,
size = I2C_SMBUS_BLOCK_DATA;
read_write = I2C_SMBUS_READ;
- count = cp2112_write_read_req(buf, addr, I2C_SMBUS_BLOCK_MAX,
+ /* data_length is data->block[0] + 1, so
+ * so data->block[0] should be less than or
+ * equal to the I2C_SMBUS_BLOCK_MAX + 1
+ */
+ if (data->block[0] > I2C_SMBUS_BLOCK_MAX + 1)
+ count = -EINVAL;
+ else
+ count = cp2112_write_read_req(buf, addr, I2C_SMBUS_BLOCK_MAX,
command, data->block,
data->block[0] + 1);
break;
--
2.51.0
^ permalink raw reply related
* Re: [PATCH v2 00/16] Align availability checks on fwnode child node enumeration
From: Rafael J. Wysocki @ 2025-09-26 12:52 UTC (permalink / raw)
To: Sakari Ailus
Cc: Rafael J. Wysocki, linux-acpi, linux-kernel, linux-input,
linux-leds, linux-media, netdev, linux-spi, Len Brown,
Greg Kroah-Hartman, Danilo Krummrich, Andy Shevchenko,
Daniel Scally, Heikki Krogerus, Javier Carrasco, Dmitry Torokhov,
Lee Jones, Pavel Machek, Matthias Fend, Chanwoo Choi,
Krzysztof Kozlowski, Laurent Pinchart, Paul Elder,
Mauro Carvalho Chehab, Horatiu Vultur, UNGLinuxDriver,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Mark Brown, Thomas Gleixner, Ingo Molnar,
Jonathan Cameron
In-Reply-To: <aNZ9fbh8eLiPAJzR@kekkonen.localdomain>
Hi Sakari,
On Fri, Sep 26, 2025 at 1:48 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Wed, Sep 24, 2025 at 12:52:12PM +0200, Rafael J. Wysocki wrote:
> > Hi Sakari,
> >
> > On Wed, Sep 24, 2025 at 9:46 AM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hello everyone,
> > >
> > > Historically the fwnode property API has enumerated only available device
> > > nodes on OF whereas on ACPI, also nodes that haven't been present in the
> > > system have been provided. Both OF and ACPI have similar concepts of node
> > > availbility, on OF it's the "status" property present on device nodes and
> > > on ACPI the _STA object evaluates to device present, enabled and
> > > functional bits, of which the present and functional bits are currently
> > > being used to determine whether to enumerate a device.
> > >
> > > Two additional functions, fwnode_get_next_available_child_node() and
> > > fwnode_for_each_available_child_node(), have been provided to enumerate
> > > the available nodes only on ACPI, whereas on OF the implementation has
> > > been the same on the non-available variants. The motivation for providing
> > > these has very likely been to provide fwnode variants of the similarly
> > > named functions but the difference isn't justifiable from API consistency
> > > viewpoint.
> > >
> > > This set switches the users away from the "available" fwnode API functions
> > > and later on removes them, aligning the functionality on all fwnode
> > > backends.
> > >
> > > since v1:
> > >
> > > - Move patch "ACPI: property: Make acpi_get_next_subnode() static" as
> > > first.
> > >
> > > - Add missing parentheses and kernel-doc Return: section in
> > > acpi_get_next_present_subnode() documentation and move the Return
> > > section: of fwnode_graph_get_endpoint_by_id() to the end of the
> > > documentation section (new patch for the latter).
> > >
> > > - Use device_get_next_child_node() instead of fwnode_get_next_child_node()
> > > in flash LED driver drivers.
> > >
> > > - Rework iterating port nodes in acpi_graph_get_next_endpoint() as
> > > suggested by Andy (new patch).
> >
> > I think that you really have four series here, or rather two series, a
> > collection of patches depending on them, and a follow-up cleanup.
> >
> > > Sakari Ailus (16):
> > > ACPI: property: Make acpi_get_next_subnode() static
> > > ACPI: property: Use ACPI functions in acpi_graph_get_next_endpoint()
> > > only
> > > ACPI: property: Rework acpi_graph_get_next_endpoint()
> > > ACPI: property: Return present device nodes only on fwnode interface
> >
> > So the above is one series, focused on ACPI property changes.
> >
> > They can go in via ACPI as soon as everyone is happy with them. I
> > think I can push them for 6.18 if that helps to process the other
> > patches.
>
> If it's an option, that would be nice. But see below.
>
> >
> > > property: Move Return: section of fwnode_graph_get_endpoint_by_id()
> > > down
> > > property: Drop DEVICE_DISABLED flag in
> > > fwnode_graph_get_endpoint_by_id()
> > > property: Drop DEVICE_DISABLED flag in
> > > fwnode_graph_get_endpoint_count()
> >
> > The above patches are another series that doesn't depend on the first
> > one AFAICS and can go in via driver core.
>
> Agreed.
>
> >
> > > property: Document that fwnode API returns available nodes
> > > driver core: Use fwnode_for_each_child_node() instead
> > > net: lan966x: Use fwnode_for_each_child_node() instead
> > > Input: touch-overlay - Use fwnode_for_each_child_node() instead
> > > media: thp7312: Use fwnode_for_each_child_node() instead
> > > leds: Use fwnode_for_each_child_node() instead
> > > leds: Use fwnode_get_next_child_node() instead
> >
> > The above can go in via respective subsystem trees when the ACPI
> > property series gets in (I'm not sure if/how they depend on the second
> > series).
> >
> > And the following one is a follow-up cleanup getting rid of code that
> > would be redundant going forward.
> >
> > > property: Drop functions operating on "available" child nodes
> > > spi: cadence: Remove explicit device node availability check
> >
> > Does the spi change depend on the previous patch?
>
> There's really only one dependency, apart from the direct dependency of
> fwnode_get_next_available_child_node() /
> fwnode_for_each_available_child_node() definitions removed in the second
> last patch: fwnode_get_next_child_node() and fwnode_for_each_child_node()
> may still return non-available nodes before the last of the ACPI patches in
> the set. So if the ACPI patches aren't merged but the rest are,
> non-available nodes could be returned.
>
> How about:
>
> 1. Merge the ACPI patches to 6.18.
>
> 2. Merge the rest, apart from the second last patch, for 6.19.
>
> 3. Once everything else is in, merge the last patch. Could wait for 6.20.
Sounds good.
> Perhaps I should split the series in three sets?
That would help I think.
> I'll send an update on the ACPI patches soon, to address a comment related
> to them.
OK
Thanks!
^ permalink raw reply
* 答复: 答复: [External Mail]Re: The zero power level of the HID device in kernel 6.12 is not reported from the kernel to the upper layer.
From: 卢国宏 @ 2025-09-26 12:03 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: José Expósito, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, jikos@kernel.org,
bentiss@kernel.org, 李鹏, Fei1 Jiang 蒋飞,
宋密密, 卢国宏
In-Reply-To: <vkm32giijggtzv7hudsvqg34utpqvw4nnccfi7d4txj5tlzstp@4bu2ox2lmtm5>
Hi Dmitry,
After testing, we found that your proposed method can solve our problem. Please help merge this method into the Linux kernel as soon as possible! Please remember to send us the relevant information of the merged git so that we can contact Google and merge their Android GKI as well. Our project is looking forward to using this feature. Thank you very much!
________________________________________
发件人: Dmitry Torokhov <dmitry.torokhov@gmail.com>
发送时间: 2025年9月25日 12:26
收件人: 卢国宏
抄送: José Expósito; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; jikos@kernel.org; bentiss@kernel.org; 李鹏; Fei1 Jiang 蒋飞; 宋密密
主题: Re: 答复: [External Mail]Re: The zero power level of the HID device in kernel 6.12 is not reported from the kernel to the upper layer.
[外部邮件] 此邮件来源于小米公司外部，请谨慎处理。若对邮件安全性存疑，请将邮件转发给misec@xiaomi.com进行反馈
On Mon, Sep 22, 2025 at 09:29:20AM +0000, 卢国宏 wrote:
>
> What kind of action are we talking about? Section 31 of the HID
> specification defines events for "Smart Battery" ("To comply with the
> Smart Battery Specification, the Battery System must support the
> functions defined in the Battery and Charger usage tables. For details,
> see Section 4.2, “Battery System Page (x85).”) and is typically used for
> "battery pack for cellular phones (principal source), the battery
> pack(s) for notebook computers (auxiliary source), and the sealed
> batteries in uninterruptible power supplies (auxiliary source)."
>
> Is your use case main battery or battery in a stylus or some other
> peripheral?
>
>
> --->>>
> What we are discussing is the code implementation of Section 31 of the
> HID protocol: 31 Battery System Page (0x85). Our scenario is: an
> Android phone is connected to a handle via USB. The handle is a HID
> device with a battery. The power of the battery in the handle is sent
> to the bottom layer (kernel) of the phone via USB. The bottom layer of
> the phone then reports this power to the upper layer of Android
> through the HID driver.
I see. I guess we can try only filtering out 0 reports for the
digitizers, leaving other devices with batteries alone. Something like
this:
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index ff1784b5c2a4..ba3f6655af9e 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -595,14 +595,18 @@ static void hidinput_cleanup_battery(struct hid_device *dev)
dev->battery = NULL;
}
-static void hidinput_update_battery(struct hid_device *dev, int value)
+static void hidinput_update_battery(struct hid_device *dev,
+ unsigned int usage, int value)
{
int capacity;
if (!dev->battery)
return;
- if (value == 0 || value < dev->battery_min || value > dev->battery_max)
+ if ((usage & HID_USAGE_PAGE) == HID_UP_DIGITIZER && value == 0)
+ return;
+
+ if (value < dev->battery_min || value > dev->battery_max)
return;
capacity = hidinput_scale_battery_capacity(dev, value);
@@ -1518,7 +1522,7 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
bool handled = hidinput_set_battery_charge_status(hid, usage->hid, value);
if (!handled)
- hidinput_update_battery(hid, value);
+ hidinput_update_battery(hid, usage->hid, value);
return;
}
Thanks.
--
Dmitry
#/******本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！ This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#
^ permalink raw reply related
* Re: [PATCH v2 03/16] ACPI: property: Rework acpi_graph_get_next_endpoint()
From: Sakari Ailus @ 2025-09-26 11:49 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-acpi, linux-kernel, linux-input, linux-leds, linux-media,
netdev, linux-spi, Rafael J. Wysocki, Len Brown,
Greg Kroah-Hartman, Danilo Krummrich, Andy Shevchenko,
Daniel Scally, Heikki Krogerus, Javier Carrasco, Dmitry Torokhov,
Lee Jones, Pavel Machek, Matthias Fend, Chanwoo Choi,
Krzysztof Kozlowski, Paul Elder, Mauro Carvalho Chehab,
Horatiu Vultur, UNGLinuxDriver, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Mark Brown,
Thomas Gleixner, Ingo Molnar, Jonathan Cameron
In-Reply-To: <20250924093934.GC28073@pendragon.ideasonboard.com>
Hi Laurent,
Thank you for the review.
On Wed, Sep 24, 2025 at 12:39:34PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Wed, Sep 24, 2025 at 10:45:49AM +0300, Sakari Ailus wrote:
> > Rework the code obtaining the next endpoint in
> > acpi_graph_get_next_endpoint(). The resulting code removes unnecessary
> > contitionals and should be easier to follow.
> >
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/acpi/property.c | 13 +++++++------
> > 1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > index 3e85900080ac..5438592dc136 100644
> > --- a/drivers/acpi/property.c
> > +++ b/drivers/acpi/property.c
> > @@ -1399,14 +1399,15 @@ static struct fwnode_handle *acpi_graph_get_next_endpoint(
> > if (!port)
> > return NULL;
> >
> > - endpoint = acpi_get_next_subnode(port, prev);
> > - while (!endpoint) {
> > - port = acpi_get_next_subnode(fwnode, port);
> > - if (!port)
> > + do {
> > + endpoint = acpi_get_next_subnode(port, prev);
> > + if (endpoint)
> > break;
> > +
> > + port = acpi_get_next_subnode(fwnode, port);
> > if (is_acpi_graph_node(port, "port"))
> > - endpoint = acpi_get_next_subnode(port, NULL);
> > - }
> > + prev = NULL;
>
> Isn't there an issue here ? If the next subnode of fwnode is not a port,
> the next iteration of the do loop will attempt to get an endpoint from
> that non-port node. Maybe the acpi_get_next_subnode() that will try to
> get the endpoint from the non-port port will return NULL because prev
> won't be a child of port, but that seems fragile.
>
> I think the following would be easier to understand:
>
> do {
> endpoint = acpi_get_next_subnode(port, prev);
> if (endpoint)
> break;
>
> prev = NULL;
>
> do {
> port = acpi_get_next_subnode(fwnode, port);
> } while (port && !is_acpi_graph_node(port, "port"));
> } while (port);
Yes, this indeed ensures port will be a port node. I'll use this in the
next version.
>
> > + } while (port);
> >
> > /*
> > * The names of the endpoint nodes begin with "endpoint@" followed by
> >
>
--
Regards,
Sakari Ailus
^ permalink raw reply
* Re: [PATCH v2 00/16] Align availability checks on fwnode child node enumeration
From: Sakari Ailus @ 2025-09-26 11:48 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-acpi, linux-kernel, linux-input, linux-leds, linux-media,
netdev, linux-spi, Len Brown, Greg Kroah-Hartman,
Danilo Krummrich, Andy Shevchenko, Daniel Scally, Heikki Krogerus,
Javier Carrasco, Dmitry Torokhov, Lee Jones, Pavel Machek,
Matthias Fend, Chanwoo Choi, Krzysztof Kozlowski,
Laurent Pinchart, Paul Elder, Mauro Carvalho Chehab,
Horatiu Vultur, UNGLinuxDriver, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Mark Brown,
Thomas Gleixner, Ingo Molnar, Jonathan Cameron
In-Reply-To: <CAJZ5v0hSy9zQd6cP9B4QPSZi-6ughmkW=VoEBV-0MbUr2xcaAQ@mail.gmail.com>
Hi Rafael,
On Wed, Sep 24, 2025 at 12:52:12PM +0200, Rafael J. Wysocki wrote:
> Hi Sakari,
>
> On Wed, Sep 24, 2025 at 9:46 AM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hello everyone,
> >
> > Historically the fwnode property API has enumerated only available device
> > nodes on OF whereas on ACPI, also nodes that haven't been present in the
> > system have been provided. Both OF and ACPI have similar concepts of node
> > availbility, on OF it's the "status" property present on device nodes and
> > on ACPI the _STA object evaluates to device present, enabled and
> > functional bits, of which the present and functional bits are currently
> > being used to determine whether to enumerate a device.
> >
> > Two additional functions, fwnode_get_next_available_child_node() and
> > fwnode_for_each_available_child_node(), have been provided to enumerate
> > the available nodes only on ACPI, whereas on OF the implementation has
> > been the same on the non-available variants. The motivation for providing
> > these has very likely been to provide fwnode variants of the similarly
> > named functions but the difference isn't justifiable from API consistency
> > viewpoint.
> >
> > This set switches the users away from the "available" fwnode API functions
> > and later on removes them, aligning the functionality on all fwnode
> > backends.
> >
> > since v1:
> >
> > - Move patch "ACPI: property: Make acpi_get_next_subnode() static" as
> > first.
> >
> > - Add missing parentheses and kernel-doc Return: section in
> > acpi_get_next_present_subnode() documentation and move the Return
> > section: of fwnode_graph_get_endpoint_by_id() to the end of the
> > documentation section (new patch for the latter).
> >
> > - Use device_get_next_child_node() instead of fwnode_get_next_child_node()
> > in flash LED driver drivers.
> >
> > - Rework iterating port nodes in acpi_graph_get_next_endpoint() as
> > suggested by Andy (new patch).
>
> I think that you really have four series here, or rather two series, a
> collection of patches depending on them, and a follow-up cleanup.
>
> > Sakari Ailus (16):
> > ACPI: property: Make acpi_get_next_subnode() static
> > ACPI: property: Use ACPI functions in acpi_graph_get_next_endpoint()
> > only
> > ACPI: property: Rework acpi_graph_get_next_endpoint()
> > ACPI: property: Return present device nodes only on fwnode interface
>
> So the above is one series, focused on ACPI property changes.
>
> They can go in via ACPI as soon as everyone is happy with them. I
> think I can push them for 6.18 if that helps to process the other
> patches.
If it's an option, that would be nice. But see below.
>
> > property: Move Return: section of fwnode_graph_get_endpoint_by_id()
> > down
> > property: Drop DEVICE_DISABLED flag in
> > fwnode_graph_get_endpoint_by_id()
> > property: Drop DEVICE_DISABLED flag in
> > fwnode_graph_get_endpoint_count()
>
> The above patches are another series that doesn't depend on the first
> one AFAICS and can go in via driver core.
Agreed.
>
> > property: Document that fwnode API returns available nodes
> > driver core: Use fwnode_for_each_child_node() instead
> > net: lan966x: Use fwnode_for_each_child_node() instead
> > Input: touch-overlay - Use fwnode_for_each_child_node() instead
> > media: thp7312: Use fwnode_for_each_child_node() instead
> > leds: Use fwnode_for_each_child_node() instead
> > leds: Use fwnode_get_next_child_node() instead
>
> The above can go in via respective subsystem trees when the ACPI
> property series gets in (I'm not sure if/how they depend on the second
> series).
>
> And the following one is a follow-up cleanup getting rid of code that
> would be redundant going forward.
>
> > property: Drop functions operating on "available" child nodes
> > spi: cadence: Remove explicit device node availability check
>
> Does the spi change depend on the previous patch?
There's really only one dependency, apart from the direct dependency of
fwnode_get_next_available_child_node() /
fwnode_for_each_available_child_node() definitions removed in the second
last patch: fwnode_get_next_child_node() and fwnode_for_each_child_node()
may still return non-available nodes before the last of the ACPI patches in
the set. So if the ACPI patches aren't merged but the rest are,
non-available nodes could be returned.
How about:
1. Merge the ACPI patches to 6.18.
2. Merge the rest, apart from the second last patch, for 6.19.
3. Once everything else is in, merge the last patch. Could wait for 6.20.
Perhaps I should split the series in three sets?
I'll send an update on the ACPI patches soon, to address a comment related
to them.
--
Kind regards,
Sakari Ailus
^ permalink raw reply
* [dtor-input:next] BUILD SUCCESS 9712fe00a7728c64be7e51f05162aafea231a929
From: kernel test robot @ 2025-09-26 6:32 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
branch HEAD: 9712fe00a7728c64be7e51f05162aafea231a929 Input: pm8941-pwrkey - disable wakeup for resin by default
elapsed time: 1458m
configs tested: 126
configs skipped: 4
The following configs have been built successfully.
More configs may be tested in the coming days.
tested configs:
alpha allnoconfig gcc-15.1.0
alpha allyesconfig gcc-15.1.0
arc allmodconfig gcc-15.1.0
arc allnoconfig gcc-15.1.0
arc allyesconfig gcc-15.1.0
arc randconfig-001-20250925 gcc-13.4.0
arc randconfig-002-20250925 gcc-15.1.0
arm allmodconfig gcc-15.1.0
arm allnoconfig clang-22
arm allyesconfig gcc-15.1.0
arm bcm2835_defconfig clang-22
arm pxa168_defconfig clang-19
arm randconfig-001-20250925 gcc-11.5.0
arm randconfig-002-20250925 gcc-10.5.0
arm randconfig-003-20250925 gcc-8.5.0
arm randconfig-004-20250925 gcc-14.3.0
arm sama7_defconfig clang-22
arm vf610m4_defconfig gcc-15.1.0
arm64 allmodconfig clang-19
arm64 allnoconfig gcc-15.1.0
arm64 randconfig-001-20250925 gcc-11.5.0
arm64 randconfig-002-20250925 gcc-15.1.0
arm64 randconfig-003-20250925 clang-19
arm64 randconfig-004-20250925 gcc-8.5.0
csky allnoconfig gcc-15.1.0
csky randconfig-001-20250925 gcc-14.3.0
csky randconfig-002-20250925 gcc-15.1.0
hexagon alldefconfig clang-22
hexagon allmodconfig clang-17
hexagon allnoconfig clang-22
hexagon allyesconfig clang-22
hexagon randconfig-001-20250925 clang-22
hexagon randconfig-002-20250925 clang-22
i386 allmodconfig gcc-14
i386 allnoconfig gcc-14
i386 allyesconfig gcc-14
i386 buildonly-randconfig-001-20250925 clang-20
i386 buildonly-randconfig-002-20250925 clang-20
i386 buildonly-randconfig-003-20250925 gcc-14
i386 buildonly-randconfig-004-20250925 gcc-14
i386 buildonly-randconfig-005-20250925 clang-20
i386 buildonly-randconfig-006-20250925 clang-20
i386 defconfig clang-20
loongarch allmodconfig clang-19
loongarch allnoconfig clang-22
loongarch randconfig-001-20250925 clang-18
loongarch randconfig-002-20250925 gcc-12.5.0
m68k allmodconfig gcc-15.1.0
m68k allnoconfig gcc-15.1.0
m68k allyesconfig gcc-15.1.0
m68k apollo_defconfig gcc-15.1.0
m68k m5307c3_defconfig gcc-15.1.0
microblaze allmodconfig gcc-15.1.0
microblaze allnoconfig gcc-15.1.0
microblaze allyesconfig gcc-15.1.0
microblaze defconfig gcc-15.1.0
mips allnoconfig gcc-15.1.0
mips ip28_defconfig gcc-15.1.0
mips rt305x_defconfig clang-22
nios2 allnoconfig gcc-11.5.0
nios2 defconfig gcc-11.5.0
nios2 randconfig-001-20250925 gcc-8.5.0
nios2 randconfig-002-20250925 gcc-10.5.0
openrisc allnoconfig gcc-15.1.0
openrisc allyesconfig gcc-15.1.0
openrisc defconfig gcc-15.1.0
parisc allmodconfig gcc-15.1.0
parisc allnoconfig gcc-15.1.0
parisc allyesconfig gcc-15.1.0
parisc defconfig gcc-15.1.0
parisc randconfig-001-20250925 gcc-8.5.0
parisc randconfig-002-20250925 gcc-15.1.0
parisc64 alldefconfig gcc-15.1.0
parisc64 defconfig gcc-15.1.0
powerpc allmodconfig gcc-15.1.0
powerpc allnoconfig gcc-15.1.0
powerpc allyesconfig clang-22
powerpc canyonlands_defconfig clang-22
powerpc mpc512x_defconfig clang-22
powerpc pasemi_defconfig clang-22
powerpc randconfig-001-20250925 clang-22
powerpc randconfig-002-20250925 gcc-8.5.0
powerpc randconfig-003-20250925 gcc-8.5.0
powerpc64 randconfig-001-20250925 clang-22
powerpc64 randconfig-002-20250925 gcc-14.3.0
powerpc64 randconfig-003-20250925 gcc-8.5.0
riscv allnoconfig gcc-15.1.0
riscv randconfig-001-20250925 clang-22
riscv randconfig-002-20250925 clang-22
s390 allmodconfig clang-18
s390 allnoconfig clang-22
s390 allyesconfig gcc-15.1.0
s390 randconfig-001-20250925 gcc-15.1.0
s390 randconfig-002-20250925 gcc-13.4.0
sh allmodconfig gcc-15.1.0
sh allnoconfig gcc-15.1.0
sh allyesconfig gcc-15.1.0
sh randconfig-001-20250925 gcc-15.1.0
sh randconfig-002-20250925 gcc-13.4.0
sh se7712_defconfig gcc-15.1.0
sparc allmodconfig gcc-15.1.0
sparc allnoconfig gcc-15.1.0
sparc defconfig gcc-15.1.0
sparc randconfig-001-20250925 gcc-15.1.0
sparc randconfig-002-20250925 gcc-12.5.0
sparc64 randconfig-001-20250925 gcc-10.5.0
sparc64 randconfig-002-20250925 gcc-10.5.0
um allmodconfig clang-19
um allnoconfig clang-22
um allyesconfig gcc-14
um randconfig-001-20250925 gcc-14
um randconfig-002-20250925 clang-22
x86_64 allnoconfig clang-20
x86_64 allyesconfig clang-20
x86_64 buildonly-randconfig-001-20250925 clang-20
x86_64 buildonly-randconfig-002-20250925 gcc-14
x86_64 buildonly-randconfig-003-20250925 gcc-14
x86_64 buildonly-randconfig-004-20250925 clang-20
x86_64 buildonly-randconfig-005-20250925 clang-20
x86_64 buildonly-randconfig-006-20250925 gcc-14
x86_64 defconfig gcc-14
x86_64 rhel-9.4-rust clang-20
xtensa allnoconfig gcc-15.1.0
xtensa common_defconfig gcc-15.1.0
xtensa randconfig-001-20250925 gcc-12.5.0
xtensa randconfig-002-20250925 gcc-11.5.0
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v2 02/16] ACPI: property: Use ACPI functions in acpi_graph_get_next_endpoint() only
From: Sakari Ailus @ 2025-09-26 6:18 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-acpi, linux-kernel, linux-input, linux-leds, linux-media,
netdev, linux-spi, Rafael J. Wysocki, Len Brown,
Greg Kroah-Hartman, Danilo Krummrich, Andy Shevchenko,
Daniel Scally, Heikki Krogerus, Javier Carrasco, Lee Jones,
Pavel Machek, Matthias Fend, Chanwoo Choi, Krzysztof Kozlowski,
Laurent Pinchart, Paul Elder, Mauro Carvalho Chehab,
Horatiu Vultur, UNGLinuxDriver, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Mark Brown,
Thomas Gleixner, Ingo Molnar, Jonathan Cameron
In-Reply-To: <iqfarpvf72l7qbhfinopjb27qvfm7wg77d4yhuy5qyubcwtcd2@exmcuvgqr353>
Hi Dmitry,
On Wed, Sep 24, 2025 at 11:32:56AM -0700, Dmitry Torokhov wrote:
> Hi Sakari,
>
> On Wed, Sep 24, 2025 at 10:45:48AM +0300, Sakari Ailus wrote:
> > Calling fwnode_get_next_child_node() in ACPI implementation of the fwnode
> > property API is somewhat problematic as the latter is used in the
>
> How exactly is this problematic?
In general, the fwnode property API is implemented by the (currently three)
backends so the backend calling fwnode property API to call itself without
knowing what exactly gets called may end up in an infinite recursion.
Keeping ACPI implementation separate from the fwnode property frontend
avoids even needing to think about this.
>
> > impelementation of the former. Instead of using
> > fwnode_get_next_child_node() in acpi_graph_get_next_endpoint(), call
> > acpi_get_next_subnode() directly instead.
>
> I think we are moving into the world of mixed fwnode types with software
> nodes/secondary fwnodes, so I do not think this is a step in right
> direction.
This is not how it works. If you have an ACPI node, it's and ACPI node, not
a software node or an OF node. A software node would be attached as
fwnode->secondary, it's not the same ACPI (device or data) node.
--
Regards,
Sakari Ailus
^ permalink raw reply
* Re: [PATCH v2 00/11] mfd: macsmc: add rtc, hwmon and hid subdevices
From: Janne Grunau @ 2025-09-25 22:13 UTC (permalink / raw)
To: James Calligeros
Cc: Sven Peter, Alyssa Rosenzweig, Neal Gompa, Lee Jones, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Alexandre Belloni,
Jean Delvare, Guenter Roeck, Dmitry Torokhov, asahi,
linux-arm-kernel, devicetree, linux-kernel, linux-rtc,
linux-hwmon, linux-input, Mark Kettenis, Hector Martin
In-Reply-To: <20250827-macsmc-subdevs-v2-0-ce5e99d54c28@gmail.com>
On Wed, Aug 27, 2025 at 09:22:34PM +1000, James Calligeros wrote:
> Hi all,
>
> This series adds support for the remaining SMC subdevices. These are the
> RTC, hwmon, and HID devices. They are being submitted together as the RTC
> and hwmon drivers both require changes to the SMC DT schema.
I we would instantiate all sub devices via MFD_CELL_OF macsmc-hid (or
-input) and the omitted macsmc-power would require SMC dt-binding
changes as well. I'm conflicted on that since both use SMC to determine
if they are present.
Janne
^ permalink raw reply
* Re: [PATCH v2 10/11] arm64: dts: apple: Add common hwmon sensors and fans
From: Janne Grunau @ 2025-09-25 22:02 UTC (permalink / raw)
To: James Calligeros
Cc: Sven Peter, Alyssa Rosenzweig, Neal Gompa, Lee Jones, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Alexandre Belloni,
Jean Delvare, Guenter Roeck, Dmitry Torokhov, asahi,
linux-arm-kernel, devicetree, linux-kernel, linux-rtc,
linux-hwmon, linux-input
In-Reply-To: <20250827-macsmc-subdevs-v2-10-ce5e99d54c28@gmail.com>
On Wed, Aug 27, 2025 at 09:22:44PM +1000, James Calligeros wrote:
> Each Apple Silicon device exposes a unique set of sensors and fans,
> however some have been found to be reliably common across devices.
>
> Add these as .dtsi files so that they can be combined with any
> device-specific sensors without excessive repetition.
>
> Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
> ---
> .../boot/dts/apple/hwmon-common.dtsi | 37 +++++++++++++++++++++++++
> .../boot/dts/apple/hwmon-fan-dual.dtsi | 24 ++++++++++++++++
> arch/arm64/boot/dts/apple/hwmon-fan.dtsi | 19 +++++++++++++
> .../boot/dts/apple/hwmon-laptop.dtsi | 35 +++++++++++++++++++++++
> .../boot/dts/apple/hwmon-mac-mini.dtsi | 17 ++++++++++++
> 5 files changed, 132 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/apple/hwmon-common.dtsi b/arch/arm64/boot/dts/apple/hwmon-common.dtsi
> new file mode 100644
> index 0000000000000000000000000000000000000000..bac4e44d3f9892fe8ad04125e47dcccb2bcf57a0
> --- /dev/null
> +++ b/arch/arm64/boot/dts/apple/hwmon-common.dtsi
> @@ -0,0 +1,37 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT
> +/*
> + * Hardawre monitoring sensors expected to be found on all Apple Silicon devices
typo: "Hardawre"
> + *
> + * Copyright The Asahi Linux Contributors
> + */
> +
> +&smc {
> + hwmon {
> + compatible = "apple,smc-hwmon";
I think it woiuld make more sense to add the hwmon child node with the
compatible in the SoC *.dtsi and then only add the sensor nodes to
"&smc_hwmon" in these hwmon-*.dtsi files
Janne
^ permalink raw reply
* Re: [PATCH v2 08/11] mfd: macsmc: Wire up Apple SMC HID subdevice
From: Janne Grunau @ 2025-09-25 21:55 UTC (permalink / raw)
To: James Calligeros
Cc: Sven Peter, Alyssa Rosenzweig, Neal Gompa, Lee Jones, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Alexandre Belloni,
Jean Delvare, Guenter Roeck, Dmitry Torokhov, asahi,
linux-arm-kernel, devicetree, linux-kernel, linux-rtc,
linux-hwmon, linux-input
In-Reply-To: <20250827-macsmc-subdevs-v2-8-ce5e99d54c28@gmail.com>
On Wed, Aug 27, 2025 at 09:22:42PM +1000, James Calligeros wrote:
> Add the new SMC HID function to the mfd device
>
> Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
> ---
> drivers/mfd/macsmc.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mfd/macsmc.c b/drivers/mfd/macsmc.c
> index 286dc150aa6cfdd6d29f769094278daaddabe7c3..893fc47d62b2c956d966d1841895a3fa2b9a3005 100644
> --- a/drivers/mfd/macsmc.c
> +++ b/drivers/mfd/macsmc.c
> @@ -45,6 +45,7 @@
> #define SMC_TIMEOUT_MS 500
>
> static const struct mfd_cell apple_smc_devs[] = {
> + MFD_CELL_NAME("macsmc-hid"),
I think it would make sense to use MFD_CELL_OF and device nodes for all
macsmc even though it's in the case of macsmc-input (or -hid) not
strictly necessary
> MFD_CELL_OF("macsmc-gpio", NULL, NULL, 0, 0, "apple,smc-gpio"),
> MFD_CELL_OF("macsmc_hwmon", NULL, NULL, 0, 0, "apple,smc-hwmon"),
> MFD_CELL_OF("macsmc-reboot", NULL, NULL, 0, 0, "apple,smc-reboot"),
Janne
^ permalink raw reply
* Re: [PATCH v2 07/11] input: macsmc-hid: New driver to handle the Apple Mac SMC buttons/lid
From: Janne Grunau @ 2025-09-25 21:49 UTC (permalink / raw)
To: James Calligeros
Cc: Sven Peter, Alyssa Rosenzweig, Neal Gompa, Lee Jones, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Alexandre Belloni,
Jean Delvare, Guenter Roeck, Dmitry Torokhov, asahi,
linux-arm-kernel, devicetree, linux-kernel, linux-rtc,
linux-hwmon, linux-input, Hector Martin
In-Reply-To: <20250827-macsmc-subdevs-v2-7-ce5e99d54c28@gmail.com>
On Wed, Aug 27, 2025 at 09:22:41PM +1000, James Calligeros wrote:
> From: Hector Martin <marcan@marcan.st>
>
> This driver implements power button and lid switch support for Apple Mac
> devices using SMC controllers driven by the macsmc driver.
>
> In addition to basic input support, this also responds to the final
> shutdown warning (when the power button is held down long enough) by
> doing an emergency kernel poweroff. This allows the NVMe controller to
> be cleanly shut down, which prevents data loss for in-cache data.
>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> Co-developed-by: Sven Peter <sven@kernel.org>
> Signed-off-by: Sven Peter <sven@kernel.org>
> Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
> ---
> MAINTAINERS | 1 +
> drivers/input/misc/Kconfig | 11 ++
> drivers/input/misc/Makefile | 1 +
> drivers/input/misc/macsmc-hid.c | 209 +++++++++++++++++++++++++
I would rename this driver to macsmc-input. I would expect hid driver to
live in drivers/hid/ and use HID report descriptors
> 4 files changed, 222 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2eb23522654dd050262eb06e077587030cc335aa..b3b5220fcd0d4bbef67613c8ee9afa880c0aa45d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2412,6 +2412,7 @@ F: drivers/hwmon/macsmc_hwmon.c
> F: drivers/pmdomain/apple/
> F: drivers/i2c/busses/i2c-pasemi-core.c
> F: drivers/i2c/busses/i2c-pasemi-platform.c
> +F: drivers/input/misc/macsmc-hid.c
> F: drivers/input/touchscreen/apple_z2.c
> F: drivers/iommu/apple-dart.c
> F: drivers/iommu/io-pgtable-dart.c
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 0fb21c99a5e3d1230d7f40f99e0c2d360bbe80e8..a430c50e7f63f245bba56bd526026ec7901cf821 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -961,4 +961,15 @@ config INPUT_STPMIC1_ONKEY
> To compile this driver as a module, choose M here: the
> module will be called stpmic1_onkey.
>
> +config INPUT_MACSMC_HID
> + tristate "Apple Mac SMC lid/buttons"
> + depends on MFD_MACSMC
> + help
> + Say Y here if you want to use the input events delivered via the
> + SMC controller on Apple Mac machines using the macsmc driver.
> + This includes lid open/close and the power button.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called macsmc-hid.
> +
> endif
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index d468c8140b93da5bb537e8a3baea2b90e7bb4229..95b8ebbb9ebbe6f3afc9db724d2ebeba1d75d1a6 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_INPUT_IQS7222) += iqs7222.o
> obj-$(CONFIG_INPUT_KEYSPAN_REMOTE) += keyspan_remote.o
> obj-$(CONFIG_INPUT_KXTJ9) += kxtj9.o
> obj-$(CONFIG_INPUT_M68K_BEEP) += m68kspkr.o
> +obj-$(CONFIG_INPUT_MACSMC_HID) += macsmc-hid.o
> obj-$(CONFIG_INPUT_MAX77650_ONKEY) += max77650-onkey.o
> obj-$(CONFIG_INPUT_MAX77693_HAPTIC) += max77693-haptic.o
> obj-$(CONFIG_INPUT_MAX8925_ONKEY) += max8925_onkey.o
> diff --git a/drivers/input/misc/macsmc-hid.c b/drivers/input/misc/macsmc-hid.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..c7cd56e5c17f08936cc623f7d3690f27e05012a5
> --- /dev/null
> +++ b/drivers/input/misc/macsmc-hid.c
> @@ -0,0 +1,209 @@
> +// SPDX-License-Identifier: GPL-2.0-only OR MIT
> +/*
> + * Apple SMC input event driver
> + * Copyright The Asahi Linux Contributors
> + *
> + * This driver exposes HID events from the SMC as an input device.
> + * This includes the lid open/close and power button notifications.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/input.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/macsmc.h>
> +#include <linux/module.h>
> +#include <linux/reboot.h>
> +
> +/**
> + * struct macsmc_hid
> + * @dev: Underlying struct device for the HID sub-device
> + * @smc: Pointer to apple_smc struct of the mfd parent
> + * @input: Allocated input_dev; devres managed
> + * @nb: Notifier block used for incoming events from SMC (e.g. button pressed down)
> + * @wakeup_mode: Set to true when system is suspended and power button events should wake it
> + */
> +struct macsmc_hid {
> + struct device *dev;
> + struct apple_smc *smc;
> + struct input_dev *input;
> + struct notifier_block nb;
> + bool wakeup_mode;
> +};
> +
> +#define SMC_EV_BTN 0x7201
> +#define SMC_EV_LID 0x7203
> +
> +#define BTN_POWER 0x01 /* power button on e.g. Mac Mini chasis pressed */
> +#define BTN_TOUCHID 0x06 /* combined TouchID / power button on MacBooks pressed */
> +#define BTN_POWER_HELD_SHORT 0xfe /* power button briefly held down */
> +#define BTN_POWER_HELD_LONG 0x00 /* power button held down; sent just before forced poweroff */
> +
> +static void macsmc_hid_event_button(struct macsmc_hid *smchid, unsigned long event)
> +{
> + u8 button = (event >> 8) & 0xff;
> + u8 state = !!(event & 0xff);
> +
> + switch (button) {
> + case BTN_POWER:
> + case BTN_TOUCHID:
> + if (smchid->wakeup_mode) {
> + if (state)
> + pm_wakeup_hard_event(smchid->dev);
> + } else {
> + input_report_key(smchid->input, KEY_POWER, state);
> + input_sync(smchid->input);
> + }
> + break;
> + case BTN_POWER_HELD_SHORT: /* power button held down; ignore */
> + break;
> + case BTN_POWER_HELD_LONG:
> + /*
> + * If we get here the power button has been held down for a while and
> + * we have about 4 seconds before forced power-off is triggered by SMC.
> + * Try to do an emergency shutdown to make sure the NVMe cache is
> + * flushed. macOS actually does this by panicing (!)...
> + */
> + if (state) {
> + dev_crit(smchid->dev, "Triggering forced shutdown!\n");
> + if (kernel_can_power_off())
> + kernel_power_off();
> + else /* Missing macsmc-reboot driver? */
> + kernel_restart("SMC power button triggered restart");
> + }
> + break;
> + default:
> + dev_warn(smchid->dev, "Unknown SMC button event: %04lx\n", event & 0xffff);
> + }
> +}
> +
> +static void macsmc_hid_event_lid(struct macsmc_hid *smchid, unsigned long event)
> +{
> + u8 lid_state = !!((event >> 8) & 0xff);
> +
> + if (smchid->wakeup_mode && !lid_state)
> + pm_wakeup_hard_event(smchid->dev);
> +
> + input_report_switch(smchid->input, SW_LID, lid_state);
> + input_sync(smchid->input);
> +}
> +
> +static int macsmc_hid_event(struct notifier_block *nb, unsigned long event, void *data)
> +{
> + struct macsmc_hid *smchid = container_of(nb, struct macsmc_hid, nb);
> + u16 type = event >> 16;
> +
> + switch (type) {
> + case SMC_EV_BTN:
> + macsmc_hid_event_button(smchid, event);
> + return NOTIFY_OK;
> + case SMC_EV_LID:
> + macsmc_hid_event_lid(smchid, event);
> + return NOTIFY_OK;
> + default:
> + /* SMC event meant for another driver */
> + return NOTIFY_DONE;
> + }
> +}
> +
> +static int macsmc_hid_probe(struct platform_device *pdev)
> +{
> + struct apple_smc *smc = dev_get_drvdata(pdev->dev.parent);
> + struct macsmc_hid *smchid;
> + bool have_lid, have_power;
> + int ret;
> +
> + /* Bail early if this SMC neither supports power button nor lid events */
> + have_lid = apple_smc_key_exists(smc, SMC_KEY(MSLD));
> + have_power = apple_smc_key_exists(smc, SMC_KEY(bHLD));
> + if (!have_lid && !have_power)
> + return -ENODEV;
> +
> + smchid = devm_kzalloc(&pdev->dev, sizeof(*smchid), GFP_KERNEL);
> + if (!smchid)
> + return -ENOMEM;
> +
> + smchid->dev = &pdev->dev;
> + smchid->smc = smc;
> + platform_set_drvdata(pdev, smchid);
> +
> + smchid->input = devm_input_allocate_device(&pdev->dev);
> + if (!smchid->input)
> + return -ENOMEM;
> +
> + smchid->input->phys = "macsmc-hid (0)";
> + smchid->input->name = "Apple SMC power/lid events";
> +
> + if (have_lid)
> + input_set_capability(smchid->input, EV_SW, SW_LID);
> + if (have_power)
> + input_set_capability(smchid->input, EV_KEY, KEY_POWER);
> +
> + ret = input_register_device(smchid->input);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to register input device: %d\n", ret);
> + return ret;
> + }
> +
> + if (have_lid) {
> + u8 val;
> +
> + ret = apple_smc_read_u8(smc, SMC_KEY(MSLD), &val);
> + if (ret < 0)
> + dev_warn(&pdev->dev, "Failed to read initial lid state\n");
> + else
> + input_report_switch(smchid->input, SW_LID, val);
> + }
> +
> + if (have_power) {
> + u32 val;
> +
> + ret = apple_smc_read_u32(smc, SMC_KEY(bHLD), &val);
> + if (ret < 0)
> + dev_warn(&pdev->dev, "Failed to read initial power button state\n");
> + else
> + input_report_key(smchid->input, KEY_POWER, val & 1);
> + }
> +
> + input_sync(smchid->input);
> +
> + smchid->nb.notifier_call = macsmc_hid_event;
> + blocking_notifier_chain_register(&smc->event_handlers, &smchid->nb);
> +
> + device_init_wakeup(&pdev->dev, 1);
> +
> + return 0;
> +}
> +
> +static int macsmc_hid_pm_prepare(struct device *dev)
> +{
> + struct macsmc_hid *smchid = dev_get_drvdata(dev);
> +
> + smchid->wakeup_mode = true;
> + return 0;
> +}
> +
> +static void macsmc_hid_pm_complete(struct device *dev)
> +{
> + struct macsmc_hid *smchid = dev_get_drvdata(dev);
> +
> + smchid->wakeup_mode = false;
> +}
> +
> +static const struct dev_pm_ops macsmc_hid_pm_ops = {
> + .prepare = macsmc_hid_pm_prepare,
> + .complete = macsmc_hid_pm_complete,
> +};
> +
> +static struct platform_driver macsmc_hid_driver = {
> + .driver = {
> + .name = "macsmc-hid",
> + .pm = &macsmc_hid_pm_ops,
> + },
> + .probe = macsmc_hid_probe,
> +};
> +module_platform_driver(macsmc_hid_driver);
> +
> +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>");
> +MODULE_LICENSE("Dual MIT/GPL");
> +MODULE_DESCRIPTION("Apple SMC HID driver");
> +MODULE_ALIAS("platform:macsmc-hid");
The module alias should be removed.
Janne
^ permalink raw reply
* Re: [PATCH v2 02/11] dt-bindings: hwmon: Add Apple System Management Controller hwmon schema
From: Rob Herring @ 2025-09-25 21:43 UTC (permalink / raw)
To: Janne Grunau
Cc: James Calligeros, Sven Peter, Alyssa Rosenzweig, Neal Gompa,
Lee Jones, Krzysztof Kozlowski, Conor Dooley, Alexandre Belloni,
Jean Delvare, Guenter Roeck, Dmitry Torokhov, asahi,
linux-arm-kernel, devicetree, linux-kernel, linux-rtc,
linux-hwmon, linux-input
In-Reply-To: <20250925204925.GA637503@robin.jannau.net>
On Thu, Sep 25, 2025 at 3:49 PM Janne Grunau <j@jannau.net> wrote:
>
> On Fri, Aug 29, 2025 at 11:40:57AM -0500, Rob Herring wrote:
> > On Wed, Aug 27, 2025 at 09:22:36PM +1000, James Calligeros wrote:
> > > Apple Silicon devices integrate a vast array of sensors, monitoring
> > > current, power, temperature, and voltage across almost every part of
> > > the system. The sensors themselves are all connected to the System
> > > Management Controller (SMC). The SMC firmware exposes the data
> > > reported by these sensors via its standard FourCC-based key-value
> > > API. The SMC is also responsible for monitoring and controlling any
> > > fans connected to the system, exposing them in the same way.
> > >
> > > For reasons known only to Apple, each device exposes its sensors with
> > > an almost totally unique set of keys. This is true even for devices
> > > which share an SoC. An M1 Mac mini, for example, will report its core
> > > temperatures on different keys to an M1 MacBook Pro. Worse still, the
> > > SMC does not provide a way to enumerate the available keys at runtime,
> > > nor do the keys follow any sort of reasonable or consistent naming
> > > rules that could be used to deduce their purpose. We must therefore
> > > know which keys are present on any given device, and which function
> > > they serve, ahead of time.
> > >
> > > Add a schema so that we can describe the available sensors for a given
> > > Apple Silicon device in the Devicetree.
> > >
> > > Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
> > > ---
> > > .../bindings/hwmon/apple,smc-hwmon.yaml | 132 +++++++++++++++++++++++++
> > > .../bindings/mfd/apple,smc.yaml | 36 +++++++
> > > MAINTAINERS | 1 +
> > > 3 files changed, 169 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/hwmon/apple,smc-hwmon.yaml b/Documentation/devicetree/bindings/hwmon/apple,smc-hwmon.yaml
> > > new file mode 100644
> > > index 0000000000000000000000000000000000000000..08cc4f55f3a41ca8b3b428088f96240266fa42e8
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/hwmon/apple,smc-hwmon.yaml
> > > @@ -0,0 +1,132 @@
> >
> > This should be something like this:
> >
> > "^current-[A-Za-z0-9]{4}$":
> > $ref: "#/$defs/sensor"
> > unevaluatedProperties: false
> >
> > With the $defs/sensor being:
> >
> > $defs:
> > sensor:
> > type: object
> >
> > properties:
> > apple,key-id:
> > $ref: /schemas/types.yaml#/definitions/string
> > pattern: "^[A-Za-z0-9]{4}$"
> > description:
> > The SMC FourCC key of the desired sensor. Must match the
> > node's suffix.
> >
> > label:
> > description: Human-readable name for the sensor
> >
> > required:
> > - apple,key-id
> > - label
> >
> > Though in general, 'label' should never be required being just for human
> > convenience.
>
> That does not sound as it would be compatible with skipping nodes in the
> driver if the node misses label. The driver could of course fall back
> to create a hwmon sensors without labels.
The driver absolutely should.
> I looks to me it would be a
> stretch to call the presence of the labels human convenience.
Then it is an abuse of 'label". "label" is supposed to be literally
that. Matching a sticker on a port of a device.
If you need to associate a sensor with some other piece of h/w, then
that should be via a phandle or something.
Rob
^ permalink raw reply
* Re: [PATCH v2 05/11] hwmon: Add Apple Silicon SMC hwmon driver
From: Janne Grunau @ 2025-09-25 21:33 UTC (permalink / raw)
To: James Calligeros
Cc: Sven Peter, Alyssa Rosenzweig, Neal Gompa, Lee Jones, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Alexandre Belloni,
Jean Delvare, Guenter Roeck, Dmitry Torokhov, asahi,
linux-arm-kernel, devicetree, linux-kernel, linux-rtc,
linux-hwmon, linux-input
In-Reply-To: <20250827-macsmc-subdevs-v2-5-ce5e99d54c28@gmail.com>
On Wed, Aug 27, 2025 at 09:22:39PM +1000, James Calligeros wrote:
> The System Management Controller on Apple Silicon devices is responsible
> for integrating and exposing the data reported by the vast array of
> hardware monitoring sensors present on these devices. It is also
> responsible for fan control, and allows users to manually set fan
> speeds if they so desire. Add a hwmon driver to expose current,
> power, temperature, and voltage monitoring sensors, as well as
> fan speed monitoring and control via the SMC on Apple Silicon devices.
>
> The SMC firmware has no consistency between devices, even when they
> share an SoC. The FourCC keys used to access sensors are almost
> random. An M1 Mac mini will have different FourCCs for its CPU core
> temperature sensors to an M1 MacBook Pro, for example. For this
> reason, the valid sensors for a given device are specified in a
> child of the SMC Devicetree node. The driver uses this information
> to determine which sensors to make available at runtime.
>
> Co-developed-by: Janne Grunau <j@jannau.net>
> Signed-off-by: Janne Grunau <j@jannau.net>
> Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
> ---
> MAINTAINERS | 1 +
> drivers/hwmon/Kconfig | 12 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/macsmc_hwmon.c | 848 +++++++++++++++++++++++++
Please use macsmc-hwmon.c as name. All other macsmc sub device drivers
use '-' and it's more common in 'drivers/hwmon/*.c'
> 4 files changed, 862 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5d53d243dc9abdf1db5865f8e6bcddbac3eafebe..2eb23522654dd050262eb06e077587030cc335aa 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2408,6 +2408,7 @@ F: drivers/clk/clk-apple-nco.c
> F: drivers/cpufreq/apple-soc-cpufreq.c
> F: drivers/dma/apple-admac.c
> F: drivers/gpio/gpio-macsmc.c
> +F: drivers/hwmon/macsmc_hwmon.c
> F: drivers/pmdomain/apple/
> F: drivers/i2c/busses/i2c-pasemi-core.c
> F: drivers/i2c/busses/i2c-pasemi-platform.c
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 9d28fcf7cd2a6f9e2f54694a717bd85ff4047b46..1ca6db71e4d9da32717fd14487c10944433ada41 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1164,6 +1164,18 @@ config SENSORS_LTQ_CPUTEMP
> If you say yes here you get support for the temperature
> sensor inside your CPU.
>
> +config SENSORS_MACSMC_HWMON
> + tristate "Apple SMC (Apple Silicon)"
> + depends on MFD_MACSMC && OF
> + help
> + This driver enables hwmon support for current, power, temperature,
> + and voltage sensors, as well as fan speed reporting and control
> + on Apple Silicon devices. Say Y here if you have an Apple Silicon
> + device.
> +
> + This driver can also be built as a module. If so, the module will
> + be called macsmc_hwmon.
macsmc-hwmon
> +
> config SENSORS_MAX1111
> tristate "Maxim MAX1111 Serial 8-bit ADC chip and compatibles"
> depends on SPI_MASTER
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index cd8bc4752b4dbf015c6eb46157626f4e8f87dfae..80fc8447aff15b3b8e8583dc755c8accb3b6a93e 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -147,6 +147,7 @@ obj-$(CONFIG_SENSORS_LTC4260) += ltc4260.o
> obj-$(CONFIG_SENSORS_LTC4261) += ltc4261.o
> obj-$(CONFIG_SENSORS_LTC4282) += ltc4282.o
> obj-$(CONFIG_SENSORS_LTQ_CPUTEMP) += ltq-cputemp.o
> +obj-$(CONFIG_SENSORS_MACSMC_HWMON) += macsmc_hwmon.o
> obj-$(CONFIG_SENSORS_MAX1111) += max1111.o
> obj-$(CONFIG_SENSORS_MAX127) += max127.o
> obj-$(CONFIG_SENSORS_MAX16065) += max16065.o
> diff --git a/drivers/hwmon/macsmc_hwmon.c b/drivers/hwmon/macsmc_hwmon.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..e44ea357870238aa0b38d7b674a6c456271fdf0c
> --- /dev/null
> +++ b/drivers/hwmon/macsmc_hwmon.c
> @@ -0,0 +1,848 @@
> +// SPDX-License-Identifier: GPL-2.0-only OR MIT
> +/*
> + * Apple SMC hwmon driver for Apple Silicon platforms
> + *
> + * The System Management Controller on Apple Silicon devices is responsible for
> + * measuring data from sensors across the SoC and machine. These include power,
> + * temperature, voltage and current sensors. Some "sensors" actually expose
> + * derived values. An example of this is the key PHPC, which is an estimate
> + * of the heat energy being dissipated by the SoC.
> + *
> + * While each SoC only has one SMC variant, each platform exposes a different
> + * set of sensors. For example, M1 MacBooks expose battery telemetry sensors
> + * which are not present on the M1 Mac mini. For this reason, the available
> + * sensors for a given platform are described in the device tree in a child
> + * node of the SMC device. We must walk this list of available sensors and
> + * populate the required hwmon data structures at runtime.
> + *
> + * Originally based on a concept by Jean-Francois Bortolotti <jeff@borto.fr>
> + *
> + * Copyright The Asahi Linux Contributors
> + */
> +
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/mfd/macsmc.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#define MAX_LABEL_LENGTH 32
> +#define NUM_SENSOR_TYPES 5 /* temp, volt, current, power, fan */
I would write voltage and temperature out. At least in comments.
Shortening "voltage" to volt seems strange when "current" and "power"
are used otherwise.
> +
> +#define FLT_EXP_BIAS 127
> +#define FLT_EXP_MASK GENMASK(30, 23)
> +#define FLT_MANT_BIAS 23
> +#define FLT_MANT_MASK GENMASK(22, 0)
> +#define FLT_SIGN_MASK BIT(31)
> +
> +static bool fan_control;
> +module_param_unsafe(fan_control, bool, 0644);
> +MODULE_PARM_DESC(fan_control,
> + "Override the SMC to set your own fan speeds on supported machines");
> +
> +struct macsmc_hwmon_sensor {
> + struct apple_smc_key_info info;
> + smc_key macsmc_key;
> + char label[MAX_LABEL_LENGTH];
> +};
> +
> +struct macsmc_hwmon_fan {
> + struct macsmc_hwmon_sensor now;
> + struct macsmc_hwmon_sensor min;
> + struct macsmc_hwmon_sensor max;
> + struct macsmc_hwmon_sensor set;
> + struct macsmc_hwmon_sensor mode;
> + char label[MAX_LABEL_LENGTH];
> + u32 attrs;
> + bool manual;
> +};
> +
> +struct macsmc_hwmon_sensors {
> + struct hwmon_channel_info channel_info;
> + struct macsmc_hwmon_sensor *sensors;
> + u32 count;
> +};
> +
> +struct macsmc_hwmon_fans {
> + struct hwmon_channel_info channel_info;
> + struct macsmc_hwmon_fan *fans;
> + u32 count;
> +};
> +
> +struct macsmc_hwmon {
> + struct device *dev;
> + struct apple_smc *smc;
> + struct device *hwmon_dev;
> + struct hwmon_chip_info chip_info;
> + /* Chip + sensor types + NULL */
> + const struct hwmon_channel_info *channel_infos[1 + NUM_SENSOR_TYPES + 1];
> + struct macsmc_hwmon_sensors temp;
> + struct macsmc_hwmon_sensors volt;
> + struct macsmc_hwmon_sensors curr;
matching the "hwmon_*" names is a convincing argument not sure what to
do about the voltage sensors though
> + struct macsmc_hwmon_sensors power;
> + struct macsmc_hwmon_fans fan;
> +};
> +
> +static int macsmc_hwmon_read_label(struct device *dev,
> + enum hwmon_sensor_types type, u32 attr,
> + int channel, const char **str)
> +{
> + struct macsmc_hwmon *hwmon = dev_get_drvdata(dev);
> +
> + switch (type) {
> + case hwmon_temp:
> + if (channel >= hwmon->temp.count)
> + return -EINVAL;
> + *str = hwmon->temp.sensors[channel].label;
> + break;
> + case hwmon_in:
> + if (channel >= hwmon->volt.count)
> + return -EINVAL;
> + *str = hwmon->volt.sensors[channel].label;
> + break;
> + case hwmon_curr:
> + if (channel >= hwmon->curr.count)
> + return -EINVAL;
> + *str = hwmon->curr.sensors[channel].label;
> + break;
> + case hwmon_power:
> + if (channel >= hwmon->power.count)
> + return -EINVAL;
> + *str = hwmon->power.sensors[channel].label;
> + break;
> + case hwmon_fan:
> + if (channel >= hwmon->fan.count)
> + return -EINVAL;
> + *str = hwmon->fan.fans[channel].label;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * A number of sensors report data in a 48.16 fixed-point decimal format that is
> + * not used by any other function of the SMC.
> + */
> +static int macsmc_hwmon_read_ioft_scaled(struct apple_smc *smc, smc_key key,
> + u64 *p, int scale)
> +{
> + u64 val;
> + int ret;
> +
> + ret = apple_smc_read_u64(smc, key, &val);
> + if (ret < 0)
> + return ret;
> +
> + *p = mult_frac(val, scale, 65536);
> +
> + return 0;
> +}
> +
> +/*
> + * Many sensors report their data as IEEE-754 floats. No other SMC function uses
> + * them.
> + */
> +static int macsmc_hwmon_read_f32_scaled(struct apple_smc *smc, smc_key key,
> + int *p, int scale)
> +{
> + u32 fval;
> + u64 val;
> + int ret, exp;
> +
> + ret = apple_smc_read_u32(smc, key, &fval);
> + if (ret < 0)
> + return ret;
> +
> + val = ((u64)((fval & FLT_MANT_MASK) | BIT(23)));
> + exp = ((fval >> 23) & 0xff) - FLT_EXP_BIAS - FLT_MANT_BIAS;
> + if (scale < 0) {
> + val <<= 32;
> + exp -= 32;
> + val /= -scale;
64-bit division is not universally available. The easiest solution for
Apple silicon would be to depend on CONFIG_64BIT. However so far
macsmc_hwmon_read_key() is only called with positive scale values so it
would make sense to just not support negative scale values.
> + } else {
> + val *= scale;
> + }
> +
> + if (exp > 63)
> + val = U64_MAX;
> + else if (exp < -63)
> + val = 0;
> + else if (exp < 0)
> + val >>= -exp;
> + else if (exp != 0 && (val & ~((1UL << (64 - exp)) - 1))) /* overflow */
> + val = U64_MAX;
> + else
> + val <<= exp;
> +
> + if (fval & FLT_SIGN_MASK) {
> + if (val > (-(s64)INT_MIN))
> + *p = INT_MIN;
> + else
> + *p = -val;
> + } else {
> + if (val > INT_MAX)
> + *p = INT_MAX;
> + else
> + *p = val;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * The SMC has keys of multiple types, denoted by a FourCC of the same format
> + * as the key ID. We don't know what data type a key encodes until we poke at it.
> + */
> +static int macsmc_hwmon_read_key(struct apple_smc *smc,
> + struct macsmc_hwmon_sensor *sensor, int scale,
> + long *val)
> +{
> + int ret;
> +
> + switch (sensor->info.type_code) {
> + /* 32-bit IEEE 754 float */
> + case _SMC_KEY("flt "): {
This fails here with gcc (GCC) 15.2.1 20250808 (Red Hat 15.2.1-1)"
"error: case label does not reduce to an integer constant"
The same for the other "case _SMC_KEY()" statements
> + u32 flt_ = 0;
> +
> + ret = macsmc_hwmon_read_f32_scaled(smc, sensor->macsmc_key,
> + &flt_, scale);
> + *val = flt_;
> + break;
> + }
> + /* 48.16 fixed point decimal */
> + case _SMC_KEY("ioft"): {
> + u64 ioft = 0;
> +
> + ret = macsmc_hwmon_read_ioft_scaled(smc, sensor->macsmc_key,
> + &ioft, scale);
> + *val = (long)ioft;
> + break;
> + }
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + if (ret)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int macsmc_hwmon_write_f32_scaled(struct apple_smc *smc, smc_key key,
> + int value, int scale)
> +{
> + u64 val;
> + u32 fval = 0;
> + int exp, neg;
> +
> + val = abs(value);
> + neg = val != value;
> +
> + if (scale > 1) {
> + val <<= 32;
> + exp = 32;
> + val /= scale;
Similar here. macsmc_hwmon_write_key() is only called with scale == 1
> + } else if (scale < 1) {
> + val *= -scale;
> + }
> +
> + if (val) {
> + int msb = __fls(val) - exp;
> +
> + if (msb > 23) {
> + val >>= msb - 23;
> + exp -= msb - 23;
> + } else if (msb < 23) {
> + val <<= 23 - msb;
> + exp += msb;
> + }
> +
> + fval = FIELD_PREP(FLT_SIGN_MASK, neg) |
> + FIELD_PREP(FLT_EXP_MASK, exp + FLT_EXP_BIAS) |
> + FIELD_PREP(FLT_MANT_MASK, val);
> + }
> +
> + return apple_smc_write_u32(smc, key, fval);
> +}
> +
> +static int macsmc_hwmon_write_key(struct apple_smc *smc,
> + struct macsmc_hwmon_sensor *sensor, long val,
> + int scale)
> +{
> + switch (sensor->info.type_code) {
> + /* 32-bit IEEE 754 float */
> + case _SMC_KEY("flt "):
> + return macsmc_hwmon_write_f32_scaled(smc, sensor->macsmc_key,
> + val, scale);
> + /* unsigned 8-bit integer */
> + case _SMC_KEY("ui8 "):
> + return apple_smc_write_u8(smc, sensor->macsmc_key, val);
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int macsmc_hwmon_read_fan(struct macsmc_hwmon *hwmon, u32 attr, int chan,
> + long *val)
> +{
> + if (!(hwmon->fan.fans[chan].attrs & BIT(attr)))
> + return -EINVAL;
> +
> + switch (attr) {
> + case hwmon_fan_input:
> + return macsmc_hwmon_read_key(hwmon->smc,
> + &hwmon->fan.fans[chan].now, 1, val);
> + case hwmon_fan_min:
> + return macsmc_hwmon_read_key(hwmon->smc,
> + &hwmon->fan.fans[chan].min, 1, val);
> + case hwmon_fan_max:
> + return macsmc_hwmon_read_key(hwmon->smc,
> + &hwmon->fan.fans[chan].max, 1, val);
> + case hwmon_fan_target:
> + return macsmc_hwmon_read_key(hwmon->smc,
> + &hwmon->fan.fans[chan].set, 1, val);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int macsmc_hwmon_write_fan(struct device *dev, u32 attr, int channel,
> + long val)
> +{
> + struct macsmc_hwmon *hwmon = dev_get_drvdata(dev);
> + long min, max;
> + int ret;
> +
> + if (!fan_control || hwmon->fan.fans[channel].mode.macsmc_key == 0)
> + return -EOPNOTSUPP;
> +
> + if (channel >= hwmon->fan.count ||
> + !(hwmon->fan.fans[channel].attrs & BIT(attr)) ||
> + attr != hwmon_fan_target)
> + return -EINVAL;
> +
> + /*
> + * The SMC does no sanity checks on requested fan speeds, so we need to.
> + */
> + ret = macsmc_hwmon_read_key(hwmon->smc, &hwmon->fan.fans[channel].min,
> + 1, &min);
> + if (ret)
> + return ret;
> +
> + ret = macsmc_hwmon_read_key(hwmon->smc, &hwmon->fan.fans[channel].max,
> + 1, &max);
> + if (ret)
> + return ret;
> +
> + if (val >= min && val <= max) {
> + if (!hwmon->fan.fans[channel].manual) {
> + /* Write 1 to mode key for manual control */
> + ret = macsmc_hwmon_write_key(hwmon->smc,
> + &hwmon->fan.fans[channel].mode, 1,
> + 1);
> + if (ret < 0)
> + return ret;
> +
> + hwmon->fan.fans[channel].manual = true;
> + }
> + return macsmc_hwmon_write_key(hwmon->smc,
> + &hwmon->fan.fans[channel].set, val, 1);
> + } else if (!val) {
> + if (hwmon->fan.fans[channel].manual) {
> + ret = macsmc_hwmon_write_key(hwmon->smc,
> + &hwmon->fan.fans[channel].mode, 0, 1);
> + if (ret < 0)
> + return ret;
> +
> + hwmon->fan.fans[channel].manual = false;
> + }
> + } else {
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int macsmc_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct macsmc_hwmon *hwmon = dev_get_drvdata(dev);
> + int ret = 0;
> +
> + switch (type) {
> + case hwmon_temp:
> + ret = macsmc_hwmon_read_key(hwmon->smc,
> + &hwmon->temp.sensors[channel], 1000, val);
> + break;
> + case hwmon_in:
> + ret = macsmc_hwmon_read_key(hwmon->smc,
> + &hwmon->volt.sensors[channel], 1000, val);
> + break;
> + case hwmon_curr:
> + ret = macsmc_hwmon_read_key(hwmon->smc,
> + &hwmon->curr.sensors[channel], 1000, val);
> + break;
> + case hwmon_power:
> + /* SMC returns power in Watts with acceptable precision to scale to uW */
> + ret = macsmc_hwmon_read_key(hwmon->smc,
> + &hwmon->power.sensors[channel],
> + 1000000, val);
> + break;
> + case hwmon_fan:
> + ret = macsmc_hwmon_read_fan(hwmon, attr, channel, val);
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return ret;
> +}
> +
> +static int macsmc_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long val)
> +{
> + switch (type) {
> + case hwmon_fan:
> + return macsmc_hwmon_write_fan(dev, attr, channel, val);
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static umode_t macsmc_hwmon_fan_is_visible(const void *data, u32 attr,
> + int channel)
> +{
> + const struct macsmc_hwmon *hwmon = data;
> +
> + if (fan_control && attr == hwmon_fan_target &&
> + hwmon->fan.fans[channel].mode.macsmc_key != 0)
> + return 0644;
> +
> + return 0444;
> +}
> +
> +static umode_t macsmc_hwmon_is_visible(const void *data,
> + enum hwmon_sensor_types type, u32 attr,
> + int channel)
> +{
> + switch (type) {
> + case hwmon_fan:
> + return macsmc_hwmon_fan_is_visible(data, attr, channel);
> + default:
> + break;
> + }
> +
> + return 0444;
> +}
> +
> +static const struct hwmon_ops macsmc_hwmon_ops = {
> + .is_visible = macsmc_hwmon_is_visible,
> + .read = macsmc_hwmon_read,
> + .read_string = macsmc_hwmon_read_label,
> + .write = macsmc_hwmon_write,
> +};
> +
> +/*
> + * Get the key metadata, including key data type, from the SMC.
> + */
> +static int macsmc_hwmon_parse_key(struct device *dev, struct apple_smc *smc,
> + struct macsmc_hwmon_sensor *sensor,
> + const char *key)
> +{
> + int ret;
> +
> + ret = apple_smc_get_key_info(smc, _SMC_KEY(key), &sensor->info);
> + if (ret) {
> + dev_err(dev, "Failed to retrieve key info for %s\n", key);
> + return ret;
> + }
> +
> + sensor->macsmc_key = _SMC_KEY(key);
> +
> + return 0;
> +}
> +
> +/*
> + * A sensor is a single key-value pair as made available by the SMC.
> + * The devicetree gives us the SMC key ID and a friendly name where the
> + * purpose of the sensor is known.
> + */
> +static int macsmc_hwmon_create_sensor(struct device *dev, struct apple_smc *smc,
> + struct device_node *sensor_node,
> + struct macsmc_hwmon_sensor *sensor)
> +{
> + const char *key, *label;
> + int ret;
> +
> + ret = of_property_read_string(sensor_node, "apple,key-id", &key);
> + if (ret) {
> + dev_err(dev, "Could not find apple,key-id in sensor node\n");
> + return ret;
> + }
> +
> + ret = macsmc_hwmon_parse_key(dev, smc, sensor, key);
> + if (ret)
> + return ret;
> +
> + ret = of_property_read_string(sensor_node, "label", &label);
> + if (ret) {
> + dev_err(dev, "No label found for sensor %s\n", key);
see discussion in the dt-bindings patch, if the label is just for human
convenience this should not fail but use the key-id as fallback.
> + return ret;
> + }
> +
> + strscpy_pad(sensor->label, label, sizeof(sensor->label));
> +
> + return 0;
> +}
> +
> +/*
> + * Fan data is exposed by the SMC as multiple sensors.
> + *
> + * The devicetree schema reuses apple,key-id for the actual fan speed sensor.
> + * Min, max and target keys do not need labels, so we can reuse label
> + * for naming the entire fan.
> + */
> +static int macsmc_hwmon_create_fan(struct device *dev, struct apple_smc *smc,
> + struct device_node *fan_node,
> + struct macsmc_hwmon_fan *fan)
> +{
> + const char *label, *now, *min, *max, *set, *mode;
> + int ret;
> +
> + ret = of_property_read_string(fan_node, "apple,key-id", &now);
> + if (ret) {
> + dev_err(dev, "apple,key-id not found in fan node!\n");
> + return -EINVAL;
> + }
> +
> + ret = macsmc_hwmon_parse_key(dev, smc, &fan->now, now);
> + if (ret)
> + return ret;
> +
> + ret = of_property_read_string(fan_node, "label", &label);
> + if (ret) {
> + dev_err(dev, "No label found for fan %s\n", now);
> + return ret;
> + }
> +
> + strscpy_pad(fan->label, label, sizeof(fan->label));
> +
> + fan->attrs = HWMON_F_LABEL | HWMON_F_INPUT;
> +
> + /* The following keys are not required to simply monitor fan speed */
> + if (!of_property_read_string(fan_node, "apple,fan-minimum", &min)) {
> + ret = macsmc_hwmon_parse_key(dev, smc, &fan->min, min);
> + if (ret)
> + return ret;
> +
> + fan->attrs |= HWMON_F_MIN;
> + }
> +
> + if (!of_property_read_string(fan_node, "apple,fan-maximum", &max)) {
> + ret = macsmc_hwmon_parse_key(dev, smc, &fan->max, max);
> + if (ret)
> + return ret;
> +
> + fan->attrs |= HWMON_F_MAX;
> + }
> +
> + if (!of_property_read_string(fan_node, "apple,fan-target", &set)) {
> + ret = macsmc_hwmon_parse_key(dev, smc, &fan->set, set);
> + if (ret)
> + return ret;
> +
> + fan->attrs |= HWMON_F_TARGET;
> + }
> +
> + if (!of_property_read_string(fan_node, "apple,fan-mode", &mode)) {
> + ret = macsmc_hwmon_parse_key(dev, smc, &fan->mode, mode);
> + if (ret)
> + return ret;
> + }
> +
> + /* Initialise fan control mode to automatic */
> + fan->manual = false;
> +
> + return 0;
> +}
> +
> +static int macsmc_hwmon_populate_sensors(struct macsmc_hwmon *hwmon,
> + struct device_node *hwmon_node)
> +{
> + struct device_node *key_node __maybe_unused;
> + u32 n_current = 0, n_fan = 0, n_power = 0, n_temperature = 0, n_voltage = 0;
> +
> + for_each_child_of_node_with_prefix(hwmon_node, key_node, "current-") {
> + n_current++;
> + }
> +
> + if (n_current) {
> + hwmon->curr.sensors = devm_kcalloc(hwmon->dev, n_current,
> + sizeof(struct macsmc_hwmon_sensor), GFP_KERNEL);
> + if (!hwmon->curr.sensors)
> + return -ENOMEM;
> +
> + for_each_child_of_node_with_prefix(hwmon_node, key_node, "current-") {
> + if (!macsmc_hwmon_create_sensor(hwmon->dev, hwmon->smc, key_node,
> + &hwmon->curr.sensors[hwmon->curr.count]))
> + hwmon->curr.count++;
> + }
> + }
> +
> + for_each_child_of_node_with_prefix(hwmon_node, key_node, "fan-") {
> + n_fan++;
> + }
> +
> + if (n_fan) {
> + hwmon->fan.fans = devm_kcalloc(hwmon->dev, n_fan,
> + sizeof(struct macsmc_hwmon_fan), GFP_KERNEL);
> + if (!hwmon->fan.fans)
> + return -ENOMEM;
> +
> + for_each_child_of_node_with_prefix(hwmon_node, key_node, "fan-") {
> + if (!macsmc_hwmon_create_fan(hwmon->dev, hwmon->smc, key_node,
> + &hwmon->fan.fans[hwmon->fan.count]))
> + hwmon->fan.count++;
> + }
> + }
> +
> + for_each_child_of_node_with_prefix(hwmon_node, key_node, "power-") {
> + n_power++;
> + }
> +
> + if (n_power) {
> + hwmon->power.sensors = devm_kcalloc(hwmon->dev, n_power,
> + sizeof(struct macsmc_hwmon_sensor), GFP_KERNEL);
> + if (!hwmon->power.sensors)
> + return -ENOMEM;
> +
> + for_each_child_of_node_with_prefix(hwmon_node, key_node, "power-") {
> + if (!macsmc_hwmon_create_sensor(hwmon->dev, hwmon->smc, key_node,
> + &hwmon->power.sensors[hwmon->power.count]))
> + hwmon->power.count++;
> + }
> + }
> +
> + for_each_child_of_node_with_prefix(hwmon_node, key_node, "temperature-") {
> + n_temperature++;
> + }
> +
> + if (n_temperature) {
> + hwmon->temp.sensors = devm_kcalloc(hwmon->dev, n_temperature,
> + sizeof(struct macsmc_hwmon_sensor), GFP_KERNEL);
> + if (!hwmon->temp.sensors)
> + return -ENOMEM;
> +
> + for_each_child_of_node_with_prefix(hwmon_node, key_node, "temperature-") {
> + if (!macsmc_hwmon_create_sensor(hwmon->dev, hwmon->smc, key_node,
> + &hwmon->temp.sensors[hwmon->temp.count]))
> + hwmon->temp.count++;
> + }
> + }
> +
> + for_each_child_of_node_with_prefix(hwmon_node, key_node, "voltage-") {
> + n_voltage++;
> + }
> +
> + if (n_voltage) {
> + hwmon->volt.sensors = devm_kcalloc(hwmon->dev, n_voltage,
> + sizeof(struct macsmc_hwmon_sensor), GFP_KERNEL);
> + if (!hwmon->volt.sensors)
> + return -ENOMEM;
> +
> + for_each_child_of_node_with_prefix(hwmon_node, key_node, "volt-") {
> + if (!macsmc_hwmon_create_sensor(hwmon->dev, hwmon->smc, key_node,
> + &hwmon->volt.sensors[hwmon->volt.count]))
> + hwmon->volt.count++;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/* Create NULL-terminated config arrays */
> +static void macsmc_hwmon_populate_configs(u32 *configs, u32 num_keys, u32 flags)
> +{
> + int idx;
> +
> + for (idx = 0; idx < num_keys; idx++)
> + configs[idx] = flags;
> +}
> +
> +static void macsmc_hwmon_populate_fan_configs(u32 *configs, u32 num_keys,
> + struct macsmc_hwmon_fans *fans)
> +{
> + int idx;
> +
> + for (idx = 0; idx < num_keys; idx++)
> + configs[idx] = fans->fans[idx].attrs;
> +}
> +
> +static const struct hwmon_channel_info *const macsmc_chip_channel_info =
> + HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ);
> +
> +static int macsmc_hwmon_create_infos(struct macsmc_hwmon *hwmon)
> +{
> + struct hwmon_channel_info *channel_info;
> + int i = 0;
> +
> + /* chip */
> + hwmon->channel_infos[i++] = macsmc_chip_channel_info;
> +
> + if (hwmon->curr.count) {
> + channel_info = &hwmon->curr.channel_info;
> + channel_info->type = hwmon_curr;
> + channel_info->config = devm_kcalloc(hwmon->dev, hwmon->curr.count + 1,
> + sizeof(u32), GFP_KERNEL);
> + if (!channel_info->config)
> + return -ENOMEM;
> +
> + macsmc_hwmon_populate_configs((u32 *)channel_info->config,
> + hwmon->curr.count,
> + (HWMON_C_INPUT | HWMON_C_LABEL));
> + hwmon->channel_infos[i++] = channel_info;
> + }
> +
> + if (hwmon->fan.count) {
> + channel_info = &hwmon->fan.channel_info;
> + channel_info->type = hwmon_fan;
> + channel_info->config = devm_kcalloc(hwmon->dev, hwmon->fan.count + 1,
> + sizeof(u32), GFP_KERNEL);
> + if (!channel_info->config)
> + return -ENOMEM;
> +
> + macsmc_hwmon_populate_fan_configs((u32 *)channel_info->config,
> + hwmon->fan.count,
> + &hwmon->fan);
> + hwmon->channel_infos[i++] = channel_info;
> + }
> +
> + if (hwmon->power.count) {
> + channel_info = &hwmon->power.channel_info;
> + channel_info->type = hwmon_power;
> + channel_info->config = devm_kcalloc(hwmon->dev, hwmon->power.count + 1,
> + sizeof(u32), GFP_KERNEL);
> + if (!channel_info->config)
> + return -ENOMEM;
> +
> + macsmc_hwmon_populate_configs((u32 *)channel_info->config,
> + hwmon->power.count,
> + (HWMON_P_INPUT | HWMON_P_LABEL));
> + hwmon->channel_infos[i++] = channel_info;
> + }
> +
> + if (hwmon->temp.count) {
> + channel_info = &hwmon->temp.channel_info;
> + channel_info->type = hwmon_temp;
> + channel_info->config = devm_kcalloc(hwmon->dev, hwmon->temp.count + 1,
> + sizeof(u32), GFP_KERNEL);
> + if (!channel_info->config)
> + return -ENOMEM;
> +
> + macsmc_hwmon_populate_configs((u32 *)channel_info->config,
> + hwmon->temp.count,
> + (HWMON_T_INPUT | HWMON_T_LABEL));
> + hwmon->channel_infos[i++] = channel_info;
> + }
> +
> + if (hwmon->volt.count) {
> + channel_info = &hwmon->volt.channel_info;
> + channel_info->type = hwmon_in;
> + channel_info->config = devm_kcalloc(hwmon->dev, hwmon->volt.count + 1,
> + sizeof(u32), GFP_KERNEL);
> + if (!channel_info->config)
> + return -ENOMEM;
> +
> + macsmc_hwmon_populate_configs((u32 *)channel_info->config,
> + hwmon->volt.count,
> + (HWMON_I_INPUT | HWMON_I_LABEL));
> + hwmon->channel_infos[i++] = channel_info;
> + }
> +
> + return 0;
> +}
> +
> +static int macsmc_hwmon_probe(struct platform_device *pdev)
> +{
> + struct apple_smc *smc = dev_get_drvdata(pdev->dev.parent);
> + struct macsmc_hwmon *hwmon;
> + int ret;
> +
> + /*
> + * The MFD driver will try to probe us unconditionally. Some devices
> + * with the SMC do not have hwmon capabilities. Only probe if we have
> + * a hwmon node.
> + */
> + if (!pdev->dev.of_node)
> + return -ENODEV;
> +
> + hwmon = devm_kzalloc(&pdev->dev, sizeof(*hwmon),
> + GFP_KERNEL);
> + if (!hwmon)
> + return -ENOMEM;
> +
> + hwmon->dev = &pdev->dev;
> + hwmon->smc = smc;
> +
> + ret = macsmc_hwmon_populate_sensors(hwmon, hwmon->dev->of_node);
> + if (ret) {
> + dev_err(hwmon->dev, "Could not populate keys!\n");
> + return ret;
> + }
> +
> + if (!hwmon->curr.count && !hwmon->fan.count &&
> + !hwmon->power.count && !hwmon->temp.count &&
> + !hwmon->volt.count) {
> + dev_err(hwmon->dev,
> + "No valid keys found of any supported type");
> + return -ENODEV;
> + }
> +
> + ret = macsmc_hwmon_create_infos(hwmon);
> + if (ret)
> + return ret;
> +
> + hwmon->chip_info.ops = &macsmc_hwmon_ops;
> + hwmon->chip_info.info =
> + (const struct hwmon_channel_info *const *)&hwmon->channel_infos;
> +
> + hwmon->hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
> + "macsmc_hwmon", hwmon,
> + &hwmon->chip_info, NULL);
> + if (IS_ERR(hwmon->hwmon_dev))
> + return dev_err_probe(hwmon->dev, PTR_ERR(hwmon->hwmon_dev),
> + "Probing SMC hwmon device failed\n");
> +
> + dev_info(hwmon->dev, "Registered SMC hwmon device. Sensors:");
> + dev_info(hwmon->dev,
> + "Current: %d, Fans: %d, Power: %d, Temperature: %d, Voltage: %d",
> + hwmon->curr.count, hwmon->fan.count,
> + hwmon->power.count, hwmon->temp.count,
> + hwmon->volt.count);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id macsmc_hwmon_of_table[] = {
> + { .compatible = "apple,smc-hwmon" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, macsmc_hwmon_of_table);
> +
> +static struct platform_driver macsmc_hwmon_driver = {
> + .probe = macsmc_hwmon_probe,
> + .driver = {
> + .name = "macsmc_hwmon",
macsmc-hwmon
> + .of_match_table = macsmc_hwmon_of_table,
> + },
> +};
> +module_platform_driver(macsmc_hwmon_driver);
> +
> +MODULE_DESCRIPTION("Apple Silicon SMC hwmon driver");
> +MODULE_AUTHOR("James Calligeros <jcalligeros99@gmail.com>");
> +MODULE_LICENSE("Dual MIT/GPL");
> +MODULE_ALIAS("platform:macsmc_hwmon");
The module alias should be removed when using MFD_CELL_OF in macsmc.c
Janne
^ permalink raw reply
* Re: [PATCH v2 02/11] dt-bindings: hwmon: Add Apple System Management Controller hwmon schema
From: Janne Grunau @ 2025-09-25 20:49 UTC (permalink / raw)
To: Rob Herring
Cc: James Calligeros, Sven Peter, Alyssa Rosenzweig, Neal Gompa,
Lee Jones, Krzysztof Kozlowski, Conor Dooley, Alexandre Belloni,
Jean Delvare, Guenter Roeck, Dmitry Torokhov, asahi,
linux-arm-kernel, devicetree, linux-kernel, linux-rtc,
linux-hwmon, linux-input
In-Reply-To: <20250829164057.GA976361-robh@kernel.org>
On Fri, Aug 29, 2025 at 11:40:57AM -0500, Rob Herring wrote:
> On Wed, Aug 27, 2025 at 09:22:36PM +1000, James Calligeros wrote:
> > Apple Silicon devices integrate a vast array of sensors, monitoring
> > current, power, temperature, and voltage across almost every part of
> > the system. The sensors themselves are all connected to the System
> > Management Controller (SMC). The SMC firmware exposes the data
> > reported by these sensors via its standard FourCC-based key-value
> > API. The SMC is also responsible for monitoring and controlling any
> > fans connected to the system, exposing them in the same way.
> >
> > For reasons known only to Apple, each device exposes its sensors with
> > an almost totally unique set of keys. This is true even for devices
> > which share an SoC. An M1 Mac mini, for example, will report its core
> > temperatures on different keys to an M1 MacBook Pro. Worse still, the
> > SMC does not provide a way to enumerate the available keys at runtime,
> > nor do the keys follow any sort of reasonable or consistent naming
> > rules that could be used to deduce their purpose. We must therefore
> > know which keys are present on any given device, and which function
> > they serve, ahead of time.
> >
> > Add a schema so that we can describe the available sensors for a given
> > Apple Silicon device in the Devicetree.
> >
> > Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
> > ---
> > .../bindings/hwmon/apple,smc-hwmon.yaml | 132 +++++++++++++++++++++++++
> > .../bindings/mfd/apple,smc.yaml | 36 +++++++
> > MAINTAINERS | 1 +
> > 3 files changed, 169 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/apple,smc-hwmon.yaml b/Documentation/devicetree/bindings/hwmon/apple,smc-hwmon.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..08cc4f55f3a41ca8b3b428088f96240266fa42e8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/apple,smc-hwmon.yaml
> > @@ -0,0 +1,132 @@
>
> This should be something like this:
>
> "^current-[A-Za-z0-9]{4}$":
> $ref: "#/$defs/sensor"
> unevaluatedProperties: false
>
> With the $defs/sensor being:
>
> $defs:
> sensor:
> type: object
>
> properties:
> apple,key-id:
> $ref: /schemas/types.yaml#/definitions/string
> pattern: "^[A-Za-z0-9]{4}$"
> description:
> The SMC FourCC key of the desired sensor. Must match the
> node's suffix.
>
> label:
> description: Human-readable name for the sensor
>
> required:
> - apple,key-id
> - label
>
> Though in general, 'label' should never be required being just for human
> convenience.
That does not sound as it would be compatible with skipping nodes in the
driver if the node misses label. The driver could of course fall back
to create a hwmon sensors without labels. I looks to me it would be a
stretch to call the presence of the labels human convenience.
Janne
^ permalink raw reply
* Re: [PATCH 1/1] dt-bindings: touchscreen: move ar1021.txt to maxim,max11801.yaml
From: Conor Dooley @ 2025-09-25 19:46 UTC (permalink / raw)
To: Frank Li
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)...,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list, imx
In-Reply-To: <aNWX+DxUOXlOvsmo@lizhi-Precision-Tower-5810>
[-- Attachment #1: Type: text/plain, Size: 972 bytes --]
On Thu, Sep 25, 2025 at 03:28:56PM -0400, Frank Li wrote:
> On Thu, Sep 25, 2025 at 08:20:02PM +0100, Conor Dooley wrote:
> > On Thu, Sep 25, 2025 at 02:56:47PM -0400, Frank Li wrote:
> > > ar1021 have only reg and interrupts property beside touch common
> > > properties. So move context of ar1021.txt into maxim,max11801.yaml.
> >
> > Are these devices even remotely related, other than both being touch
> > devices?
>
> No, just properties is the same. There are many binding files, which bundle
> similar properties's yaml to one file.
>
> we may create trivial-touch.yaml, which similar with trivial-rtc.yaml.
I think putting trivial devices that are completely unrelated into one
file is confusing. It makes it seem like these devices are similar in
programming model etc.
Merging them into a explicitly designated trivial-touch binding would be
an improvement, if there are lots of very similar devices that could
also be merged in together.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH 1/1] dt-bindings: touchscreen: move ar1021.txt to maxim,max11801.yaml
From: Frank Li @ 2025-09-25 19:28 UTC (permalink / raw)
To: Conor Dooley
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)...,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list, imx
In-Reply-To: <20250925-boundless-announcer-007f08404112@spud>
On Thu, Sep 25, 2025 at 08:20:02PM +0100, Conor Dooley wrote:
> On Thu, Sep 25, 2025 at 02:56:47PM -0400, Frank Li wrote:
> > ar1021 have only reg and interrupts property beside touch common
> > properties. So move context of ar1021.txt into maxim,max11801.yaml.
>
> Are these devices even remotely related, other than both being touch
> devices?
No, just properties is the same. There are many binding files, which bundle
similar properties's yaml to one file.
we may create trivial-touch.yaml, which similar with trivial-rtc.yaml.
Frank
>
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > .../bindings/input/touchscreen/ar1021.txt | 15 ---------------
> > .../input/touchscreen/maxim,max11801.yaml | 4 +++-
> > 2 files changed, 3 insertions(+), 16 deletions(-)
> > delete mode 100644 Documentation/devicetree/bindings/input/touchscreen/ar1021.txt
> >
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/ar1021.txt b/Documentation/devicetree/bindings/input/touchscreen/ar1021.txt
> > deleted file mode 100644
> > index 82019bd6094ee..0000000000000
> > --- a/Documentation/devicetree/bindings/input/touchscreen/ar1021.txt
> > +++ /dev/null
> > @@ -1,15 +0,0 @@
> > -* Microchip AR1020 and AR1021 touchscreen interface (I2C)
> > -
> > -Required properties:
> > -- compatible : "microchip,ar1021-i2c"
> > -- reg : I2C slave address
> > -- interrupts : touch controller interrupt
> > -
> > -Example:
> > -
> > - touchscreen@4d {
> > - compatible = "microchip,ar1021-i2c";
> > - reg = <0x4d>;
> > - interrupt-parent = <&gpio3>;
> > - interrupts = <11 IRQ_TYPE_LEVEL_HIGH>;
> > - };
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/maxim,max11801.yaml b/Documentation/devicetree/bindings/input/touchscreen/maxim,max11801.yaml
> > index 4f528d2201992..288c7e6e1b3b7 100644
> > --- a/Documentation/devicetree/bindings/input/touchscreen/maxim,max11801.yaml
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/maxim,max11801.yaml
> > @@ -11,7 +11,9 @@ maintainers:
> >
> > properties:
> > compatible:
> > - const: maxim,max11801
> > + enum:
> > + - maxim,max11801
> > + - microchip,ar1021-i2c
> >
> > reg:
> > maxItems: 1
> > --
> > 2.34.1
> >
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox