Add support for multiple sensors to pmw3360 (#15996)

4 files changed, 123 insertions, 84 deletions

diff --git a/docs/feature_pointing_device.md b/docs/feature_pointing_device.md
index 8c51865558..02c1e64a31 100644
--- a/docs/feature_pointing_device.md
+++ b/docs/feature_pointing_device.md
@@ -134,6 +134,7 @@ The Pimoroni Trackball module is a I2C based breakout board with an RGB enable t
 
 ### PMW 3360 Sensor
 
+This drivers supports multiple sensors _per_ controller, so 2 can be attached at the same side for split keyboards (or unsplit keyboards).
 To use the PMW 3360 sensor, add this to your `rules.mk`
 
 ```make
@@ -145,6 +146,7 @@ The PMW 3360 is an SPI driven optical sensor, that uses a built in IR LED for su
 | Setting                     | Description                                                                                | Default       |
 |-----------------------------|--------------------------------------------------------------------------------------------|---------------|
 |`PMW3360_CS_PIN`                 | (Required) Sets the Cable Select pin connected to the sensor.                              | _not defined_ |
+|`PMW3360_CS_PINS`                | (Alternative) Sets the Cable Select pins connected to multiple sensors.                    | _not defined_ |
 |`PMW3360_CLOCK_SPEED`            | (Optional) Sets the clock speed that the sensor runs at.                                   | `2000000`     |
 |`PMW3360_SPI_LSBFIRST`           | (Optional) Sets the Least/Most Significant Byte First setting for SPI.                     | `false`       |
 |`PMW3360_SPI_MODE`               | (Optional) Sets the SPI Mode for the sensor.                                               | `3`           |
@@ -155,6 +157,36 @@ The PMW 3360 is an SPI driven optical sensor, that uses a built in IR LED for su
 
 The CPI range is 100-12000, in increments of 100. Defaults to 1600 CPI.
 
+To use multiple sensors, instead of setting `PMW3360_CS_PIN` you need to set `PMW3360_CS_PINS` and also handle and merge the read from this sensor in user code.
+Note that different (per sensor) values of CPI, speed liftoff, rotational angle or flipping of X/Y is not currently supported.
+
+```c
+// in config.h:
+#define PMW3360_CS_PINS { B5, B6 }
+
+// in keyboard.c:
+#ifdef POINTING_DEVICE_ENABLE
+void pointing_device_init_kb(void) {
+    pmw3360_init(1);  // index 1 is the second device.
+    pointing_device_set_cpi(800);  // applies to both sensors
+    pointing_device_init_user();
+}
+
+// Contains report from sensor #0 already, need to merge in from sensor #1
+report_mouse_t pointing_device_task_kb(report_mouse_t mouse_report) {
+    report_pmw3360_t data = pmw3360_read_burst(1);
+    if (data.isOnSurface && data.isMotion) {
+// From quantum/pointing_device_drivers.c
+#define constrain_hid(amt) ((amt) < -127 ? -127 : ((amt) > 127 ? 127 : (amt)))
+        mouse_report.x = constrain_hid(mouse_report.x + data.dx);
+        mouse_report.y = constrain_hid(mouse_report.y + data.dy);
+    }
+    return pointing_device_task_user(mouse_report);
+}
+#endif
+
+```
+
 ### PMW 3389 Sensor
 
 To use the PMW 3389 sensor, add this to your `rules.mk`
diff --git a/drivers/sensors/pmw3360.c b/drivers/sensors/pmw3360.c
index 8c977be1c8..5f4d17a3f0 100644
--- a/drivers/sensors/pmw3360.c
+++ b/drivers/sensors/pmw3360.c
@@ -1,6 +1,7 @@
 /* Copyright 2020 Christopher Courtney, aka Drashna Jael're  (@drashna) <drashna@live.com>
  * Copyright 2019 Sunjun Kim
  * Copyright 2020 Ploopy Corporation
+ * Copyright 2022 Ulrich Spörlein
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -83,7 +84,11 @@
 #    define MAX_CPI 0x77
 #endif
 
-bool _inBurst = false;
+static const pin_t pins[] = PMW3360_CS_PINS;
+#define NUMBER_OF_SENSORS (sizeof(pins) / sizeof(pin_t))
+
+// per-sensor driver state
+static bool _inBurst[NUMBER_OF_SENSORS] = {0};
 
 #ifdef CONSOLE_ENABLE
 void print_byte(uint8_t byte) {
@@ -92,18 +97,18 @@ void print_byte(uint8_t byte) {
 #endif
 #define constrain(amt, low, high) ((amt) < (low) ? (low) : ((amt) > (high) ? (high) : (amt)))
 
-bool pmw3360_spi_start(void) {
-    bool status = spi_start(PMW3360_CS_PIN, PMW3360_SPI_LSBFIRST, PMW3360_SPI_MODE, PMW3360_SPI_DIVISOR);
+bool pmw3360_spi_start(int8_t index) {
+    bool status = spi_start(pins[index], PMW3360_SPI_LSBFIRST, PMW3360_SPI_MODE, PMW3360_SPI_DIVISOR);
     // tNCS-SCLK, 120ns
     wait_us(1);
     return status;
 }
 
-spi_status_t pmw3360_write(uint8_t reg_addr, uint8_t data) {
-    pmw3360_spi_start();
+spi_status_t pmw3360_write(int8_t index, uint8_t reg_addr, uint8_t data) {
+    pmw3360_spi_start(index);
 
     if (reg_addr != REG_Motion_Burst) {
-        _inBurst = false;
+        _inBurst[index] = false;
     }
 
     // send address of the register, with MSBit = 1 to indicate it's a write
@@ -114,13 +119,13 @@ spi_status_t pmw3360_write(uint8_t reg_addr, uint8_t data) {
     wait_us(35);
     spi_stop();
 
-    // tSWW/tSWR (=180us) minus tSCLK-NCS. Could be shortened, but is looks like a safe lower bound
+    // tSWW/tSWR (=180us) minus tSCLK-NCS. Could be shortened, but it looks like a safe lower bound
     wait_us(145);
     return status;
 }
 
-uint8_t pmw3360_read(uint8_t reg_addr) {
-    pmw3360_spi_start();
+uint8_t pmw3360_read(int8_t index, uint8_t reg_addr) {
+    pmw3360_spi_start(index);
     // send adress of the register, with MSBit = 0 to indicate it's a read
     spi_write(reg_addr & 0x7f);
     // tSRAD (=160us)
@@ -136,36 +141,62 @@ uint8_t pmw3360_read(uint8_t reg_addr) {
     return data;
 }
 
-bool pmw3360_init(void) {
-    setPinOutput(PMW3360_CS_PIN);
+bool pmw3360_check_signature(int8_t index) {
+    uint8_t pid      = pmw3360_read(index, REG_Product_ID);
+    uint8_t iv_pid   = pmw3360_read(index, REG_Inverse_Product_ID);
+    uint8_t SROM_ver = pmw3360_read(index, REG_SROM_ID);
+    return (pid == firmware_signature[0] && iv_pid == firmware_signature[1] && SROM_ver == firmware_signature[2]); // signature for SROM 0x04
+}
 
-    spi_init();
-    _inBurst = false;
+void pmw3360_upload_firmware(int8_t index) {
+    // Datasheet claims we need to disable REST mode first, but during startup
+    // it's already disabled and we're not turning it on ...
+    // pmw3360_write(index, REG_Config2, 0x00); // disable REST mode
+    pmw3360_write(index, REG_SROM_Enable, 0x1d);
 
-    spi_stop();
-    pmw3360_spi_start();
-    spi_stop();
+    wait_ms(10);
+
+    pmw3360_write(index, REG_SROM_Enable, 0x18);
+
+    pmw3360_spi_start(index);
+    spi_write(REG_SROM_Load_Burst | 0x80);
+    wait_us(15);
 
-    pmw3360_write(REG_Shutdown, 0xb6); // Shutdown first
-    wait_ms(300);
+    for (uint16_t i = 0; i < FIRMWARE_LENGTH; i++) {
+        spi_write(pgm_read_byte(firmware_data + i));
+#ifndef PMW3360_FIRMWARE_UPLOAD_FAST
+        wait_us(15);
+#endif
+    }
+    wait_us(200);
 
-    pmw3360_spi_start();
+    pmw3360_read(index, REG_SROM_ID);
+    pmw3360_write(index, REG_Config2, 0x00);
+}
+
+bool pmw3360_init(int8_t index) {
+    if (index >= NUMBER_OF_SENSORS) {
+        return false;
+    }
+    spi_init();
+
+    // power up, need to first drive NCS high then low.
+    // the datasheet does not say for how long, 40us works well in practice.
+    pmw3360_spi_start(index);
     wait_us(40);
     spi_stop();
     wait_us(40);
-
-    // power up, need to first drive NCS high then low, see above.
-    pmw3360_write(REG_Power_Up_Reset, 0x5a);
+    pmw3360_write(index, REG_Power_Up_Reset, 0x5a);
     wait_ms(50);
 
     // read registers and discard
-    pmw3360_read(REG_Motion);
-    pmw3360_read(REG_Delta_X_L);
-    pmw3360_read(REG_Delta_X_H);
-    pmw3360_read(REG_Delta_Y_L);
-    pmw3360_read(REG_Delta_Y_H);
+    pmw3360_read(index, REG_Motion);
+    pmw3360_read(index, REG_Delta_X_L);
+    pmw3360_read(index, REG_Delta_X_H);
+    pmw3360_read(index, REG_Delta_Y_L);
+    pmw3360_read(index, REG_Delta_Y_H);
 
-    pmw3360_upload_firmware();
+    pmw3360_upload_firmware(index);
 
     spi_stop();
 
@@ -174,13 +205,13 @@ bool pmw3360_init(void) {
 
     wait_ms(1);
 
-    pmw3360_write(REG_Config2, 0x00);
+    pmw3360_write(index, REG_Config2, 0x00);
 
-    pmw3360_write(REG_Angle_Tune, constrain(ROTATIONAL_TRANSFORM_ANGLE, -127, 127));
+    pmw3360_write(index, REG_Angle_Tune, constrain(ROTATIONAL_TRANSFORM_ANGLE, -127, 127));
 
-    pmw3360_write(REG_Lift_Config, PMW3360_LIFTOFF_DISTANCE);
+    pmw3360_write(index, REG_Lift_Config, PMW3360_LIFTOFF_DISTANCE);
 
-    bool init_success = pmw3360_check_signature();
+    bool init_success = pmw3360_check_signature(index);
 #ifdef CONSOLE_ENABLE
     if (init_success) {
         dprintf("pmw3360 signature verified");
@@ -189,66 +220,38 @@ bool pmw3360_init(void) {
     }
 #endif
 
-    writePinLow(PMW3360_CS_PIN);
-
     return init_success;
 }
 
-void pmw3360_upload_firmware(void) {
-    // Datasheet claims we need to disable REST mode first, but during startup
-    // it's already disabled and we're not turning it on ...
-    // pmw3360_write(REG_Config2, 0x00);  // disable REST mode
-    pmw3360_write(REG_SROM_Enable, 0x1d);
-
-    wait_ms(10);
-
-    pmw3360_write(REG_SROM_Enable, 0x18);
-
-    pmw3360_spi_start();
-    spi_write(REG_SROM_Load_Burst | 0x80);
-    wait_us(15);
-
-    for (uint16_t i = 0; i < FIRMWARE_LENGTH; i++) {
-        spi_write(pgm_read_byte(firmware_data + i));
-#ifndef PMW3360_FIRMWARE_UPLOAD_FAST
-        wait_us(15);
-#endif
-    }
-    wait_us(200);
-
-    pmw3360_read(REG_SROM_ID);
-    pmw3360_write(REG_Config2, 0x00);
-}
-
-bool pmw3360_check_signature(void) {
-    uint8_t pid      = pmw3360_read(REG_Product_ID);
-    uint8_t iv_pid   = pmw3360_read(REG_Inverse_Product_ID);
-    uint8_t SROM_ver = pmw3360_read(REG_SROM_ID);
-    return (pid == firmware_signature[0] && iv_pid == firmware_signature[1] && SROM_ver == firmware_signature[2]); // signature for SROM 0x04
-}
-
+// Only support reading the value from sensor #0, no one is using this anyway.
 uint16_t pmw3360_get_cpi(void) {
-    uint8_t cpival = pmw3360_read(REG_Config1);
+    uint8_t cpival = pmw3360_read(0, REG_Config1);
     return (uint16_t)((cpival + 1) & 0xFF) * CPI_STEP;
 }
 
+// Write same CPI to all sensors.
 void pmw3360_set_cpi(uint16_t cpi) {
     uint8_t cpival = constrain((cpi / CPI_STEP) - 1, 0, MAX_CPI);
-    pmw3360_write(REG_Config1, cpival);
+    for (size_t i = 0; i < NUMBER_OF_SENSORS; i++) {
+        pmw3360_write(i, REG_Config1, cpival);
+    }
 }
 
-report_pmw3360_t pmw3360_read_burst(void) {
+report_pmw3360_t pmw3360_read_burst(int8_t index) {
     report_pmw3360_t report = {0};
+    if (index >= NUMBER_OF_SENSORS) {
+        return report;
+    }
 
-    if (!_inBurst) {
+    if (!_inBurst[index]) {
 #ifdef CONSOLE_ENABLE
-        dprintf("burst on");
+        dprintf("burst on for index %d", index);
 #endif
-        pmw3360_write(REG_Motion_Burst, 0x00);
-        _inBurst = true;
+        pmw3360_write(index, REG_Motion_Burst, 0x00);
+        _inBurst[index] = true;
     }
 
-    pmw3360_spi_start();
+    pmw3360_spi_start(index);
     spi_write(REG_Motion_Burst);
     wait_us(35); // waits for tSRAD_MOTBR
 
@@ -261,7 +264,7 @@ report_pmw3360_t pmw3360_read_burst(void) {
     report.mdy = spi_read();
 
     if (report.motion & 0b111) { // panic recovery, sometimes burst mode works weird.
-        _inBurst = false;
+        _inBurst[index] = false;
     }
 
     spi_stop();
diff --git a/drivers/sensors/pmw3360.h b/drivers/sensors/pmw3360.h
index eec7295871..3aa8ed0ed8 100644
--- a/drivers/sensors/pmw3360.h
+++ b/drivers/sensors/pmw3360.h
@@ -52,8 +52,14 @@
 #    define ROTATIONAL_TRANSFORM_ANGLE 0x00
 #endif
 
-#ifndef PMW3360_CS_PIN
-#    error "No chip select pin defined -- missing PMW3360_CS_PIN"
+// Support single and plural spellings
+#ifndef PMW3360_CS_PINS
+#    ifndef PMW3360_CS_PIN
+#        error "No chip select pin defined -- missing PMW3360_CS_PIN or PMW3360_CS_PINS"
+#    else
+#        define PMW3360_CS_PINS \
+            { PMW3360_CS_PIN }
+#    endif
 #endif
 
 typedef struct {
@@ -66,10 +72,8 @@ typedef struct {
     int8_t  mdy;
 } report_pmw3360_t;
 
-bool     pmw3360_init(void);
-void     pmw3360_upload_firmware(void);
-bool     pmw3360_check_signature(void);
+bool     pmw3360_init(int8_t index);
 uint16_t pmw3360_get_cpi(void);
 void     pmw3360_set_cpi(uint16_t cpi);
 /* Reads and clears the current delta values on the sensor */
-report_pmw3360_t pmw3360_read_burst(void);
+report_pmw3360_t pmw3360_read_burst(int8_t index);
diff --git a/quantum/pointing_device_drivers.c b/quantum/pointing_device_drivers.c
index 11cbf6594e..56363c7ac6 100644
--- a/quantum/pointing_device_drivers.c
+++ b/quantum/pointing_device_drivers.c
@@ -200,11 +200,11 @@ const pointing_device_driver_t pointing_device_driver = {
 // clang-format on
 #elif defined(POINTING_DEVICE_DRIVER_pmw3360)
 static void pmw3360_device_init(void) {
-    pmw3360_init();
+    pmw3360_init(0);
 }
 
 report_mouse_t pmw3360_get_report(report_mouse_t mouse_report) {
-    report_pmw3360_t data        = pmw3360_read_burst();
+    report_pmw3360_t data        = pmw3360_read_burst(0);
     static uint16_t  MotionStart = 0; // Timer for accel, 0 is resting state
 
     if (data.isOnSurface && data.isMotion) {