Received: by 2002:a05:6a10:413:0:0:0:0 with SMTP id 19csp2850657pxp; Tue, 22 Mar 2022 07:14:22 -0700 (PDT) X-Google-Smtp-Source: ABdhPJztZpTTzH1EM/jbYXAfTctvKCeENhc5WBC9dwiKaL2cf87iafoSaelSPz70L0FuKKiRq+Tu X-Received: by 2002:a05:6a00:328f:b0:4fa:95b5:e1ff with SMTP id ck15-20020a056a00328f00b004fa95b5e1ffmr12271862pfb.60.1647958462505; Tue, 22 Mar 2022 07:14:22 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1647958462; cv=none; d=google.com; s=arc-20160816; b=ULpg7rVqx+/r0NWGnYhFvJFKbQkf0fWQu2HdvupsDcPaurCSiALWLHkQ0Xgn6iFu3q 3uD19kgsufZDBdytB52KdeAm5/niu21T24cVOJ2BXHTi4lDYWnIcApsc95VW5VQAT0R0 uxkbub6yt4k8HTZvFywgSSMoOkv6hDiC9KAYBqSZxW1BFOq0LK3S+BC4Wb8/9K/y4vQE n1OR5OG6TCbu7J9l3ZEXe725a4l0EwxQxOwxiPRs08q42HFvgbSsVOaq1wkfo6bOUQYt 28zy/IDODJE6psfx4O5FEWQd42dkQpKHFAa6czSnIJfS0Q/vEkoB7UgXyezAAdx/Jhai Zd3g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:reply-to :in-reply-to:references:mime-version:dkim-signature; bh=A8Uwl5R9FFhyS86G5PyV/+OCLLYlqbgHEjOZGBcExKg=; b=m/fQ7CgHW0EudvrBbJZyLtjuUif6QWOb7EjBx7/mKRHv8d3+F+RjVL0ngwYRGXRM2A g4wR/MViu3iMrzVPDyy3jGnMcNCumMpzjNl2srJedZtLZc/IrCN5x+11YxKwqPqzqcsB h99Y/3PDpI0HtRHxNz5aA5mPo/k/EaRYiRQblit+JBLMI3dG2BGFvOUjQDPqt6D/puVw e0i8GOBNmJKfdR4AdqbJFPXA55a2+BwWy6VwDjRak9gvXPzypGynhDQd/r6FFKt10maI bJMvU3AQ9eZ/ZLhy5T7SbALsA2MXGL9K2RLLKTzXEevCJIGvcGKa+XcjRJ3v7NeHR22i W2GQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=THgrE5uq; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id g6-20020a655806000000b003816043f0dfsi16360821pgr.724.2022.03.22.07.14.07; Tue, 22 Mar 2022 07:14:22 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=THgrE5uq; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233113AbiCVKja (ORCPT + 99 others); Tue, 22 Mar 2022 06:39:30 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52350 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233103AbiCVKj0 (ORCPT ); Tue, 22 Mar 2022 06:39:26 -0400 Received: from mail-vs1-xe2e.google.com (mail-vs1-xe2e.google.com [IPv6:2607:f8b0:4864:20::e2e]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 72D4919294; Tue, 22 Mar 2022 03:37:59 -0700 (PDT) Received: by mail-vs1-xe2e.google.com with SMTP id y4so18538614vsd.11; Tue, 22 Mar 2022 03:37:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:reply-to:from:date:message-id :subject:to:cc; bh=A8Uwl5R9FFhyS86G5PyV/+OCLLYlqbgHEjOZGBcExKg=; b=THgrE5uqHYUftr0rIVpMCDI3nJyG9ASl9fqWURbpF4ljru1ObqwKEASBFjIfwRL4jb ibo83ukpDUhcM1wXLObYxw/DKH0YggtwvueERPXiluejhpnLgz/zlWFI1e/ZBgLlInlQ BSA7Uj6kfM1zfd0mjnQ2v5YVwJJp2kFrMC4Fo437YxZlbJ0fU/ea4onz/Se7kkBLcAFu 9eLGReIr1uzNJk8Cyu4zVMxhfHAFaZN1Y5JGxBNpa/h3yQb61cpilkvBVrFM9CIoU4J/ QU8jutKgLFC6jXxR4pixEpyWFfEkDJ4y9f8yyqMIhtZoDxg/ddRIzRIfosIz6Nvhobun PTig== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:reply-to :from:date:message-id:subject:to:cc; bh=A8Uwl5R9FFhyS86G5PyV/+OCLLYlqbgHEjOZGBcExKg=; b=zzHl0IBrrMphWQfWNB5IGTlp7drWpKo3/lXWE9/Nvy7xIN68JDrH/aVaqXQVG3XsMU 2l/45FAqGVPuyprFyaQA3hKOjsCnwKSMiGM8a7L2fjNqiUFh1xl7MyZbR4rH8v4zVB1m mK7NdxiSqkSd5k5r/hcMogY9eExMTvJnxLP06xo42qCMcRu04cNwiOv5pkQ7ZCYOC3tc kWIz+z+CzQzD38O5d/1qcs/2h/r13dJqIgXAT2dfto9HGPu5er1qLfYsMjYpZ0njHBGb q3TMv7TpjimbwBzUG8/2SyWezXqwm89v3XkNcEpfEf2UUpHEnWmjcH0j+lNvfP7gcBBt 9c2g== X-Gm-Message-State: AOAM530ddrLTU4VDjffYb9/vTx+w05vJzL29on1+4W/srNk2/SrfU9j/ hd7tWtd+JosUFCw6NnzeJ2qQc0OkT6VM00tGJSwVbQ/0I/d0Fw== X-Received: by 2002:a05:6102:284a:b0:31e:c455:5dee with SMTP id az10-20020a056102284a00b0031ec4555deemr8594948vsb.27.1647945478537; Tue, 22 Mar 2022 03:37:58 -0700 (PDT) MIME-Version: 1.0 References: <20220217162710.33615-1-andrea.merello@gmail.com> <20220217162710.33615-12-andrea.merello@gmail.com> In-Reply-To: Reply-To: andrea.merello@gmail.com From: Andrea Merello Date: Tue, 22 Mar 2022 11:37:47 +0100 Message-ID: Subject: Re: [v3 11/13] iio: imu: add BNO055 serdev driver To: Andy Shevchenko Cc: Jonathan Cameron , Mauro Carvalho Chehab , linux-iio , linux-kernel , devicetree , Lars-Peter Clausen , Rob Herring , Matt Ranostay , Alexandru Ardelean , Jacopo Mondi , Andrea Merello Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Some inline comments, OK for the rest. Il giorno lun 21 feb 2022 alle ore 21:28 Andy Shevchenko ha scritto: > > On Thu, Feb 17, 2022 at 5:27 PM Andrea Merello wrote: > > > > This path adds a serdev driver for communicating to a BNO055 IMU via > > serial bus, and it enables the BNO055 core driver to work in this > > scenario. [...] > > + /** > > + * enum cmd_status - represent the status of a command sent to the HW. > > + * @STATUS_OK: The command executed successfully. > > + * @STATUS_FAIL: The command failed: HW responded with an error. > > + * @STATUS_CRIT: The command failed: the serial communication failed. > > + */ > > + enum { > > + STATUS_OK = 0, > > + STATUS_FAIL = 1, > > + STATUS_CRIT = -1 > > + Comma and sort them by value? > For the second part is an additional question, why negative? For STATUS_CRIT, being a (bad) error, a negative value seemed appropriate to me. STATUS_OK is zero as usual, but maybe STATUS_FAIL should be negative also? It is some legal protocol status (unlike the STATUS_CRIT mess), in this sense I may consider it not an error, but still our command failed (because the IMU politely didn't accept it), so it's an error in this sense... I may just let all of them implicit if you prefer. [...] > > + while (1) { > > + ret = bno055_sl_send_chunk(priv, hdr + i * 2, 2); > > + if (ret) > > + goto fail; > > + > > + if (i++ == 1) > > + break; > > + usleep_range(2000, 3000); > > + } > > The infinite loops are hard to read and understand. > Can you convert it to the regular while or for one? > > Also, this looks like a repetition of something (however it seems that > it's two sequencial packets to send). Maybe it's worth to unroll then? > ... > > > + const int retry_max = 5; > > + int retry = retry_max; > > > + while (retry--) { > > Instead simply use > > unsigned int retries = 5; > > do { > ... > } while (--retries); > > which is much better to understand. OK, but still have the const var for the max (see below) > ... > > > + if (retry != (retry_max - 1)) > > + dev_dbg(&priv->serdev->dev, "cmd retry: %d", > > + retry_max - retry); > > This is an invariant to the loop. why? This triggers at all retries, not at the first attempt i.e. it prints only if this doesn't succeed at the first time. Indeed what seems wrong to me is that you need -1 also in the dev_dbg() argument to produce correct text. [...] > > > + reg_addr = ((u8 *)reg)[0]; > > This looks ugly. > Can't you supply the data struct pointer instead of void pointer? I confirm that it's ugly :) Not sure about what you exactly meant, sorry; what I can do is to introduce a local and split this ugly loc, as done in bno055_sl_write_reg(). Is this what you are suggesting? > ... > > > + if (serdev_device_set_baudrate(serdev, 115200) != 115200) { > > Is it limitation / requirement by the hardware? Otherwise it should > come from DT / ACPI. It's a requirement. Not sure it's really by the HW; possibly it's statically set in device firmware. > ... > > > + ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE); > > Ditto. Ditto :) [...]