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@xxxxxxxxxx/ On Fri, Jan 8, 2021 at 2:03 AM Barnabás Pőcze <pobrn@xxxxxxxxxxxxxx> wrote: > > Hi > > > 2021. január 8., péntek 7:06 keltezéssel, Roderick Colenbrander írta: > > > [...] > > > > +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. > > > > > > > > > > > + 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 != ...) { > > > ... > > > } > > > ``` > > > > > > > > > > + 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. > > > > > > > > > > + ret = -EINVAL; > > > > + goto err_free; > > > > + } > > [...] > > > Regards, > Barnabás Pőcze
