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=-0.8 required=3.0 tests=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 F2926C433E0 for ; Tue, 2 Jun 2020 17:59:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D8D622072F for ; Tue, 2 Jun 2020 17:59:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726380AbgFBR7i (ORCPT ); Tue, 2 Jun 2020 13:59:38 -0400 Received: from smtpout1.mo803.mail-out.ovh.net ([79.137.123.219]:47279 "EHLO smtpout1.mo803.mail-out.ovh.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726019AbgFBR7i (ORCPT ); Tue, 2 Jun 2020 13:59:38 -0400 Received: from pro2.mail.ovh.net (unknown [10.108.20.232]) by mo803.mail-out.ovh.net (Postfix) with ESMTPS id 97FD5550CFC6; Tue, 2 Jun 2020 19:59:35 +0200 (CEST) Received: from localhost (34.103.240.103) by DAG2EX1.emp2.local (172.16.2.11) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1847.3; Tue, 2 Jun 2020 19:59:35 +0200 Date: Tue, 2 Jun 2020 19:57:23 +0200 From: Tomasz Duszynski To: Andy Shevchenko CC: Tomasz Duszynski , linux-iio , Linux Kernel Mailing List , devicetree , Rob Herring , Jonathan Cameron , Peter Meerwald Subject: Re: [PATCH v3 3/4] iio: chemical: scd30: add serial interface driver Message-ID: <20200602175723.GC2668@arch> References: <20200602164723.28858-1-tomasz.duszynski@octakon.com> <20200602164723.28858-4-tomasz.duszynski@octakon.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline In-Reply-To: X-Originating-IP: [34.103.240.103] X-ClientProxiedBy: DAG3EX2.emp2.local (172.16.2.22) To DAG2EX1.emp2.local (172.16.2.11) X-Ovh-Tracer-Id: 10630465448542035103 X-VR-SPAMSTATE: OK X-VR-SPAMSCORE: -100 X-VR-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrgeduhedrudefjedgleduucetufdoteggodetrfdotffvucfrrhhofhhilhgvmecuqfggjfdpvefjgfevmfevgfenuceurghilhhouhhtmecuhedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujfgurhepfffhvffukfhfgggtuggjihesthdtredttddtjeenucfhrhhomhepvfhomhgrshiiucffuhhsiiihnhhskhhiuceothhomhgrshiirdguuhhsiiihnhhskhhisehotghtrghkohhnrdgtohhmqeenucggtffrrghtthgvrhhnpedtheevtefhffduteejfedtkeeuheejgeejvdetfffgveekffefgeffueeghefgjeenucfkpheptddrtddrtddrtddpfeegrddutdefrddvgedtrddutdefnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmohguvgepshhmthhpqdhouhhtpdhhvghlohepphhrohdvrdhmrghilhdrohhvhhdrnhgvthdpihhnvghtpedtrddtrddtrddtpdhmrghilhhfrhhomhepthhomhgrshiirdguuhhsiiihnhhskhhisehotghtrghkohhnrdgtohhmpdhrtghpthhtohepphhmvggvrhifsehpmhgvvghrfidrnhgvth Sender: devicetree-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On Tue, Jun 02, 2020 at 08:04:16PM +0300, Andy Shevchenko wrote: > On Tue, Jun 2, 2020 at 7:49 PM Tomasz Duszynski > wrote: > > > > Add serial interface driver for the SCD30 sensor. > > ... > > > +static u16 scd30_serdev_cmd_lookup_tbl[] = { > > + [CMD_START_MEAS] = 0x0036, > > + [CMD_STOP_MEAS] = 0x0037, > > + [CMD_MEAS_INTERVAL] = 0x0025, > > + [CMD_MEAS_READY] = 0x0027, > > + [CMD_READ_MEAS] = 0x0028, > > + [CMD_ASC] = 0x003a, > > + [CMD_FRC] = 0x0039, > > + [CMD_TEMP_OFFSET] = 0x003b, > > + [CMD_FW_VERSION] = 0x0020, > > + [CMD_RESET] = 0x0034, > > Hmm... Can't we keep them ordered by value? > > > +}; > > ... > > > + ret = wait_for_completion_interruptible_timeout(&priv->meas_ready, > > + SCD30_SERDEV_TIMEOUT); > > + if (ret > 0) > > + ret = 0; > > + else if (!ret) > > + ret = -ETIMEDOUT; > > + > > + return ret; > > Perhaps > > if (ret < 0) > return ret; > if (ret == 0) > return -ETIMEDOUT; > return 0; > > ? Matter of style but since I will be doing other changes I can touch that too. > > ... > > > + char txbuf[SCD30_SERDEV_MAX_BUF_SIZE] = { SCD30_SERDEV_ADDR }, > > + rxbuf[SCD30_SERDEV_MAX_BUF_SIZE], *rsp = response; > > Please, apply type to each variable separately. > Fine. > ... > > > + switch (txbuf[1]) { > > + case SCD30_SERDEV_WRITE: > > > + if (memcmp(txbuf, txbuf, txsize)) { > > I'm not sure I understand this. > Ah, thanks for catching this. tx should be compared to rx. > > + dev_err(state->dev, "wrong message received\n"); > > + return -EIO; > > + } > > + break; > > + case SCD30_SERDEV_READ: > > > + if (rxbuf[2] != (rxsize - > > + SCD30_SERDEV_RX_HEADER_SIZE - > > + SCD30_SERDEV_CRC_SIZE)) { > > Perhaps you can come up with better indentation/ line split? > I'd rather leave it as is. > > + dev_err(state->dev, > > + "received data size does not match header\n"); > > + return -EIO; > > + } > > + > > + rxsize -= SCD30_SERDEV_CRC_SIZE; > > + crc = get_unaligned_le16(rxbuf + rxsize); > > + if (crc != scd30_serdev_calc_crc(rxbuf, rxsize)) { > > + dev_err(state->dev, "data integrity check failed\n"); > > + return -EIO; > > + } > > + > > + rxsize -= SCD30_SERDEV_RX_HEADER_SIZE; > > + memcpy(rsp, rxbuf + SCD30_SERDEV_RX_HEADER_SIZE, rxsize); > > + break; > > + default: > > + dev_err(state->dev, "received unknown op code\n"); > > + return -EIO; > > + } > > + > > + return 0; > > +} > > ... > > > +static struct serdev_device_driver scd30_serdev_driver = { > > + .driver = { > > > + .name = KBUILD_MODNAME, > > This is not the best what we can do. The name is an ABI and better if > it will be constant (independent on file name). > > > + .of_match_table = scd30_serdev_of_match, > > + .pm = &scd30_pm_ops, > > + }, > > + .probe = scd30_serdev_probe, > > +}; > > -- > With Best Regards, > Andy Shevchenko