Received: by 2002:a25:683:0:0:0:0:0 with SMTP id 125csp1479099ybg; Tue, 2 Jun 2020 11:05:01 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxOim2Oxn/z3fJnevyzvLA08DQnQVx0qpgIjExpYGJ5cUtfgCGcEeWeOhcQEChOGw8F5/xX X-Received: by 2002:a17:907:72cf:: with SMTP id du15mr22299199ejc.151.1591121100987; Tue, 02 Jun 2020 11:05:00 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1591121100; cv=none; d=google.com; s=arc-20160816; b=USOiOytDqe6kt84aKd9G1xg6uCSz2lTEWKS/kFWNyy4p/ShHfHgdNcA8HCbbZglLHf 0KeqmE7KkdCcxSLLRxL90tDKycTMjtNQn8ah0uHWxInZUoi1ZVozkrU9XB02VOwflgeq VyP3t8bk+pHOg+7znYmbmClT1/4zJCEUIZ8Y889fTguAt3b16mUGDA7yMBc9TkzNB9If Rdr2fm4vYVv2gIl3sVDRwRUU793upcWEmw6Oq/lGcBx1BaUlbKCchzbzf3NvnHUnytM5 1gY2nZ1iP131UVa5L8+CfhA/WpVL11Pyh3nX9z1L/+VvKgiVRbV3inM3v5jxPYTWatrT QY5w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:in-reply-to:content-disposition :mime-version:references:message-id:subject:cc:to:from:date; bh=LZUZJvfzDwWDgenPiexMrX1vwRQPM0x5QBm1AX/csgE=; b=LP8IBSFEETqaqnK7oueJRVkRsE7cKB9pSeZaFj2AoEhgGCbn6rGRurinsxALhQWotI fo8tNDSkYRG1U+MCFrMWk40EEQt7YA2oHBRwepThvlzCsjYnZt6MIF8TQuHWe5CW+ngG GAvjW6eZInscj1sknAEyIjmvfYPwHzsFVKPSHM3hfDzJmnpXr5ZrbfSzO3YABTYrVHWh 9rrw8WRccNhmZAWL9eRIUPCPQhA+K5tqJfgjbNnP+fauHSg5vB01NBvNsS6dWXCyEjRE Hk7v8FvLX37ftSvq3Zh3JEfKzk4Q/PiHJGMomk1MbXtA5sxmuWE++MlMNbRwVRhARro7 Oijw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id cb22si1876663ejb.99.2020.06.02.11.04.36; Tue, 02 Jun 2020 11:05:00 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727055AbgFBR7i (ORCPT + 99 others); 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: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@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