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

["Barnabás Pőcze" <pobrn@protonmail.com>]

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