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 <svv@xxxxxxxxxx> 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@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 -- Roderick Colenbrander Senior Manager of Hardware & Systems Engineering Sony Interactive Entertainment LLC roderick.colenbrander@xxxxxxxx
