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

[Roderick Colenbrander <roderick@gaikai.com>]

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@google.com> 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 <pobrn@protonmail.com> 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@sony.com