Received: by 2002:ac0:a5a6:0:0:0:0:0 with SMTP id m35-v6csp402992imm; Sat, 22 Sep 2018 02:43:26 -0700 (PDT) X-Google-Smtp-Source: ACcGV62SuX3o2C6az5cUlu7reBXFFDWZTzUqPVMRqHrbG9pVjdx3f3JWZA1B3cwLvN5wBrpjiM59 X-Received: by 2002:a63:69c3:: with SMTP id e186-v6mr1613591pgc.431.1537609406490; Sat, 22 Sep 2018 02:43:26 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1537609406; cv=none; d=google.com; s=arc-20160816; b=KhS5SeFH3nGcG/xFNG615DEF6OnxDpTf6YgPZaXbpl8tnQ8aQ1hhf/+DZ8jSyYP1C0 6Vi+qIR2bZp2bIsOWS8xNAE8FM1tcygiP6BAd3Y0n5RlcdUMDL7JCMLKolQG5Q9fqP/+ UhTHD/R748QsMKPn0iyyFJnE5ffL8Ye5OkwVkoNLIr9kt6b0+PzBgifZ6MNvrK9biCYZ J8RlR0A77vsVfwA0KB3rR72r3+0Oj2ESiE+j1YKHE6JN/WZ15g6bEcwP9bKWH79ydg4K sAIUaDvuKyO8oOZZsROvQr33k4k1Ky9IM5Xjv9JbTQdBV6PgVwcQbj2hM8DoxfUvcjNg Jing== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :references:in-reply-to:message-id:subject:cc:to:from:date :dkim-signature; bh=XPyUzyP06IQldVDO0CZt2lpaNXOjlOqEdAl7qY89tCs=; b=lryQX67k32uA+DGo1leVicz7wobJF5NL9rdAzDo4T94SV4zmJFEmH8lRq4RyxA/jS0 Ru/YqbZ/xmjG5BnEL5HqF+ZgKz+TsdJbQZhGNFaRy+ETNRuNWWxiz27KaK7VEiHjHPM+ VSS06x1SSSjkKYAozB6nSfbI1OykeP2YkeeeF/NiGv8SFuiQbvLw2eEw6R9krGfiIT58 gPnKMZIR5tlIY8hhKH+ys1y7GRqfTixH5km+FE8+e8iCDLxxlFxja0kGdDkiuYcmPQ9f c5hDM68ZAXo6+p4B0uTWvA24oKNCG80PZaNF9I8v+tJDXmjb5VnyhoTfvLx8hk8SXjdt 61cw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=2jz3YkxQ; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id l185-v6si845217pgl.270.2018.09.22.02.42.55; Sat, 22 Sep 2018 02:43:26 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=2jz3YkxQ; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727806AbeIVPfq (ORCPT + 99 others); Sat, 22 Sep 2018 11:35:46 -0400 Received: from mail.kernel.org ([198.145.29.99]:57920 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726320AbeIVPfq (ORCPT ); Sat, 22 Sep 2018 11:35:46 -0400 Received: from archlinux (cpc91196-cmbg18-2-0-cust659.5-4.cable.virginm.net [81.96.234.148]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 5E78A21523; Sat, 22 Sep 2018 09:42:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1537609369; bh=mrZJE6uKX5ch7PF0ACHYhLfYw0pV+gVriGXnN3bRFh4=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=2jz3YkxQj9Ue44+NuLNI4dFl5Cj+91J682ObIMwVpY00/lYiqh9OMoU//Uv2AeENQ bTa9/IOqIFukrA5Yz4HXcP+O4UpCUeBr4MP5zT2E4eYcapcpWTjJvq/HKu38RgySmF QJUHr0dhuy28KX2Ms7uo0k/dvUTW6MwdqMtbv6xY= Date: Sat, 22 Sep 2018 10:42:44 +0100 From: Jonathan Cameron To: Song Qiang Cc: Peter Meerwald-Stadler , lars@metafoo.de, knaack.h@gmx.de, robh+dt@kernel.org, mark.rutland@arm.com, devicetree@vger.kernel.org, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer Message-ID: <20180922104244.651c802d@archlinux> In-Reply-To: <20180920180511.GA18559@Eros> References: <20180920131340.6699-1-songqiang1304521@gmail.com> <20180920180511.GA18559@Eros> X-Mailer: Claws Mail 3.17.1 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org A few follow ups to the discussion here from me.. Note it's helpful to crop and email and no one really minds if you just act on their review without acknowledging it (so cut the bits you fully agree with out too - saves on scrolling / reading time ;) A catch all, "Agree with everything else and will fix for v2" covers everything you don't want to discuss further. (not that I'm great at doing this either :( ... > > > diff --git a/drivers/iio/magnetometer/rm3100-core.c b/drivers/iio/magnetometer/rm3100-core.c > > > new file mode 100644 > > > index 000000000000..55d515e0fe67 > > > --- /dev/null > > > +++ b/drivers/iio/magnetometer/rm3100-core.c > > > @@ -0,0 +1,399 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > > +/* > > > + * PNI RM3100 9-axis geomagnetic sensor driver core. > > > + * > > > + * Copyright (C) 2018 Song Qiang > > > + * > > > + * User Manual available at > > > + * > > > + * > > > + * TODO: Scale channel, event generaton, pm. > > > > at least read support for _SCALE is mandatory, IMHO > > > > Okay, I'll add it in next version. > Just for the record, it's only mandatory in cases where it is known (you'd be amazed how often we only know the value is monotonic) Now as you put it in the todo list, it's presumably known here hence Peter's comment :) (just avoiding setting precedence) ... > > > +static const struct regmap_range rm3100_readable_ranges[] = { > > > + regmap_reg_range(RM_W_REG_START, RM_W_REG_END), > > > +}; > > > + > > > +const struct regmap_access_table rm3100_readable_table = { > > > > static > > > > I was planning to let rm3100-i2c.c and rm3100-spi.c to share these 6 > structures, because the only different configuration of regmap between > these two files lies in 'struct regmap_config'. To achieve this, I have > to expose these 3 structures to be referenced in rm3100-i2c.c and > rm3100-spi.c > Since *_common_probe() and *_common_remove() are exposed, I thought it > was fine to expose these structures to reduce redundant code, is this > prohibited? Nope, but are you doing it in this patch? + you need to export the symbols if you are going to do that otherwise modular builds will fail to probe (and possibly build - I can't recall and am too lazy to check this morning - not enough coffee yet!) .. > > > + *val = le32_to_cpu((buffer[0] << 16) + (buffer[1] << 8) + buffer[2]); > > > > no need for le32_to_cpu() > > > > I think I didn't fully understand this, I'll look into it. > To point you in the right direction here. When you explicitly pull out individual bytes like you are doing here, you are getting them in a particular endian order. Shifts and adding (though normally convention is | when doing this) are done in machine endianness, hence the *val is already in the endian type of your cpu. > > > + *val = sign_extend32(*val, 23); > > > + > > > + return IIO_VAL_INT; > > > +} > > > + > > > +#define RM_CHANNEL(axis, idx) \ > > > > use RM3100_ prefix please > > > > In the last driver I wrote, name of registers are so long that I have to > suppress them as possible as I can, it seems like this one doesn't need > to. :) > > > > + { \ > > > + .type = IIO_MAGN, \ > > > + .modified = 1, \ > > > + .channel2 = IIO_MOD_##axis, \ > > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ),\ > > > + .scan_index = idx, \ > > > + .scan_type = { \ > > > + .sign = 's', \ > > > + .realbits = 24, \ > > > + .storagebits = 32, \ > > > + .shift = 8, \ > > > + .endianness = IIO_LE, \ > > > + }, \ > > > + } > > > + > > > +static const struct iio_chan_spec rm3100_channels[] = { > > > + RM_CHANNEL(X, 0), > > > + RM_CHANNEL(Y, 1), > > > + RM_CHANNEL(Z, 2), > > > + IIO_CHAN_SOFT_TIMESTAMP(3), > > > +}; > > > + > > > +static const unsigned long rm3100_scan_masks[] = {GENMASK(2, 0), 0}; > > > + > > > +#define RM_SAMP_NUM 14 > > > > prefix > > > > > + > > > +/* Frequency : rm3100_samp_rates[][0].rm3100_samp_rates[][1]Hz. > > > + * Time between reading: rm3100_sam_rates[][2]ms (The first on is actially 1.7). > > > > one > > actually > > 1.7 what unit? > > > > It's in milliseconds. These time values are used for lookup so I do not > need to compute time between conversion from measurement frequency, and > they are used for wait time, I thought a little longer is better. > I think the comment above this structure isn't very clear, I'll find a > better way to explain it. Please also use kernel standard comment syntax /* * Frequency... */ > ... > > > + if (i != RM_SAMP_NUM) { > > > + mutex_lock(&data->lock); > > > + ret = regmap_write(regmap, RM_REG_TMRC, i + RM_TMRC_OFFSET); > > > + if (ret < 0) > > > > unlock? > > > > These actions are for changing the sampling frequency. This device > cannot start conversion if CMM register is not reset after reading from > CCX/CCY/CCZ registers. So I unlock it later since conversion should have > already been stopped and other threads should not access the bus. Hmm. If you are holding the lock across function calls you need to look at lockdep annotations. Also, I suspect something is wrong here as you are unlocking in the good path but not the bad one which seems unlikely to be as intended... > > > > + return ret; > > > + > > > + /* Checking if cycle count registers need changing. */ > > > + if (val == 600 && cycle_count == 200) { > > > + for (i = 0; i < 3; i++) { > > > + regmap_write(regmap, RM_REG_CCXL + 2 * i, 100); > > > + if (ret < 0) > > > > unlock? > > > > > + return ret; > > > + } > > > + } else if (val != 600 && cycle_count == 100) { > > > + for (i = 0; i < 3; i++) { > > > + regmap_write(regmap, RM_REG_CCXL + 2 * i, 200); > > > + if (ret < 0) > > > > unlock? > > > > > + return ret; > > > + } > > > + } > > > + /* Writing TMRC registers requires CMM reset. */ > > > + ret = regmap_write(regmap, RM_REG_CMM, 0); > > > + if (ret < 0) > > > > unlock? > > > > > + return ret; > > > + ret = regmap_write(regmap, RM_REG_CMM, RM_CMM_PMX | > > > + RM_CMM_PMY | RM_CMM_PMZ | RM_CMM_START); > > > + if (ret < 0) > > > > unlock? > > > > > + return ret; > > > + mutex_unlock(&data->lock); > > > + > > > + data->conversion_time = rm3100_samp_rates[i][2] + 3000; > > > + return 0; > > > + } > > > + return -EINVAL; > > > +} > > > + > > > +static int rm3100_read_raw(struct iio_dev *indio_dev, > > > + const struct iio_chan_spec *chan, > > > + int *val, int *val2, long mask) > > > +{ > > > + struct rm3100_data *data = iio_priv(indio_dev); > > > + int ret; > > > + > > > + switch (mask) { > > > + case IIO_CHAN_INFO_RAW: > > > + ret = iio_device_claim_direct_mode(indio_dev); > > > + if (ret < 0) > > > > release_direct_mode() here? > > > > Oh..yes! This should have stopped you reading more than once so I'm surprised this slipped through. I guess the usual last minute change problem ;) (we all do it however much we know we should retest properly) Jonathan