Received: by 2002:a25:683:0:0:0:0:0 with SMTP id 125csp1473487ybg; Tue, 2 Jun 2020 10:55:30 -0700 (PDT) X-Google-Smtp-Source: ABdhPJz5eDRpUQLRPmH37w4HJ63mOLl6L5PwJMVzUDEch8RSpjPCd+6+2nyUPSaxTx8vSjXtqrrf X-Received: by 2002:a17:906:c155:: with SMTP id dp21mr23616019ejc.92.1591120530681; Tue, 02 Jun 2020 10:55:30 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1591120530; cv=none; d=google.com; s=arc-20160816; b=zwMDMGvjlLEyoWMvCi/d1ylmUXFFOYqSc7RBwAIbHB4gREvYHOGtFJYtHmj200CcBT oIfYYu0chJyMumsPmjPfgRf6dqvlmuFNO4ma6gZDOAfnR6UNmpMuNY4wRSh2iyzUNOCq KG/RCfvCtTgyZNQs75+BzRdCSy03S4AcfnqVdYL2GlfbFjO0tUCBssF0WsF2xe2lj2ME JiBYKgTEpuJ/wfZICm46b522sLBSdEAHH689DPrvcdpnX6zUh6ub3FEORNYUG4XOWMxh lYV6MHcHzLYfrBl8T8EaW94W1p7lfq6v06xszLUNtyYsrzZfpJvvHfYnNykmkC05rRah Zlrw== 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=ctWSouxkTe1B2SuX2wJ8d8p2cE9RiBSIhmTFWxgxHP8=; b=AOr6U9v9x3/srMl3NZVS1dGuiCAB/2T4i3Ea+9q5rJ5bV5V5k/HRb9yVUKmxIbTcGt Ay1ZKesOke+npouZMxSI4RpCVuleSqQ9gPd7njl+XbzGSRB32021+9gMta2TtGXw/ZRX JL1GPoRB0kRS86fcykM4/ix5xt/Y04qx/OyVLMVHTUHrDRIw66QkIPjDazX5hgabApyX Vxebon+WGwmjf8D2uY4mXSDxLlv7tMwm+mncgRrWt7KoOdPbcM0daiEaFUiuoIwE+2pD inbprbnkyVivU1sWsnUzUppVNFwbMqQtZiU2pc5uwLMxKl6SMmKYt+WzvdH2Zt3CGUi+ fMHg== 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 v13si1836319ejr.250.2020.06.02.10.55.07; Tue, 02 Jun 2020 10:55:30 -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 S1726589AbgFBRvH (ORCPT + 99 others); Tue, 2 Jun 2020 13:51:07 -0400 Received: from smtpout1.mo803.mail-out.ovh.net ([79.137.123.219]:53673 "EHLO smtpout1.mo803.mail-out.ovh.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726019AbgFBRvH (ORCPT ); Tue, 2 Jun 2020 13:51:07 -0400 Received: from pro2.mail.ovh.net (unknown [10.109.143.250]) by mo803.mail-out.ovh.net (Postfix) with ESMTPS id 7A652550CCA6; Tue, 2 Jun 2020 19:51:04 +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:51:03 +0200 Date: Tue, 2 Jun 2020 19:48:46 +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 2/4] iio: chemical: scd30: add I2C interface driver Message-ID: <20200602174846.GB2668@arch> References: <20200602164723.28858-1-tomasz.duszynski@octakon.com> <20200602164723.28858-3-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: 10486631732823481503 X-VR-SPAMSTATE: OK X-VR-SPAMSCORE: -100 X-VR-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrgeduhedrudefjedgkeelucetufdoteggodetrfdotffvucfrrhhofhhilhgvmecuqfggjfdpvefjgfevmfevgfenuceurghilhhouhhtmecuhedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujfgurhepfffhvffukfhfgggtuggjihesthdtredttddtjeenucfhrhhomhepvfhomhgrshiiucffuhhsiiihnhhskhhiuceothhomhgrshiirdguuhhsiiihnhhskhhisehotghtrghkohhnrdgtohhmqeenucggtffrrghtthgvrhhnpedtheevtefhffduteejfedtkeeuheejgeejvdetfffgveekffefgeffueeghefgjeenucfkpheptddrtddrtddrtddpfeegrddutdefrddvgedtrddutdefnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmohguvgepshhmthhpqdhouhhtpdhhvghlohepphhrohdvrdhmrghilhdrohhvhhdrnhgvthdpihhnvghtpedtrddtrddtrddtpdhmrghilhhfrhhomhepthhomhgrshiirdguuhhsiiihnhhskhhisehotghtrghkohhnrdgtohhmpdhrtghpthhtohepphhmvggvrhifsehpmhgvvghrfidrnhgvth Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Andy, On Tue, Jun 02, 2020 at 08:14:13PM +0300, Andy Shevchenko wrote: > On Tue, Jun 2, 2020 at 7:49 PM Tomasz Duszynski > wrote: > > > > Add I2C interface driver for the SCD30 sensor. > > ... > > > +static u16 scd30_i2c_cmd_lookup_tbl[] = { > > + [CMD_START_MEAS] = 0x0010, > > + [CMD_STOP_MEAS] = 0x0104, > > + [CMD_MEAS_INTERVAL] = 0x4600, > > + [CMD_MEAS_READY] = 0x0202, > > + [CMD_READ_MEAS] = 0x0300, > > + [CMD_ASC] = 0x5306, > > + [CMD_FRC] = 0x5204, > > + [CMD_TEMP_OFFSET] = 0x5403, > > + [CMD_FW_VERSION] = 0xd100, > > + [CMD_RESET] = 0xd304, > > Keep sorted by value? > I'd rather leave it as is simply because order here matches order in sensor datasheet. > > +}; > > ... > > > + ret = i2c_master_send(client, txbuf, txsize); > > > + if (ret != txsize) > > + return ret < 0 ? ret : -EIO; > > Wouldn't be better > > if (ret < 0) > return ret; > if (ret != txsize) > return -EIO; > > ? > Hmm, okay. Perhaps slightly easier to read. > > + if (!rxbuf) > > + return 0; > > + > > + ret = i2c_master_recv(client, rxbuf, rxsize); > > > + if (ret != rxsize) > > + return ret < 0 ? ret : -EIO; > > Ditto. > > ... > > > +static int scd30_i2c_command(struct scd30_state *state, enum scd30_cmd cmd, > > + u16 arg, void *response, int size) > > +{ > > + char crc, buf[SCD30_I2C_MAX_BUF_SIZE], *rsp = response; > > + int i, ret; > > i -> offset ? > 'i' is shorter and I am lazy :). > > + put_unaligned_be16(scd30_i2c_cmd_lookup_tbl[cmd], buf); > > + i = 2; > > + > > + if (rsp) { > > + /* each two bytes are followed by a crc8 */ > > + size += size / 2; > > + } else { > > + put_unaligned_be16(arg, buf + i); > > + crc = crc8(scd30_i2c_crc8_tbl, buf + i, 2, CRC8_INIT_VALUE); > > + i += 2; > > > + buf[i] = crc; > > + i += 1; > > buf[offset++] = crc; ? > I'd rather stick to what I have now. It looks more consistent. > > + /* commands below don't take an argument */ > > + if ((cmd == CMD_STOP_MEAS) || (cmd == CMD_RESET)) > > + i -= 3; > > + } > > + > > + ret = scd30_i2c_xfer(state, buf, i, buf, size); > > + if (ret) > > + return ret; > > + > > + /* validate received data and strip off crc bytes */ > > + for (i = 0; i < size; i += 3) { > > + crc = crc8(scd30_i2c_crc8_tbl, buf + i, 2, CRC8_INIT_VALUE); > > + if (crc != buf[i + 2]) { > > + dev_err(state->dev, "data integrity check failed\n"); > > + return -EIO; > > + } > > + > > > + *rsp++ = buf[i]; > > + 0 (for the sake of consistency? > Adding 0 is a little bit odd. > > + *rsp++ = buf[i + 1]; > > + } > > + > > + return 0; > > +} > > ... > > > +static struct i2c_driver scd30_i2c_driver = { > > + .driver = { > > > + .name = KBUILD_MODNAME, > > Better to hard code. > I seriously doubt anyone will ever want to change module name. What for? > > + .of_match_table = scd30_i2c_of_match, > > + .pm = &scd30_pm_ops, > > + }, > > + .probe_new = scd30_i2c_probe, > > +}; > > -- > With Best Regards, > Andy Shevchenko