From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E96E5C433E6 for ; Fri, 8 Jan 2021 12:02:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A3F9D239FD for ; Fri, 8 Jan 2021 12:02:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725884AbhAHMCd (ORCPT ); Fri, 8 Jan 2021 07:02:33 -0500 Received: from mail1.protonmail.ch ([185.70.40.18]:24382 "EHLO mail1.protonmail.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725791AbhAHMCd (ORCPT ); Fri, 8 Jan 2021 07:02:33 -0500 Date: Fri, 08 Jan 2021 12:01:44 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=protonmail; t=1610107309; bh=zpTfmAFmzzyF12fSZ+3SgpCbVFEAuo4rlSl1vkrvXaA=; h=Date:To:From:Cc:Reply-To:Subject:In-Reply-To:References:From; b=mLIaQG0a0PQehMZP5SeWiE7S9xE1MjgcPGyeBGstN3f6eazeDQtjREUBRKS0PB2xl AlzB8OYSZlmz0ufXHbcINBavaNiY7d6Bb5DImVhLsGTpqIQfIMnBh2Ybj6MuZ+uzwC 7PVi0GQHjeRP3EHImvALt+7yQEGOTh4QHxUTrXE4= To: Roderick Colenbrander From: =?utf-8?Q?Barnab=C3=A1s_P=C5=91cze?= Cc: Jiri Kosina , Benjamin Tissoires , "linux-input@vger.kernel.org" , Chris Ye , Roderick Colenbrander Reply-To: =?utf-8?Q?Barnab=C3=A1s_P=C5=91cze?= Subject: Re: [PATCH v2 05/13] HID: playstation: add DualSense accelerometer and gyroscope support. Message-ID: In-Reply-To: References: <20210102223109.996781-1-roderick@gaikai.com> <20210102223109.996781-6-roderick@gaikai.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org Hi 2021. janu=C3=A1r 8., p=C3=A9ntek 7:06 keltez=C3=A9ssel, Roderick Colenbran= der =C3=ADrta: > [...] > > > +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 =3D 0; > > > + uint8_t *buf; > > > + > > > + buf =3D kzalloc(DS_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL)= ; > > > + if (!buf) > > > + return -ENOMEM; > > > + > > > + ret =3D hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_CAL= IBRATION, 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 !=3D 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 !=3D ...) { > > ... > > } > > ``` > > > > > > > + hid_err(ds->base.hdev, "failed to retrieve DualSense ca= libration info\n"); > > > > I think this message could be improved to better pinpoint the exact pro= blem > > that triggered it. > > > > > > > + ret =3D -EINVAL; > > > + goto err_free; > > > + } > [...] Regards, Barnab=C3=A1s P=C5=91cze