From: Roderick Colenbrander <roderick.colenbrander@sony.com>
Hi,
I would like to share an updated version of the hid-playstation driver.
This new revision is very similar to the original release, but with
various small changes and minor improvements. Thanks to everyone who
provided feedback through the mailing list or privately.
Changes since v1:
- Export ps_devices table using MODULE_DEVICE_TABLE.
- Uses 'width - 1' and 'height - 1' for touchpad ABS_MT_POSITION_X/Y.
- Uses 'sysfs_emit' instead of 'snprintf' in sysfs show functions.
- Switched to devm_device_add_group to manage attribute groups.
- Changed player LED lookup to use pointer calculations instead of a loop.
- Added readonly mute set_brightness call to silence ENOTSUP.
- Lightbar is not dynamically allocated and now allocated as part of 'struct dualsense'
- Size checking on hid_hw_raw_request for calibration, firmware and mac addrees info.
- Replaced magic constants with defines e.g. for HID version or output report values.
- Minor code changes e.g. const changes.
- Uses container_of to retrieve 'struct ps_device' from 'struct dualsense'.
- Added an explicit module_init/module_exit to clean up the player IDA.
Thanks,
Roderick Colenbrander
Sony Interactive Entertainment, LLC
Roderick Colenbrander (13):
HID: playstation: initial DualSense USB support.
HID: playstation: use DualSense MAC address as unique identifier.
HID: playstation: add DualSense battery support.
HID: playstation: add DualSense touchpad support.
HID: playstation: add DualSense accelerometer and gyroscope support.
HID: playstation: track devices in list.
HID: playstation: add DualSense Bluetooth support.
HID: playstation: add DualSense classic rumble support.
HID: playstation: add DualSense lightbar support
HID: playstation: add microphone mute support for DualSense.
HID: playstation: add DualSense player LEDs support.
HID: playstation: DualSense set LEDs to default player id.
HID: playstation: report DualSense hardware and firmware version.
MAINTAINERS | 6 +
drivers/hid/Kconfig | 20 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-playstation.c | 1448 +++++++++++++++++++++++++++++++++
drivers/hid/hid-quirks.c | 4 +
6 files changed, 1480 insertions(+)
create mode 100644 drivers/hid/hid-playstation.c
--
2.26.2
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
Implement support for PlayStation DualSense gamepad in USB mode.
Support features include buttons and sticks, which adhere to the
Linux gamepad spec.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
MAINTAINERS | 6 +
drivers/hid/Kconfig | 9 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-playstation.c | 321 ++++++++++++++++++++++++++++++++++
drivers/hid/hid-quirks.c | 3 +
6 files changed, 341 insertions(+)
create mode 100644 drivers/hid/hid-playstation.c
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
Use the DualSense MAC address as a unique identifier for the HID device.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
drivers/hid/hid-playstation.c | 40 +++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
@@ -162,6 +167,34 @@ static struct input_dev *ps_gamepad_create(struct hid_device *hdev)returngamepad;}+staticintdualsense_get_mac_address(structdualsense*ds)+{+uint8_t*buf;+intret=0;++buf=kzalloc(DS_FEATURE_REPORT_PAIRING_INFO_SIZE,GFP_KERNEL);+if(!buf)+return-ENOMEM;++ret=hid_hw_raw_request(ds->base.hdev,DS_FEATURE_REPORT_PAIRING_INFO,buf,+DS_FEATURE_REPORT_PAIRING_INFO_SIZE,HID_FEATURE_REPORT,+HID_REQ_GET_REPORT);+if(ret<0)+gotoerr_free;+elseif(ret!=DS_FEATURE_REPORT_PAIRING_INFO_SIZE){+hid_err(ds->base.hdev,"failed to retrieve DualSense pairing info\n");+ret=-EINVAL;+gotoerr_free;+}++/* Note MAC address is stored in little endian order. */+memcpy(ds->base.mac_address,&buf[1],sizeof(ds->base.mac_address));++err_free:+kfree(buf);+returnret;+}+staticintdualsense_parse_report(structps_device*ps_dev,structhid_report*report,u8*data,intsize){
@@ -230,6 +263,13 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)ds->base.parse_report=dualsense_parse_report;hid_set_drvdata(hdev,ds);+ret=dualsense_get_mac_address(ds);+if(ret<0){+hid_err(hdev,"Failed to get MAC address from DualSense\n");+returnERR_PTR(ret);+}+snprintf(hdev->uniq,sizeof(hdev->uniq),"%pMR",ds->base.mac_address);+ds->gamepad=ps_gamepad_create(hdev);if(IS_ERR(ds->gamepad)){ret=PTR_ERR(ds->gamepad);
@@ -21,6 +21,13 @@/* Base class for playstation devices. */structps_device{structhid_device*hdev;+spinlock_tlock;++structpower_supply_descbattery_desc;+structpower_supply*battery;+uint8_tbattery_capacity;+intbattery_status;+uint8_tmac_address[6];int(*parse_report)(structps_device*dev,structhid_report*report,u8*data,intsize);
@@ -48,6 +55,11 @@ struct ps_device {#define DS_BUTTONS2_PS_HOME BIT(0)#define DS_BUTTONS2_TOUCHPAD BIT(1)+/* Status field of DualSense input report. */+#define DS_STATUS_BATTERY_CAPACITY GENMASK(3, 0)+#define DS_STATUS_CHARGING GENMASK(7, 4)+#define DS_STATUS_CHARGING_SHIFT 4+structdualsense{structps_devicebase;structinput_dev*gamepad;
@@ -201,7 +278,9 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *rstructhid_device*hdev=ps_dev->hdev;structdualsense*ds=container_of(ps_dev,structdualsense,base);structdualsense_input_report*ds_report;-uint8_tvalue;+uint8_tbattery_data,battery_capacity,charging_status,value;+intbattery_status;+unsignedlongflags;/* DualSense in USB uses the full HID report for reportID 1, but*BluetoothusesaminimalHIDreportforreportID1andreports
@@ -242,12 +321,48 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *rinput_report_key(ds->gamepad,BTN_MODE,ds_report->buttons[2]&DS_BUTTONS2_PS_HOME);input_sync(ds->gamepad);+battery_data=ds_report->status&DS_STATUS_BATTERY_CAPACITY;+charging_status=(ds_report->status&DS_STATUS_CHARGING)>>DS_STATUS_CHARGING_SHIFT;++switch(charging_status){+case0x0:+/* Each unit of battery data corresponds to 10%+*0=0-9%,1=10-19%,..and10=100%+*/+battery_capacity=battery_data==10?100:battery_data*10+5;+battery_status=POWER_SUPPLY_STATUS_DISCHARGING;+break;+case0x1:+battery_capacity=battery_data==10?100:battery_data*10+5;+battery_status=POWER_SUPPLY_STATUS_CHARGING;+break;+case0x2:+battery_capacity=100;+battery_status=POWER_SUPPLY_STATUS_FULL;+break;+case0xa:/* voltage or temperature out of range */+case0xb:/* temperature error */+battery_capacity=0;+battery_status=POWER_SUPPLY_STATUS_NOT_CHARGING;+break;+case0xf:/* charging error */+default:+battery_capacity=0;+battery_status=POWER_SUPPLY_STATUS_UNKNOWN;+}++spin_lock_irqsave(&ps_dev->lock,flags);+ps_dev->battery_capacity=battery_capacity;+ps_dev->battery_status=battery_status;+spin_unlock_irqrestore(&ps_dev->lock,flags);+return0;}staticstructps_device*dualsense_create(structhid_device*hdev){structdualsense*ds;+structps_device*ps_dev;intret;ds=devm_kzalloc(&hdev->dev,sizeof(*ds),GFP_KERNEL);
@@ -259,8 +374,11 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)*/hdev->version|=HID_PLAYSTATION_VERSION_PATCH;-ds->base.hdev=hdev;-ds->base.parse_report=dualsense_parse_report;+ps_dev=&ds->base;+ps_dev->hdev=hdev;+ps_dev->battery_capacity=100;/* initial value until parse_report. */+ps_dev->battery_status=POWER_SUPPLY_STATUS_UNKNOWN;+ps_dev->parse_report=dualsense_parse_report;hid_set_drvdata(hdev,ds);ret=dualsense_get_mac_address(ds);
@@ -281,6 +320,7 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *ruint8_tbattery_data,battery_capacity,charging_status,value;intbattery_status;unsignedlongflags;+inti;/* DualSense in USB uses the full HID report for reportID 1, but*BluetoothusesaminimalHIDreportforreportID1andreports
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
The DualSense features an accelerometer and gyroscope. The data is
embedded into the main HID input reports. Expose both sensors through
through a separate evdev node.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
drivers/hid/hid-playstation.c | 202 ++++++++++++++++++++++++++++++++++
1 file changed, 202 insertions(+)
@@ -283,6 +342,97 @@ static struct input_dev *ps_touchpad_create(struct hid_device *hdev, int width,returntouchpad;}+staticintdualsense_get_calibration_data(structdualsense*ds)+{+shortgyro_pitch_bias,gyro_pitch_plus,gyro_pitch_minus;+shortgyro_yaw_bias,gyro_yaw_plus,gyro_yaw_minus;+shortgyro_roll_bias,gyro_roll_plus,gyro_roll_minus;+shortgyro_speed_plus,gyro_speed_minus;+shortacc_x_plus,acc_x_minus;+shortacc_y_plus,acc_y_minus;+shortacc_z_plus,acc_z_minus;+intspeed_2x;+intrange_2g;+intret=0;+uint8_t*buf;++buf=kzalloc(DS_FEATURE_REPORT_CALIBRATION_SIZE,GFP_KERNEL);+if(!buf)+return-ENOMEM;++ret=hid_hw_raw_request(ds->base.hdev,DS_FEATURE_REPORT_CALIBRATION,buf,+DS_FEATURE_REPORT_CALIBRATION_SIZE,HID_FEATURE_REPORT,HID_REQ_GET_REPORT);+if(ret<0)+gotoerr_free;+elseif(ret!=DS_FEATURE_REPORT_CALIBRATION_SIZE){+hid_err(ds->base.hdev,"failed to retrieve DualSense calibration info\n");+ret=-EINVAL;+gotoerr_free;+}++gyro_pitch_bias=get_unaligned_le16(&buf[1]);+gyro_yaw_bias=get_unaligned_le16(&buf[3]);+gyro_roll_bias=get_unaligned_le16(&buf[5]);+gyro_pitch_plus=get_unaligned_le16(&buf[7]);+gyro_pitch_minus=get_unaligned_le16(&buf[9]);+gyro_yaw_plus=get_unaligned_le16(&buf[11]);+gyro_yaw_minus=get_unaligned_le16(&buf[13]);+gyro_roll_plus=get_unaligned_le16(&buf[15]);+gyro_roll_minus=get_unaligned_le16(&buf[17]);+gyro_speed_plus=get_unaligned_le16(&buf[19]);+gyro_speed_minus=get_unaligned_le16(&buf[21]);+acc_x_plus=get_unaligned_le16(&buf[23]);+acc_x_minus=get_unaligned_le16(&buf[25]);+acc_y_plus=get_unaligned_le16(&buf[27]);+acc_y_minus=get_unaligned_le16(&buf[29]);+acc_z_plus=get_unaligned_le16(&buf[31]);+acc_z_minus=get_unaligned_le16(&buf[33]);++/* Set gyroscope calibration and normalization parameters.+*Datavalueswillbenormalizedto1/DS_GYRO_RES_PER_DEG_Sdegree/s.+*/+speed_2x=(gyro_speed_plus+gyro_speed_minus);+ds->gyro_calib_data[0].abs_code=ABS_RX;+ds->gyro_calib_data[0].bias=gyro_pitch_bias;+ds->gyro_calib_data[0].sens_numer=speed_2x*DS_GYRO_RES_PER_DEG_S;+ds->gyro_calib_data[0].sens_denom=gyro_pitch_plus-gyro_pitch_minus;++ds->gyro_calib_data[1].abs_code=ABS_RY;+ds->gyro_calib_data[1].bias=gyro_yaw_bias;+ds->gyro_calib_data[1].sens_numer=speed_2x*DS_GYRO_RES_PER_DEG_S;+ds->gyro_calib_data[1].sens_denom=gyro_yaw_plus-gyro_yaw_minus;++ds->gyro_calib_data[2].abs_code=ABS_RZ;+ds->gyro_calib_data[2].bias=gyro_roll_bias;+ds->gyro_calib_data[2].sens_numer=speed_2x*DS_GYRO_RES_PER_DEG_S;+ds->gyro_calib_data[2].sens_denom=gyro_roll_plus-gyro_roll_minus;++/* Set accelerometer calibration and normalization parameters.+*Datavalueswillbenormalizedto1/DS_ACC_RES_PER_GG.+*/+range_2g=acc_x_plus-acc_x_minus;+ds->accel_calib_data[0].abs_code=ABS_X;+ds->accel_calib_data[0].bias=acc_x_plus-range_2g/2;+ds->accel_calib_data[0].sens_numer=2*DS_ACC_RES_PER_G;+ds->accel_calib_data[0].sens_denom=range_2g;++range_2g=acc_y_plus-acc_y_minus;+ds->accel_calib_data[1].abs_code=ABS_Y;+ds->accel_calib_data[1].bias=acc_y_plus-range_2g/2;+ds->accel_calib_data[1].sens_numer=2*DS_ACC_RES_PER_G;+ds->accel_calib_data[1].sens_denom=range_2g;++range_2g=acc_z_plus-acc_z_minus;+ds->accel_calib_data[2].abs_code=ABS_Z;+ds->accel_calib_data[2].bias=acc_z_plus-range_2g/2;+ds->accel_calib_data[2].sens_numer=2*DS_ACC_RES_PER_G;+ds->accel_calib_data[2].sens_denom=range_2g;++err_free:+kfree(buf);+returnret;+}+staticintdualsense_get_mac_address(structdualsense*ds){uint8_t*buf;
@@ -361,6 +512,44 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *rinput_report_key(ds->gamepad,BTN_MODE,ds_report->buttons[2]&DS_BUTTONS2_PS_HOME);input_sync(ds->gamepad);+/* Parse and calibrate gyroscope data. */+for(i=0;i<3;i++){+intraw_data=(short)le16_to_cpu(ds_report->gyro[i]);+intcalib_data=mult_frac(ds->gyro_calib_data[i].sens_numer,+raw_data-ds->gyro_calib_data[i].bias,+ds->gyro_calib_data[i].sens_denom);++input_report_abs(ds->sensors,ds->gyro_calib_data[i].abs_code,calib_data);+}++/* Parse and calibrate accelerometer data. */+for(i=0;i<3;i++){+intraw_data=(short)le16_to_cpu(ds_report->accel[i]);+intcalib_data=mult_frac(ds->accel_calib_data[i].sens_numer,+raw_data-ds->accel_calib_data[i].bias,+ds->accel_calib_data[i].sens_denom);++input_report_abs(ds->sensors,ds->accel_calib_data[i].abs_code,calib_data);+}++/* Convert timestamp (in 0.33us unit) to timestamp_us */+sensor_timestamp=le32_to_cpu(ds_report->sensor_timestamp);+if(!ds->sensor_timestamp_initialized){+ds->sensor_timestamp_us=sensor_timestamp/3;+ds->sensor_timestamp_initialized=true;+}else{+uint32_tdelta;++if(ds->prev_sensor_timestamp>sensor_timestamp)+delta=(U32_MAX-ds->prev_sensor_timestamp+sensor_timestamp+1);+else+delta=sensor_timestamp-ds->prev_sensor_timestamp;+ds->sensor_timestamp_us+=delta/3;+}+ds->prev_sensor_timestamp=sensor_timestamp;+input_event(ds->sensors,EV_MSC,MSC_TIMESTAMP,ds->sensor_timestamp_us);+input_sync(ds->sensors);+for(i=0;i<2;i++){boolactive=(ds_report->points[i].contact&DS_TOUCH_POINT_INACTIVE)?false:true;
@@ -446,12 +635,25 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)}snprintf(hdev->uniq,sizeof(hdev->uniq),"%pMR",ds->base.mac_address);+ret=dualsense_get_calibration_data(ds);+if(ret<0){+hid_err(hdev,"Failed to get calibration data from DualSense\n");+gotoerr;+}+ds->gamepad=ps_gamepad_create(hdev);if(IS_ERR(ds->gamepad)){ret=PTR_ERR(ds->gamepad);gotoerr;}+ds->sensors=ps_sensors_create(hdev,DS_ACC_RANGE,DS_ACC_RES_PER_G,+DS_GYRO_RANGE,DS_GYRO_RES_PER_DEG_S);+if(IS_ERR(ds->sensors)){+ret=PTR_ERR(ds->sensors);+gotoerr;+}+ds->touchpad=ps_touchpad_create(hdev,DS_TOUCHPAD_WIDTH,DS_TOUCHPAD_HEIGHT,2);if(IS_ERR(ds->touchpad)){ret=PTR_ERR(ds->touchpad);
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
Track devices in a list, so we can detect when a device is connected
twice when using Bluetooth and USB.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
drivers/hid/hid-playstation.c | 45 +++++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
@@ -16,10 +16,15 @@#include"hid-ids.h"+/* List of connected playstation devices. */+staticDEFINE_MUTEX(ps_devices_lock);+staticLIST_HEAD(ps_devices_list);+#define HID_PLAYSTATION_VERSION_PATCH 0x8000/* Base class for playstation devices. */structps_device{+structlist_headlist;structhid_device*hdev;spinlock_tlock;
@@ -155,6 +160,37 @@ static const struct {int x; int y; } ps_gamepad_hat_mapping[] = {{0,0}};+/* Add a new ps_device to ps_devices if it doesn't exist.+*Returnerroronduplicatedevice,whichcanhappenifthesame+*deviceisconnectedusingbothBluetoothandUSB.+*/+staticintps_devices_list_add(structps_device*dev)+{+structps_device*entry;++mutex_lock(&ps_devices_lock);+list_for_each_entry(entry,&ps_devices_list,list){+if(!memcmp(entry->mac_address,dev->mac_address,sizeof(dev->mac_address))){+hid_err(dev->hdev,"Duplicate device found for MAC address %pMR\n",+dev->mac_address);+mutex_unlock(&ps_devices_lock);+return-EEXIST;+}+}++list_add_tail(&dev->list,&ps_devices_list);+mutex_unlock(&ps_devices_lock);+return0;+}++staticintps_devices_list_remove(structps_device*dev)+{+mutex_lock(&ps_devices_lock);+list_del(&dev->list);+mutex_unlock(&ps_devices_lock);+return0;+}+staticstructinput_dev*ps_allocate_input_dev(structhid_device*hdev,constchar*name_suffix){structinput_dev*input_dev;
@@ -635,6 +671,10 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)}snprintf(hdev->uniq,sizeof(hdev->uniq),"%pMR",ds->base.mac_address);+ret=ps_devices_list_add(ps_dev);+if(ret<0)+returnERR_PTR(ret);+ret=dualsense_get_calibration_data(ds);if(ret<0){hid_err(hdev,"Failed to get calibration data from DualSense\n");
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
This patch adds support for the DualSense when operating in Bluetooth mode.
The device has the same behavior as the DualShock 4 in that by default it
sends a limited input report (0x1), but after requesting calibration data,
it switches to an extended input report (report 49), which adds data for
touchpad, motion sensors, battery and more.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
drivers/hid/hid-playstation.c | 35 +++++++++++++++++++++++++++++++++++
drivers/hid/hid-quirks.c | 1 +
2 files changed, 36 insertions(+)
@@ -285,6 +286,17 @@ static int ps_device_register_battery(struct ps_device *dev)return0;}+/* Compute crc32 of HID data and compare against expected CRC. */+staticboolps_check_crc32(uint8_tseed,uint8_t*data,size_tlen,uint32_treport_crc)+{+uint32_tcrc;++crc=crc32_le(0xFFFFFFFF,&seed,1);+crc=~crc32_le(crc,data,len);++returncrc==report_crc;+}+staticstructinput_dev*ps_gamepad_create(structhid_device*hdev){structinput_dev*gamepad;
@@ -406,6 +418,18 @@ static int dualsense_get_calibration_data(struct dualsense *ds)gotoerr_free;}+if(ds->base.hdev->bus==BUS_BLUETOOTH){+/* Last 4 bytes contains crc32 */+uint8_tcrc_offset=DS_FEATURE_REPORT_CALIBRATION_SIZE-4;+uint32_treport_crc=get_unaligned_le32(&buf[crc_offset]);++if(!ps_check_crc32(0xa3,buf,crc_offset,report_crc)){+hid_err(ds->base.hdev,"DualSense calibration report CRC's check failed\n");+ret=-EILSEQ;+gotoerr_free;+}+}+gyro_pitch_bias=get_unaligned_le16(&buf[1]);gyro_yaw_bias=get_unaligned_le16(&buf[3]);gyro_roll_bias=get_unaligned_le16(&buf[5]);
@@ -515,6 +539,16 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r*/if(report->id==DS_INPUT_REPORT_USB&&hdev->bus==BUS_USB){ds_report=(structdualsense_input_report*)&data[1];+}elseif(report->id==DS_INPUT_REPORT_BT&&hdev->bus==BUS_BLUETOOTH){+/* Last 4 bytes of input report contain crc32 */+uint32_treport_crc=get_unaligned_le32(&data[size-4]);++if(!ps_check_crc32(0xa1,data,size-4,report_crc)){+hid_err(hdev,"DualSense input CRC's check failed, size=%d\n",size);+return-EILSEQ;+}++ds_report=(structdualsense_input_report*)&data[2];}else{hid_err(hdev,"Unhandled reportID=%d\n",report->id);return-1;
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
The DualSense features a haptics system based on voicecoil motors,
which requires PCM data (or special HID packets using Bluetooth). There
is no appropriate API yet in the Linux kernel to expose these. The
controller also provides a classic rumble feature for backwards
compatibility. Expose this classic rumble feature using the FF framework.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
drivers/hid/Kconfig | 8 ++
drivers/hid/hid-playstation.c | 196 +++++++++++++++++++++++++++++++++-
2 files changed, 202 insertions(+), 2 deletions(-)
@@ -104,6 +110,15 @@ struct dualsense {boolsensor_timestamp_initialized;uint32_tprev_sensor_timestamp;uint32_tsensor_timestamp_us;++/* Compatible rumble state */+boolupdate_rumble;+uint8_tmotor_left;+uint8_tmotor_right;++structwork_structoutput_worker;+void*output_report_dmabuf;+uint8_toutput_seq;/* Sequence number for output report. */};structdualsense_touch_point{
@@ -136,6 +151,63 @@ struct dualsense_input_report {uint8_treserved4[11];}__packed;+/* Common data between DualSense BT/USB main output report. */+structdualsense_output_report_common{+uint8_tvalid_flag0;+uint8_tvalid_flag1;++/* For DualShock 4 compatibility mode. */+uint8_tmotor_right;+uint8_tmotor_left;++/* Audio controls */+uint8_treserved[4];+uint8_tmute_button_led;++uint8_tpower_save_control;+uint8_treserved2[28];++/* LEDs and lightbar */+uint8_tvalid_flag2;+uint8_treserved3[2];+uint8_tlightbar_setup;+uint8_tled_brightness;+uint8_tplayer_leds;+uint8_tlightbar_red;+uint8_tlightbar_green;+uint8_tlightbar_blue;+}__packed;++structdualsense_output_report_bt{+uint8_treport_id;/* 0x31 */+uint8_tseq_tag;+uint8_ttag;+structdualsense_output_report_commoncommon;+uint8_treserved[24];+__le32crc32;+}__packed;++structdualsense_output_report_usb{+uint8_treport_id;/* 0x02 */+structdualsense_output_report_commoncommon;+}__packed;++/* The DualSense has a main output report used to control most features. It is+*largelythesamebetweenBluetoothandUSBexceptfordifferentheadersandCRC.+*Thisstructurehidethedifferencesbetweenthetwotosimplifysendingoutputreports.+*/+structdualsense_output_report{+uint8_t*data;/* Start of data */+uint8_tlen;/* Size of output report */++/* Points to Bluetooth data payload in case for a Bluetooth report else NULL. */+structdualsense_output_report_bt*bt;+/* Points to USB data payload in case for a USB report else NULL. */+structdualsense_output_report_usb*usb;+/* Points to common section of report, so past any headers */+structdualsense_output_report_common*common;+};+/* Common gamepad buttons across DualShock 3 / 4 and DualSense.*Note:fordevicewithatouchpad,touchpadbuttonisnotincluded*asitwillbepartofthetouchpaddevice.
@@ -521,6 +601,92 @@ static int dualsense_get_mac_address(struct dualsense *ds)returnret;}+staticvoiddualsense_init_output_report(structdualsense*ds,structdualsense_output_report*rp,+void*buf)+{+structhid_device*hdev=ds->base.hdev;++if(hdev->bus==BUS_BLUETOOTH){+structdualsense_output_report_bt*bt=buf;++memset(bt,0,sizeof(*bt));+bt->report_id=DS_OUTPUT_REPORT_BT;+bt->tag=0x10;/* Magic number must be set to 0x10 */++/* Highest 4-bit is a sequence number, which needs to be increased+*everyreport.Lowest4-bitistagandcanbezerofornow.+*/+bt->seq_tag=(ds->output_seq<<4)|0x0;+if(++ds->output_seq==15)+ds->output_seq=0;++rp->data=buf;+rp->len=sizeof(*bt);+rp->bt=bt;+rp->usb=NULL;+rp->common=&bt->common;+}else{/* USB */+structdualsense_output_report_usb*usb=buf;++memset(usb,0,sizeof(*usb));+usb->report_id=DS_OUTPUT_REPORT_USB;++rp->data=buf;+rp->len=sizeof(*usb);+rp->bt=NULL;+rp->usb=usb;+rp->common=&usb->common;+}+}++/* Helper function to send DualSense output reports. Applies a CRC at the end of a report+*forBluetoothreports.+*/+staticvoiddualsense_send_output_report(structdualsense*ds,+structdualsense_output_report*report)+{+structhid_device*hdev=ds->base.hdev;++/* Bluetooth packets need to be signed with a CRC in the last 4 bytes. */+if(report->bt){+uint32_tcrc;+uint8_tseed=0xA2;++crc=crc32_le(0xFFFFFFFF,&seed,1);+crc=~crc32_le(crc,report->data,report->len-4);++report->bt->crc32=cpu_to_le32(crc);+}++hid_hw_output_report(hdev,report->data,report->len);+}++staticvoiddualsense_output_worker(structwork_struct*work)+{+structdualsense*ds=container_of(work,structdualsense,output_worker);+structdualsense_output_reportreport;+structdualsense_output_report_common*common;+unsignedlongflags;++dualsense_init_output_report(ds,&report,ds->output_report_dmabuf);+common=report.common;++spin_lock_irqsave(&ds->base.lock,flags);++if(ds->update_rumble){+/* Select classic rumble style haptics and enable it. */+common->valid_flag0|=DS_OUTPUT_VALID_FLAG0_HAPTICS_SELECT;+common->valid_flag0|=DS_OUTPUT_VALID_FLAG0_COMPATIBLE_VIBRATION;+common->motor_left=ds->motor_left;+common->motor_right=ds->motor_right;+ds->update_rumble=false;+}++spin_unlock_irqrestore(&ds->base.lock,flags);++dualsense_send_output_report(ds,&report);+}+staticintdualsense_parse_report(structps_device*ps_dev,structhid_report*report,u8*data,intsize){
@@ -696,8 +882,14 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)ps_dev->battery_capacity=100;/* initial value until parse_report. */ps_dev->battery_status=POWER_SUPPLY_STATUS_UNKNOWN;ps_dev->parse_report=dualsense_parse_report;+INIT_WORK(&ds->output_worker,dualsense_output_worker);hid_set_drvdata(hdev,ds);+max_output_report_size=sizeof(structdualsense_output_report_bt);+ds->output_report_dmabuf=devm_kzalloc(&hdev->dev,max_output_report_size,GFP_KERNEL);+if(!ds->output_report_dmabuf)+returnERR_PTR(-ENOMEM);+ret=dualsense_get_mac_address(ds);if(ret<0){hid_err(hdev,"Failed to get MAC address from DualSense\n");
@@ -861,6 +942,30 @@ static int dualsense_play_effect(struct input_dev *dev, void *data, struct ff_efreturn0;}+staticintdualsense_reset_leds(structdualsense*ds)+{+structdualsense_output_reportreport;+uint8_t*buf;++buf=kzalloc(sizeof(structdualsense_output_report_bt),GFP_KERNEL);+if(!buf)+return-ENOMEM;++dualsense_init_output_report(ds,&report,buf);+/* On Bluetooth the DualSense outputs an animation on the lightbar+*duringstartupandmaintainsacolorafterwards.Weneedtoexplicitly+*reconfigurethelightbarbeforewecandoanyprogramminglateron.+*InUSBthelightbarisnotonbydefault,butredoingthesetupthere+*doesn'thurt.+*/+report.common->valid_flag2=DS_OUTPUT_VALID_FLAG2_LIGHTBAR_SETUP_CONTROL_ENABLE;+report.common->lightbar_setup=DS_OUTPUT_LIGHTBAR_SETUP_LIGHT_OUT;/* Fade light out. */+dualsense_send_output_report(ds,&report);++kfree(buf);+return0;+}+staticstructps_device*dualsense_create(structhid_device*hdev){structdualsense*ds;
@@ -930,6 +1035,18 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)if(ret<0)gotoerr;+/* The hardware may have control over the LEDs (e.g. in Bluetooth on startup).+*ResettheLEDs(lightbar,mute,playerleds),sowecancontrolthem+*fromsoftware.+*/+ret=dualsense_reset_leds(ds);+if(ret<0)+gotoerr;++ret=ps_lightbar_register(ps_dev,&ds->lightbar,dualsense_lightbar_set_brightness);+if(ret<0)+gotoerr;+return&ds->base;err:
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
The DualSense controller has a built-in microphone exposed as an
audio device over USB (or HID using Bluetooth). A dedicated
button on the controller handles mute, but software has to configure
the device to mute the audio stream.
This patch captures the mute button and schedules an output report
to mute/unmute the audio stream as well as toggle the mute LED.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
drivers/hid/Kconfig | 1 +
drivers/hid/hid-playstation.c | 102 ++++++++++++++++++++++++++++++++++
2 files changed, 103 insertions(+)
@@ -128,6 +139,12 @@ struct dualsense {uint8_tlightbar_green;uint8_tlightbar_blue;+/* Microphone */+boolupdate_mic_mute;+boolmic_muted;+boollast_btn_mic_state;+structled_classdevmute_led;+structwork_structoutput_worker;void*output_report_dmabuf;uint8_toutput_seq;/* Sequence number for output report. */
@@ -419,6 +436,32 @@ static struct input_dev *ps_gamepad_create(struct hid_device *hdev,returngamepad;}+staticintps_led_register(structps_device*ps_dev,structled_classdev*led,+structps_led_info*led_info)+{+intret;++led->name=devm_kasprintf(&ps_dev->hdev->dev,GFP_KERNEL,+"playstation::%pMR::%s",ps_dev->mac_address,led_info->name);++if(!led->name)+return-ENOMEM;++led->brightness=0;+led->max_brightness=1;+led->flags=LED_CORE_SUSPENDRESUME;+led->brightness_get=led_info->brightness_get;+led->brightness_set=led_info->brightness_set;++ret=devm_led_classdev_register(&ps_dev->hdev->dev,led);+if(ret){+hid_err(ps_dev->hdev,"Failed to register LED %s: %d\n",led_info->name,ret);+returnret;+}++return0;+}+/* Register a DualSense/DualShock4 RGB lightbar represented by a multicolor LED. */staticintps_lightbar_register(structps_device*ps_dev,structled_classdev_mc*lightbar_mc_dev,int(*brightness_set)(structled_classdev*,enumled_brightness))
@@ -673,6 +716,20 @@ static int dualsense_lightbar_set_brightness(struct led_classdev *cdev,return0;}+staticenumled_brightnessdualsense_mute_led_get_brightness(structled_classdev*led)+{+structhid_device*hdev=to_hid_device(led->dev->parent);+structdualsense*ds=hid_get_drvdata(hdev);++returnds->mic_muted;+}++/* The mute LED is treated as read-only. This set call prevents ENOTSUP errors e.g. on unload. */+staticvoiddualsense_mute_led_set_brightness(structled_classdev*led,enumled_brightnessvalue)+{++}+staticvoiddualsense_init_output_report(structdualsense*ds,structdualsense_output_report*rp,void*buf){
@@ -829,6 +907,22 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *rinput_report_key(ds->gamepad,BTN_MODE,ds_report->buttons[2]&DS_BUTTONS2_PS_HOME);input_sync(ds->gamepad);+/* The DualSense has an internal microphone, which can be muted through a mute button+*onthedevice.Thedriverexpectedtoreadthebuttonstateandprogramthedevice+*tomute/unmuteaudioatthehardwarelevel.+*/+btn_mic_state=!!(ds_report->buttons[2]&DS_BUTTONS2_MIC_MUTE);+if(btn_mic_state&&!ds->last_btn_mic_state){+spin_lock_irqsave(&ps_dev->lock,flags);+ds->update_mic_mute=true;+ds->mic_muted=!ds->mic_muted;/* toggle */+spin_unlock_irqrestore(&ps_dev->lock,flags);++/* Schedule updating of microphone state at hardware level. */+schedule_work(&ds->output_worker);+}+ds->last_btn_mic_state=btn_mic_state;+/* Parse and calibrate gyroscope data. */for(i=0;i<3;i++){intraw_data=(short)le16_to_cpu(ds_report->gyro[i]);
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
The DualSense features 5 player LEDs below its touchpad, which are
meant as player id indications. This patch exposes the player LEDs
as individual LEDs.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
drivers/hid/hid-playstation.c | 60 ++++++++++++++++++++++++++++++++++-
1 file changed, 59 insertions(+), 1 deletion(-)
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
Add a ID allocator to assign player ids to ps_device instances.
Utilize the player id to set a default color on the DualSense its
player LED strip.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
drivers/hid/hid-playstation.c | 69 ++++++++++++++++++++++++++++++++++-
1 file changed, 68 insertions(+), 1 deletion(-)
@@ -22,6 +23,8 @@staticDEFINE_MUTEX(ps_devices_lock);staticLIST_HEAD(ps_devices_list);+staticDEFINE_IDA(ps_player_id_allocator);+#define HID_PLAYSTATION_VERSION_PATCH 0x8000/* Base class for playstation devices. */
@@ -299,6 +304,24 @@ static int ps_devices_list_remove(struct ps_device *dev)return0;}+staticintps_device_set_player_id(structps_device*dev)+{+intret=ida_alloc(&ps_player_id_allocator,GFP_KERNEL);++if(ret<0)+returnret;++dev->player_id=ret;+return0;+}++staticvoidps_device_release_player_id(structps_device*dev)+{+ida_free(&ps_player_id_allocator,dev->player_id);++dev->player_id=-1;+}+staticstructinput_dev*ps_allocate_input_dev(structhid_device*hdev,constchar*name_suffix){structinput_dev*input_dev;
@@ -1102,6 +1125,28 @@ static int dualsense_reset_leds(struct dualsense *ds)return0;}+staticvoiddualsense_set_player_leds(structdualsense*ds)+{+/* The DualSense controller has a row of 5 LEDs used for player ids.+*BehavioronthePlayStation5consoleistocentertheplayerid+*acrosstheLEDs,soe.g.player1wouldbe"--x--"withxbeing'on'.+*Followasimilarmappinghere.+*/+intplayer_ids[5]={+BIT(2),+BIT(3)|BIT(1),+BIT(4)|BIT(2)|BIT(0),+BIT(4)|BIT(3)|BIT(1)|BIT(0),+BIT(4)|BIT(3)|BIT(2)|BIT(1)|BIT(0)+};++uint8_tplayer_id=ds->base.player_id%5;++ds->update_player_leds=true;+ds->player_leds_state=player_ids[player_id];+schedule_work(&ds->output_worker);+}+staticstructps_device*dualsense_create(structhid_device*hdev){structdualsense*ds;
@@ -1207,6 +1252,15 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)gotoerr;}+ret=ps_device_set_player_id(ps_dev);+if(ret<0){+hid_err(hdev,"Failed to assign player id for DualSense\n");+gotoerr;+}++/* Set player LEDs to our player id. */+dualsense_set_player_leds(ds);+return&ds->base;err:
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
Retrieve DualSense hardware and firmware information using a vendor
specific feature report. Report the data through sysfs and also
report using hid_info as there can be signficant differences between
versions.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
drivers/hid/hid-playstation.c | 84 +++++++++++++++++++++++++++++++++++
1 file changed, 84 insertions(+)
@@ -696,6 +734,34 @@ static int dualsense_get_calibration_data(struct dualsense *ds)returnret;}+staticintdualsense_get_firmware_info(structdualsense*ds)+{+uint8_t*buf;+intret;++buf=kzalloc(DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE,GFP_KERNEL);+if(!buf)+return-ENOMEM;++ret=hid_hw_raw_request(ds->base.hdev,DS_FEATURE_REPORT_FIRMWARE_INFO,buf,+DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE,HID_FEATURE_REPORT,+HID_REQ_GET_REPORT);+if(ret<0)+gotoerr_free;+elseif(ret!=DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE){+hid_err(ds->base.hdev,"failed to retrieve DualSense firmware info\n");+ret=-EINVAL;+gotoerr_free;+}++ds->base.hw_version=get_unaligned_le32(&buf[24]);+ds->base.fw_version=get_unaligned_le32(&buf[28]);++err_free:+kfree(buf);+returnret;+}+staticintdualsense_get_mac_address(structdualsense*ds){uint8_t*buf;
@@ -1195,6 +1261,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)}snprintf(hdev->uniq,sizeof(hdev->uniq),"%pMR",ds->base.mac_address);+ret=dualsense_get_firmware_info(ds);+if(ret<0){+hid_err(hdev,"Failed to get firmware info from DualSense\n");+returnERR_PTR(ret);+}+ret=ps_devices_list_add(ps_dev);if(ret<0)returnERR_PTR(ret);
@@ -1261,6 +1333,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)/* Set player LEDs to our player id. */dualsense_set_player_leds(ds);+/* Reporting hardware and firmware is important as there are frequent updates, which+*canchangebehavior.+*/+hid_info(hdev,"Registered DualSense controller hw_version=%x fw_version=%x\n",+ds->base.hw_version,ds->base.fw_version);+return&ds->base;err:
@@ -1311,6 +1389,12 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)}}+ret=devm_device_add_group(&hdev->dev,&ps_device_attribute_group);+if(ret<0){+hid_err(hdev,"Failed to register sysfs nodes.\n");+gotoerr_close;+}+returnret;err_close:
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
Implement support for PlayStation DualSense gamepad in USB mode.
Support features include buttons and sticks, which adhere to the
Linux gamepad spec.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
MAINTAINERS | 6 +
drivers/hid/Kconfig | 9 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-playstation.c | 321 ++++++++++++++++++++++++++++++++++
drivers/hid/hid-quirks.c | 3 +
6 files changed, 341 insertions(+)
create mode 100644 drivers/hid/hid-playstation.c
Minor nit: these '!EXPERT' defaults are there only for drivers that were
created during the big "let's separate all the quirks from hid-core into
individual driver" bang that happened ages ago. For new drivers, we follow
what's common in other driver subsystems, and don't force the default.
No need to resend if it'd be just for this change, I can adjust it when
applying.
Thanks,
--
Jiri Kosina
SUSE Labs
From: Benjamin Tissoires <hidden> Date: 2021-01-05 08:22:17
Hi Roderick,
Thanks for the v2. I'll also let the reviewers of v1 do a second pass
on this version, but I have one comment here:
On Sat, Jan 2, 2021 at 11:31 PM Roderick Colenbrander
[off-list ref] wrote:
quoted hunk
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
Implement support for PlayStation DualSense gamepad in USB mode.
Support features include buttons and sticks, which adhere to the
Linux gamepad spec.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
MAINTAINERS | 6 +
drivers/hid/Kconfig | 9 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-playstation.c | 321 ++++++++++++++++++++++++++++++++++
drivers/hid/hid-quirks.c | 3 +
6 files changed, 341 insertions(+)
create mode 100644 drivers/hid/hid-playstation.c
Can you please drop this hunk?
The DS controller "works" fine with hid-generic, and I'd rather not
have this list grows just for the sake of it.
Cheers,
Benjamin
The bits for EV_MSC/MSC_TIMESTAMP events are not set here, hence
timestamp events would not delivered:
__set_bit(EV_MSC, sensors->evbit);
__set_bit(MSC_TIMESTAMP, sensors->mscbit);
quoted hunk
static int dualsense_get_mac_address(struct dualsense *ds)
{
uint8_t *buf;
This uint16_t variable overflows just after a few events. Since the
timestamp from the controller is 32bit and the event value too, I assume
this should be too.
--
Florian Märkl
https://metallic.software
I know that at the moment ENOMEM is the only possible error, but I believe
`return ERR_CAST(gamepad);` would be better. (Or even just `return gamepad;`.)
I think `size` should be checked somewhere around here.
+ /* DualSense in USB uses the full HID report for reportID 1, but
+ * Bluetooth uses a minimal HID report for reportID 1 and reports
+ * the full report using reportID 49.
+ */
+ if (report->id == DS_INPUT_REPORT_USB && hdev->bus == BUS_USB) {
+ ds_report = (struct dualsense_input_report *)&data[1];
+ } else {
+ hid_err(hdev, "Unhandled reportID=%d\n", report->id);
+ return -1;
+ }
+
+ input_report_abs(ds->gamepad, ABS_X, ds_report->x);
+ input_report_abs(ds->gamepad, ABS_Y, ds_report->y);
+ input_report_abs(ds->gamepad, ABS_RX, ds_report->rx);
+ input_report_abs(ds->gamepad, ABS_RY, ds_report->ry);
+ input_report_abs(ds->gamepad, ABS_Z, ds_report->z);
+ input_report_abs(ds->gamepad, ABS_RZ, ds_report->rz);
+
+ value = ds_report->buttons[0] & DS_BUTTONS0_HAT_SWITCH;
+ if (value > 7)
+ value = 8; /* center */
This seems a bit flimsy to me, it relies on a different part of the code
being in a certain way that is not enforced by anything. I'd probably do something
like this:
enum {
HAT_DIR_W = 0,
HAT_DIR_NW,
...
HAT_DIR_SW,
HAT_DIR_NONE,
};
static const struct {int x; int y; } ps_gamepad_hat_mapping[] = {
[HAT_DIR_W] = {0, -1},
...
[HAT_DIR_NONE] = {0, 0},
};
and then
if (value >= ARRAY_SIZE(ps_gamepad_hat_mapping))
value = HAT_DIR_NONE;
Please consider it. By the way, are values 9..15 actually sent by the controller?
I'm still not fully seeing the purpose of this `if`. The probe should not be
called for devices not in the id_table, so this seems to me to be a long way
of writing `if (true)`. Or am I missing something?
+ dev = dualsense_create(hdev);
+ if (IS_ERR(dev)) {
+ hid_err(hdev, "Failed to create dualsense.\n");
I think it'd be preferable if all log messages would either be lowercase or
uppercase, not a mix of both. Same for punctuation. This applies to all patches.
Hi
2021. január 2., szombat 23:30 keltezéssel, Roderick Colenbrander írta:
quoted hunk
From: Roderick Colenbrander roderick.colenbrander@sony.com
Use the DualSense MAC address as a unique identifier for the HID device.
Signed-off-by: Roderick Colenbrander roderick.colenbrander@sony.com
[...]
/* Button masks for DualSense input report. */
#define DS_BUTTONS0_HAT_SWITCH GENMASK(3, 0)
#define DS_BUTTONS0_SQUARE BIT(4)
I believe the preferred way is to have a comma after each array/enum/etc. element
unless it is a terminating entry.
quoted hunk
+};
[...]
@@ -201,7 +278,9 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r struct hid_device *hdev = ps_dev->hdev; struct dualsense *ds = container_of(ps_dev, struct dualsense, base); struct dualsense_input_report *ds_report;- uint8_t value;+ uint8_t battery_data, battery_capacity, charging_status, value;+ int battery_status;+ unsigned long flags; /* DualSense in USB uses the full HID report for reportID 1, but * Bluetooth uses a minimal HID report for reportID 1 and reports
@@ -242,12 +321,48 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r input_report_key(ds->gamepad, BTN_MODE, ds_report->buttons[2] & DS_BUTTONS2_PS_HOME); input_sync(ds->gamepad);+ battery_data = ds_report->status & DS_STATUS_BATTERY_CAPACITY;+ charging_status = (ds_report->status & DS_STATUS_CHARGING) >> DS_STATUS_CHARGING_SHIFT;++ switch (charging_status) {+ case 0x0:+ /* Each unit of battery data corresponds to 10%+ * 0 = 0-9%, 1 = 10-19%, .. and 10 = 100%+ */+ battery_capacity = battery_data == 10 ? 100 : battery_data * 10 + 5;
In my opinion `min(battery_data * 10 + 5, 100)` seems cleaner, what do you think?
quoted hunk
+ battery_status = POWER_SUPPLY_STATUS_DISCHARGING;
+ break;
+ case 0x1:
+ battery_capacity = battery_data == 10 ? 100 : battery_data * 10 + 5;
+ battery_status = POWER_SUPPLY_STATUS_CHARGING;
+ break;
+ case 0x2:
+ battery_capacity = 100;
+ battery_status = POWER_SUPPLY_STATUS_FULL;
+ break;
+ case 0xa: /* voltage or temperature out of range */
+ case 0xb: /* temperature error */
+ battery_capacity = 0;
+ battery_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
+ break;
+ case 0xf: /* charging error */
+ default:
+ battery_capacity = 0;
+ battery_status = POWER_SUPPLY_STATUS_UNKNOWN;
+ }
+
+ spin_lock_irqsave(&ps_dev->lock, flags);
+ ps_dev->battery_capacity = battery_capacity;
+ ps_dev->battery_status = battery_status;
+ spin_unlock_irqrestore(&ps_dev->lock, flags);
+
return 0;
}
static struct ps_device *dualsense_create(struct hid_device *hdev)
{
struct dualsense *ds;
+ struct ps_device *ps_dev;
int ret;
ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL);
Hi
2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
Implement support for DualSense touchpad as a separate input device.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
[...]
+static struct input_dev *ps_touchpad_create(struct hid_device *hdev, int width, int height,
+ unsigned int num_contacts)
+{
+ struct input_dev *touchpad;
+ int ret;
+
+ touchpad = ps_allocate_input_dev(hdev, "Touchpad");
+ if (IS_ERR(touchpad))
+ return ERR_PTR(-ENOMEM);
I know that at the moment ENOMEM is the only possible error, but I believe
`return ERR_CAST(touchpad);` would be better. (Or even just `return touchpad;`.)
Hi
2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
The DualSense features an accelerometer and gyroscope. The data is
embedded into the main HID input reports. Expose both sensors through
through a separate evdev node.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
[...]
+static struct input_dev *ps_sensors_create(struct hid_device *hdev, int accel_range, int accel_res,
+ int gyro_range, int gyro_res)
+{
+ struct input_dev *sensors;
+ int ret;
+
+ sensors = ps_allocate_input_dev(hdev, "Motion Sensors");
+ if (IS_ERR(sensors))
+ return ERR_PTR(-ENOMEM);
I know that at the moment ENOMEM is the only possible error, but I believe
`return ERR_CAST(sensors);` would be better. (Or even just `return sensors;`.)
A small note, as written in [2], the preferred style of multi-line comments is different,
so you might want to change the comments. If you decide to make this change, please
do it everywhere.
@@ -319,6 +469,7 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r struct dualsense_input_report *ds_report; uint8_t battery_data, battery_capacity, charging_status, value; int battery_status;+ uint16_t sensor_timestamp; unsigned long flags; int i;
@@ -361,6 +512,44 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r input_report_key(ds->gamepad, BTN_MODE, ds_report->buttons[2] & DS_BUTTONS2_PS_HOME); input_sync(ds->gamepad);+ /* Parse and calibrate gyroscope data. */+ for (i = 0; i < 3; i++) {
I think `i < ARRAY_SIZE(...)` would be better.
And I would add a `static_assert(ARRAY_SIZE(ds_report->gyro) == ARRAY_SIZE(ds->gyro_calib_data))`
somewhere around here just to be safe. Or define a new constant like `DS_GYRO_DIMS`
and use that to define the arrays. Or both. *
+ int raw_data = (short)le16_to_cpu(ds_report->gyro[i]);
+ int calib_data = mult_frac(ds->gyro_calib_data[i].sens_numer,
+ raw_data - ds->gyro_calib_data[i].bias,
+ ds->gyro_calib_data[i].sens_denom);
I believe it would be better if the second and third lines was aligned. **
+
+ input_report_abs(ds->sensors, ds->gyro_calib_data[i].abs_code, calib_data);
+ }
+
+ /* Parse and calibrate accelerometer data. */
+ for (i = 0; i < 3; i++) {
Same here. *
+ int raw_data = (short)le16_to_cpu(ds_report->accel[i]);
+ int calib_data = mult_frac(ds->accel_calib_data[i].sens_numer,
+ raw_data - ds->accel_calib_data[i].bias,
+ ds->accel_calib_data[i].sens_denom);
Hi
2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:
quoted hunk
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
Track devices in a list, so we can detect when a device is connected
twice when using Bluetooth and USB.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
[...]
It's a minor thing, and I have already mentioned it in regards to other functions,
but I believe if `ps_devices_list_add()` can only return 0 on success, or an errno,
then `if (ret)` would be better.
quoted hunk
+ return ERR_PTR(ret);
+
ret = dualsense_get_calibration_data(ds);
if (ret < 0) {
hid_err(hdev, "Failed to get calibration data from DualSense\n");
Hi
2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:
quoted hunk
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
This patch adds support for the DualSense when operating in Bluetooth mode.
The device has the same behavior as the DualShock 4 in that by default it
sends a limited input report (0x1), but after requesting calibration data,
it switches to an extended input report (report 49), which adds data for
touchpad, motion sensors, battery and more.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
[...]
static struct input_dev *ps_gamepad_create(struct hid_device *hdev)
{
struct input_dev *gamepad;
@@ -406,6 +418,18 @@ static int dualsense_get_calibration_data(struct dualsense *ds) goto err_free; }+ if (ds->base.hdev->bus == BUS_BLUETOOTH) {+ /* Last 4 bytes contains crc32 */
Most other comments have a period at the end. I know this is a minor thing,
but I believe being consistent is better than not.
And shouldn't this CRC be checked in `dualsense_get_mac_address()`?
If so, maybe a helper function could be created that wraps the `hid_hw_raw_request()`
call?
Hi
2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:
quoted hunk
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
The DualSense features a haptics system based on voicecoil motors,
which requires PCM data (or special HID packets using Bluetooth). There
is no appropriate API yet in the Linux kernel to expose these. The
controller also provides a classic rumble feature for backwards
compatibility. Expose this classic rumble feature using the FF framework.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
I'm wondering if HID_PLAYSTATION_FF would be a better name?
quoted hunk
+ bool "PlayStation force feedback support"
+ depends on HID_PLAYSTATION
+ select INPUT_FF_MEMLESS
+ help
+ Say Y here if you would like to enable force feedback support for
+ PlayStation game controllers.
+
config HID_PRIMAX
tristate "Primax non-fully HID-compliant devices"
depends on HID
I think it'd be good if you could add static_asserts to check the sizes of
the __packed structs at compile time.
+/* The DualSense has a main output report used to control most features. It is
+ * largely the same between Bluetooth and USB except for different headers and CRC.
+ * This structure hide the differences between the two to simplify sending output reports.
+ */
+struct dualsense_output_report {
+ uint8_t *data; /* Start of data */
+ uint8_t len; /* Size of output report */
+
+ /* Points to Bluetooth data payload in case for a Bluetooth report else NULL. */
+ struct dualsense_output_report_bt *bt;
+ /* Points to USB data payload in case for a USB report else NULL. */
+ struct dualsense_output_report_usb *usb;
+ /* Points to common section of report, so past any headers */
+ struct dualsense_output_report_common *common;
+};
[...]
+static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp,
+ void *buf)
If the dualsense struct is already passed in, couldn't this function use
`ds->output_report_dmabuf` directly?
+{
+ struct hid_device *hdev = ds->base.hdev;
+
+ if (hdev->bus == BUS_BLUETOOTH) {
+ struct dualsense_output_report_bt *bt = buf;
+
+ memset(bt, 0, sizeof(*bt));
+ bt->report_id = DS_OUTPUT_REPORT_BT;
+ bt->tag = 0x10; /* Magic number must be set to 0x10 */
I think it would be preferable if that 0x10 were named.
+
+ /* Highest 4-bit is a sequence number, which needs to be increased
+ * every report. Lowest 4-bit is tag and can be zero for now.
+ */
+ bt->seq_tag = (ds->output_seq << 4) | 0x0;
+ if (++ds->output_seq == 15)
+ ds->output_seq = 0;
If I see it correctly, the maximum sequence number is 14; is that intentional?
Or am I missing something?
+
+ rp->data = buf;
+ rp->len = sizeof(*bt);
+ rp->bt = bt;
+ rp->usb = NULL;
+ rp->common = &bt->common;
+ } else { /* USB */
+ struct dualsense_output_report_usb *usb = buf;
+
+ memset(usb, 0, sizeof(*usb));
+ usb->report_id = DS_OUTPUT_REPORT_USB;
+
+ rp->data = buf;
+ rp->len = sizeof(*usb);
+ rp->bt = NULL;
+ rp->usb = usb;
+ rp->common = &usb->common;
+ }
+}
+
+/* Helper function to send DualSense output reports. Applies a CRC at the end of a report
+ * for Bluetooth reports.
+ */
+static void dualsense_send_output_report(struct dualsense *ds,
+ struct dualsense_output_report *report)
+{
+ struct hid_device *hdev = ds->base.hdev;
+
+ /* Bluetooth packets need to be signed with a CRC in the last 4 bytes. */
+ if (report->bt) {
+ uint32_t crc;
+ uint8_t seed = 0xA2;
Maybe this '0xA2' could be named as well? And I think it would be better if
all hexadecimal constants would either be lowercase or uppercase.
I think `max(sizeof(..._bt), sizeof(..._usb))` (linux/minmax.h) would be
more expressive?
quoted hunk
+ ds->output_report_dmabuf = devm_kzalloc(&hdev->dev, max_output_report_size, GFP_KERNEL);
+ if (!ds->output_report_dmabuf)
+ return ERR_PTR(-ENOMEM);
+
ret = dualsense_get_mac_address(ds);
if (ret < 0) {
hid_err(hdev, "Failed to get MAC address from DualSense\n");
2021. január 7., csütörtök 21:41 keltezéssel, Barnabás Pőcze írta:
[...]
quoted
+struct dualsense_output_report {
+ uint8_t *data; /* Start of data */
+ uint8_t len; /* Size of output report */
+
+ /* Points to Bluetooth data payload in case for a Bluetooth report else NULL. */
+ struct dualsense_output_report_bt *bt;
+ /* Points to USB data payload in case for a USB report else NULL. */
+ struct dualsense_output_report_usb *usb;
+ /* Points to common section of report, so past any headers */
+ struct dualsense_output_report_common *common;
+};
[...]
+static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp,
+ void *buf)
If the dualsense struct is already passed in, couldn't this function use
`ds->output_report_dmabuf` directly?
[...]
Never mind, sorry, the next patch answered this question.
Just a small note, as far as I'm aware, the `channel` member is not used by
multicolor LED functions, and it's not even used in this module as far as I see.
@@ -930,6 +1035,18 @@ static struct ps_device *dualsense_create(struct hid_device *hdev) if (ret < 0) goto err;+ /* The hardware may have control over the LEDs (e.g. in Bluetooth on startup).+ * Reset the LEDs (lightbar, mute, player leds), so we can control them+ * from software.+ */+ ret = dualsense_reset_leds(ds);+ if (ret < 0)
I believe if `dualsense_reset_leds()` can only return 0 on success, or an errno,
then `if (ret)` would be better.
+ goto err;
+
+ ret = ps_lightbar_register(ps_dev, &ds->lightbar, dualsense_lightbar_set_brightness);
+ if (ret < 0)
[...]
Hi
2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:
quoted hunk
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
The DualSense controller has a built-in microphone exposed as an
audio device over USB (or HID using Bluetooth). A dedicated
button on the controller handles mute, but software has to configure
the device to mute the audio stream.
This patch captures the mute button and schedules an output report
to mute/unmute the audio stream as well as toggle the mute LED.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
Another inconsistency with log messages is that sometimes the error code is printed,
sometimes it is not. I believe it would be preferable if there was no incosistency.
@@ -777,6 +854,7 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r uint8_t battery_data, battery_capacity, charging_status, value; int battery_status; uint16_t sensor_timestamp;+ bool btn_mic_state; unsigned long flags; int i;
@@ -829,6 +907,22 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r input_report_key(ds->gamepad, BTN_MODE, ds_report->buttons[2] & DS_BUTTONS2_PS_HOME); input_sync(ds->gamepad);+ /* The DualSense has an internal microphone, which can be muted through a mute button+ * on the device. The driver expected to read the button state and program the device
[...]
"The driver is expected" ?
Regards,
Barnabás Pőcze
Hi
2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:
quoted hunk
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
The DualSense features 5 player LEDs below its touchpad, which are
meant as player id indications. This patch exposes the player LEDs
as individual LEDs.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
Hi
2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
Add a ID allocator to assign player ids to ps_device instances.
Utilize the player id to set a default color on the DualSense its
player LED strip.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
[...]
+static void ps_device_release_player_id(struct ps_device *dev)
+{
+ ida_free(&ps_player_id_allocator, dev->player_id);
+
+ dev->player_id = -1;
A minor thing, but I believe U32_MAX would be better here. I'd avoid
(especially) negative magic numbers for an unsigned value. You could even
#define PS_PLAYER_ID_INVALID U32_MAX
or something similar.
@@ -1102,6 +1125,28 @@ static int dualsense_reset_leds(struct dualsense *ds) return 0; }+static void dualsense_set_player_leds(struct dualsense *ds)+{+ /* The DualSense controller has a row of 5 LEDs used for player ids.+ * Behavior on the PlayStation 5 console is to center the player id+ * across the LEDs, so e.g. player 1 would be "--x--" with x being 'on'.+ * Follow a similar mapping here.+ */+ int player_ids[5] = {+ BIT(2),+ BIT(3) | BIT(1),+ BIT(4) | BIT(2) | BIT(0),+ BIT(4) | BIT(3) | BIT(1) | BIT(0),+ BIT(4) | BIT(3) | BIT(2) | BIT(1) | BIT(0)+ };++ uint8_t player_id = ds->base.player_id % 5;
@@ -1195,6 +1261,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev) } snprintf(hdev->uniq, sizeof(hdev->uniq), "%pMR", ds->base.mac_address);+ ret = dualsense_get_firmware_info(ds);+ if (ret < 0) {+ hid_err(hdev, "Failed to get firmware info from DualSense\n");+ return ERR_PTR(ret);+ }+ ret = ps_devices_list_add(ps_dev); if (ret < 0) return ERR_PTR(ret);
@@ -1261,6 +1333,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev) /* Set player LEDs to our player id. */ dualsense_set_player_leds(ds);+ /* Reporting hardware and firmware is important as there are frequent updates, which+ * can change behavior.+ */+ hid_info(hdev, "Registered DualSense controller hw_version=%x fw_version=%x\n",
Maybe the format could be same as in the device attributes (0x%08x)?
The bits for EV_MSC/MSC_TIMESTAMP events are not set here, hence
timestamp events would not delivered:
__set_bit(EV_MSC, sensors->evbit);
__set_bit(MSC_TIMESTAMP, sensors->mscbit);
I don't know how I missed this... I remember adding it, but nope
apparently not :(
quoted
static int dualsense_get_mac_address(struct dualsense *ds)
{
uint8_t *buf;
This uint16_t variable overflows just after a few events. Since the
timestamp from the controller is 32bit and the event value too, I assume
this should be too.
Hi Barnabás,
Thanks for all your feedback (including other patches).
On Thu, Jan 7, 2021 at 10:52 AM Barnabás Pőcze [off-list ref] wrote:
Hi
2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:
quoted
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
The DualSense features an accelerometer and gyroscope. The data is
embedded into the main HID input reports. Expose both sensors through
through a separate evdev node.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
[...]
+static struct input_dev *ps_sensors_create(struct hid_device *hdev, int accel_range, int accel_res,
+ int gyro_range, int gyro_res)
+{
+ struct input_dev *sensors;
+ int ret;
+
+ sensors = ps_allocate_input_dev(hdev, "Motion Sensors");
+ if (IS_ERR(sensors))
+ return ERR_PTR(-ENOMEM);
I know that at the moment ENOMEM is the only possible error, but I believe
`return ERR_CAST(sensors);` would be better. (Or even just `return sensors;`.)
I think it would be better if lines were aligned. I have missed this in other patches,
so if you decide to make this change, please do it everywhere.
What do you mean with "if lines were aligned"? You mean aligning the
DS_FEATURE.. part with ds->base.hdev?
I'm almost tempted in the future (as part of a future patch series) to
perhaps have a ps_device_get_feature_report or something like that as
there is the same code in multiple places. It can do some nicer
checking as well (including to see if the first byte is the report ID
number, which is guaranteed for DualSense). I think it is a bit much
to add now, but probably in the future also when I add DualShock 4 in
here.
quoted
+ if (ret < 0)
+ goto err_free;
+ else if (ret != DS_FEATURE_REPORT_CALIBRATION_SIZE) {
As per coding style[1], please either use {} for all branches, or just drop the
`else` and maybe add a new line:
if(ret<0)goto...if(ret!=...){...}
quoted
+ hid_err(ds->base.hdev, "failed to retrieve DualSense calibration info\n");
I think this message could be improved to better pinpoint the exact problem
that triggered it.
A small note, as written in [2], the preferred style of multi-line comments is different,
so you might want to change the comments. If you decide to make this change, please
do it everywhere.
@@ -319,6 +469,7 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r struct dualsense_input_report *ds_report; uint8_t battery_data, battery_capacity, charging_status, value; int battery_status;+ uint16_t sensor_timestamp; unsigned long flags; int i;
@@ -361,6 +512,44 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r input_report_key(ds->gamepad, BTN_MODE, ds_report->buttons[2] & DS_BUTTONS2_PS_HOME); input_sync(ds->gamepad);+ /* Parse and calibrate gyroscope data. */+ for (i = 0; i < 3; i++) {
I think `i < ARRAY_SIZE(...)` would be better.
And I would add a `static_assert(ARRAY_SIZE(ds_report->gyro) == ARRAY_SIZE(ds->gyro_calib_data))`
somewhere around here just to be safe. Or define a new constant like `DS_GYRO_DIMS`
and use that to define the arrays. Or both. *
quoted
+ int raw_data = (short)le16_to_cpu(ds_report->gyro[i]);
+ int calib_data = mult_frac(ds->gyro_calib_data[i].sens_numer,
+ raw_data - ds->gyro_calib_data[i].bias,
+ ds->gyro_calib_data[i].sens_denom);
I believe it would be better if the second and third lines was aligned. **
quoted
+
+ input_report_abs(ds->sensors, ds->gyro_calib_data[i].abs_code, calib_data);
+ }
+
+ /* Parse and calibrate accelerometer data. */
+ for (i = 0; i < 3; i++) {
Same here. *
quoted
+ int raw_data = (short)le16_to_cpu(ds_report->accel[i]);
+ int calib_data = mult_frac(ds->accel_calib_data[i].sens_numer,
+ raw_data - ds->accel_calib_data[i].bias,
+ ds->accel_calib_data[i].sens_denom);
I know that at the moment ENOMEM is the only possible error, but I believe
`return ERR_CAST(gamepad);` would be better. (Or even just `return gamepad;`.)
I think `size` should be checked somewhere around here.
quoted
+ /* DualSense in USB uses the full HID report for reportID 1, but
+ * Bluetooth uses a minimal HID report for reportID 1 and reports
+ * the full report using reportID 49.
+ */
+ if (report->id == DS_INPUT_REPORT_USB && hdev->bus == BUS_USB) {
+ ds_report = (struct dualsense_input_report *)&data[1];
+ } else {
+ hid_err(hdev, "Unhandled reportID=%d\n", report->id);
+ return -1;
+ }
+
+ input_report_abs(ds->gamepad, ABS_X, ds_report->x);
+ input_report_abs(ds->gamepad, ABS_Y, ds_report->y);
+ input_report_abs(ds->gamepad, ABS_RX, ds_report->rx);
+ input_report_abs(ds->gamepad, ABS_RY, ds_report->ry);
+ input_report_abs(ds->gamepad, ABS_Z, ds_report->z);
+ input_report_abs(ds->gamepad, ABS_RZ, ds_report->rz);
+
+ value = ds_report->buttons[0] & DS_BUTTONS0_HAT_SWITCH;
+ if (value > 7)
+ value = 8; /* center */
This seems a bit flimsy to me, it relies on a different part of the code
being in a certain way that is not enforced by anything
What do you mean with not enforced? I'm not saying I'm a big fan of
the code, but HATs seem to work like this. The DualShock4/DualSense
describe it in their HID descriptors with a logical minimum value of 0
and a max value of 7.
The code is very similar to hid-input.c:
static const struct {
__s32 x;
__s32 y;
} hid_hat_to_axis[] = {{ 0, 0}, { 0,-1}, { 1,-1}, { 1, 0}, { 1, 1}, {
0, 1}, {-1, 1}, {-1, 0}, {-1,-1}};
int hat_dir = usage->hat_dir;
if (!hat_dir)
hat_dir = (value - usage->hat_min) * 8 / (usage->hat_max -
usage->hat_min + 1) + 1;
if (hat_dir < 0 || hat_dir > 8) hat_dir = 0;
input_event(input, usage->type, usage->code , hid_hat_to_axis[hat_dir].x);
input_event(input, usage->type, usage->code + 1, hid_hat_to_axis[hat_dir].y);
Main difference seems to be that this code places {0, 0} at the start
and adds a "+1" to avoid having to set the value to "8" when out of
range.
I'd probably do something
like this:
enum {
HAT_DIR_W = 0,
HAT_DIR_NW,
...
HAT_DIR_SW,
HAT_DIR_NONE,
};
static const struct {int x; int y; } ps_gamepad_hat_mapping[] = {
[HAT_DIR_W] = {0, -1},
...
[HAT_DIR_NONE] = {0, 0},
};
and then
if (value >= ARRAY_SIZE(ps_gamepad_hat_mapping))
value = HAT_DIR_NONE;
Please consider it. By the way, are values 9..15 actually sent by the controller?
See above. They are not sent. The Hat Switch in the report descriptor
is reported with a logical minimum of 0 and a max of 8.
I'm still not fully seeing the purpose of this `if`. The probe should not be
called for devices not in the id_table, so this seems to me to be a long way
of writing `if (true)`. Or am I missing something?
It is not used. It more there for the future, when we will add
DualShock 4 and perhaps some other devices here.
quoted
+ dev = dualsense_create(hdev);
+ if (IS_ERR(dev)) {
+ hid_err(hdev, "Failed to create dualsense.\n");
I think it'd be preferable if all log messages would either be lowercase or
uppercase, not a mix of both. Same for punctuation. This applies to all patches.
I think `size` should be checked somewhere around here.
quoted
+ /* DualSense in USB uses the full HID report for reportID 1, but
+ * Bluetooth uses a minimal HID report for reportID 1 and reports
+ * the full report using reportID 49.
+ */
+ if (report->id == DS_INPUT_REPORT_USB && hdev->bus == BUS_USB) {
+ ds_report = (struct dualsense_input_report *)&data[1];
+ } else {
+ hid_err(hdev, "Unhandled reportID=%d\n", report->id);
+ return -1;
+ }
+
+ input_report_abs(ds->gamepad, ABS_X, ds_report->x);
+ input_report_abs(ds->gamepad, ABS_Y, ds_report->y);
+ input_report_abs(ds->gamepad, ABS_RX, ds_report->rx);
+ input_report_abs(ds->gamepad, ABS_RY, ds_report->ry);
+ input_report_abs(ds->gamepad, ABS_Z, ds_report->z);
+ input_report_abs(ds->gamepad, ABS_RZ, ds_report->rz);
+
+ value = ds_report->buttons[0] & DS_BUTTONS0_HAT_SWITCH;
+ if (value > 7)
+ value = 8; /* center */
This seems a bit flimsy to me, it relies on a different part of the code
being in a certain way that is not enforced by anything
What do you mean with not enforced? I'm not saying I'm a big fan of
the code, but HATs seem to work like this. The DualShock4/DualSense
describe it in their HID descriptors with a logical minimum value of 0
and a max value of 7.
What I really meant is that I think it would be better to somehow decrease
the chances of unintentionally breaking the code by providing stronger
guarantees that `ps_gamepad_hat_mapping[value]` will not be an out of bounds
access. E.g. checking with static_assert that it has 9 elements, using
ARRAY_SIZE() in some way in the condition, etc. I'm not trying to say it's
very probable, but I think it's easily preventable and thus it should be done.
By "not enforced", I meant that there are no compile time or even runtime guarantees
that the `ps_gamepad_hat_mapping` contains as many elements as expected by this
part of the code.
The code is very similar to hid-input.c:
static const struct {
__s32 x;
__s32 y;
} hid_hat_to_axis[] = {{ 0, 0}, { 0,-1}, { 1,-1}, { 1, 0}, { 1, 1}, {
0, 1}, {-1, 1}, {-1, 0}, {-1,-1}};
int hat_dir = usage->hat_dir;
if (!hat_dir)
hat_dir = (value - usage->hat_min) * 8 / (usage->hat_max -
usage->hat_min + 1) + 1;
if (hat_dir < 0 || hat_dir > 8) hat_dir = 0;
input_event(input, usage->type, usage->code , hid_hat_to_axis[hat_dir].x);
input_event(input, usage->type, usage->code + 1, hid_hat_to_axis[hat_dir].y);
Main difference seems to be that this code places {0, 0} at the start
and adds a "+1" to avoid having to set the value to "8" when out of
range.
I'd probably do something
quoted
like this:
enum {
HAT_DIR_W = 0,
HAT_DIR_NW,
...
HAT_DIR_SW,
HAT_DIR_NONE,
};
static const struct {int x; int y; } ps_gamepad_hat_mapping[] = {
[HAT_DIR_W] = {0, -1},
...
[HAT_DIR_NONE] = {0, 0},
};
and then
if (value >= ARRAY_SIZE(ps_gamepad_hat_mapping))
value = HAT_DIR_NONE;
Please consider it. By the way, are values 9..15 actually sent by the controller?
See above. They are not sent. The Hat Switch in the report descriptor
is reported with a logical minimum of 0 and a max of 8.
Hi
2021. január 8., péntek 7:06 keltezéssel, Roderick Colenbrander írta:
[...]
quoted
quoted
+static int dualsense_get_calibration_data(struct dualsense *ds)
+{
+ short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
+ short gyro_yaw_bias, gyro_yaw_plus, gyro_yaw_minus;
+ short gyro_roll_bias, gyro_roll_plus, gyro_roll_minus;
+ short gyro_speed_plus, gyro_speed_minus;
+ short acc_x_plus, acc_x_minus;
+ short acc_y_plus, acc_y_minus;
+ short acc_z_plus, acc_z_minus;
+ int speed_2x;
+ int range_2g;
+ int ret = 0;
+ uint8_t *buf;
+
+ buf = kzalloc(DS_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_CALIBRATION, buf,
+ DS_FEATURE_REPORT_CALIBRATION_SIZE, HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
I think it would be better if lines were aligned. I have missed this in other patches,
so if you decide to make this change, please do it everywhere.
What do you mean with "if lines were aligned"? You mean aligning the
DS_FEATURE.. part with ds->base.hdev?
Yes, exactly.
I'm almost tempted in the future (as part of a future patch series) to
perhaps have a ps_device_get_feature_report or something like that as
there is the same code in multiple places. It can do some nicer
checking as well (including to see if the first byte is the report ID
number, which is guaranteed for DualSense). I think it is a bit much
to add now, but probably in the future also when I add DualShock 4 in
here.
I think it's a good idea to add such a function sometime.
quoted
quoted
+ if (ret < 0)
+ goto err_free;
+ else if (ret != DS_FEATURE_REPORT_CALIBRATION_SIZE) {
As per coding style[1], please either use {} for all branches, or just drop the
`else` and maybe add a new line:
if(ret<0)goto...if(ret!=...){...}
quoted
+ hid_err(ds->base.hdev, "failed to retrieve DualSense calibration info\n");
I think this message could be improved to better pinpoint the exact problem
that triggered it.
Hi Barnabás,
Thanks for your comments.
On Thu, Jan 7, 2021 at 12:41 PM Barnabás Pőcze [off-list ref] wrote:
Hi
2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:
quoted
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
The DualSense features a haptics system based on voicecoil motors,
which requires PCM data (or special HID packets using Bluetooth). There
is no appropriate API yet in the Linux kernel to expose these. The
controller also provides a classic rumble feature for backwards
compatibility. Expose this classic rumble feature using the FF framework.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
I'm wondering if HID_PLAYSTATION_FF would be a better name?
I'm not sure. Other drivers use "NAME_FF". It seems that FF-only
drivers use HID_NAME_FF.
quoted
+ bool "PlayStation force feedback support"
+ depends on HID_PLAYSTATION
+ select INPUT_FF_MEMLESS
+ help
+ Say Y here if you would like to enable force feedback support for
+ PlayStation game controllers.
+
config HID_PRIMAX
tristate "Primax non-fully HID-compliant devices"
depends on HID
I think it'd be good if you could add static_asserts to check the sizes of
the __packed structs at compile time.
Good idea. Added some checks.
quoted
+/* The DualSense has a main output report used to control most features. It is
+ * largely the same between Bluetooth and USB except for different headers and CRC.
+ * This structure hide the differences between the two to simplify sending output reports.
+ */
+struct dualsense_output_report {
+ uint8_t *data; /* Start of data */
+ uint8_t len; /* Size of output report */
+
+ /* Points to Bluetooth data payload in case for a Bluetooth report else NULL. */
+ struct dualsense_output_report_bt *bt;
+ /* Points to USB data payload in case for a USB report else NULL. */
+ struct dualsense_output_report_usb *usb;
+ /* Points to common section of report, so past any headers */
+ struct dualsense_output_report_common *common;
+};
[...]
+static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp,
+ void *buf)
If the dualsense struct is already passed in, couldn't this function use
`ds->output_report_dmabuf` directly?
quoted
+{
+ struct hid_device *hdev = ds->base.hdev;
+
+ if (hdev->bus == BUS_BLUETOOTH) {
+ struct dualsense_output_report_bt *bt = buf;
+
+ memset(bt, 0, sizeof(*bt));
+ bt->report_id = DS_OUTPUT_REPORT_BT;
+ bt->tag = 0x10; /* Magic number must be set to 0x10 */
I think it would be preferable if that 0x10 were named.
Not sure yet what I want to do. I was only given this magic number and
I don't even know what it is, so even for me it is magic :)
quoted
+
+ /* Highest 4-bit is a sequence number, which needs to be increased
+ * every report. Lowest 4-bit is tag and can be zero for now.
+ */
+ bt->seq_tag = (ds->output_seq << 4) | 0x0;
+ if (++ds->output_seq == 15)
+ ds->output_seq = 0;
If I see it correctly, the maximum sequence number is 14; is that intentional?
Or am I missing something?
quoted
+
+ rp->data = buf;
+ rp->len = sizeof(*bt);
+ rp->bt = bt;
+ rp->usb = NULL;
+ rp->common = &bt->common;
+ } else { /* USB */
+ struct dualsense_output_report_usb *usb = buf;
+
+ memset(usb, 0, sizeof(*usb));
+ usb->report_id = DS_OUTPUT_REPORT_USB;
+
+ rp->data = buf;
+ rp->len = sizeof(*usb);
+ rp->bt = NULL;
+ rp->usb = usb;
+ rp->common = &usb->common;
+ }
+}
+
+/* Helper function to send DualSense output reports. Applies a CRC at the end of a report
+ * for Bluetooth reports.
+ */
+static void dualsense_send_output_report(struct dualsense *ds,
+ struct dualsense_output_report *report)
+{
+ struct hid_device *hdev = ds->base.hdev;
+
+ /* Bluetooth packets need to be signed with a CRC in the last 4 bytes. */
+ if (report->bt) {
+ uint32_t crc;
+ uint8_t seed = 0xA2;
Maybe this '0xA2' could be named as well? And I think it would be better if
all hexadecimal constants would either be lowercase or uppercase.
I think `max(sizeof(..._bt), sizeof(..._usb))` (linux/minmax.h) would be
more expressive?
quoted
+ ds->output_report_dmabuf = devm_kzalloc(&hdev->dev, max_output_report_size, GFP_KERNEL);
+ if (!ds->output_report_dmabuf)
+ return ERR_PTR(-ENOMEM);
+
ret = dualsense_get_mac_address(ds);
if (ret < 0) {
hid_err(hdev, "Failed to get MAC address from DualSense\n");
Hi Roderick,
Is there any way to align the sensor timestamps with the real clock on
this new device? If so, there's input_set_timestamp api [0] that could
be used for setting the timestamps of the actual input_events rather
than having to send out parallel MSC_TIMESTAMP messages. It would make
it easier for user space to process these events.
[0] https://patchwork.kernel.org/project/linux-input/patch/20190718194133.64034-1-atifniyaz@google.com/
On Fri, Jan 8, 2021 at 2:03 AM Barnabás Pőcze [off-list ref] wrote:
Hi
2021. január 8., péntek 7:06 keltezéssel, Roderick Colenbrander írta:
quoted
[...]
quoted
quoted
+static int dualsense_get_calibration_data(struct dualsense *ds)
+{
+ short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
+ short gyro_yaw_bias, gyro_yaw_plus, gyro_yaw_minus;
+ short gyro_roll_bias, gyro_roll_plus, gyro_roll_minus;
+ short gyro_speed_plus, gyro_speed_minus;
+ short acc_x_plus, acc_x_minus;
+ short acc_y_plus, acc_y_minus;
+ short acc_z_plus, acc_z_minus;
+ int speed_2x;
+ int range_2g;
+ int ret = 0;
+ uint8_t *buf;
+
+ buf = kzalloc(DS_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_CALIBRATION, buf,
+ DS_FEATURE_REPORT_CALIBRATION_SIZE, HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
I think it would be better if lines were aligned. I have missed this in other patches,
so if you decide to make this change, please do it everywhere.
What do you mean with "if lines were aligned"? You mean aligning the
DS_FEATURE.. part with ds->base.hdev?
Yes, exactly.
quoted
I'm almost tempted in the future (as part of a future patch series) to
perhaps have a ps_device_get_feature_report or something like that as
there is the same code in multiple places. It can do some nicer
checking as well (including to see if the first byte is the report ID
number, which is guaranteed for DualSense). I think it is a bit much
to add now, but probably in the future also when I add DualShock 4 in
here.
I think it's a good idea to add such a function sometime.
quoted
quoted
quoted
+ if (ret < 0)
+ goto err_free;
+ else if (ret != DS_FEATURE_REPORT_CALIBRATION_SIZE) {
As per coding style[1], please either use {} for all branches, or just drop the
`else` and maybe add a new line:
if(ret<0)goto...if(ret!=...){...}
quoted
+ hid_err(ds->base.hdev, "failed to retrieve DualSense calibration info\n");
I think this message could be improved to better pinpoint the exact problem
that triggered it.
Hi Siarhei,
It might be an idea to indeed use that API. I wasn't aware of its
existence. Though I don't fully understand how it works (and how you
can guarantee alignment). Unfortunately I don't see any drivers in
upstream Linux using it. Do you happen to know of drivers using it? I
guess the might be some in Android kernel-common?
Thanks,
Roderick
On Fri, Jan 8, 2021 at 9:15 AM Siarhei Vishniakou [off-list ref] wrote:
Hi Roderick,
Is there any way to align the sensor timestamps with the real clock on
this new device? If so, there's input_set_timestamp api [0] that could
be used for setting the timestamps of the actual input_events rather
than having to send out parallel MSC_TIMESTAMP messages. It would make
it easier for user space to process these events.
[0] https://patchwork.kernel.org/project/linux-input/patch/20190718194133.64034-1-atifniyaz@google.com/
On Fri, Jan 8, 2021 at 2:03 AM Barnabás Pőcze [off-list ref] wrote:
quoted
Hi
2021. január 8., péntek 7:06 keltezéssel, Roderick Colenbrander írta:
quoted
[...]
quoted
quoted
+static int dualsense_get_calibration_data(struct dualsense *ds)
+{
+ short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
+ short gyro_yaw_bias, gyro_yaw_plus, gyro_yaw_minus;
+ short gyro_roll_bias, gyro_roll_plus, gyro_roll_minus;
+ short gyro_speed_plus, gyro_speed_minus;
+ short acc_x_plus, acc_x_minus;
+ short acc_y_plus, acc_y_minus;
+ short acc_z_plus, acc_z_minus;
+ int speed_2x;
+ int range_2g;
+ int ret = 0;
+ uint8_t *buf;
+
+ buf = kzalloc(DS_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_CALIBRATION, buf,
+ DS_FEATURE_REPORT_CALIBRATION_SIZE, HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
I think it would be better if lines were aligned. I have missed this in other patches,
so if you decide to make this change, please do it everywhere.
What do you mean with "if lines were aligned"? You mean aligning the
DS_FEATURE.. part with ds->base.hdev?
Yes, exactly.
quoted
I'm almost tempted in the future (as part of a future patch series) to
perhaps have a ps_device_get_feature_report or something like that as
there is the same code in multiple places. It can do some nicer
checking as well (including to see if the first byte is the report ID
number, which is guaranteed for DualSense). I think it is a bit much
to add now, but probably in the future also when I add DualShock 4 in
here.
I think it's a good idea to add such a function sometime.
quoted
quoted
quoted
+ if (ret < 0)
+ goto err_free;
+ else if (ret != DS_FEATURE_REPORT_CALIBRATION_SIZE) {
As per coding style[1], please either use {} for all branches, or just drop the
`else` and maybe add a new line:
if(ret<0)goto...if(ret!=...){...}
quoted
+ hid_err(ds->base.hdev, "failed to retrieve DualSense calibration info\n");
I think this message could be improved to better pinpoint the exact problem
that triggered it.
quoted
+ ret = -EINVAL;
+ goto err_free;
+ }
[...]
Regards,
Barnabás Pőcze
--
Roderick Colenbrander
Senior Manager of Hardware & Systems Engineering
Sony Interactive Entertainment LLC
roderick.colenbrander@sony.com
This api is used by some of our touch drivers to more accurately set
the timestamps of touch events. This allows us to better measure touch
latency. An example can be found in [0].
From what I remember, you call this api to apply a specific timestamp
to all of the subsequent input_events that are produced. When
input_sync happens, this timestamp is erased and you revert to the
default behaviour (acquiring a timestamp in evdev) until this api is
called again.
So if you choose to use this api, you would have to take care to only
apply it to the sensor events and not other events (unless you can
figure out the timestamps for all), as well as finding a way to align
the hardware timestamps with the wall clock.
For the touch driver case, it's easy because we are just taking the
current time at the interrupt. This still misses the portions where
the touch scanning and data preprocessing on the touch IC occurs, but
it gets us closer to the real number (for example, it helps account
for the i2c/spi data transfer time, which happens after the
interrupt).
[0] https://github.com/android-linux-stable/bluecross/blob/android-msm-bluecross-4.9/drivers/input/touchscreen/stm/fts.c#L3451
On Fri, Jan 8, 2021 at 9:54 AM Roderick Colenbrander
[off-list ref] wrote:
Hi Siarhei,
It might be an idea to indeed use that API. I wasn't aware of its
existence. Though I don't fully understand how it works (and how you
can guarantee alignment). Unfortunately I don't see any drivers in
upstream Linux using it. Do you happen to know of drivers using it? I
guess the might be some in Android kernel-common?
Thanks,
Roderick
On Fri, Jan 8, 2021 at 9:15 AM Siarhei Vishniakou [off-list ref] wrote:
quoted
Hi Roderick,
Is there any way to align the sensor timestamps with the real clock on
this new device? If so, there's input_set_timestamp api [0] that could
be used for setting the timestamps of the actual input_events rather
than having to send out parallel MSC_TIMESTAMP messages. It would make
it easier for user space to process these events.
[0] https://patchwork.kernel.org/project/linux-input/patch/20190718194133.64034-1-atifniyaz@google.com/
On Fri, Jan 8, 2021 at 2:03 AM Barnabás Pőcze [off-list ref] wrote:
quoted
Hi
2021. január 8., péntek 7:06 keltezéssel, Roderick Colenbrander írta:
quoted
[...]
quoted
quoted
+static int dualsense_get_calibration_data(struct dualsense *ds)
+{
+ short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
+ short gyro_yaw_bias, gyro_yaw_plus, gyro_yaw_minus;
+ short gyro_roll_bias, gyro_roll_plus, gyro_roll_minus;
+ short gyro_speed_plus, gyro_speed_minus;
+ short acc_x_plus, acc_x_minus;
+ short acc_y_plus, acc_y_minus;
+ short acc_z_plus, acc_z_minus;
+ int speed_2x;
+ int range_2g;
+ int ret = 0;
+ uint8_t *buf;
+
+ buf = kzalloc(DS_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_CALIBRATION, buf,
+ DS_FEATURE_REPORT_CALIBRATION_SIZE, HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
I think it would be better if lines were aligned. I have missed this in other patches,
so if you decide to make this change, please do it everywhere.
What do you mean with "if lines were aligned"? You mean aligning the
DS_FEATURE.. part with ds->base.hdev?
Yes, exactly.
quoted
I'm almost tempted in the future (as part of a future patch series) to
perhaps have a ps_device_get_feature_report or something like that as
there is the same code in multiple places. It can do some nicer
checking as well (including to see if the first byte is the report ID
number, which is guaranteed for DualSense). I think it is a bit much
to add now, but probably in the future also when I add DualShock 4 in
here.
I think it's a good idea to add such a function sometime.
quoted
quoted
quoted
+ if (ret < 0)
+ goto err_free;
+ else if (ret != DS_FEATURE_REPORT_CALIBRATION_SIZE) {
As per coding style[1], please either use {} for all branches, or just drop the
`else` and maybe add a new line:
if(ret<0)goto...if(ret!=...){...}
quoted
+ hid_err(ds->base.hdev, "failed to retrieve DualSense calibration info\n");
I think this message could be improved to better pinpoint the exact problem
that triggered it.
quoted
+ ret = -EINVAL;
+ goto err_free;
+ }
[...]
Regards,
Barnabás Pőcze
--
Roderick Colenbrander
Senior Manager of Hardware & Systems Engineering
Sony Interactive Entertainment LLC
roderick.colenbrander@sony.com
Hi Barnabás,
A couple of places lacked MAC address checks (some of these reports I
didn't have datasheets on). In the end I decided to make a new helper
function as there is so much common nasty code. It also exposed a few
tiny bugs as some reports were an incorrect size (not critical as the
data wasn't used). It is a lot simpler now with more and better
checking.
static int ps_get_report(struct hid_device *hdev, uint8_t report_id,
uint8_t *buf, size_t size)
{
int ret;
ret = hid_hw_raw_request(hdev, report_id, buf, size,
HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
if (ret < 0) {
hid_err(hdev, "Failed to retrieve feature report %d,
ret=%d\n", report_id, ret);
return ret;
}
if (ret != size) {
hid_err(hdev, "Invalid byte count transferred, expected %zu
got %d\n", size, ret);
return -EINVAL;
}
if (buf[0] != report_id) {
hid_err(hdev, "Incorrect reportID received, expected %d got
%d\n", report_id, buf[0]);
return -EINVAL;
}
if (hdev->bus == BUS_BLUETOOTH) {
/* Last 4 bytes contains crc32. */
uint8_t crc_offset = size - 4;
uint32_t report_crc = get_unaligned_le32(&buf[crc_offset]);
if (!ps_check_crc32(PS_FEATURE_CRC32_SEED, buf, crc_offset,
report_crc)) {
hid_err(hdev, "CRC check failed for reportID=%d\n", report_id);
return -EILSEQ;
}
}
return 0;
}
On Thu, Jan 7, 2021 at 2:26 PM Barnabás Pőcze [off-list ref] wrote:
Hi
2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:
@@ -1195,6 +1261,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev) } snprintf(hdev->uniq, sizeof(hdev->uniq), "%pMR", ds->base.mac_address);+ ret = dualsense_get_firmware_info(ds);+ if (ret < 0) {+ hid_err(hdev, "Failed to get firmware info from DualSense\n");+ return ERR_PTR(ret);+ }+ ret = ps_devices_list_add(ps_dev); if (ret < 0) return ERR_PTR(ret);
@@ -1261,6 +1333,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev) /* Set player LEDs to our player id. */ dualsense_set_player_leds(ds);+ /* Reporting hardware and firmware is important as there are frequent updates, which+ * can change behavior.+ */+ hid_info(hdev, "Registered DualSense controller hw_version=%x fw_version=%x\n",
Maybe the format could be same as in the device attributes (0x%08x)?
Hi Siarhei,
Thanks for sharing the example. I see now how the API could be used.
In case of DualSense, the 'dualsense_parse_report' call is effectively
the ISR and the timestamp is derived there by the input framework. We
would like to use the hardware timestamp (as in timestamp at which the
device created the event), so we can do accurate motion tracking. In
particular in case of Bluetooth there is variation in timestamps.
We could do our own time tracking by taking an initial CLOCK_MONOTONIC
and then adding our own hardware timestamps to it. Something like:
if (!sensor_timestamp_initialized) {
timestamp = ktime_get();
hw_time0 = get sensor timestamp();
}
hw_delta = get_sensor_timestamp() - hw_time0;
ktime_add_ns(timestamp, hw_delta);
input_set_timestamp(sensor_dev, timestamp);
I just don't know what others would think about such an approach vs
MSC_TIMESTAMP (or we can do both).
Thanks,
Roderick
On Fri, Jan 8, 2021 at 4:11 PM Siarhei Vishniakou [off-list ref] wrote:
This api is used by some of our touch drivers to more accurately set
the timestamps of touch events. This allows us to better measure touch
latency. An example can be found in [0].
From what I remember, you call this api to apply a specific timestamp
to all of the subsequent input_events that are produced. When
input_sync happens, this timestamp is erased and you revert to the
default behaviour (acquiring a timestamp in evdev) until this api is
called again.
So if you choose to use this api, you would have to take care to only
apply it to the sensor events and not other events (unless you can
figure out the timestamps for all), as well as finding a way to align
the hardware timestamps with the wall clock.
For the touch driver case, it's easy because we are just taking the
current time at the interrupt. This still misses the portions where
the touch scanning and data preprocessing on the touch IC occurs, but
it gets us closer to the real number (for example, it helps account
for the i2c/spi data transfer time, which happens after the
interrupt).
[0] https://github.com/android-linux-stable/bluecross/blob/android-msm-bluecross-4.9/drivers/input/touchscreen/stm/fts.c#L3451
On Fri, Jan 8, 2021 at 9:54 AM Roderick Colenbrander
[off-list ref] wrote:
quoted
Hi Siarhei,
It might be an idea to indeed use that API. I wasn't aware of its
existence. Though I don't fully understand how it works (and how you
can guarantee alignment). Unfortunately I don't see any drivers in
upstream Linux using it. Do you happen to know of drivers using it? I
guess the might be some in Android kernel-common?
Thanks,
Roderick
On Fri, Jan 8, 2021 at 9:15 AM Siarhei Vishniakou [off-list ref] wrote:
quoted
Hi Roderick,
Is there any way to align the sensor timestamps with the real clock on
this new device? If so, there's input_set_timestamp api [0] that could
be used for setting the timestamps of the actual input_events rather
than having to send out parallel MSC_TIMESTAMP messages. It would make
it easier for user space to process these events.
[0] https://patchwork.kernel.org/project/linux-input/patch/20190718194133.64034-1-atifniyaz@google.com/
On Fri, Jan 8, 2021 at 2:03 AM Barnabás Pőcze [off-list ref] wrote:
quoted
Hi
2021. január 8., péntek 7:06 keltezéssel, Roderick Colenbrander írta:
quoted
[...]
quoted
quoted
+static int dualsense_get_calibration_data(struct dualsense *ds)
+{
+ short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
+ short gyro_yaw_bias, gyro_yaw_plus, gyro_yaw_minus;
+ short gyro_roll_bias, gyro_roll_plus, gyro_roll_minus;
+ short gyro_speed_plus, gyro_speed_minus;
+ short acc_x_plus, acc_x_minus;
+ short acc_y_plus, acc_y_minus;
+ short acc_z_plus, acc_z_minus;
+ int speed_2x;
+ int range_2g;
+ int ret = 0;
+ uint8_t *buf;
+
+ buf = kzalloc(DS_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_CALIBRATION, buf,
+ DS_FEATURE_REPORT_CALIBRATION_SIZE, HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
I think it would be better if lines were aligned. I have missed this in other patches,
so if you decide to make this change, please do it everywhere.
What do you mean with "if lines were aligned"? You mean aligning the
DS_FEATURE.. part with ds->base.hdev?
Yes, exactly.
quoted
I'm almost tempted in the future (as part of a future patch series) to
perhaps have a ps_device_get_feature_report or something like that as
there is the same code in multiple places. It can do some nicer
checking as well (including to see if the first byte is the report ID
number, which is guaranteed for DualSense). I think it is a bit much
to add now, but probably in the future also when I add DualShock 4 in
here.
I think it's a good idea to add such a function sometime.
quoted
quoted
quoted
+ if (ret < 0)
+ goto err_free;
+ else if (ret != DS_FEATURE_REPORT_CALIBRATION_SIZE) {
As per coding style[1], please either use {} for all branches, or just drop the
`else` and maybe add a new line:
if(ret<0)goto...if(ret!=...){...}
quoted
+ hid_err(ds->base.hdev, "failed to retrieve DualSense calibration info\n");
I think this message could be improved to better pinpoint the exact problem
that triggered it.
quoted
+ ret = -EINVAL;
+ goto err_free;
+ }
[...]
Regards,
Barnabás Pőcze
--
Roderick Colenbrander
Senior Manager of Hardware & Systems Engineering
Sony Interactive Entertainment LLC
roderick.colenbrander@sony.com