* [PATCH v4 3/3] selftests/hid: Add HIDIOCREVOKE tests
From: Benjamin Tissoires @ 2024-08-26 15:47 UTC (permalink / raw)
To: Jiri Kosina, Shuah Khan, Peter Hutterer
Cc: linux-input, linux-kernel, linux-kselftest, Benjamin Tissoires
In-Reply-To: <20240827-hidraw-revoke-v4-0-88c6795bf867@kernel.org>
Add 4 tests for the new revoke ioctl, for read/write/ioctl and poll.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
new in v4
---
tools/testing/selftests/hid/hidraw.c | 143 +++++++++++++++++++++++++++++++++++
1 file changed, 143 insertions(+)
diff --git a/tools/testing/selftests/hid/hidraw.c b/tools/testing/selftests/hid/hidraw.c
index f8c933476dcd..669eada8886b 100644
--- a/tools/testing/selftests/hid/hidraw.c
+++ b/tools/testing/selftests/hid/hidraw.c
@@ -19,6 +19,11 @@
__typeof__(b) _b = (b); \
_a < _b ? _a : _b; })
+/* for older kernels */
+#ifndef HIDIOCREVOKE
+#define HIDIOCREVOKE _IOW('H', 0x0D, int) /* Revoke device access */
+#endif /* HIDIOCREVOKE */
+
static unsigned char rdesc[] = {
0x06, 0x00, 0xff, /* Usage Page (Vendor Defined Page 1) */
0x09, 0x21, /* Usage (Vendor Usage 0x21) */
@@ -516,6 +521,144 @@ TEST_F(hidraw, raw_event)
ASSERT_EQ(buf[1], 42);
}
+/*
+ * After initial opening/checks of hidraw, revoke the hidraw
+ * node and check that we can not read any more data.
+ */
+TEST_F(hidraw, raw_event_revoked)
+{
+ __u8 buf[10] = {0};
+ int err;
+
+ /* inject one event */
+ buf[0] = 1;
+ buf[1] = 42;
+ uhid_send_event(_metadata, self->uhid_fd, buf, 6);
+
+ /* read the data from hidraw */
+ memset(buf, 0, sizeof(buf));
+ err = read(self->hidraw_fd, buf, sizeof(buf));
+ ASSERT_EQ(err, 6) TH_LOG("read_hidraw");
+ ASSERT_EQ(buf[0], 1);
+ ASSERT_EQ(buf[1], 42);
+
+ /* call the revoke ioctl */
+ err = ioctl(self->hidraw_fd, HIDIOCREVOKE, NULL);
+ ASSERT_OK(err) TH_LOG("couldn't revoke the hidraw fd");
+
+ /* inject one other event */
+ buf[0] = 1;
+ buf[1] = 43;
+ uhid_send_event(_metadata, self->uhid_fd, buf, 6);
+
+ /* read the data from hidraw */
+ memset(buf, 0, sizeof(buf));
+ err = read(self->hidraw_fd, buf, sizeof(buf));
+ ASSERT_EQ(err, -1) TH_LOG("read_hidraw");
+}
+
+/*
+ * Revoke the hidraw node and check that we can not do any ioctl.
+ */
+TEST_F(hidraw, ioctl_revoked)
+{
+ int err, desc_size = 0;
+
+ /* call the revoke ioctl */
+ err = ioctl(self->hidraw_fd, HIDIOCREVOKE, NULL);
+ ASSERT_OK(err) TH_LOG("couldn't revoke the hidraw fd");
+
+ /* do an ioctl */
+ err = ioctl(self->hidraw_fd, HIDIOCGRDESCSIZE, &desc_size);
+ ASSERT_EQ(err, -1) TH_LOG("read_hidraw");
+}
+
+/*
+ * Setup polling of the fd, and check that revoke works properly.
+ */
+TEST_F(hidraw, poll_revoked)
+{
+ struct pollfd pfds[1];
+ __u8 buf[10] = {0};
+ int err, ready;
+
+ /* setup polling */
+ pfds[0].fd = self->hidraw_fd;
+ pfds[0].events = POLLIN;
+
+ /* inject one event */
+ buf[0] = 1;
+ buf[1] = 42;
+ uhid_send_event(_metadata, self->uhid_fd, buf, 6);
+
+ while (true) {
+ ready = poll(pfds, 1, 5000);
+ ASSERT_EQ(ready, 1) TH_LOG("poll return value");
+
+ if (pfds[0].revents & POLLIN) {
+ memset(buf, 0, sizeof(buf));
+ err = read(self->hidraw_fd, buf, sizeof(buf));
+ ASSERT_EQ(err, 6) TH_LOG("read_hidraw");
+ ASSERT_EQ(buf[0], 1);
+ ASSERT_EQ(buf[1], 42);
+
+ /* call the revoke ioctl */
+ err = ioctl(self->hidraw_fd, HIDIOCREVOKE, NULL);
+ ASSERT_OK(err) TH_LOG("couldn't revoke the hidraw fd");
+ } else {
+ break;
+ }
+ }
+
+ ASSERT_TRUE(pfds[0].revents & POLLHUP);
+}
+
+/*
+ * After initial opening/checks of hidraw, revoke the hidraw
+ * node and check that we can not read any more data.
+ */
+TEST_F(hidraw, write_event_revoked)
+{
+ struct timespec time_to_wait;
+ __u8 buf[10] = {0};
+ int err;
+
+ /* inject one event from hidraw */
+ buf[0] = 1; /* report ID */
+ buf[1] = 2;
+ buf[2] = 42;
+
+ pthread_mutex_lock(&uhid_output_mtx);
+
+ memset(output_report, 0, sizeof(output_report));
+ clock_gettime(CLOCK_REALTIME, &time_to_wait);
+ time_to_wait.tv_sec += 2;
+
+ err = write(self->hidraw_fd, buf, 3);
+ ASSERT_EQ(err, 3) TH_LOG("unexpected error while writing to hidraw node: %d", err);
+
+ err = pthread_cond_timedwait(&uhid_output_cond, &uhid_output_mtx, &time_to_wait);
+ ASSERT_OK(err) TH_LOG("error while calling waiting for the condition");
+
+ ASSERT_EQ(output_report[0], 1);
+ ASSERT_EQ(output_report[1], 2);
+ ASSERT_EQ(output_report[2], 42);
+
+ /* call the revoke ioctl */
+ err = ioctl(self->hidraw_fd, HIDIOCREVOKE, NULL);
+ ASSERT_OK(err) TH_LOG("couldn't revoke the hidraw fd");
+
+ /* inject one other event */
+ buf[0] = 1;
+ buf[1] = 43;
+ err = write(self->hidraw_fd, buf, 3);
+ ASSERT_LT(err, 0) TH_LOG("unexpected success while writing to hidraw node: %d", err);
+ ASSERT_EQ(errno, 19) TH_LOG("unexpected error code while writing to hidraw node: %d",
+ errno);
+
+ pthread_mutex_unlock(&uhid_output_mtx);
+}
+
int main(int argc, char **argv)
{
return test_harness_run(argc, argv);
--
2.46.0
^ permalink raw reply related
* [PATCH v4 2/3] selftests/hid: Add initial hidraw tests skeleton
From: Benjamin Tissoires @ 2024-08-26 15:47 UTC (permalink / raw)
To: Jiri Kosina, Shuah Khan, Peter Hutterer
Cc: linux-input, linux-kernel, linux-kselftest, Benjamin Tissoires
In-Reply-To: <20240827-hidraw-revoke-v4-0-88c6795bf867@kernel.org>
Copy/paste from hid_bpf.c (we can revisit the common code later if needed),
and create a couple of tests for hidraw:
- create a uhid device and check if the fixture is working properly
- inject one uhid event and read it through hidraw
These tests are not that useful for now, but will be once we start adding
the ioctl and BPFs to revoke the hidraw node.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
new in v4
---
tools/testing/selftests/hid/.gitignore | 1 +
tools/testing/selftests/hid/Makefile | 2 +-
tools/testing/selftests/hid/hidraw.c | 522 +++++++++++++++++++++++++++++++++
3 files changed, 524 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/hid/.gitignore b/tools/testing/selftests/hid/.gitignore
index 995af0670f69..746c62361f77 100644
--- a/tools/testing/selftests/hid/.gitignore
+++ b/tools/testing/selftests/hid/.gitignore
@@ -2,4 +2,5 @@ bpftool
*.skel.h
/tools
hid_bpf
+hidraw
results
diff --git a/tools/testing/selftests/hid/Makefile b/tools/testing/selftests/hid/Makefile
index 2b5ea18bde38..72be55ac4bdf 100644
--- a/tools/testing/selftests/hid/Makefile
+++ b/tools/testing/selftests/hid/Makefile
@@ -32,7 +32,7 @@ CFLAGS += -Wno-unused-command-line-argument
endif
# Order correspond to 'make run_tests' order
-TEST_GEN_PROGS = hid_bpf
+TEST_GEN_PROGS = hid_bpf hidraw
# Emit succinct information message describing current building step
# $1 - generic step name (e.g., CC, LINK, etc);
diff --git a/tools/testing/selftests/hid/hidraw.c b/tools/testing/selftests/hid/hidraw.c
new file mode 100644
index 000000000000..f8c933476dcd
--- /dev/null
+++ b/tools/testing/selftests/hid/hidraw.c
@@ -0,0 +1,522 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Red Hat */
+
+#include "../kselftest_harness.h"
+
+#include <fcntl.h>
+#include <fnmatch.h>
+#include <dirent.h>
+#include <poll.h>
+#include <pthread.h>
+#include <stdbool.h>
+#include <linux/hidraw.h>
+#include <linux/uhid.h>
+
+#define SHOW_UHID_DEBUG 0
+
+#define min(a, b) \
+ ({ __typeof__(a) _a = (a); \
+ __typeof__(b) _b = (b); \
+ _a < _b ? _a : _b; })
+
+static unsigned char rdesc[] = {
+ 0x06, 0x00, 0xff, /* Usage Page (Vendor Defined Page 1) */
+ 0x09, 0x21, /* Usage (Vendor Usage 0x21) */
+ 0xa1, 0x01, /* COLLECTION (Application) */
+ 0x09, 0x01, /* Usage (Vendor Usage 0x01) */
+ 0xa1, 0x00, /* COLLECTION (Physical) */
+ 0x85, 0x02, /* REPORT_ID (2) */
+ 0x19, 0x01, /* USAGE_MINIMUM (1) */
+ 0x29, 0x08, /* USAGE_MAXIMUM (3) */
+ 0x15, 0x00, /* LOGICAL_MINIMUM (0) */
+ 0x25, 0xff, /* LOGICAL_MAXIMUM (255) */
+ 0x95, 0x08, /* REPORT_COUNT (8) */
+ 0x75, 0x08, /* REPORT_SIZE (8) */
+ 0x81, 0x02, /* INPUT (Data,Var,Abs) */
+ 0xc0, /* END_COLLECTION */
+ 0x09, 0x01, /* Usage (Vendor Usage 0x01) */
+ 0xa1, 0x00, /* COLLECTION (Physical) */
+ 0x85, 0x01, /* REPORT_ID (1) */
+ 0x06, 0x00, 0xff, /* Usage Page (Vendor Defined Page 1) */
+ 0x19, 0x01, /* USAGE_MINIMUM (1) */
+ 0x29, 0x03, /* USAGE_MAXIMUM (3) */
+ 0x15, 0x00, /* LOGICAL_MINIMUM (0) */
+ 0x25, 0x01, /* LOGICAL_MAXIMUM (1) */
+ 0x95, 0x03, /* REPORT_COUNT (3) */
+ 0x75, 0x01, /* REPORT_SIZE (1) */
+ 0x81, 0x02, /* INPUT (Data,Var,Abs) */
+ 0x95, 0x01, /* REPORT_COUNT (1) */
+ 0x75, 0x05, /* REPORT_SIZE (5) */
+ 0x81, 0x01, /* INPUT (Cnst,Var,Abs) */
+ 0x05, 0x01, /* USAGE_PAGE (Generic Desktop) */
+ 0x09, 0x30, /* USAGE (X) */
+ 0x09, 0x31, /* USAGE (Y) */
+ 0x15, 0x81, /* LOGICAL_MINIMUM (-127) */
+ 0x25, 0x7f, /* LOGICAL_MAXIMUM (127) */
+ 0x75, 0x10, /* REPORT_SIZE (16) */
+ 0x95, 0x02, /* REPORT_COUNT (2) */
+ 0x81, 0x06, /* INPUT (Data,Var,Rel) */
+
+ 0x06, 0x00, 0xff, /* Usage Page (Vendor Defined Page 1) */
+ 0x19, 0x01, /* USAGE_MINIMUM (1) */
+ 0x29, 0x03, /* USAGE_MAXIMUM (3) */
+ 0x15, 0x00, /* LOGICAL_MINIMUM (0) */
+ 0x25, 0x01, /* LOGICAL_MAXIMUM (1) */
+ 0x95, 0x03, /* REPORT_COUNT (3) */
+ 0x75, 0x01, /* REPORT_SIZE (1) */
+ 0x91, 0x02, /* Output (Data,Var,Abs) */
+ 0x95, 0x01, /* REPORT_COUNT (1) */
+ 0x75, 0x05, /* REPORT_SIZE (5) */
+ 0x91, 0x01, /* Output (Cnst,Var,Abs) */
+
+ 0x06, 0x00, 0xff, /* Usage Page (Vendor Defined Page 1) */
+ 0x19, 0x06, /* USAGE_MINIMUM (6) */
+ 0x29, 0x08, /* USAGE_MAXIMUM (8) */
+ 0x15, 0x00, /* LOGICAL_MINIMUM (0) */
+ 0x25, 0x01, /* LOGICAL_MAXIMUM (1) */
+ 0x95, 0x03, /* REPORT_COUNT (3) */
+ 0x75, 0x01, /* REPORT_SIZE (1) */
+ 0xb1, 0x02, /* Feature (Data,Var,Abs) */
+ 0x95, 0x01, /* REPORT_COUNT (1) */
+ 0x75, 0x05, /* REPORT_SIZE (5) */
+ 0x91, 0x01, /* Output (Cnst,Var,Abs) */
+
+ 0xc0, /* END_COLLECTION */
+ 0xc0, /* END_COLLECTION */
+};
+
+static __u8 feature_data[] = { 1, 2 };
+
+#define ASSERT_OK(data) ASSERT_FALSE(data)
+#define ASSERT_OK_PTR(ptr) ASSERT_NE(NULL, ptr)
+
+#define UHID_LOG(fmt, ...) do { \
+ if (SHOW_UHID_DEBUG) \
+ TH_LOG(fmt, ##__VA_ARGS__); \
+} while (0)
+
+static pthread_mutex_t uhid_started_mtx = PTHREAD_MUTEX_INITIALIZER;
+static pthread_cond_t uhid_started = PTHREAD_COND_INITIALIZER;
+
+static pthread_mutex_t uhid_output_mtx = PTHREAD_MUTEX_INITIALIZER;
+static pthread_cond_t uhid_output_cond = PTHREAD_COND_INITIALIZER;
+static unsigned char output_report[10];
+
+/* no need to protect uhid_stopped, only one thread accesses it */
+static bool uhid_stopped;
+
+static int uhid_write(struct __test_metadata *_metadata, int fd, const struct uhid_event *ev)
+{
+ ssize_t ret;
+
+ ret = write(fd, ev, sizeof(*ev));
+ if (ret < 0) {
+ TH_LOG("Cannot write to uhid: %m");
+ return -errno;
+ } else if (ret != sizeof(*ev)) {
+ TH_LOG("Wrong size written to uhid: %zd != %zu",
+ ret, sizeof(ev));
+ return -EFAULT;
+ } else {
+ return 0;
+ }
+}
+
+static int uhid_create(struct __test_metadata *_metadata, int fd, int rand_nb)
+{
+ struct uhid_event ev;
+ char buf[25];
+
+ sprintf(buf, "test-uhid-device-%d", rand_nb);
+
+ memset(&ev, 0, sizeof(ev));
+ ev.type = UHID_CREATE;
+ strcpy((char *)ev.u.create.name, buf);
+ ev.u.create.rd_data = rdesc;
+ ev.u.create.rd_size = sizeof(rdesc);
+ ev.u.create.bus = BUS_USB;
+ ev.u.create.vendor = 0x0001;
+ ev.u.create.product = 0x0a37;
+ ev.u.create.version = 0;
+ ev.u.create.country = 0;
+
+ sprintf(buf, "%d", rand_nb);
+ strcpy((char *)ev.u.create.phys, buf);
+
+ return uhid_write(_metadata, fd, &ev);
+}
+
+static void uhid_destroy(struct __test_metadata *_metadata, int fd)
+{
+ struct uhid_event ev;
+
+ memset(&ev, 0, sizeof(ev));
+ ev.type = UHID_DESTROY;
+
+ uhid_write(_metadata, fd, &ev);
+}
+
+static int uhid_event(struct __test_metadata *_metadata, int fd)
+{
+ struct uhid_event ev, answer;
+ ssize_t ret;
+
+ memset(&ev, 0, sizeof(ev));
+ ret = read(fd, &ev, sizeof(ev));
+ if (ret == 0) {
+ UHID_LOG("Read HUP on uhid-cdev");
+ return -EFAULT;
+ } else if (ret < 0) {
+ UHID_LOG("Cannot read uhid-cdev: %m");
+ return -errno;
+ } else if (ret != sizeof(ev)) {
+ UHID_LOG("Invalid size read from uhid-dev: %zd != %zu",
+ ret, sizeof(ev));
+ return -EFAULT;
+ }
+
+ switch (ev.type) {
+ case UHID_START:
+ pthread_mutex_lock(&uhid_started_mtx);
+ pthread_cond_signal(&uhid_started);
+ pthread_mutex_unlock(&uhid_started_mtx);
+
+ UHID_LOG("UHID_START from uhid-dev");
+ break;
+ case UHID_STOP:
+ uhid_stopped = true;
+
+ UHID_LOG("UHID_STOP from uhid-dev");
+ break;
+ case UHID_OPEN:
+ UHID_LOG("UHID_OPEN from uhid-dev");
+ break;
+ case UHID_CLOSE:
+ UHID_LOG("UHID_CLOSE from uhid-dev");
+ break;
+ case UHID_OUTPUT:
+ UHID_LOG("UHID_OUTPUT from uhid-dev");
+
+ pthread_mutex_lock(&uhid_output_mtx);
+ memcpy(output_report,
+ ev.u.output.data,
+ min(ev.u.output.size, sizeof(output_report)));
+ pthread_cond_signal(&uhid_output_cond);
+ pthread_mutex_unlock(&uhid_output_mtx);
+ break;
+ case UHID_GET_REPORT:
+ UHID_LOG("UHID_GET_REPORT from uhid-dev");
+
+ answer.type = UHID_GET_REPORT_REPLY;
+ answer.u.get_report_reply.id = ev.u.get_report.id;
+ answer.u.get_report_reply.err = ev.u.get_report.rnum == 1 ? 0 : -EIO;
+ answer.u.get_report_reply.size = sizeof(feature_data);
+ memcpy(answer.u.get_report_reply.data, feature_data, sizeof(feature_data));
+
+ uhid_write(_metadata, fd, &answer);
+
+ break;
+ case UHID_SET_REPORT:
+ UHID_LOG("UHID_SET_REPORT from uhid-dev");
+ break;
+ default:
+ TH_LOG("Invalid event from uhid-dev: %u", ev.type);
+ }
+
+ return 0;
+}
+
+struct uhid_thread_args {
+ int fd;
+ struct __test_metadata *_metadata;
+};
+static void *uhid_read_events_thread(void *arg)
+{
+ struct uhid_thread_args *args = (struct uhid_thread_args *)arg;
+ struct __test_metadata *_metadata = args->_metadata;
+ struct pollfd pfds[1];
+ int fd = args->fd;
+ int ret = 0;
+
+ pfds[0].fd = fd;
+ pfds[0].events = POLLIN;
+
+ uhid_stopped = false;
+
+ while (!uhid_stopped) {
+ ret = poll(pfds, 1, 100);
+ if (ret < 0) {
+ TH_LOG("Cannot poll for fds: %m");
+ break;
+ }
+ if (pfds[0].revents & POLLIN) {
+ ret = uhid_event(_metadata, fd);
+ if (ret)
+ break;
+ }
+ }
+
+ return (void *)(long)ret;
+}
+
+static int uhid_start_listener(struct __test_metadata *_metadata, pthread_t *tid, int uhid_fd)
+{
+ struct uhid_thread_args args = {
+ .fd = uhid_fd,
+ ._metadata = _metadata,
+ };
+ int err;
+
+ pthread_mutex_lock(&uhid_started_mtx);
+ err = pthread_create(tid, NULL, uhid_read_events_thread, (void *)&args);
+ ASSERT_EQ(0, err) {
+ TH_LOG("Could not start the uhid thread: %d", err);
+ pthread_mutex_unlock(&uhid_started_mtx);
+ close(uhid_fd);
+ return -EIO;
+ }
+ pthread_cond_wait(&uhid_started, &uhid_started_mtx);
+ pthread_mutex_unlock(&uhid_started_mtx);
+
+ return 0;
+}
+
+static int uhid_send_event(struct __test_metadata *_metadata, int fd, __u8 *buf, size_t size)
+{
+ struct uhid_event ev;
+
+ if (size > sizeof(ev.u.input.data))
+ return -E2BIG;
+
+ memset(&ev, 0, sizeof(ev));
+ ev.type = UHID_INPUT2;
+ ev.u.input2.size = size;
+
+ memcpy(ev.u.input2.data, buf, size);
+
+ return uhid_write(_metadata, fd, &ev);
+}
+
+static int setup_uhid(struct __test_metadata *_metadata, int rand_nb)
+{
+ int fd;
+ const char *path = "/dev/uhid";
+ int ret;
+
+ fd = open(path, O_RDWR | O_CLOEXEC);
+ ASSERT_GE(fd, 0) TH_LOG("open uhid-cdev failed; %d", fd);
+
+ ret = uhid_create(_metadata, fd, rand_nb);
+ ASSERT_EQ(0, ret) {
+ TH_LOG("create uhid device failed: %d", ret);
+ close(fd);
+ }
+
+ return fd;
+}
+
+static bool match_sysfs_device(int dev_id, const char *workdir, struct dirent *dir)
+{
+ const char *target = "0003:0001:0A37.*";
+ char phys[512];
+ char uevent[1024];
+ char temp[512];
+ int fd, nread;
+ bool found = false;
+
+ if (fnmatch(target, dir->d_name, 0))
+ return false;
+
+ /* we found the correct VID/PID, now check for phys */
+ sprintf(uevent, "%s/%s/uevent", workdir, dir->d_name);
+
+ fd = open(uevent, O_RDONLY | O_NONBLOCK);
+ if (fd < 0)
+ return false;
+
+ sprintf(phys, "PHYS=%d", dev_id);
+
+ nread = read(fd, temp, ARRAY_SIZE(temp));
+ if (nread > 0 && (strstr(temp, phys)) != NULL)
+ found = true;
+
+ close(fd);
+
+ return found;
+}
+
+static int get_hid_id(int dev_id)
+{
+ const char *workdir = "/sys/devices/virtual/misc/uhid";
+ const char *str_id;
+ DIR *d;
+ struct dirent *dir;
+ int found = -1, attempts = 3;
+
+ /* it would be nice to be able to use nftw, but the no_alu32 target doesn't support it */
+
+ while (found < 0 && attempts > 0) {
+ attempts--;
+ d = opendir(workdir);
+ if (d) {
+ while ((dir = readdir(d)) != NULL) {
+ if (!match_sysfs_device(dev_id, workdir, dir))
+ continue;
+
+ str_id = dir->d_name + sizeof("0003:0001:0A37.");
+ found = (int)strtol(str_id, NULL, 16);
+
+ break;
+ }
+ closedir(d);
+ }
+ if (found < 0)
+ usleep(100000);
+ }
+
+ return found;
+}
+
+static int get_hidraw(int dev_id)
+{
+ const char *workdir = "/sys/devices/virtual/misc/uhid";
+ char sysfs[1024];
+ DIR *d, *subd;
+ struct dirent *dir, *subdir;
+ int i, found = -1;
+
+ /* retry 5 times in case the system is loaded */
+ for (i = 5; i > 0; i--) {
+ usleep(10);
+ d = opendir(workdir);
+
+ if (!d)
+ continue;
+
+ while ((dir = readdir(d)) != NULL) {
+ if (!match_sysfs_device(dev_id, workdir, dir))
+ continue;
+
+ sprintf(sysfs, "%s/%s/hidraw", workdir, dir->d_name);
+
+ subd = opendir(sysfs);
+ if (!subd)
+ continue;
+
+ while ((subdir = readdir(subd)) != NULL) {
+ if (fnmatch("hidraw*", subdir->d_name, 0))
+ continue;
+
+ found = atoi(subdir->d_name + strlen("hidraw"));
+ }
+
+ closedir(subd);
+
+ if (found > 0)
+ break;
+ }
+ closedir(d);
+ }
+
+ return found;
+}
+
+static int open_hidraw(int dev_id)
+{
+ int hidraw_number;
+ char hidraw_path[64] = { 0 };
+
+ hidraw_number = get_hidraw(dev_id);
+ if (hidraw_number < 0)
+ return hidraw_number;
+
+ /* open hidraw node to check the other side of the pipe */
+ sprintf(hidraw_path, "/dev/hidraw%d", hidraw_number);
+ return open(hidraw_path, O_RDWR | O_NONBLOCK);
+}
+
+FIXTURE(hidraw) {
+ int dev_id;
+ int uhid_fd;
+ int hidraw_fd;
+ int hid_id;
+ pthread_t tid;
+};
+static void close_hidraw(FIXTURE_DATA(hidraw) * self)
+{
+ if (self->hidraw_fd)
+ close(self->hidraw_fd);
+ self->hidraw_fd = 0;
+}
+
+FIXTURE_TEARDOWN(hidraw) {
+ void *uhid_err;
+
+ uhid_destroy(_metadata, self->uhid_fd);
+
+ close_hidraw(self);
+ pthread_join(self->tid, &uhid_err);
+}
+#define TEARDOWN_LOG(fmt, ...) do { \
+ TH_LOG(fmt, ##__VA_ARGS__); \
+ hidraw_teardown(_metadata, self, variant); \
+} while (0)
+
+FIXTURE_SETUP(hidraw)
+{
+ time_t t;
+ int err;
+
+ /* initialize random number generator */
+ srand((unsigned int)time(&t));
+
+ self->dev_id = rand() % 1024;
+
+ self->uhid_fd = setup_uhid(_metadata, self->dev_id);
+
+ /* locate the uev, self, variant);ent file of the created device */
+ self->hid_id = get_hid_id(self->dev_id);
+ ASSERT_GT(self->hid_id, 0)
+ TEARDOWN_LOG("Could not locate uhid device id: %d", self->hid_id);
+
+ err = uhid_start_listener(_metadata, &self->tid, self->uhid_fd);
+ ASSERT_EQ(0, err) TEARDOWN_LOG("could not start udev listener: %d", err);
+
+ self->hidraw_fd = open_hidraw(self->dev_id);
+ ASSERT_GE(self->hidraw_fd, 0) TH_LOG("open_hidraw");
+}
+
+/*
+ * A simple test to see if the fixture is working fine.
+ * If this fails, none of the other tests will pass.
+ */
+TEST_F(hidraw, test_create_uhid)
+{
+}
+
+/*
+ * Inject one event in the uhid device,
+ * check that we get the same data through hidraw
+ */
+TEST_F(hidraw, raw_event)
+{
+ __u8 buf[10] = {0};
+ int err;
+
+ /* inject one event */
+ buf[0] = 1;
+ buf[1] = 42;
+ uhid_send_event(_metadata, self->uhid_fd, buf, 6);
+
+ /* read the data from hidraw */
+ memset(buf, 0, sizeof(buf));
+ err = read(self->hidraw_fd, buf, sizeof(buf));
+ ASSERT_EQ(err, 6) TH_LOG("read_hidraw");
+ ASSERT_EQ(buf[0], 1);
+ ASSERT_EQ(buf[1], 42);
+}
+
+int main(int argc, char **argv)
+{
+ return test_harness_run(argc, argv);
+}
--
2.46.0
^ permalink raw reply related
* [PATCH v4 1/3] HID: hidraw: add HIDIOCREVOKE ioctl
From: bentiss @ 2024-08-26 15:47 UTC (permalink / raw)
To: Jiri Kosina, Shuah Khan, Peter Hutterer
Cc: linux-input, linux-kernel, linux-kselftest, Benjamin Tissoires
In-Reply-To: <20240827-hidraw-revoke-v4-0-88c6795bf867@kernel.org>
From: Peter Hutterer <peter.hutterer@who-t.net>
There is a need for userspace applications to open HID devices directly.
Use-cases include configuration of gaming mice or direct access to
joystick devices. The latter is currently handled by the uaccess tag in
systemd, other devices include more custom/local configurations or just
sudo.
A better approach is what we already have for evdev devices: give the
application a file descriptor and revoke it when it may no longer access
that device.
This patch is the hidraw equivalent to the EVIOCREVOKE ioctl, see
commit c7dc65737c9a ("Input: evdev - add EVIOCREVOKE ioctl") for full
details.
An MR for systemd-logind has been filed here:
https://github.com/systemd/systemd/pull/33970
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
First version of the patch:
https://patchwork.kernel.org/project/linux-input/patch/YmEAPZKDisM2HAsG@quokka/
Changes to v1:
- add the hidraw_is_revoked and hidraw_open_errno weak functions as
suggested by Benjamin
Changes to v2:
- use __bpf_hook_start/end to silence compiler warnings (see kernel
test bot)
Changes to v3 (bentiss):
- drop the BPF related changes, the API is not clear ATM, and it's wrong
in v2, as ERROR_INJECTION should not be used for normal fmodret.
- drop hidraw_open_errno() new function. The API seems interesting, but
nothing is really ready, so let's not add a useless API for nothing.
---
drivers/hid/hidraw.c | 39 +++++++++++++++++++++++++++++++++++----
include/linux/hidraw.h | 1 +
include/uapi/linux/hidraw.h | 1 +
3 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 716294e40e8a..c887f48756f4 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -38,12 +38,20 @@ static const struct class hidraw_class = {
static struct hidraw *hidraw_table[HIDRAW_MAX_DEVICES];
static DECLARE_RWSEM(minors_rwsem);
+static inline bool hidraw_is_revoked(struct hidraw_list *list)
+{
+ return list->revoked;
+}
+
static ssize_t hidraw_read(struct file *file, char __user *buffer, size_t count, loff_t *ppos)
{
struct hidraw_list *list = file->private_data;
int ret = 0, len;
DECLARE_WAITQUEUE(wait, current);
+ if (hidraw_is_revoked(list))
+ return -ENODEV;
+
mutex_lock(&list->read_mutex);
while (ret == 0) {
@@ -161,9 +169,13 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
static ssize_t hidraw_write(struct file *file, const char __user *buffer, size_t count, loff_t *ppos)
{
+ struct hidraw_list *list = file->private_data;
ssize_t ret;
down_read(&minors_rwsem);
- ret = hidraw_send_report(file, buffer, count, HID_OUTPUT_REPORT);
+ if (hidraw_is_revoked(list))
+ ret = -ENODEV;
+ else
+ ret = hidraw_send_report(file, buffer, count, HID_OUTPUT_REPORT);
up_read(&minors_rwsem);
return ret;
}
@@ -256,7 +268,7 @@ static __poll_t hidraw_poll(struct file *file, poll_table *wait)
poll_wait(file, &list->hidraw->wait, wait);
if (list->head != list->tail)
mask |= EPOLLIN | EPOLLRDNORM;
- if (!list->hidraw->exist)
+ if (!list->hidraw->exist || hidraw_is_revoked(list))
mask |= EPOLLERR | EPOLLHUP;
return mask;
}
@@ -320,6 +332,9 @@ static int hidraw_fasync(int fd, struct file *file, int on)
{
struct hidraw_list *list = file->private_data;
+ if (hidraw_is_revoked(list))
+ return -ENODEV;
+
return fasync_helper(fd, file, on, &list->fasync);
}
@@ -372,6 +387,13 @@ static int hidraw_release(struct inode * inode, struct file * file)
return 0;
}
+static int hidraw_revoke(struct hidraw_list *list)
+{
+ list->revoked = true;
+
+ return 0;
+}
+
static long hidraw_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
@@ -379,11 +401,12 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
unsigned int minor = iminor(inode);
long ret = 0;
struct hidraw *dev;
+ struct hidraw_list *list = file->private_data;
void __user *user_arg = (void __user*) arg;
down_read(&minors_rwsem);
dev = hidraw_table[minor];
- if (!dev || !dev->exist) {
+ if (!dev || !dev->exist || hidraw_is_revoked(list)) {
ret = -ENODEV;
goto out;
}
@@ -421,6 +444,14 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
ret = -EFAULT;
break;
}
+ case HIDIOCREVOKE:
+ {
+ if (user_arg)
+ ret = -EINVAL;
+ else
+ ret = hidraw_revoke(list);
+ break;
+ }
default:
{
struct hid_device *hid = dev->hid;
@@ -527,7 +558,7 @@ int hidraw_report_event(struct hid_device *hid, u8 *data, int len)
list_for_each_entry(list, &dev->list, node) {
int new_head = (list->head + 1) & (HIDRAW_BUFFER_SIZE - 1);
- if (new_head == list->tail)
+ if (hidraw_is_revoked(list) || new_head == list->tail)
continue;
if (!(list->buffer[list->head].value = kmemdup(data, len, GFP_ATOMIC))) {
diff --git a/include/linux/hidraw.h b/include/linux/hidraw.h
index cd67f4ca5599..18fd30a288de 100644
--- a/include/linux/hidraw.h
+++ b/include/linux/hidraw.h
@@ -32,6 +32,7 @@ struct hidraw_list {
struct hidraw *hidraw;
struct list_head node;
struct mutex read_mutex;
+ bool revoked;
};
#ifdef CONFIG_HIDRAW
diff --git a/include/uapi/linux/hidraw.h b/include/uapi/linux/hidraw.h
index 33ebad81720a..d5ee269864e0 100644
--- a/include/uapi/linux/hidraw.h
+++ b/include/uapi/linux/hidraw.h
@@ -46,6 +46,7 @@ struct hidraw_devinfo {
/* The first byte of SOUTPUT and GOUTPUT is the report number */
#define HIDIOCSOUTPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0B, len)
#define HIDIOCGOUTPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0C, len)
+#define HIDIOCREVOKE _IOW('H', 0x0D, int) /* Revoke device access */
#define HIDRAW_FIRST_MINOR 0
#define HIDRAW_MAX_DEVICES 64
--
2.46.0
^ permalink raw reply related
* [PATCH v4 0/3] HID: hidraw: HIDIOCREVOKE introduction
From: bentiss @ 2024-08-26 15:47 UTC (permalink / raw)
To: Jiri Kosina, Shuah Khan, Peter Hutterer
Cc: linux-input, linux-kernel, linux-kselftest, Benjamin Tissoires
The is the v4 of the HIDIOCREVOKE patches.
Link to v3: https://lore.kernel.org/all/20240812052753.GA478917@quokka/
After a small discussion with Peter, we decided to:
- drop the BPF hooks that are problematic (Linus doesn't want
"ALLOW_ERROR_INJECTION" to be used as "normal" fmodret bpf hooks)
- punt those BPF hooks later once we get the API right
- I'll be the one sending that new version, given that it's easier for
me ATM
For testing the patch, and for convenience, I added a new selftest
program that can test this new ioctl. This will also allow us to
integrate the (future) BPF hooks and show how this should be used.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
Benjamin Tissoires (2):
selftests/hid: Add initial hidraw tests skeleton
selftests/hid: Add HIDIOCREVOKE tests
Peter Hutterer (1):
HID: hidraw: add HIDIOCREVOKE ioctl
drivers/hid/hidraw.c | 39 +-
include/linux/hidraw.h | 1 +
include/uapi/linux/hidraw.h | 1 +
tools/testing/selftests/hid/.gitignore | 1 +
tools/testing/selftests/hid/Makefile | 2 +-
tools/testing/selftests/hid/hidraw.c | 665 +++++++++++++++++++++++++++++++++
6 files changed, 704 insertions(+), 5 deletions(-)
---
base-commit: 6e4436539ae182dc86d57d13849862bcafaa4709
change-id: 20240826-hidraw-revoke-0a02ebb21743
Best regards,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply
* Re: [PATCH v25 01/33] xhci: add helper to stop endpoint and wait for completion
From: Wesley Cheng @ 2024-08-26 15:11 UTC (permalink / raw)
To: Pierre-Louis Bossart, srinivas.kandagatla, mathias.nyman, perex,
conor+dt, dmitry.torokhov, corbet, broonie, lgirdwood, tiwai,
krzk+dt, Thinh.Nguyen, bgoswami, robh, gregkh
Cc: linux-kernel, devicetree, linux-sound, linux-usb, linux-input,
linux-arm-msm, linux-doc, alsa-devel, Mathias Nyman
In-Reply-To: <9f25b900-ae1c-41af-a380-ac5e00860283@linux.intel.com>
Hi Pierre,
On 8/26/2024 1:48 AM, Pierre-Louis Bossart wrote:
>
> On 8/23/24 22:00, Wesley Cheng wrote:
>> From: Mathias Nyman <mathias.nyman@linux.intel.com>
>>
>> Expose xhci_stop_endpoint_sync() which is a synchronous variant of
>> xhci_queue_stop_endpoint(). This is useful for client drivers that are
>> using the secondary interrupters, and need to stop/clean up the current
>> session. The stop endpoint command handler will also take care of cleaning
>> up the ring.
>>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>> drivers/usb/host/xhci.c | 39 +++++++++++++++++++++++++++++++++++++++
>> drivers/usb/host/xhci.h | 2 ++
>> 2 files changed, 41 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index 37eb37b0affa..3a051ed32907 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -2784,6 +2784,45 @@ static int xhci_reserve_bandwidth(struct xhci_hcd *xhci,
>> return -ENOMEM;
>> }
>>
>> +/*
>> + * Synchronous XHCI stop endpoint helper. Issues the stop endpoint command and
>> + * waits for the command completion before returning.
>> + */
>> +int xhci_stop_endpoint_sync(struct xhci_hcd *xhci, struct xhci_virt_ep *ep, int suspend,
>> + gfp_t gfp_flags)
>> +{
>> + struct xhci_command *command;
>> + unsigned long flags;
>> + int ret;
>> +
>> + command = xhci_alloc_command(xhci, true, gfp_flags);
>> + if (!command)
>> + return -ENOMEM;
>> +
>> + spin_lock_irqsave(&xhci->lock, flags);
>> + ret = xhci_queue_stop_endpoint(xhci, command, ep->vdev->slot_id,
>> + ep->ep_index, suspend);
>> + if (ret < 0) {
>> + spin_unlock_irqrestore(&xhci->lock, flags);
>> + goto out;
>> + }
>> +
>> + xhci_ring_cmd_db(xhci);
>> + spin_unlock_irqrestore(&xhci->lock, flags);
>> +
>> + wait_for_completion(command->completion);
>> +
>> + if (command->status == COMP_COMMAND_ABORTED ||
>> + command->status == COMP_COMMAND_RING_STOPPED) {
>> + xhci_warn(xhci, "Timeout while waiting for stop endpoint command\n");
> nit-pick: is this really a timeout? In that case you would have used
> wait_for_completion_timeout(), no?
With respects to the xHCI command implementation, every time a command is queued to the host controller, it arms timer (xhci->cmd_timer) that is used to handle the timeout conditions. This is the reason for not using the _timeout() variant, as we can let the xHCI command timeout handler do the cleanup and stopping of the HCD. (marking as dead) It will also ensure that any completion events are completed as part of the timeout handler as well (xhci_handle_command_timeout() --> xhci_abort_cmd_ring())
Thanks
Wesley Cheng
>> + ret = -ETIME;
>> + }
>> +out:
>> + xhci_free_command(xhci, command);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(xhci_stop_endpoint_sync);
>>
>> /* Issue a configure endpoint command or evaluate context command
>> * and wait for it to finish.
>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index 30415158ed3c..1c6126ed55b0 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -1914,6 +1914,8 @@ void xhci_ring_doorbell_for_active_rings(struct xhci_hcd *xhci,
>> void xhci_cleanup_command_queue(struct xhci_hcd *xhci);
>> void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring);
>> unsigned int count_trbs(u64 addr, u64 len);
>> +int xhci_stop_endpoint_sync(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
>> + int suspend, gfp_t gfp_flags);
>>
>> /* xHCI roothub code */
>> void xhci_set_link_state(struct xhci_hcd *xhci, struct xhci_port *port,
^ permalink raw reply
* Re: [PATCH v5 1/1] dt-bindings: input: touchscreen: convert ads7846.txt to yaml
From: Rob Herring @ 2024-08-26 13:57 UTC (permalink / raw)
To: Frank Li
Cc: Dmitry Torokhov, Krzysztof Kozlowski, Conor Dooley, Linus Walleij,
Marek Vasut, Mark Hasemeyer, Alexander Stein,
open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)...,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list, imx
In-Reply-To: <20240820163710.448302-1-Frank.Li@nxp.com>
On Tue, Aug 20, 2024 at 12:37:03PM -0400, Frank Li wrote:
> Convert binding doc ads7846.txt to yaml format.
> Additional change:
> - add ref to touchscreen.yaml and spi-peripheral-props.yaml.
> - use common node name touchscreen.
> - sort ti properties alphabetically.
> - sort common properties alphabetically.
> - sort compatible string alphabetically.
> - remove vcc-supply from required list.
> - deprecated ti,x-min, ti,y,min
ti,y-min?
>
> Fix below warning: arch/arm64/boot/dts/freescale/imx8mm-var-som-symphony.dtb: touchscreen@0:
> ti,x-min: b'\x00}' is not of type 'object', 'array', 'boolean', 'null'
>
> Reviewed-by: Marek Vasut <marex@denx.de>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Change from v4 to v5
> - Add Reviewed-by: Marek Vasut <marex@denx.de>
> - Start sentence with uppercase letter
>
> Change from v3 to v4
> - new line for all descrptions
> - add . after sentense.
>
> Change from v2 to v3
> - Remove u16(u32) in descriptions
> - deprecated ti,x-min and ti, y-min
>
> Change from v1 to v2
> - sort properties, by 3 group:
> 1. General (compatible, reg, interrupt)
> 2. Common properties
> 3. ti properties
> - sort maintainers name alphabetically.
> - uint16 have to be kept because default is uint32
> - remove vcc-supply from required list
> - remove unfinished sentence "all mandatory properties described in"
> because it refer to /schemas/spi/spi-peripheral-props.yaml#
> - fix make refcheckdoc error
> ---
> .../bindings/input/touchscreen/ads7846.txt | 107 ----------
> .../input/touchscreen/ti,ads7843.yaml | 182 ++++++++++++++++++
> .../bindings/power/wakeup-source.txt | 2 +-
> 3 files changed, 183 insertions(+), 108 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/input/touchscreen/ads7846.txt
> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/ti,ads7843.yaml
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti,ads7843.yaml b/Documentation/devicetree/bindings/input/touchscreen/ti,ads7843.yaml
> new file mode 100644
> index 0000000000000..92d5e7d3b1ffd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/ti,ads7843.yaml
> @@ -0,0 +1,182 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/touchscreen/ti,ads7843.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI's SPI driven touch screen controllers.
Drop period.
> +
> +maintainers:
> + - Alexander Stein <alexander.stein@ew.tq-group.com>
> + - Dmitry Torokhov <dmitry.torokhov@gmail.com>
> + - Marek Vasut <marex@denx.de>
> +
> +description:
> + Device tree bindings for TI's ADS7843, ADS7845, ADS7846, ADS7873, TSC2046
Drop 'Device tree bindings for '.
> + SPI driven touch screen controllers.
> +
> +properties:
> + compatible:
> + enum:
> + - ti,ads7843
> + - ti,ads7845
> + - ti,ads7846
> + - ti,ads7873
> + - ti,tsc2046
> +
> + interrupts:
> + maxItems: 1
> +
> + pendown-gpio:
> + description:
> + GPIO handle describing the pin the !PENIRQ line is connected to.
maxItems: 1
> +
> + vcc-supply:
> + description:
> + A regulator node for the supply voltage.
> +
> + wakeup-source: true
> +
> + ti,debounce-max:
> + deprecated: true
> + $ref: /schemas/types.yaml#/definitions/uint16
> + description:
> + Max number of additional readings per sample.
> +
> + ti,debounce-rep:
> + $ref: /schemas/types.yaml#/definitions/uint16
> + description:
> + Additional consecutive good readings required after the first two.
> +
> + ti,debounce-tol:
> + $ref: /schemas/types.yaml#/definitions/uint16
> + description:
> + Tolerance used for filtering.
> +
> + ti,hsync-gpios:
> + description:
> + GPIO line to poll for hsync.
maxItems: 1
With those fixes,
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v25 00/33] Introduce QC USB SND audio offloading support
From: Pierre-Louis Bossart @ 2024-08-26 9:28 UTC (permalink / raw)
To: Wesley Cheng, srinivas.kandagatla, mathias.nyman, perex, conor+dt,
dmitry.torokhov, corbet, broonie, lgirdwood, tiwai, krzk+dt,
Thinh.Nguyen, bgoswami, robh, gregkh
Cc: linux-kernel, devicetree, linux-sound, linux-usb, linux-input,
linux-arm-msm, linux-doc, alsa-devel
In-Reply-To: <20240823200101.26755-1-quic_wcheng@quicinc.com>
> Changelog
> --------------------------------------------
> Changes in v25:
> - Cleanups on typos mentioned within the xHCI layers
> - Modified the xHCI interrupter search if clients specify interrupter index
> - Moved mixer_usb_offload into its own module, so that other vendor offload USB
> modules can utilize it also.
> - Added support for USB audio devices that may have multiple PCM streams, as
> previous implementation only assumed a single PCM device. SOC USB will be
> able to handle an array of PCM indexes supported by the USB audio device.
> - Added some additional checks in the QC USB offload driver to check that device
> has at least one playback stream before allowing to bind
> - Reordered DT bindings to fix the error found by Rob's bot. The patch that
> added USB_RX was after the example was updated.
> - Updated comments within SOC USB to clarify terminology and to keep it consistent
> - Added SND_USB_JACK type for notifying of USB device audio connections
I went through the code and didn't find anything that looked like a
major blocker. There are still a number of cosmetic things you'd want to
fix such as using checkpatch.pl --strict --codespell to look for obvious
style issues and typos, see selection below. git am also complains about
EOF lines.
Overall this is starting to look good and ready for other reviewers to
look at.
WARNING: 'reaquire' may be misspelled - perhaps 'reacquire'?
#54: FILE: drivers/usb/host/xhci-ring.c:3037:
+ * for non OS owned interrupter event ring. It may drop and reaquire
xhci->lock
^^^^^^^^
WARNING: 'compliation' may be misspelled - perhaps 'compilation'?
#16:
module compliation added by Wesley Cheng to complete original concept code
^^^^^^^^^^^
CHECK: Prefer kzalloc(sizeof(*sgt)...) over kzalloc(sizeof(struct
sg_table)...)
#105: FILE: drivers/usb/host/xhci-sideband.c:35:
+ sgt = kzalloc(sizeof(struct sg_table), GFP_KERNEL);
CHECK: struct mutex definition without comment
#557: FILE: include/linux/usb/xhci-sideband.h:35:
+ struct mutex mutex;
WARNING: 'straightfoward' may be misspelled - perhaps 'straightforward'?
#22:
straightfoward, as the ASoC components have direct references to the ASoC
^^^^^^^^^^^^^^
CHECK: Unnecessary parentheses around 'card == sdev->card_idx'
#142: FILE: sound/soc/qcom/qdsp6/q6usb.c:217:
+ if ((card == sdev->card_idx) &&
+ (pcm == sdev->ppcm_idx[sdev->num_playback - 1])) {
CHECK: Unnecessary parentheses around 'pcm ==
sdev->ppcm_idx[sdev->num_playback - 1]'
#142: FILE: sound/soc/qcom/qdsp6/q6usb.c:217:
+ if ((card == sdev->card_idx) &&
+ (pcm == sdev->ppcm_idx[sdev->num_playback - 1])) {
WARNING: 'seqeunces' may be misspelled - perhaps 'sequences'?
#8:
seqeunces. This allows for platform USB SND modules to properly initialize
^^^^^^^^^
WARNING: 'exisiting' may be misspelled - perhaps 'existing'?
#12:
exisiting parameters.
^^^^^^^^^
CHECK: Please use a blank line after function/struct/union/enum declarations
#1020: FILE: sound/usb/qcom/usb_audio_qmi_v01.h:98:
+};
+#define QMI_UAUDIO_STREAM_REQ_MSG_V01_MAX_MSG_LEN 46
CHECK: Please use a blank line after function/struct/union/enum declarations
#1054: FILE: sound/usb/qcom/usb_audio_qmi_v01.h:132:
+};
+#define QMI_UAUDIO_STREAM_RESP_MSG_V01_MAX_MSG_LEN 202
CHECK: Please use a blank line after function/struct/union/enum declarations
#1081: FILE: sound/usb/qcom/usb_audio_qmi_v01.h:159:
+};
+#define QMI_UAUDIO_STREAM_IND_MSG_V01_MAX_MSG_LEN 181
CHECK: Macro argument 'n' may be better as '(n)' to avoid precedence issues
#100: FILE: sound/usb/mixer_usb_offload.c:19:
+#define PCM_IDX(n) (n & 0xffff)
CHECK: Macro argument 'n' may be better as '(n)' to avoid precedence issues
#101: FILE: sound/usb/mixer_usb_offload.c:20:
+#define CARD_IDX(n) (n >> 16)
^ permalink raw reply
* Re: [PATCH v25 31/33] ALSA: usb-audio: Add USB offload route kcontrol
From: Pierre-Louis Bossart @ 2024-08-26 9:09 UTC (permalink / raw)
To: Wesley Cheng, srinivas.kandagatla, mathias.nyman, perex, conor+dt,
dmitry.torokhov, corbet, broonie, lgirdwood, tiwai, krzk+dt,
Thinh.Nguyen, bgoswami, robh, gregkh
Cc: linux-kernel, devicetree, linux-sound, linux-usb, linux-input,
linux-arm-msm, linux-doc, alsa-devel
In-Reply-To: <20240823200101.26755-32-quic_wcheng@quicinc.com>
> +config SND_USB_OFFLOAD_MIXER
> + tristate "Qualcomm USB Audio Offload mixer control"
> + help
> + Say Y to enable the Qualcomm USB audio offloading mixer controls.
> + This exposes an USB offload capable kcontrol to signal to
> + applications about which platform sound card can support USB
> + audio offload. This can potentially be used to fetch further
> + information about the offloading status from the platform sound
> + card.
I would remove reference to Qualcomm for this Kconfig, all the code
seems generic to me? Probably a left-over from the previous version.
^ permalink raw reply
* Re: [PATCH v25 01/33] xhci: add helper to stop endpoint and wait for completion
From: Pierre-Louis Bossart @ 2024-08-26 8:48 UTC (permalink / raw)
To: Wesley Cheng, srinivas.kandagatla, mathias.nyman, perex, conor+dt,
dmitry.torokhov, corbet, broonie, lgirdwood, tiwai, krzk+dt,
Thinh.Nguyen, bgoswami, robh, gregkh
Cc: linux-kernel, devicetree, linux-sound, linux-usb, linux-input,
linux-arm-msm, linux-doc, alsa-devel, Mathias Nyman
In-Reply-To: <20240823200101.26755-2-quic_wcheng@quicinc.com>
On 8/23/24 22:00, Wesley Cheng wrote:
> From: Mathias Nyman <mathias.nyman@linux.intel.com>
>
> Expose xhci_stop_endpoint_sync() which is a synchronous variant of
> xhci_queue_stop_endpoint(). This is useful for client drivers that are
> using the secondary interrupters, and need to stop/clean up the current
> session. The stop endpoint command handler will also take care of cleaning
> up the ring.
>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
> drivers/usb/host/xhci.c | 39 +++++++++++++++++++++++++++++++++++++++
> drivers/usb/host/xhci.h | 2 ++
> 2 files changed, 41 insertions(+)
>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 37eb37b0affa..3a051ed32907 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -2784,6 +2784,45 @@ static int xhci_reserve_bandwidth(struct xhci_hcd *xhci,
> return -ENOMEM;
> }
>
> +/*
> + * Synchronous XHCI stop endpoint helper. Issues the stop endpoint command and
> + * waits for the command completion before returning.
> + */
> +int xhci_stop_endpoint_sync(struct xhci_hcd *xhci, struct xhci_virt_ep *ep, int suspend,
> + gfp_t gfp_flags)
> +{
> + struct xhci_command *command;
> + unsigned long flags;
> + int ret;
> +
> + command = xhci_alloc_command(xhci, true, gfp_flags);
> + if (!command)
> + return -ENOMEM;
> +
> + spin_lock_irqsave(&xhci->lock, flags);
> + ret = xhci_queue_stop_endpoint(xhci, command, ep->vdev->slot_id,
> + ep->ep_index, suspend);
> + if (ret < 0) {
> + spin_unlock_irqrestore(&xhci->lock, flags);
> + goto out;
> + }
> +
> + xhci_ring_cmd_db(xhci);
> + spin_unlock_irqrestore(&xhci->lock, flags);
> +
> + wait_for_completion(command->completion);
> +
> + if (command->status == COMP_COMMAND_ABORTED ||
> + command->status == COMP_COMMAND_RING_STOPPED) {
> + xhci_warn(xhci, "Timeout while waiting for stop endpoint command\n");
nit-pick: is this really a timeout? In that case you would have used
wait_for_completion_timeout(), no?
> + ret = -ETIME;
> + }
> +out:
> + xhci_free_command(xhci, command);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(xhci_stop_endpoint_sync);
>
> /* Issue a configure endpoint command or evaluate context command
> * and wait for it to finish.
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 30415158ed3c..1c6126ed55b0 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1914,6 +1914,8 @@ void xhci_ring_doorbell_for_active_rings(struct xhci_hcd *xhci,
> void xhci_cleanup_command_queue(struct xhci_hcd *xhci);
> void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring);
> unsigned int count_trbs(u64 addr, u64 len);
> +int xhci_stop_endpoint_sync(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
> + int suspend, gfp_t gfp_flags);
>
> /* xHCI roothub code */
> void xhci_set_link_state(struct xhci_hcd *xhci, struct xhci_port *port,
^ permalink raw reply
* Re: [PATCH] Input: psmouse-smbus - use guard notation when acquiring mutex
From: Markus Elfring @ 2024-08-26 9:00 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input, Benjamin Tissoires; +Cc: LKML
In-Reply-To: <ZsrAa9XcDvHeIs9T@google.com>
> This makes the code …
How do you think about to improve such a change description with imperative wordings?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc5#n94
Regards,
Markus
^ permalink raw reply
* Re: [PATCH 0/5] Remove support for platform data from matrix keypad driver
From: Linus Walleij @ 2024-08-26 8:52 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Haojian Zhuang, Daniel Mack, Robert Jarzmik, Arnd Bergmann, soc,
linux-arm-kernel, linux-kernel, linux-input
In-Reply-To: <ZsiygIj9SiP4O0OM@google.com>
On Fri, Aug 23, 2024 at 6:02 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> I'm glad that we agree that we do not want the elaborate merge process
> and instead push the changes through one tree in one shot we just need
> to decide which one - soc or input. I am fine with using either.
I'm also fine with either, but let's take the input tree because the
you're in direct control of it so it will be easier.
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH v1] Input: wistron_btns - use kmemdup_array instead of kmemdup for multiple allocation
From: Shen Lichuan @ 2024-08-26 4:52 UTC (permalink / raw)
To: mitr, dmitry.torokhov; +Cc: linux-input, linux-kernel, Shen Lichuan
Let the kmemdup_array() take care about multiplication
and possible overflows.
Using kmemdup_array() is more appropriate and makes the code
easier to audit.
Signed-off-by: Shen Lichuan <shenlichuan@vivo.com>
---
drivers/input/misc/wistron_btns.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/misc/wistron_btns.c b/drivers/input/misc/wistron_btns.c
index 5c4956678cd0..907b1a4e6633 100644
--- a/drivers/input/misc/wistron_btns.c
+++ b/drivers/input/misc/wistron_btns.c
@@ -990,8 +990,8 @@ static int __init copy_keymap(void)
for (key = keymap; key->type != KE_END; key++)
length++;
- new_keymap = kmemdup(keymap, length * sizeof(struct key_entry),
- GFP_KERNEL);
+ new_keymap = kmemdup_array(keymap, length, sizeof(struct key_entry),
+ GFP_KERNEL);
if (!new_keymap)
return -ENOMEM;
--
2.17.1
^ permalink raw reply related
* [PATCH v6 5/7] hwmon: add driver for the hwmon parts of qnap-mcu devices
From: Heiko Stuebner @ 2024-08-25 20:32 UTC (permalink / raw)
To: lee
Cc: robh, krzk+dt, conor+dt, jdelvare, linux, heiko, dmitry.torokhov,
pavel, ukleinek, devicetree, linux-kernel, linux-hwmon,
linux-arm-kernel, linux-rockchip, linux-input, linux-leds
In-Reply-To: <20240825203235.1122198-1-heiko@sntech.de>
The MCU can be found on network-attached-storage devices made by QNAP
and provides access to fan control including reading back its RPM as
well as reading the temperature of the NAS case.
Acked-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/qnap-mcu-hwmon.rst | 27 ++
MAINTAINERS | 1 +
drivers/hwmon/Kconfig | 12 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/qnap-mcu-hwmon.c | 364 +++++++++++++++++++++++++
6 files changed, 406 insertions(+)
create mode 100644 Documentation/hwmon/qnap-mcu-hwmon.rst
create mode 100644 drivers/hwmon/qnap-mcu-hwmon.c
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 913c11390a45..7adbe23f0659 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -199,6 +199,7 @@ Hardware Monitoring Kernel Drivers
pxe1610
pwm-fan
q54sj108a2
+ qnap-mcu-hwmon
raspberrypi-hwmon
sbrmi
sbtsi_temp
diff --git a/Documentation/hwmon/qnap-mcu-hwmon.rst b/Documentation/hwmon/qnap-mcu-hwmon.rst
new file mode 100644
index 000000000000..83407e3408f2
--- /dev/null
+++ b/Documentation/hwmon/qnap-mcu-hwmon.rst
@@ -0,0 +1,27 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver qnap-mcu-hwmon
+============================
+
+This driver enables the use of the hardware monitoring and fan control
+of the MCU used on some QNAP network attached storage devices.
+
+Author: Heiko Stuebner <heiko@sntech.de>
+
+Description
+-----------
+
+The driver implements a simple interface for driving the fan controlled by
+setting its PWM output value and exposes the fan rpm and case-temperature
+to user space through hwmon's sysfs interface.
+
+The fan rotation speed returned via the optional 'fan1_input' is calculated
+inside the MCU device.
+
+The driver provides the following sensor accesses in sysfs:
+
+=============== ======= =======================================================
+fan1_input ro fan tachometer speed in RPM
+pwm1 rw relative speed (0-255), 255=max. speed.
+temp1_input ro Measured temperature in millicelsius
+=============== ======= =======================================================
diff --git a/MAINTAINERS b/MAINTAINERS
index a41906b3caa3..d500a771ad8a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18657,6 +18657,7 @@ F: drivers/media/tuners/qm1d1c0042*
QNAP MCU DRIVER
M: Heiko Stuebner <heiko@sntech.de>
S: Maintained
+F: drivers/hwmon/qnap-mcu-hwmon.c
F: drivers/input/misc/qnap-mcu-input.c
F: drivers/leds/leds-qnap-mcu.c
F: drivers/mfd/qnap-mcu.c
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index b60fe2e58ad6..0118ad91057e 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1793,6 +1793,18 @@ config SENSORS_PWM_FAN
This driver can also be built as a module. If so, the module
will be called pwm-fan.
+config SENSORS_QNAP_MCU_HWMON
+ tristate "QNAP MCU hardware monitoring"
+ depends on MFD_QNAP_MCU
+ depends on THERMAL || THERMAL=n
+ help
+ Say yes here to enable support for fan and temperature sensor
+ connected to a QNAP MCU, as found in a number of QNAP network
+ attached storage devices.
+
+ This driver can also be built as a module. If so, the module
+ will be called qnap-mcu-hwmon.
+
config SENSORS_RASPBERRYPI_HWMON
tristate "Raspberry Pi voltage monitor"
depends on RASPBERRYPI_FIRMWARE || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index b1c7056c37db..d60f46a03cc9 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -187,6 +187,7 @@ obj-$(CONFIG_SENSORS_POWERZ) += powerz.o
obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
obj-$(CONFIG_SENSORS_PT5161L) += pt5161l.o
obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
+obj-$(CONFIG_SENSORS_QNAP_MCU_HWMON) += qnap-mcu-hwmon.o
obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
obj-$(CONFIG_SENSORS_SBTSI) += sbtsi_temp.o
obj-$(CONFIG_SENSORS_SBRMI) += sbrmi.o
diff --git a/drivers/hwmon/qnap-mcu-hwmon.c b/drivers/hwmon/qnap-mcu-hwmon.c
new file mode 100644
index 000000000000..8fe6310a1b59
--- /dev/null
+++ b/drivers/hwmon/qnap-mcu-hwmon.c
@@ -0,0 +1,364 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * Driver for hwmon elements found on QNAP-MCU devices
+ *
+ * Copyright (C) 2024 Heiko Stuebner <heiko@sntech.de>
+ */
+
+#include <linux/fwnode.h>
+#include <linux/hwmon.h>
+#include <linux/mfd/qnap-mcu.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/thermal.h>
+
+struct qnap_mcu_hwmon {
+ struct qnap_mcu *mcu;
+ struct device *dev;
+
+ unsigned int pwm_min;
+ unsigned int pwm_max;
+
+ struct fwnode_handle *fan_node;
+ unsigned int fan_state;
+ unsigned int fan_max_state;
+ unsigned int *fan_cooling_levels;
+
+ struct thermal_cooling_device *cdev;
+ struct hwmon_chip_info info;
+};
+
+static int qnap_mcu_hwmon_get_rpm(struct qnap_mcu_hwmon *hwm)
+{
+ static const u8 cmd[] = { 0x40, 0x46, 0x41 }; /* @, F, A */
+ u8 reply[6];
+ int ret;
+
+ /* poll the fan rpm */
+ ret = qnap_mcu_exec(hwm->mcu, cmd, sizeof(cmd), reply, sizeof(reply));
+ if (ret)
+ return ret;
+
+ /* First 2 bytes must mirror the sent command */
+ if (memcmp(cmd, reply, 2))
+ return -EIO;
+
+ return reply[4] * 30;
+}
+
+static int qnap_mcu_hwmon_get_pwm(struct qnap_mcu_hwmon *hwm)
+{
+ static const u8 cmd[] = { 0x40, 0x46, 0x5a, 0x30 }; /* @, F, Z, 0 = fan-id? */
+ u8 reply[4];
+ int ret;
+
+ /* poll the fan pwm */
+ ret = qnap_mcu_exec(hwm->mcu, cmd, sizeof(cmd), reply, sizeof(reply));
+ if (ret)
+ return ret;
+
+ /* First 3 bytes must mirror the sent command */
+ if (memcmp(cmd, reply, 3))
+ return -EIO;
+
+ return reply[3];
+}
+
+static int qnap_mcu_hwmon_set_pwm(struct qnap_mcu_hwmon *hwm, u8 pwm)
+{
+ const u8 cmd[] = { 0x40, 0x46, 0x57, 0x30, pwm }; /* @, F, W, 0 = fan-id, pwm */
+
+ /* set the fan pwm */
+ return qnap_mcu_exec_with_ack(hwm->mcu, cmd, sizeof(cmd));
+}
+
+static int qnap_mcu_hwmon_get_temp(struct qnap_mcu_hwmon *hwm)
+{
+ static const u8 cmd[] = { 0x40, 0x54, 0x33 }; /* @, T, 3 */
+ u8 reply[4];
+ int ret;
+
+ /* poll the fan rpm */
+ ret = qnap_mcu_exec(hwm->mcu, cmd, sizeof(cmd), reply, sizeof(reply));
+ if (ret)
+ return ret;
+
+ /* First bytes must mirror the sent command */
+ if (memcmp(cmd, reply, sizeof(cmd)))
+ return -EIO;
+
+ /*
+ * There is an unknown bit set in bit7.
+ * Bits [6:0] report the actual temperature as returned by the
+ * original qnap firmware-tools, so just drop bit7 for now.
+ */
+ return (reply[3] & 0x7f) * 1000;
+}
+
+static int qnap_mcu_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long val)
+{
+ struct qnap_mcu_hwmon *hwm = dev_get_drvdata(dev);
+
+ switch (attr) {
+ case hwmon_pwm_input:
+ if (val < 0 || val > 255)
+ return -EINVAL;
+
+ if (val != 0)
+ val = clamp_val(val, hwm->pwm_min, hwm->pwm_max);
+
+ return qnap_mcu_hwmon_set_pwm(hwm, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+static int qnap_mcu_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ struct qnap_mcu_hwmon *hwm = dev_get_drvdata(dev);
+ int ret;
+
+ switch (type) {
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_input:
+ ret = qnap_mcu_hwmon_get_pwm(hwm);
+ if (ret < 0)
+ return ret;
+
+ *val = ret;
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+ case hwmon_fan:
+ ret = qnap_mcu_hwmon_get_rpm(hwm);
+ if (ret < 0)
+ return ret;
+
+ *val = ret;
+ return 0;
+ case hwmon_temp:
+ ret = qnap_mcu_hwmon_get_temp(hwm);
+ if (ret < 0)
+ return ret;
+
+ *val = ret;
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static umode_t qnap_mcu_hwmon_is_visible(const void *data,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ switch (type) {
+ case hwmon_temp:
+ return 0444;
+
+ case hwmon_pwm:
+ return 0644;
+
+ case hwmon_fan:
+ return 0444;
+
+ default:
+ return 0;
+ }
+}
+
+static const struct hwmon_ops qnap_mcu_hwmon_hwmon_ops = {
+ .is_visible = qnap_mcu_hwmon_is_visible,
+ .read = qnap_mcu_hwmon_read,
+ .write = qnap_mcu_hwmon_write,
+};
+
+/* thermal cooling device callbacks */
+static int qnap_mcu_hwmon_get_max_state(struct thermal_cooling_device *cdev,
+ unsigned long *state)
+{
+ struct qnap_mcu_hwmon *hwm = cdev->devdata;
+
+ if (!hwm)
+ return -EINVAL;
+
+ *state = hwm->fan_max_state;
+
+ return 0;
+}
+
+static int qnap_mcu_hwmon_get_cur_state(struct thermal_cooling_device *cdev,
+ unsigned long *state)
+{
+ struct qnap_mcu_hwmon *hwm = cdev->devdata;
+
+ if (!hwm)
+ return -EINVAL;
+
+ *state = hwm->fan_state;
+
+ return 0;
+}
+
+static int qnap_mcu_hwmon_set_cur_state(struct thermal_cooling_device *cdev,
+ unsigned long state)
+{
+ struct qnap_mcu_hwmon *hwm = cdev->devdata;
+ int ret;
+
+ if (!hwm || state > hwm->fan_max_state)
+ return -EINVAL;
+
+ if (state == hwm->fan_state)
+ return 0;
+
+ ret = qnap_mcu_hwmon_set_pwm(hwm, hwm->fan_cooling_levels[state]);
+ if (ret)
+ return ret;
+
+ hwm->fan_state = state;
+
+ return ret;
+}
+
+static const struct thermal_cooling_device_ops qnap_mcu_hwmon_cooling_ops = {
+ .get_max_state = qnap_mcu_hwmon_get_max_state,
+ .get_cur_state = qnap_mcu_hwmon_get_cur_state,
+ .set_cur_state = qnap_mcu_hwmon_set_cur_state,
+};
+
+static void devm_fan_node_release(void *data)
+{
+ struct qnap_mcu_hwmon *hwm = data;
+
+ fwnode_handle_put(hwm->fan_node);
+}
+
+static int qnap_mcu_hwmon_get_cooling_data(struct device *dev, struct qnap_mcu_hwmon *hwm)
+{
+ struct fwnode_handle *fwnode;
+ int num, i, ret;
+
+ fwnode = device_get_named_child_node(dev->parent, "fan-0");
+ if (!fwnode)
+ return 0;
+
+ /* if we found the fan-node, we're keeping it until device-unbind */
+ hwm->fan_node = fwnode;
+ ret = devm_add_action_or_reset(dev, devm_fan_node_release, hwm);
+ if (ret)
+ return ret;
+
+ num = fwnode_property_count_u32(fwnode, "cooling-levels");
+ if (num <= 0)
+ return dev_err_probe(dev, num ? : -EINVAL,
+ "Failed to count elements in 'cooling-levels'\n");
+
+ hwm->fan_cooling_levels = devm_kcalloc(dev, num, sizeof(u32),
+ GFP_KERNEL);
+ if (!hwm->fan_cooling_levels)
+ return -ENOMEM;
+
+ ret = fwnode_property_read_u32_array(fwnode, "cooling-levels",
+ hwm->fan_cooling_levels, num);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to read 'cooling-levels'\n");
+
+ for (i = 0; i < num; i++) {
+ if (hwm->fan_cooling_levels[i] > hwm->pwm_max)
+ return dev_err_probe(dev, -EINVAL, "fan state[%d]:%d > %d\n", i,
+ hwm->fan_cooling_levels[i], hwm->pwm_max);
+ }
+
+ hwm->fan_max_state = num - 1;
+
+ return 0;
+}
+
+static const struct hwmon_channel_info * const qnap_mcu_hwmon_channels[] = {
+ HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT),
+ HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT),
+ HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
+ NULL
+};
+
+static int qnap_mcu_hwmon_probe(struct platform_device *pdev)
+{
+ struct qnap_mcu *mcu = dev_get_drvdata(pdev->dev.parent);
+ const struct qnap_mcu_variant *variant = qnap_mcu_get_variant_data(mcu);
+ struct qnap_mcu_hwmon *hwm;
+ struct thermal_cooling_device *cdev;
+ struct device *dev = &pdev->dev;
+ struct device *hwmon;
+ int ret;
+
+ hwm = devm_kzalloc(dev, sizeof(*hwm), GFP_KERNEL);
+ if (!hwm)
+ return -ENOMEM;
+
+ hwm->mcu = mcu;
+ hwm->dev = &pdev->dev;
+ hwm->pwm_min = variant->fan_pwm_min;
+ hwm->pwm_max = variant->fan_pwm_max;
+
+ platform_set_drvdata(pdev, hwm);
+
+ /*
+ * Set duty cycle to maximum allowed.
+ */
+ ret = qnap_mcu_hwmon_set_pwm(hwm, hwm->pwm_max);
+ if (ret)
+ return ret;
+
+ hwm->info.ops = &qnap_mcu_hwmon_hwmon_ops;
+ hwm->info.info = qnap_mcu_hwmon_channels;
+
+ ret = qnap_mcu_hwmon_get_cooling_data(dev, hwm);
+ if (ret)
+ return ret;
+
+ hwm->fan_state = hwm->fan_max_state;
+
+ hwmon = devm_hwmon_device_register_with_info(dev, "qnapmcu",
+ hwm, &hwm->info, NULL);
+ if (IS_ERR(hwmon))
+ return dev_err_probe(dev, PTR_ERR(hwmon), "Failed to register hwmon device\n");
+
+ /*
+ * Only register cooling device when we found cooling-levels.
+ * qnap_mcu_hwmon_get_cooling_data() will fail when reading malformed
+ * levels and only succeed with either no or correct cooling levels.
+ */
+ if (IS_ENABLED(CONFIG_THERMAL) && hwm->fan_cooling_levels) {
+ cdev = devm_thermal_of_cooling_device_register(dev,
+ to_of_node(hwm->fan_node), "qnap-mcu-hwmon",
+ hwm, &qnap_mcu_hwmon_cooling_ops);
+ if (IS_ERR(cdev))
+ return dev_err_probe(dev, PTR_ERR(cdev),
+ "Failed to register qnap-mcu-hwmon as cooling device\n");
+ hwm->cdev = cdev;
+ }
+
+ return 0;
+}
+
+static struct platform_driver qnap_mcu_hwmon_driver = {
+ .probe = qnap_mcu_hwmon_probe,
+ .driver = {
+ .name = "qnap-mcu-hwmon",
+ },
+};
+module_platform_driver(qnap_mcu_hwmon_driver);
+
+MODULE_ALIAS("platform:qnap-mcu-hwmon");
+MODULE_AUTHOR("Heiko Stuebner <heiko@sntech.de>");
+MODULE_DESCRIPTION("QNAP MCU hwmon driver");
+MODULE_LICENSE("GPL");
--
2.43.0
^ permalink raw reply related
* [PATCH v6 2/7] mfd: add base driver for qnap-mcu devices
From: Heiko Stuebner @ 2024-08-25 20:32 UTC (permalink / raw)
To: lee
Cc: robh, krzk+dt, conor+dt, jdelvare, linux, heiko, dmitry.torokhov,
pavel, ukleinek, devicetree, linux-kernel, linux-hwmon,
linux-arm-kernel, linux-rockchip, linux-input, linux-leds
In-Reply-To: <20240825203235.1122198-1-heiko@sntech.de>
These microcontroller units are used in network-attached-storage devices
made by QNAP and provide additional functionality to the system.
This adds the base driver that implements the serial protocol via
serdev and additionally hooks into the poweroff handlers to turn
off the parts of the system not supplied by the general PMIC.
Turning off (at least the TSx33 devices using Rockchip SoCs) consists of
two separate actions. Turning off the MCU alone does not turn off the main
SoC and turning off only the SoC/PMIC does not turn off the hard-drives.
Also if the MCU is not turned off, the system also won't start again until
it is unplugged from power.
So on shutdown the MCU needs to be turned off separately before the
main PMIC.
The protocol spoken by the MCU is sadly not documented, but was
obtained by listening to the chatter on the serial port, as thankfully
the "hal_app" program from QNAPs firmware allows triggering all/most
MCU actions from the command line.
The implementation of how to talk to the serial device got some
inspiration from the rave-sp servdev driver.
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
MAINTAINERS | 6 +
drivers/mfd/Kconfig | 13 ++
drivers/mfd/Makefile | 2 +
drivers/mfd/qnap-mcu.c | 339 +++++++++++++++++++++++++++++++++++
include/linux/mfd/qnap-mcu.h | 27 +++
5 files changed, 387 insertions(+)
create mode 100644 drivers/mfd/qnap-mcu.c
create mode 100644 include/linux/mfd/qnap-mcu.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 3e7359ac005c..0fbd2d953da4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18654,6 +18654,12 @@ L: linux-media@vger.kernel.org
S: Odd Fixes
F: drivers/media/tuners/qm1d1c0042*
+QNAP MCU DRIVER
+M: Heiko Stuebner <heiko@sntech.de>
+S: Maintained
+F: drivers/mfd/qnap-mcu.c
+F: include/linux/qnap-mcu.h
+
QNX4 FILESYSTEM
M: Anders Larsen <al@alarsen.net>
S: Maintained
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index bc8be2e593b6..d43d2747ac4e 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2362,6 +2362,19 @@ config MFD_INTEL_M10_BMC_PMCI
additional drivers must be enabled in order to use the functionality
of the device.
+config MFD_QNAP_MCU
+ tristate "QNAP microcontroller unit core driver"
+ depends on SERIAL_DEV_BUS
+ select MFD_CORE
+ help
+ Select this to get support for the QNAP MCU device found in
+ several devices of QNAP network attached storage devices that
+ implements additional functionality for the device, like
+ fan- and LED control.
+
+ This driver implements the base serial protocol to talk to the
+ device and provides functions for the other parts to hook into.
+
config MFD_RSMU_I2C
tristate "Renesas Synchronization Management Unit with I2C"
depends on I2C && OF
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 02b651cd7535..fc8b825725ff 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -286,5 +286,7 @@ obj-$(CONFIG_MFD_INTEL_M10_BMC_PMCI) += intel-m10-bmc-pmci.o
obj-$(CONFIG_MFD_ATC260X) += atc260x-core.o
obj-$(CONFIG_MFD_ATC260X_I2C) += atc260x-i2c.o
+obj-$(CONFIG_MFD_QNAP_MCU) += qnap-mcu.o
+
obj-$(CONFIG_MFD_RSMU_I2C) += rsmu_i2c.o rsmu_core.o
obj-$(CONFIG_MFD_RSMU_SPI) += rsmu_spi.o rsmu_core.o
diff --git a/drivers/mfd/qnap-mcu.c b/drivers/mfd/qnap-mcu.c
new file mode 100644
index 000000000000..ab6036e9a843
--- /dev/null
+++ b/drivers/mfd/qnap-mcu.c
@@ -0,0 +1,339 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Core driver for the microcontroller unit in QNAP NAS devices that is
+ * connected via a dedicated UART port.
+ *
+ * Copyright (C) 2024 Heiko Stuebner <heiko@sntech.de>
+ */
+
+#include <linux/cleanup.h>
+#include <linux/export.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/qnap-mcu.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/reboot.h>
+#include <linux/serdev.h>
+#include <linux/slab.h>
+
+/* The longest command found so far is 5 bytes long */
+#define QNAP_MCU_MAX_CMD_SIZE 5
+#define QNAP_MCU_MAX_DATA_SIZE 36
+#define QNAP_MCU_CHECKSUM_SIZE 1
+
+#define QNAP_MCU_RX_BUFFER_SIZE \
+ (QNAP_MCU_MAX_DATA_SIZE + QNAP_MCU_CHECKSUM_SIZE)
+
+#define QNAP_MCU_TX_BUFFER_SIZE \
+ (QNAP_MCU_MAX_CMD_SIZE + QNAP_MCU_CHECKSUM_SIZE)
+
+#define QNAP_MCU_ACK_LEN 2
+#define QNAP_MCU_VERSION_LEN 4
+
+/**
+ * struct qnap_mcu_reply - Reply to a command
+ *
+ * @data: Buffer to store reply payload in
+ * @length: Expected reply length, including the checksum
+ * @received: Received number of bytes, so far
+ * @done: Triggered when the entire reply has been received
+ */
+struct qnap_mcu_reply {
+ u8 *data;
+ size_t length;
+ size_t received;
+ struct completion done;
+};
+
+/**
+ * struct qnap_mcu - QNAP NAS embedded controller
+ *
+ * @serdev: Pointer to underlying serdev
+ * @bus_lock: Lock to serialize access to the device
+ * @reply: Reply data structure
+ * @variant: Device variant specific information
+ * @version: MCU firmware version
+ */
+struct qnap_mcu {
+ struct serdev_device *serdev;
+ struct mutex bus_lock;
+ struct qnap_mcu_reply reply;
+ const struct qnap_mcu_variant *variant;
+ u8 version[QNAP_MCU_VERSION_LEN];
+};
+
+/*
+ * The QNAP-MCU uses a basic XOR checksum.
+ * It is always the last byte and XORs the whole previous message.
+ */
+static u8 qnap_mcu_csum(const u8 *buf, size_t size)
+{
+ u8 csum = 0;
+
+ while (size--)
+ csum ^= *buf++;
+
+ return csum;
+}
+
+static int qnap_mcu_write(struct qnap_mcu *mcu, const u8 *data, u8 data_size)
+{
+ unsigned char tx[QNAP_MCU_TX_BUFFER_SIZE];
+ size_t length = data_size + QNAP_MCU_CHECKSUM_SIZE;
+
+ if (length > sizeof(tx)) {
+ dev_err(&mcu->serdev->dev, "data too big for transmit buffer");
+ return -EINVAL;
+ }
+
+ memcpy(tx, data, data_size);
+ tx[data_size] = qnap_mcu_csum(data, data_size);
+
+ return serdev_device_write(mcu->serdev, tx, length, HZ);
+}
+
+static size_t qnap_mcu_receive_buf(struct serdev_device *serdev, const u8 *buf, size_t size)
+{
+ struct device *dev = &serdev->dev;
+ struct qnap_mcu *mcu = dev_get_drvdata(dev);
+ struct qnap_mcu_reply *reply = &mcu->reply;
+ const u8 *src = buf;
+ const u8 *end = buf + size;
+
+ if (!reply->length) {
+ dev_warn(dev, "Received %zu bytes, we were not waiting for\n",
+ size);
+ return size;
+ }
+
+ while (src < end) {
+ reply->data[reply->received] = *src++;
+ reply->received++;
+
+ if (reply->received == reply->length) {
+ complete(&reply->done);
+
+ /*
+ * We report the consumed number of bytes. If there
+ * are still bytes remaining (though there shouldn't)
+ * the serdev layer will re-execute this handler with
+ * the remainder of the Rx bytes.
+ */
+ return src - buf;
+ }
+ }
+
+ /*
+ * The only way to get out of the above loop and end up here
+ * is through consuming all of the supplied data, so here we
+ * report that we processed it all.
+ */
+ return size;
+}
+
+static const struct serdev_device_ops qnap_mcu_serdev_device_ops = {
+ .receive_buf = qnap_mcu_receive_buf,
+ .write_wakeup = serdev_device_write_wakeup,
+};
+
+int qnap_mcu_exec(struct qnap_mcu *mcu,
+ const u8 *cmd_data, size_t cmd_data_size,
+ u8 *reply_data, size_t reply_data_size)
+{
+ unsigned char rx[QNAP_MCU_RX_BUFFER_SIZE];
+ size_t length = reply_data_size + QNAP_MCU_CHECKSUM_SIZE;
+ struct qnap_mcu_reply *reply = &mcu->reply;
+ int ret;
+
+ if (length > sizeof(rx)) {
+ dev_err(&mcu->serdev->dev, "expected data too big for receive buffer");
+ return -EINVAL;
+ }
+
+ mutex_lock(&mcu->bus_lock);
+
+ /* Initialize reply fields */
+ reply->data = rx,
+ reply->length = length,
+ reply->received = 0,
+ reinit_completion(&reply->done);
+
+ qnap_mcu_write(mcu, cmd_data, cmd_data_size);
+
+ if (!wait_for_completion_timeout(&reply->done,
+ msecs_to_jiffies(500))) {
+ dev_err(&mcu->serdev->dev, "Command timeout\n");
+ ret = -ETIMEDOUT;
+ } else {
+ u8 crc = qnap_mcu_csum(rx, reply_data_size);
+
+ if (crc != rx[reply_data_size]) {
+ dev_err(&mcu->serdev->dev,
+ "Checksum 0x%02x wrong for data\n", crc);
+ ret = -EIO;
+ } else {
+ memcpy(reply_data, rx, reply_data_size);
+ ret = 0;
+ }
+ }
+
+ /* We don't expect any characters from the device now */
+ reply->length = 0;
+
+ mutex_unlock(&mcu->bus_lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(qnap_mcu_exec);
+
+int qnap_mcu_exec_with_ack(struct qnap_mcu *mcu,
+ const u8 *cmd_data, size_t cmd_data_size)
+{
+ u8 ack[QNAP_MCU_ACK_LEN];
+ int ret;
+
+ ret = qnap_mcu_exec(mcu, cmd_data, cmd_data_size, ack, sizeof(ack));
+ if (ret)
+ return ret;
+
+ /* Should return @0 */
+ if (ack[0] != 0x40 || ack[1] != 0x30) {
+ dev_err(&mcu->serdev->dev, "Did not receive ack\n");
+ return -EIO;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(qnap_mcu_exec_with_ack);
+
+const struct qnap_mcu_variant *qnap_mcu_get_variant_data(struct qnap_mcu *mcu)
+{
+ return mcu->variant;
+}
+EXPORT_SYMBOL_GPL(qnap_mcu_get_variant_data);
+
+static int qnap_mcu_get_version(struct qnap_mcu *mcu)
+{
+ const u8 cmd[] = { 0x25, 0x56 }; /* %, V */
+ u8 rx[14];
+ int ret;
+
+ /* Reply is the 2 command-bytes + 4 bytes describing the version */
+ ret = qnap_mcu_exec(mcu, cmd, sizeof(cmd), rx, QNAP_MCU_VERSION_LEN + 2);
+ if (ret)
+ return ret;
+
+ memcpy(mcu->version, &rx[2], QNAP_MCU_VERSION_LEN);
+
+ return 0;
+}
+
+/*
+ * The MCU controls power to the peripherals but not the CPU.
+ *
+ * So using the pmic to power off the system keeps the MCU and hard-drives
+ * running. This also then prevents the system from turning back on until
+ * the MCU is turned off by unplugging the power-cable.
+ * Turning off the MCU alone on the other hand turns off the hard-drives,
+ * LEDs, etc while the main SoC stays running - including its network ports.
+ */
+static int qnap_mcu_power_off(struct sys_off_data *data)
+{
+ const u8 cmd[] = { 0x40, 0x43, 0x30 }; /* @, C, 0 */
+ struct qnap_mcu *mcu = data->cb_data;
+ int ret;
+
+ ret = qnap_mcu_exec_with_ack(mcu, cmd, sizeof(cmd));
+ if (ret) {
+ dev_err(&mcu->serdev->dev, "MCU poweroff failed %d\n", ret);
+ return NOTIFY_STOP;
+ }
+
+ return NOTIFY_DONE;
+}
+
+static const struct qnap_mcu_variant qnap_ts433_mcu = {
+ .baud_rate = 115200,
+ .num_drives = 4,
+ .fan_pwm_min = 51, /* Specified in original model.conf */
+ .fan_pwm_max = 255,
+ .usb_led = true,
+};
+
+static const struct of_device_id qnap_mcu_dt_ids[] = {
+ { .compatible = "qnap,ts433-mcu", .data = &qnap_ts433_mcu },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, qnap_mcu_dt_ids);
+
+static const struct mfd_cell qnap_mcu_subdevs[] = {
+ { .name = "qnap-mcu-input", },
+ { .name = "qnap-mcu-leds", },
+ { .name = "qnap-mcu-hwmon", }
+};
+
+static int qnap_mcu_probe(struct serdev_device *serdev)
+{
+ struct device *dev = &serdev->dev;
+ struct qnap_mcu *mcu;
+ int ret;
+
+ mcu = devm_kzalloc(dev, sizeof(*mcu), GFP_KERNEL);
+ if (!mcu)
+ return -ENOMEM;
+
+ mcu->serdev = serdev;
+ dev_set_drvdata(dev, mcu);
+
+ mcu->variant = of_device_get_match_data(dev);
+ if (!mcu->variant)
+ return -ENODEV;
+
+ mutex_init(&mcu->bus_lock);
+ init_completion(&mcu->reply.done);
+
+ serdev_device_set_client_ops(serdev, &qnap_mcu_serdev_device_ops);
+ ret = devm_serdev_device_open(dev, serdev);
+ if (ret)
+ return ret;
+
+ serdev_device_set_baudrate(serdev, mcu->variant->baud_rate);
+ serdev_device_set_flow_control(serdev, false);
+
+ ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);
+ if (ret) {
+ dev_err(dev, "Failed to set parity\n");
+ return ret;
+ }
+
+ ret = qnap_mcu_get_version(mcu);
+ if (ret)
+ return ret;
+
+ ret = devm_register_sys_off_handler(dev,
+ SYS_OFF_MODE_POWER_OFF_PREPARE,
+ SYS_OFF_PRIO_DEFAULT,
+ &qnap_mcu_power_off, mcu);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to register poweroff handler\n");
+
+ ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, qnap_mcu_subdevs,
+ ARRAY_SIZE(qnap_mcu_subdevs), NULL, 0, NULL);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to add child devices\n");
+
+ return 0;
+}
+
+static struct serdev_device_driver qnap_mcu_drv = {
+ .probe = qnap_mcu_probe,
+ .driver = {
+ .name = "qnap-mcu",
+ .of_match_table = qnap_mcu_dt_ids,
+ },
+};
+module_serdev_device_driver(qnap_mcu_drv);
+
+MODULE_AUTHOR("Heiko Stuebner <heiko@sntech.de>");
+MODULE_DESCRIPTION("QNAP MCU core driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/qnap-mcu.h b/include/linux/mfd/qnap-mcu.h
new file mode 100644
index 000000000000..903b923537c7
--- /dev/null
+++ b/include/linux/mfd/qnap-mcu.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Core definitions for QNAP MCU MFD driver.
+ * Copyright (C) 2024 Heiko Stuebner <heiko@sntech.de>
+ */
+
+#ifndef _LINUX_QNAP_MCU_H_
+#define _LINUX_QNAP_MCU_H_
+
+struct qnap_mcu;
+
+struct qnap_mcu_variant {
+ u32 baud_rate;
+ int num_drives;
+ int fan_pwm_min;
+ int fan_pwm_max;
+ bool usb_led;
+};
+
+int qnap_mcu_exec(struct qnap_mcu *mcu,
+ const u8 *cmd_data, size_t cmd_data_size,
+ u8 *reply_data, size_t reply_data_size);
+int qnap_mcu_exec_with_ack(struct qnap_mcu *mcu,
+ const u8 *cmd_data, size_t cmd_data_size);
+const struct qnap_mcu_variant *qnap_mcu_get_variant_data(struct qnap_mcu *mcu);
+
+#endif /* _LINUX_QNAP_MCU_H_ */
--
2.43.0
^ permalink raw reply related
* [PATCH v6 3/7] leds: add driver for LEDs from qnap-mcu devices
From: Heiko Stuebner @ 2024-08-25 20:32 UTC (permalink / raw)
To: lee
Cc: robh, krzk+dt, conor+dt, jdelvare, linux, heiko, dmitry.torokhov,
pavel, ukleinek, devicetree, linux-kernel, linux-hwmon,
linux-arm-kernel, linux-rockchip, linux-input, linux-leds
In-Reply-To: <20240825203235.1122198-1-heiko@sntech.de>
This adds a driver that connects to the qnap-mcu mfd driver and provides
access to the LEDs on it.
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
MAINTAINERS | 1 +
drivers/leds/Kconfig | 11 ++
drivers/leds/Makefile | 1 +
drivers/leds/leds-qnap-mcu.c | 226 +++++++++++++++++++++++++++++++++++
4 files changed, 239 insertions(+)
create mode 100644 drivers/leds/leds-qnap-mcu.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 0fbd2d953da4..4dff0e237f22 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18657,6 +18657,7 @@ F: drivers/media/tuners/qm1d1c0042*
QNAP MCU DRIVER
M: Heiko Stuebner <heiko@sntech.de>
S: Maintained
+F: drivers/leds/leds-qnap-mcu.c
F: drivers/mfd/qnap-mcu.c
F: include/linux/qnap-mcu.h
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 8d9d8da376e4..9a337478dd80 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -580,6 +580,17 @@ config LEDS_PCA995X
LED driver chips accessed via the I2C bus. Supported
devices include PCA9955BTW, PCA9952TW and PCA9955TW.
+config LEDS_QNAP_MCU
+ tristate "LED Support for QNAP MCU controllers"
+ depends on LEDS_CLASS
+ depends on MFD_QNAP_MCU
+ help
+ This option enables support for LEDs available on embedded
+ controllers used in QNAP NAS devices.
+
+ This driver can also be built as a module. If so, the module
+ will be called qnap-mcu-leds.
+
config LEDS_WM831X_STATUS
tristate "LED support for status LEDs on WM831x PMICs"
depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 18afbb5a23ee..c6f74865d729 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -79,6 +79,7 @@ obj-$(CONFIG_LEDS_PCA995X) += leds-pca995x.o
obj-$(CONFIG_LEDS_PM8058) += leds-pm8058.o
obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o
obj-$(CONFIG_LEDS_PWM) += leds-pwm.o
+obj-$(CONFIG_LEDS_QNAP_MCU) += leds-qnap-mcu.o
obj-$(CONFIG_LEDS_REGULATOR) += leds-regulator.o
obj-$(CONFIG_LEDS_SC27XX_BLTC) += leds-sc27xx-bltc.o
obj-$(CONFIG_LEDS_SUN50I_A100) += leds-sun50i-a100.o
diff --git a/drivers/leds/leds-qnap-mcu.c b/drivers/leds/leds-qnap-mcu.c
new file mode 100644
index 000000000000..0723ec52e4c5
--- /dev/null
+++ b/drivers/leds/leds-qnap-mcu.c
@@ -0,0 +1,226 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * Driver for LEDs found on QNAP MCU devices
+ *
+ * Copyright (C) 2024 Heiko Stuebner <heiko@sntech.de>
+ */
+
+#include <linux/leds.h>
+#include <linux/mfd/qnap-mcu.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <uapi/linux/uleds.h>
+
+enum qnap_mcu_err_led_mode {
+ QNAP_MCU_ERR_LED_ON = 0,
+ QNAP_MCU_ERR_LED_OFF = 1,
+ QNAP_MCU_ERR_LED_BLINK_FAST = 2,
+ QNAP_MCU_ERR_LED_BLINK_SLOW = 3,
+};
+
+struct qnap_mcu_err_led {
+ struct qnap_mcu *mcu;
+ struct led_classdev cdev;
+ char name[LED_MAX_NAME_SIZE];
+ u8 num;
+ u8 mode;
+};
+
+static inline struct qnap_mcu_err_led *
+ cdev_to_qnap_mcu_err_led(struct led_classdev *led_cdev)
+{
+ return container_of(led_cdev, struct qnap_mcu_err_led, cdev);
+}
+
+static int qnap_mcu_err_led_set(struct led_classdev *led_cdev,
+ enum led_brightness value)
+{
+ struct qnap_mcu_err_led *err_led = cdev_to_qnap_mcu_err_led(led_cdev);
+ u8 cmd[] = { 0x40, 0x52, 0x30 + err_led->num, 0x30 };
+
+ /* Don't disturb a possible set blink-mode if LED is already on */
+ if (value == 0)
+ err_led->mode = QNAP_MCU_ERR_LED_OFF;
+ else if (err_led->mode == QNAP_MCU_ERR_LED_OFF)
+ err_led->mode = QNAP_MCU_ERR_LED_ON;
+
+ cmd[3] = 0x30 + err_led->mode;
+
+ return qnap_mcu_exec_with_ack(err_led->mcu, cmd, sizeof(cmd));
+}
+
+static int qnap_mcu_err_led_blink_set(struct led_classdev *led_cdev,
+ unsigned long *delay_on,
+ unsigned long *delay_off)
+{
+ struct qnap_mcu_err_led *err_led = cdev_to_qnap_mcu_err_led(led_cdev);
+ u8 cmd[] = { 0x40, 0x52, 0x30 + err_led->num, 0x30 };
+
+ /* LED is off, nothing to do */
+ if (err_led->mode == QNAP_MCU_ERR_LED_OFF)
+ return 0;
+
+ if (*delay_on < 500) {
+ *delay_on = 100;
+ *delay_off = 100;
+ err_led->mode = QNAP_MCU_ERR_LED_BLINK_FAST;
+ } else {
+ *delay_on = 500;
+ *delay_off = 500;
+ err_led->mode = QNAP_MCU_ERR_LED_BLINK_SLOW;
+ }
+
+ cmd[3] = 0x30 + err_led->mode;
+
+ return qnap_mcu_exec_with_ack(err_led->mcu, cmd, sizeof(cmd));
+}
+
+static int qnap_mcu_register_err_led(struct device *dev, struct qnap_mcu *mcu, int num)
+{
+ struct qnap_mcu_err_led *err_led;
+ int ret;
+
+ err_led = devm_kzalloc(dev, sizeof(*err_led), GFP_KERNEL);
+ if (!err_led)
+ return -ENOMEM;
+
+ err_led->mcu = mcu;
+ err_led->num = num;
+ err_led->mode = QNAP_MCU_ERR_LED_OFF;
+
+ snprintf(err_led->name, LED_MAX_NAME_SIZE, "hdd%d:red:status", num + 1);
+ err_led->cdev.name = err_led->name;
+
+ err_led->cdev.brightness_set_blocking = qnap_mcu_err_led_set;
+ err_led->cdev.blink_set = qnap_mcu_err_led_blink_set;
+ err_led->cdev.brightness = 0;
+ err_led->cdev.max_brightness = 1;
+
+ ret = devm_led_classdev_register(dev, &err_led->cdev);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to register hdd led %d", num);
+
+ return qnap_mcu_err_led_set(&err_led->cdev, 0);
+}
+
+enum qnap_mcu_usb_led_mode {
+ QNAP_MCU_USB_LED_ON = 1,
+ QNAP_MCU_USB_LED_OFF = 3,
+ QNAP_MCU_USB_LED_BLINK = 2,
+};
+
+struct qnap_mcu_usb_led {
+ struct qnap_mcu *mcu;
+ struct led_classdev cdev;
+ u8 mode;
+};
+
+static inline struct qnap_mcu_usb_led *
+ cdev_to_qnap_mcu_usb_led(struct led_classdev *led_cdev)
+{
+ return container_of(led_cdev, struct qnap_mcu_usb_led, cdev);
+}
+
+static int qnap_mcu_usb_led_set(struct led_classdev *led_cdev,
+ enum led_brightness value)
+{
+ struct qnap_mcu_usb_led *usb_led = cdev_to_qnap_mcu_usb_led(led_cdev);
+ u8 cmd[] = { 0x40, 0x43, 0 };
+
+ /*
+ * If the led is off, turn it on. Otherwise don't disturb
+ * a possible set blink-mode.
+ */
+ if (value == 0)
+ usb_led->mode = QNAP_MCU_USB_LED_OFF;
+ else if (usb_led->mode == QNAP_MCU_USB_LED_OFF)
+ usb_led->mode = QNAP_MCU_USB_LED_ON;
+
+ /* byte 3 is shared between the usb led target and setting the mode */
+ cmd[2] = 0x44 | usb_led->mode;
+
+ return qnap_mcu_exec_with_ack(usb_led->mcu, cmd, sizeof(cmd));
+}
+
+static int qnap_mcu_usb_led_blink_set(struct led_classdev *led_cdev,
+ unsigned long *delay_on,
+ unsigned long *delay_off)
+{
+ struct qnap_mcu_usb_led *usb_led = cdev_to_qnap_mcu_usb_led(led_cdev);
+ u8 cmd[] = { 0x40, 0x43, 0 };
+
+ /* LED is off, nothing to do */
+ if (usb_led->mode == QNAP_MCU_USB_LED_OFF)
+ return 0;
+
+ *delay_on = 250;
+ *delay_off = 250;
+ usb_led->mode = QNAP_MCU_USB_LED_BLINK;
+
+ /* byte 3 is shared between the usb led target and setting the mode */
+ cmd[2] = 0x44 | usb_led->mode;
+
+ return qnap_mcu_exec_with_ack(usb_led->mcu, cmd, sizeof(cmd));
+}
+
+static int qnap_mcu_register_usb_led(struct device *dev, struct qnap_mcu *mcu)
+{
+ struct qnap_mcu_usb_led *usb_led;
+ int ret;
+
+ usb_led = devm_kzalloc(dev, sizeof(*usb_led), GFP_KERNEL);
+ if (!usb_led)
+ return -ENOMEM;
+
+ usb_led->mcu = mcu;
+ usb_led->mode = QNAP_MCU_USB_LED_OFF;
+ usb_led->cdev.name = "usb:blue:disk";
+ usb_led->cdev.brightness_set_blocking = qnap_mcu_usb_led_set;
+ usb_led->cdev.blink_set = qnap_mcu_usb_led_blink_set;
+ usb_led->cdev.brightness = 0;
+ usb_led->cdev.max_brightness = 1;
+
+ ret = devm_led_classdev_register(dev, &usb_led->cdev);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to register usb led");
+
+ return qnap_mcu_usb_led_set(&usb_led->cdev, 0);
+}
+
+static int qnap_mcu_leds_probe(struct platform_device *pdev)
+{
+ struct qnap_mcu *mcu = dev_get_drvdata(pdev->dev.parent);
+ const struct qnap_mcu_variant *variant = qnap_mcu_get_variant_data(mcu);
+ int ret, i;
+
+ for (i = 0; i < variant->num_drives; i++) {
+ ret = qnap_mcu_register_err_led(&pdev->dev, mcu, i);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret,
+ "failed to register error led %d\n", i);
+ }
+
+ if (variant->usb_led) {
+ ret = qnap_mcu_register_usb_led(&pdev->dev, mcu);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret,
+ "failed to register usb led %d\n", i);
+ }
+
+ return 0;
+}
+
+static struct platform_driver qnap_mcu_leds_driver = {
+ .probe = qnap_mcu_leds_probe,
+ .driver = {
+ .name = "qnap-mcu-leds",
+ },
+};
+module_platform_driver(qnap_mcu_leds_driver);
+
+MODULE_ALIAS("platform:qnap-mcu-leds");
+MODULE_AUTHOR("Heiko Stuebner <heiko@sntech.de>");
+MODULE_DESCRIPTION("QNAP MCU LEDs driver");
+MODULE_LICENSE("GPL");
--
2.43.0
^ permalink raw reply related
* [PATCH v6 1/7] dt-bindings: mfd: add binding for qnap,ts433-mcu devices
From: Heiko Stuebner @ 2024-08-25 20:32 UTC (permalink / raw)
To: lee
Cc: robh, krzk+dt, conor+dt, jdelvare, linux, heiko, dmitry.torokhov,
pavel, ukleinek, devicetree, linux-kernel, linux-hwmon,
linux-arm-kernel, linux-rockchip, linux-input, linux-leds,
Conor Dooley
In-Reply-To: <20240825203235.1122198-1-heiko@sntech.de>
These MCUs can be found in network attached storage devices made by QNAP.
They are connected to a serial port of the host device and provide
functionality like LEDs, power-control and temperature monitoring.
LEDs, buttons, etc are all elements of the MCU firmware itself, so don't
need devicetree input, though the fan gets its cooling settings from
a fan-0 subnode.
A binding for the LEDs for setting the linux-default-trigger may come
later, once all the LEDs are understood and ATA controllers actually
can address individual port-LEDs, but are really optional.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
.../bindings/mfd/qnap,ts433-mcu.yaml | 42 +++++++++++++++++++
1 file changed, 42 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/qnap,ts433-mcu.yaml
diff --git a/Documentation/devicetree/bindings/mfd/qnap,ts433-mcu.yaml b/Documentation/devicetree/bindings/mfd/qnap,ts433-mcu.yaml
new file mode 100644
index 000000000000..877078ac172f
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/qnap,ts433-mcu.yaml
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/qnap,ts433-mcu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: QNAP NAS on-board Microcontroller
+
+maintainers:
+ - Heiko Stuebner <heiko@sntech.de>
+
+description:
+ QNAP embeds a microcontroller on their NAS devices adding system feature
+ as PWM Fan control, additional LEDs, power button status and more.
+
+properties:
+ compatible:
+ enum:
+ - qnap,ts433-mcu
+
+patternProperties:
+ "^fan-[0-9]+$":
+ $ref: /schemas/hwmon/fan-common.yaml#
+ unevaluatedProperties: false
+
+required:
+ - compatible
+
+additionalProperties: false
+
+examples:
+ - |
+ uart {
+ mcu {
+ compatible = "qnap,ts433-mcu";
+
+ fan-0 {
+ #cooling-cells = <2>;
+ cooling-levels = <0 64 89 128 166 204 221 238>;
+ };
+ };
+ };
--
2.43.0
^ permalink raw reply related
* [PATCH v6 4/7] Input: add driver for the input part of qnap-mcu devices
From: Heiko Stuebner @ 2024-08-25 20:32 UTC (permalink / raw)
To: lee
Cc: robh, krzk+dt, conor+dt, jdelvare, linux, heiko, dmitry.torokhov,
pavel, ukleinek, devicetree, linux-kernel, linux-hwmon,
linux-arm-kernel, linux-rockchip, linux-input, linux-leds
In-Reply-To: <20240825203235.1122198-1-heiko@sntech.de>
The MCU controls the power-button and beeper, so expose them as input
device. There is of course no interrupt line, so the status of the
power-button needs to be polled. To generate an event the power-button
also needs to be held for 1-2 seconds, so the polling interval does
not need to be overly fast.
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
MAINTAINERS | 1 +
drivers/input/misc/Kconfig | 12 +++
drivers/input/misc/Makefile | 1 +
drivers/input/misc/qnap-mcu-input.c | 153 ++++++++++++++++++++++++++++
4 files changed, 167 insertions(+)
create mode 100644 drivers/input/misc/qnap-mcu-input.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 4dff0e237f22..a41906b3caa3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18657,6 +18657,7 @@ F: drivers/media/tuners/qm1d1c0042*
QNAP MCU DRIVER
M: Heiko Stuebner <heiko@sntech.de>
S: Maintained
+F: drivers/input/misc/qnap-mcu-input.c
F: drivers/leds/leds-qnap-mcu.c
F: drivers/mfd/qnap-mcu.c
F: include/linux/qnap-mcu.h
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 6a852c76331b..13d135257e06 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -917,6 +917,18 @@ config INPUT_HISI_POWERKEY
To compile this driver as a module, choose M here: the
module will be called hisi_powerkey.
+config INPUT_QNAP_MCU
+ tristate "Input Support for QNAP MCU controllers"
+ depends on MFD_QNAP_MCU
+ help
+ This option enables support for input elements available on
+ embedded controllers used in QNAP NAS devices.
+
+ This includes a polled power-button as well as a beeper.
+
+ To compile this driver as a module, choose M here: the
+ module will be called qnap-mcu-input.
+
config INPUT_RAVE_SP_PWRBUTTON
tristate "RAVE SP Power button Driver"
depends on RAVE_SP_CORE
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 4f7f736831ba..6d91804d0a6f 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_INPUT_PMIC8XXX_PWRKEY) += pmic8xxx-pwrkey.o
obj-$(CONFIG_INPUT_POWERMATE) += powermate.o
obj-$(CONFIG_INPUT_PWM_BEEPER) += pwm-beeper.o
obj-$(CONFIG_INPUT_PWM_VIBRA) += pwm-vibra.o
+obj-$(CONFIG_INPUT_QNAP_MCU) += qnap-mcu-input.o
obj-$(CONFIG_INPUT_RAVE_SP_PWRBUTTON) += rave-sp-pwrbutton.o
obj-$(CONFIG_INPUT_RB532_BUTTON) += rb532_button.o
obj-$(CONFIG_INPUT_REGULATOR_HAPTIC) += regulator-haptic.o
diff --git a/drivers/input/misc/qnap-mcu-input.c b/drivers/input/misc/qnap-mcu-input.c
new file mode 100644
index 000000000000..b9b50520ccf7
--- /dev/null
+++ b/drivers/input/misc/qnap-mcu-input.c
@@ -0,0 +1,153 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * Driver for input events on QNAP-MCUs
+ *
+ * Copyright (C) 2024 Heiko Stuebner <heiko@sntech.de>
+ */
+
+#include <linux/input.h>
+#include <linux/mfd/qnap-mcu.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <uapi/linux/input-event-codes.h>
+
+/*
+ * The power-key needs to be pressed for a while to create an event,
+ * so there is no use for overly frequent polling.
+ */
+#define POLL_INTERVAL 500
+
+struct qnap_mcu_input_dev {
+ struct input_dev *input;
+ struct qnap_mcu *mcu;
+ struct device *dev;
+
+ struct work_struct beep_work;
+ int beep_type;
+};
+
+static void qnap_mcu_input_poll(struct input_dev *input)
+{
+ struct qnap_mcu_input_dev *idev = input_get_drvdata(input);
+ static const u8 cmd[] = { 0x40, 0x43, 0x56 };
+ u8 reply[4];
+ int state, ret;
+
+ /* poll the power button */
+ ret = qnap_mcu_exec(idev->mcu, cmd, sizeof(cmd), reply, sizeof(reply));
+ if (ret)
+ return;
+
+ /* First bytes must mirror the sent command */
+ if (memcmp(cmd, reply, sizeof(cmd))) {
+ dev_err(idev->dev, "malformed data received\n");
+ return;
+ }
+
+ state = reply[3] - 0x30;
+ input_event(input, EV_KEY, KEY_POWER, state);
+ input_sync(input);
+}
+
+static void qnap_mcu_input_beeper_work(struct work_struct *work)
+{
+ struct qnap_mcu_input_dev *idev =
+ container_of(work, struct qnap_mcu_input_dev, beep_work);
+ const u8 cmd[] = { 0x40, 0x43, (idev->beep_type == SND_TONE) ? 0x33 : 0x32 };
+
+ qnap_mcu_exec_with_ack(idev->mcu, cmd, sizeof(cmd));
+}
+
+static int qnap_mcu_input_event(struct input_dev *input, unsigned int type,
+ unsigned int code, int value)
+{
+ struct qnap_mcu_input_dev *idev = input_get_drvdata(input);
+
+ if (type != EV_SND || (code != SND_BELL && code != SND_TONE))
+ return -EOPNOTSUPP;
+
+ if (value < 0)
+ return -EINVAL;
+
+ /* beep runtime is determined by the MCU */
+ if (value == 0)
+ return 0;
+
+ /* Schedule work to actually turn the beeper on */
+ idev->beep_type = code;
+ schedule_work(&idev->beep_work);
+
+ return 0;
+}
+
+static void qnap_mcu_input_close(struct input_dev *input)
+{
+ struct qnap_mcu_input_dev *idev = input_get_drvdata(input);
+
+ cancel_work_sync(&idev->beep_work);
+}
+
+static int qnap_mcu_input_probe(struct platform_device *pdev)
+{
+ struct qnap_mcu *mcu = dev_get_drvdata(pdev->dev.parent);
+ struct qnap_mcu_input_dev *idev;
+ struct device *dev = &pdev->dev;
+ struct input_dev *input;
+ int ret;
+
+ idev = devm_kzalloc(dev, sizeof(*idev), GFP_KERNEL);
+ if (!idev)
+ return -ENOMEM;
+
+ input = devm_input_allocate_device(dev);
+ if (!input)
+ return dev_err_probe(dev, -ENOMEM, "no memory for input device\n");
+
+ idev->input = input;
+ idev->dev = dev;
+ idev->mcu = mcu;
+
+ input_set_drvdata(input, idev);
+
+ input->name = "qnap-mcu";
+ input->phys = "qnap-mcu-input/input0";
+ input->id.bustype = BUS_HOST;
+ input->id.vendor = 0x0001;
+ input->id.product = 0x0001;
+ input->id.version = 0x0100;
+ input->event = qnap_mcu_input_event;
+ input->close = qnap_mcu_input_close;
+
+ input_set_capability(input, EV_KEY, KEY_POWER);
+ input_set_capability(input, EV_SND, SND_BELL);
+ input_set_capability(input, EV_SND, SND_TONE);
+
+ INIT_WORK(&idev->beep_work, qnap_mcu_input_beeper_work);
+
+ ret = input_setup_polling(input, qnap_mcu_input_poll);
+ if (ret)
+ return dev_err_probe(dev, ret, "unable to set up polling\n");
+
+ input_set_poll_interval(input, POLL_INTERVAL);
+
+ ret = input_register_device(input);
+ if (ret)
+ return dev_err_probe(dev, ret, "unable to register input device\n");
+
+ return 0;
+}
+
+static struct platform_driver qnap_mcu_input_driver = {
+ .probe = qnap_mcu_input_probe,
+ .driver = {
+ .name = "qnap-mcu-input",
+ },
+};
+module_platform_driver(qnap_mcu_input_driver);
+
+MODULE_ALIAS("platform:qnap-mcu-input");
+MODULE_AUTHOR("Heiko Stuebner <heiko@sntech.de>");
+MODULE_DESCRIPTION("QNAP MCU input driver");
+MODULE_LICENSE("GPL");
--
2.43.0
^ permalink raw reply related
* [PATCH v6 0/7] Drivers to support the MCU on QNAP NAS devices
From: Heiko Stuebner @ 2024-08-25 20:32 UTC (permalink / raw)
To: lee
Cc: robh, krzk+dt, conor+dt, jdelvare, linux, heiko, dmitry.torokhov,
pavel, ukleinek, devicetree, linux-kernel, linux-hwmon,
linux-arm-kernel, linux-rockchip, linux-input, linux-leds
This implements a set of drivers for the MCU used on QNAP NAS devices.
Of course no documentation for the serial protocol is available, so
thankfully QNAP has a tool on their rescue-inird to talk to the MCU and
I found interceptty [0] to listen to what goes over the serial connection.
In general it looks like there are two different generations in general,
an "EC" device and now this "MCU" - referenced in the strings of the
userspace handlers for those devices.
For the MCU "SPEC3" and "SPEC4" are listed which is configured in
the model.conf of the device. When setting the value from SPEC4 to
SPEC3 on my TS433, the supported commands change, but the command
interface stays the same and especially the version command is the
same.
The binding also does not expose any interals of the device that
might change, so hopefully there shouldn't be big roadblocks to
support different devices, apart from possibly adapting the commands.
changes in v6:
- format mcu commands arrays in single lines (Lee)
mfd:
- drop obsolete remain kdoc for the removed
reply_lock (kernel test robot)
changes in v5:
binding:
- add Conor's Reviewed-by
mfd:
Address comments from Lee
- improve commit message
- improve Kconfig help text
- sort headers alphabetical
- style and spelling improvements
- constants for magic numbers
- drop reply assignment, the mcu only replies to commands sent to it,
so there should only ever be one command in fligth.
hwmon:
Add Acked-by from Guenter and address some remarks
- don't allow empty fan subnode
- use num var directly when getting cooling levels, without using ret
intermediate
- use dev_err_probe in thermal init function
changes in v4:
binding:
- move cooling properties into a fan subnode and reference
fan-common.yaml (Rob)
- dropped Krzysztof's Ack because of this
mfd:
- use correct format-string for size_t (kernel test robot)
input:
- added Dmitry's Ack
hwmon:
- adapted to fan-subnode when reading cooling properties
- dropped Guenter's Ack because of this
changes in v3:
mfd
- use correct power-off priority: default
- constify the cmd-data array in command functions (Dmitry)
leds:
- don't point to temporary buffers for cdev->name (Florian Eckert)
hwmon:
- use clamp_val(), don't try to reimplement (Guenter)
- add Guenter's Ack
input:
address Dmitry's comments
- constify some cmd arrays
- add input-close callback to cancel beep worker
- drop initial input event report
changes in v2:
binding:
- rename to qnap,ts433-mcu.yaml (Krzysztof)
- drop "preserve formatting" indicator (Krzysztof)
- add Krzysztof's Review tag
mfd:
- fix checkpatch --strict CHECKs
- add a MAINTAINERS entry for all qnap-mcu-parts
Heiko Stuebner (7):
dt-bindings: mfd: add binding for qnap,ts433-mcu devices
mfd: add base driver for qnap-mcu devices
leds: add driver for LEDs from qnap-mcu devices
Input: add driver for the input part of qnap-mcu devices
hwmon: add driver for the hwmon parts of qnap-mcu devices
arm64: dts: rockchip: hook up the MCU on the QNAP TS433
arm64: dts: rockchip: set hdd led labels on qnap-ts433
.../bindings/mfd/qnap,ts433-mcu.yaml | 42 ++
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/qnap-mcu-hwmon.rst | 27 ++
MAINTAINERS | 9 +
.../boot/dts/rockchip/rk3568-qnap-ts433.dts | 61 +++
drivers/hwmon/Kconfig | 12 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/qnap-mcu-hwmon.c | 364 ++++++++++++++++++
drivers/input/misc/Kconfig | 12 +
drivers/input/misc/Makefile | 1 +
drivers/input/misc/qnap-mcu-input.c | 153 ++++++++
drivers/leds/Kconfig | 11 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-qnap-mcu.c | 226 +++++++++++
drivers/mfd/Kconfig | 13 +
drivers/mfd/Makefile | 2 +
drivers/mfd/qnap-mcu.c | 339 ++++++++++++++++
include/linux/mfd/qnap-mcu.h | 27 ++
18 files changed, 1302 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/qnap,ts433-mcu.yaml
create mode 100644 Documentation/hwmon/qnap-mcu-hwmon.rst
create mode 100644 drivers/hwmon/qnap-mcu-hwmon.c
create mode 100644 drivers/input/misc/qnap-mcu-input.c
create mode 100644 drivers/leds/leds-qnap-mcu.c
create mode 100644 drivers/mfd/qnap-mcu.c
create mode 100644 include/linux/mfd/qnap-mcu.h
--
2.43.0
^ permalink raw reply
* [PATCH v6 7/7] arm64: dts: rockchip: set hdd led labels on qnap-ts433
From: Heiko Stuebner @ 2024-08-25 20:32 UTC (permalink / raw)
To: lee
Cc: robh, krzk+dt, conor+dt, jdelvare, linux, heiko, dmitry.torokhov,
pavel, ukleinek, devicetree, linux-kernel, linux-hwmon,
linux-arm-kernel, linux-rockchip, linux-input, linux-leds
In-Reply-To: <20240825203235.1122198-1-heiko@sntech.de>
The automatically generated names for the LEDs from color and function
do not match nicely for the 4 hdds, so set them manually per the label
property to also match the LEDs generated from the MCU.
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts b/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
index 4bc5f5691d45..7bd32d230ad2 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
@@ -50,6 +50,7 @@ led-0 {
color = <LED_COLOR_ID_GREEN>;
function = LED_FUNCTION_DISK;
gpios = <&gpio1 RK_PD5 GPIO_ACTIVE_LOW>;
+ label = "hdd1:green:disk";
linux,default-trigger = "disk-activity";
pinctrl-names = "default";
pinctrl-0 = <&hdd1_led_pin>;
@@ -59,6 +60,7 @@ led-1 {
color = <LED_COLOR_ID_GREEN>;
function = LED_FUNCTION_DISK;
gpios = <&gpio1 RK_PD6 GPIO_ACTIVE_LOW>;
+ label = "hdd2:green:disk";
linux,default-trigger = "disk-activity";
pinctrl-names = "default";
pinctrl-0 = <&hdd2_led_pin>;
@@ -68,6 +70,7 @@ led-2 {
color = <LED_COLOR_ID_GREEN>;
function = LED_FUNCTION_DISK;
gpios = <&gpio1 RK_PD7 GPIO_ACTIVE_LOW>;
+ label = "hdd3:green:disk";
linux,default-trigger = "disk-activity";
pinctrl-names = "default";
pinctrl-0 = <&hdd3_led_pin>;
@@ -77,6 +80,7 @@ led-3 {
color = <LED_COLOR_ID_GREEN>;
function = LED_FUNCTION_DISK;
gpios = <&gpio2 RK_PA0 GPIO_ACTIVE_LOW>;
+ label = "hdd4:green:disk";
linux,default-trigger = "disk-activity";
pinctrl-names = "default";
pinctrl-0 = <&hdd4_led_pin>;
--
2.43.0
^ permalink raw reply related
* [PATCH v6 6/7] arm64: dts: rockchip: hook up the MCU on the QNAP TS433
From: Heiko Stuebner @ 2024-08-25 20:32 UTC (permalink / raw)
To: lee
Cc: robh, krzk+dt, conor+dt, jdelvare, linux, heiko, dmitry.torokhov,
pavel, ukleinek, devicetree, linux-kernel, linux-hwmon,
linux-arm-kernel, linux-rockchip, linux-input, linux-leds
In-Reply-To: <20240825203235.1122198-1-heiko@sntech.de>
The MCU is an important part of the device functionality. It provides
functionality like fan-control, more leds, etc and even more important
without it, the NAS-device cannot even fully turned off.
Hook up the serial device to its uart and hook into the thermal
management to control the fan according to the cpu temperature.
While the MCU also provides a temperature sensor for the case, this one
is just polled and does not provide functionality for handling trip
points in hardware, so a lot of polling would be involved.
As the cpu is only cooled passively in these devices, it's temperature
rising will indicate the temperature level of the system just earlier.
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
.../boot/dts/rockchip/rk3568-qnap-ts433.dts | 57 +++++++++++++++++++
1 file changed, 57 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts b/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
index e601d9271ba8..4bc5f5691d45 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
@@ -483,6 +483,54 @@ rgmii_phy0: ethernet-phy@0 {
};
};
+/*
+ * The MCU can provide system temperature too, but only by polling and of
+ * course also cannot set trip points. So attach to the cpu thermal-zone
+ * instead to control the fan.
+ */
+&cpu_thermal {
+ trips {
+ case_fan0: case-fan0 {
+ hysteresis = <2000>;
+ temperature = <35000>;
+ type = "active";
+ };
+
+ case_fan1: case-fan1 {
+ hysteresis = <2000>;
+ temperature = <45000>;
+ type = "active";
+ };
+
+ case_fan2: case-fan2 {
+ hysteresis = <2000>;
+ temperature = <65000>;
+ type = "active";
+ };
+ };
+
+ cooling-maps {
+ /*
+ * Always provide some air movement, due to small case
+ * full of harddrives.
+ */
+ map1 {
+ cooling-device = <&fan THERMAL_NO_LIMIT 1>;
+ trip = <&case_fan0>;
+ };
+
+ map2 {
+ cooling-device = <&fan 2 3>;
+ trip = <&case_fan1>;
+ };
+
+ map3 {
+ cooling-device = <&fan 4 THERMAL_NO_LIMIT>;
+ trip = <&case_fan2>;
+ };
+ };
+};
+
&pcie30phy {
data-lanes = <1 2>;
status = "okay";
@@ -582,6 +630,15 @@ &tsadc {
*/
&uart0 {
status = "okay";
+
+ mcu {
+ compatible = "qnap,ts433-mcu";
+
+ fan: fan-0 {
+ #cooling-cells = <2>;
+ cooling-levels = <0 64 89 128 166 204 221 238>;
+ };
+ };
};
/*
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] Input: alps - use guard notation when acquiring mutex
From: Markus Elfring @ 2024-08-25 20:26 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input; +Cc: LKML, Erick Archer, Pali Rohár
In-Reply-To: <ZsrBkWIpyEqzClUG@google.com>
> This makes the code …
Would you ever like to improve such a change description with imperative wordings?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc5#n94
Regards,
Markus
^ permalink raw reply
* Re: [PATCH] Input: alps - use guard notation when acquiring mutex
From: Pali Rohár @ 2024-08-25 20:13 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Erick Archer, linux-kernel
In-Reply-To: <ZsrBkWIpyEqzClUG@google.com>
On Saturday 24 August 2024 22:30:57 Dmitry Torokhov wrote:
> This makes the code more compact and error handling more robust
> by ensuring that mutexes are released in all code paths when control
> leaves critical section.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/mouse/alps.c | 48 +++++++++++++++++++++-----------------
> 1 file changed, 27 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index d5ef5a112d6f..4e37fc3f1a9e 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -1396,24 +1396,16 @@ static bool alps_is_valid_package_ss4_v2(struct psmouse *psmouse)
>
> static DEFINE_MUTEX(alps_mutex);
>
> -static void alps_register_bare_ps2_mouse(struct work_struct *work)
> +static int alps_do_register_bare_ps2_mouse(struct alps_data *priv)
> {
> - struct alps_data *priv =
> - container_of(work, struct alps_data, dev3_register_work.work);
> struct psmouse *psmouse = priv->psmouse;
> struct input_dev *dev3;
> - int error = 0;
> -
> - mutex_lock(&alps_mutex);
> -
> - if (priv->dev3)
> - goto out;
> + int error;
>
> dev3 = input_allocate_device();
> if (!dev3) {
> psmouse_err(psmouse, "failed to allocate secondary device\n");
> - error = -ENOMEM;
> - goto out;
> + return -ENOMEM;
> }
>
> snprintf(priv->phys3, sizeof(priv->phys3), "%s/%s",
> @@ -1446,21 +1438,35 @@ static void alps_register_bare_ps2_mouse(struct work_struct *work)
> psmouse_err(psmouse,
> "failed to register secondary device: %d\n",
> error);
> - input_free_device(dev3);
> - goto out;
> + goto err_free_input;
> }
>
> priv->dev3 = dev3;
> + return 0;
>
> -out:
> - /*
> - * Save the error code so that we can detect that we
> - * already tried to create the device.
> - */
> - if (error)
> - priv->dev3 = ERR_PTR(error);
> +err_free_input:
> + input_free_device(dev3);
> + return error;
> +}
>
> - mutex_unlock(&alps_mutex);
> +static void alps_register_bare_ps2_mouse(struct work_struct *work)
> +{
> + struct alps_data *priv = container_of(work, struct alps_data,
> + dev3_register_work.work);
> + int error;
> +
> + guard(mutex)(&alps_mutex);
> +
> + if (!priv->dev3) {
> + error = alps_do_register_bare_ps2_mouse(priv);
> + if (error) {
> + /*
> + * Save the error code so that we can detect that we
> + * already tried to create the device.
> + */
> + priv->dev3 = ERR_PTR(error);
> + }
> + }
> }
>
> static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
> --
> 2.46.0.295.g3b9ea8a38a-goog
>
>
> --
> Dmitry
Hello, I'm not familiar with new guards and their usage. But if this is
a preferred way for handling mutexes then go ahead.
I just looked at the code and I do not see any obvious error neither in
old nor in new version.
^ permalink raw reply
* Re: [PATCH] Input: bcm5974 - use guard notation when acquiring mutex
From: Markus Elfring @ 2024-08-25 20:02 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: LKML, Erick Archer, Henrik Rydberg, Javier Carrasco
In-Reply-To: <ZsrBVO2w9WwX73iU@google.com>
> This makes the code …
Is there a need to improve such a change description with imperative wordings?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc5#n94
Regards,
Markus
^ permalink raw reply
* Re: [PATCH] Input: matrix-keymap - switch to using __free() cleanup facility
From: Hans de Goede @ 2024-08-25 13:13 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input; +Cc: linux-kernel, Benjamin Tissoires
In-Reply-To: <ZspoEPdTcH-hpciy@google.com>
Hi,
On 8/25/24 1:09 AM, Dmitry Torokhov wrote:
> Use __free(kfree) cleanup facility in matrix_keypad_parse_keymap() to
> automatically free temporarily allocated memory.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> ---
> drivers/input/matrix-keymap.c | 25 +++++++++----------------
> 1 file changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/input/matrix-keymap.c b/drivers/input/matrix-keymap.c
> index 5d93043bad8e..3bea3575a0a9 100644
> --- a/drivers/input/matrix-keymap.c
> +++ b/drivers/input/matrix-keymap.c
> @@ -73,10 +73,9 @@ static int matrix_keypad_parse_keymap(const char *propname,
> struct device *dev = input_dev->dev.parent;
> unsigned int row_shift = get_count_order(cols);
> unsigned int max_keys = rows << row_shift;
> - u32 *keys;
> int i;
> int size;
> - int retval;
> + int error;
>
> if (!propname)
> propname = "linux,keymap";
> @@ -94,30 +93,24 @@ static int matrix_keypad_parse_keymap(const char *propname,
> return -EINVAL;
> }
>
> - keys = kmalloc_array(size, sizeof(u32), GFP_KERNEL);
> + u32 *keys __free(kfree) = kmalloc_array(size, sizeof(*keys), GFP_KERNEL);
> if (!keys)
> return -ENOMEM;
>
> - retval = device_property_read_u32_array(dev, propname, keys, size);
> - if (retval) {
> + error = device_property_read_u32_array(dev, propname, keys, size);
> + if (error) {
> dev_err(dev, "failed to read %s property: %d\n",
> - propname, retval);
> - goto out;
> + propname, error);
> + return error;
> }
>
> for (i = 0; i < size; i++) {
> if (!matrix_keypad_map_key(input_dev, rows, cols,
> - row_shift, keys[i])) {
> - retval = -EINVAL;
> - goto out;
> - }
> + row_shift, keys[i]))
> + return -EINVAL;
> }
>
> - retval = 0;
> -
> -out:
> - kfree(keys);
> - return retval;
> + return 0;
> }
>
> /**
^ permalink raw reply
* Re: [PATCH 03/17] Input: atkbd - use guard notation when acquiring mutex
From: Hans de Goede @ 2024-08-25 13:12 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: Michael Hennerich, Shawn Guo, Sascha Hauer, Fabio Estevam,
Laxman Dewangan, Thierry Reding, Tony Lindgren, Jeff LaBundy,
linux-kernel, imx, linux-arm-kernel, linux-tegra
In-Reply-To: <20240825051627.2848495-4-dmitry.torokhov@gmail.com>
Hi,
On 8/25/24 7:16 AM, Dmitry Torokhov wrote:
> This makes the code more compact and error handling more robust
> by ensuring that mutexes are released in all code paths when control
> leaves critical section.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> ---
> drivers/input/keyboard/atkbd.c | 37 ++++++++++++++--------------------
> 1 file changed, 15 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
> index f4f2078cf501..5855d4fc6e6a 100644
> --- a/drivers/input/keyboard/atkbd.c
> +++ b/drivers/input/keyboard/atkbd.c
> @@ -639,7 +639,7 @@ static void atkbd_event_work(struct work_struct *work)
> {
> struct atkbd *atkbd = container_of(work, struct atkbd, event_work.work);
>
> - mutex_lock(&atkbd->mutex);
> + guard(mutex)(&atkbd->mutex);
>
> if (!atkbd->enabled) {
> /*
> @@ -657,8 +657,6 @@ static void atkbd_event_work(struct work_struct *work)
> if (test_and_clear_bit(ATKBD_REP_EVENT_BIT, &atkbd->event_mask))
> atkbd_set_repeat_rate(atkbd);
> }
> -
> - mutex_unlock(&atkbd->mutex);
> }
>
> /*
> @@ -1361,7 +1359,7 @@ static int atkbd_reconnect(struct serio *serio)
> {
> struct atkbd *atkbd = atkbd_from_serio(serio);
> struct serio_driver *drv = serio->drv;
> - int retval = -1;
> + int error;
>
> if (!atkbd || !drv) {
> dev_dbg(&serio->dev,
> @@ -1369,16 +1367,17 @@ static int atkbd_reconnect(struct serio *serio)
> return -1;
> }
>
> - mutex_lock(&atkbd->mutex);
> + guard(mutex)(&atkbd->mutex);
>
> atkbd_disable(atkbd);
>
> if (atkbd->write) {
> - if (atkbd_probe(atkbd))
> - goto out;
> + error = atkbd_probe(atkbd);
> + if (error)
> + return error;
>
> if (atkbd->set != atkbd_select_set(atkbd, atkbd->set, atkbd->extra))
> - goto out;
> + return -EIO;
>
> /*
> * Restore LED state and repeat rate. While input core
> @@ -1404,11 +1403,7 @@ static int atkbd_reconnect(struct serio *serio)
> if (atkbd->write)
> atkbd_activate(atkbd);
>
> - retval = 0;
> -
> - out:
> - mutex_unlock(&atkbd->mutex);
> - return retval;
> + return 0;
> }
>
> static const struct serio_device_id atkbd_serio_ids[] = {
> @@ -1465,17 +1460,15 @@ static ssize_t atkbd_attr_set_helper(struct device *dev, const char *buf, size_t
> struct atkbd *atkbd = atkbd_from_serio(serio);
> int retval;
>
> - retval = mutex_lock_interruptible(&atkbd->mutex);
> - if (retval)
> - return retval;
> + scoped_guard(mutex_intr, &atkbd->mutex) {
> + atkbd_disable(atkbd);
> + retval = handler(atkbd, buf, count);
> + atkbd_enable(atkbd);
>
> - atkbd_disable(atkbd);
> - retval = handler(atkbd, buf, count);
> - atkbd_enable(atkbd);
> -
> - mutex_unlock(&atkbd->mutex);
> + return retval;
> + }
>
> - return retval;
> + return -EINTR;
> }
>
> static ssize_t atkbd_show_extra(struct atkbd *atkbd, char *buf)
^ 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