Re: [PATCH v2 05/13] HID: playstation: add DualSense accelerometer and gyroscope support.

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.

quoted

+             ret = -EINVAL;
+             goto err_free;
+     }

[...]

Regards,
Barnabás Pőcze