Received: by 2002:ab2:7b86:0:b0:1f7:5705:b850 with SMTP id q6csp1411685lqh; Mon, 6 May 2024 07:06:23 -0700 (PDT) X-Forwarded-Encrypted: i=3; AJvYcCX41PzwaMTujHsfI7n2ZT+8RKOBBlkeMLA69TYAK/vP27qMsgauC4MA7wWPC0Bom8F8WNH3XkzIveeW3A/wqFht0DnmfXFYVUMqPH7/Lg== X-Google-Smtp-Source: AGHT+IESTC15Jj7xHkP7OStUjpyNMfKAtAn0CJU8I1yjt6FpuCDT+vRvvFDSvuTGs01zJKIrCpNi X-Received: by 2002:a50:d699:0:b0:56e:603:9fc9 with SMTP id r25-20020a50d699000000b0056e06039fc9mr7269197edi.3.1715004383266; Mon, 06 May 2024 07:06:23 -0700 (PDT) ARC-Seal: i=2; a=rsa-sha256; t=1715004383; cv=pass; d=google.com; s=arc-20160816; b=XLZPkk0nMBxSVfkb/W0vJJdZvqKzUKF1C+TN7Wmtg8lbLO4w2gwGKOlnu+6wITkARf 6SjmvydPvVXfohBhPcdmsm2+RLsBMd4/CWnNdGVd14ZMd2LaaoaiXZAHUcICviR8n5Cj YHe0SE7nyHOhAwSls5rbxT2IADm/17yJqMpYXVNB/zpf89jd8+8xystP3AXzP9mQK+cw kkxad5pyDE0kVE3QGY2+ci/GahMHqwcXZr7V3pUMWfRVMhlPuZLM2us/KfG63GM2OR1d PbJoMzJlgbk9WNutssc5bW/3OZHcdC+2Ko9KWPEnn8cCyzzEm2JXu/tQLTlCkxrUTtX/ J9rg== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:in-reply-to:message-id :subject:cc:to:from:date:dkim-signature; bh=w9pu3/MGAZiCb1H6m687tjKbkPl8Ekrs3jVSdHCha0U=; fh=2qYXGo5fpfJwEM8VGdvpKV9NMfKfWT+2pOyklyaTPCE=; b=dEYoFWRtvm3FanPs7onGK2WRdWZFg/xheFW+GqKR06ihBb+Z/7JiA9doVjnbiTYFIE Q8IVb+8/vfFGXX6DyJ2APZned6Ru0sYiNSensGldayHVLfA/F5mKi9Bw6rqa/jOKXErw F0LMMs0TYhquAeEhhzbDUPqUvcsoP5ZxjhVUQCh3RMHSKijGXfSHQ2MVTihhGyxzbPxT m0vBiQA36RzOUvDW6OxMkRtMiQ9ytFPViKrXwHm5C0ou4io+AebCHQh8sJJcG0vtNwFK EFitEpmEqrE/wOEz9jCKDhCwtnFkQmD+dEePOyvbklBItaPCIhAjGH0R5YnJcleeL/mQ kqIQ==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b="f//CutHQ"; arc=pass (i=1 dkim=pass dkdomain=kernel.org); spf=pass (google.com: domain of linux-kernel+bounces-169945-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) smtp.mailfrom="linux-kernel+bounces-169945-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from am.mirrors.kernel.org (am.mirrors.kernel.org. [2604:1380:4601:e00::3]) by mx.google.com with ESMTPS id v7-20020aa7d647000000b00572aff17edesi4890597edr.246.2024.05.06.07.06.23 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 May 2024 07:06:23 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel+bounces-169945-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b="f//CutHQ"; arc=pass (i=1 dkim=pass dkdomain=kernel.org); spf=pass (google.com: domain of linux-kernel+bounces-169945-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) smtp.mailfrom="linux-kernel+bounces-169945-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id C9DF11F23DBA for ; Mon, 6 May 2024 14:06:22 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 341F381ACC; Mon, 6 May 2024 14:06:13 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="f//CutHQ" Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 340A515A5; Mon, 6 May 2024 14:06:11 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1715004372; cv=none; b=Yz0jxRrZqDMRiuapRRQ9r1lkxGpoGtSNX01m1Qg1z6SDeOHvmZ+IU0B0uyNDHqxXTeAFzgpLd7wChotArEj5LwWqNaeI5qL/7hapsNft6Oq7UxQAJCz69FFbflvZbWNedAYqE5bUju2aBD0oLmpXsTHsXP5zC/YQZVMl9BQuhLk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1715004372; c=relaxed/simple; bh=cZZmZVxWa908an4uMtHbVpLQV8WR9PbqQyordvaexD4=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=DFsBCqdDLDXVqv5PUP4lFzm8MfAzmnED6bAObSRI7BPugJgimWxBE0Zgl5hToD/pnSArL/vDhiiknHP684snkVaKoruJoL7g2vLYlVOaL/WHXmqcEVMqOvBEzb+5BtgdY297Mz9RbvNHfmbPrYerQmyy1+UMMjfRQFs1p1/q0Ws= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=f//CutHQ; arc=none smtp.client-ip=10.30.226.201 Received: by smtp.kernel.org (Postfix) with ESMTPSA id A1B06C116B1; Mon, 6 May 2024 14:06:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1715004371; bh=cZZmZVxWa908an4uMtHbVpLQV8WR9PbqQyordvaexD4=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=f//CutHQaYJsnJznM3/58nHl0guQPZ/BTuELJ+JQt1zui4ZGKeFhABWVDnqbdtPY5 WXOFgbvVu1hIj39YpX4wzuvZaN7BF/EfkSj+TE8BTHtrE1F/+u9ZzPqk24sqXOUlMW cUsT8oBJEn0GQfkleL7Qf1lB8dXViM/IGzmZ32l130rw8OHti+WWVv/M2pVDkxfZHm 0Th5KrI2LLSsRuktS7ruTgn86T4P2X4sq0PNcEChYqgmWSKSVtd//KW4DGIPR1mG/b pXa7gYlUR6/Cuu0jO4FvaQAqkLR2i7nYncMbxxlSpV4fHFzl2LqKq2UnV27e2EIFfq CJSH1q1yB4e9A== Date: Mon, 6 May 2024 15:05:57 +0100 From: Jonathan Cameron To: Julien Stephan Cc: Lars-Peter Clausen , Michael Hennerich , Nuno =?UTF-8?B?U8Oh?= , David Lechner , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Liam Girdwood , Mark Brown , kernel test robot , linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH RFC v6 08/10] iio: adc: ad7380: add oversampling support Message-ID: <20240506150557.1149c394@jic23-huawei> In-Reply-To: <20240501-adding-new-ad738x-driver-v6-8-3c0741154728@baylibre.com> References: <20240501-adding-new-ad738x-driver-v6-0-3c0741154728@baylibre.com> <20240501-adding-new-ad738x-driver-v6-8-3c0741154728@baylibre.com> X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Wed, 01 May 2024 16:55:41 +0200 Julien Stephan wrote: > ad7380x(-4) parts are able to do oversampling to increase accuracy. > They support 2 average modes: normal average and rolling overage. > This commits focus on enabling normal average oversampling, which is the > default one. The other case got me curious. If you do want to support the rolling average in future, it could probably be handled as a low pass filter control rather than a form of oversampling. Anyhow, not relevant here! > > Normal averaging involves taking a number of samples, adding them together, > and dividing the result by the number of samples taken. > This result is then output from the device. The sample data is cleared when > the process completes. Because we need more samples to output a value, > the data output rate decrease with the oversampling ratio. > > Signed-off-by: Julien Stephan Hi Julien. A few additional comments inline. Jonathan > --- > drivers/iio/adc/ad7380.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 114 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c > index 020959759170..1e3869f5e48c 100644 > --- a/drivers/iio/adc/ad7380.c > +++ b/drivers/iio/adc/ad7380.c > @@ -88,7 +88,10 @@ struct ad7380_chip_info { > .type = IIO_VOLTAGE, \ > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > ((diff) ? 0 : BIT(IIO_CHAN_INFO_OFFSET)), \ > - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ > + .info_mask_shared_by_type_available = \ > + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ > .indexed = 1, \ > .differential = (diff), \ > .channel = (diff) ? (2 * (index)) : (index), \ > @@ -156,6 +159,16 @@ static const struct ad7380_timing_specs ad7380_4_timing = { > .t_csh_ns = 20, > }; > > +/* > + * Available oversampling ratios. The indices correspond > + * with the bit value expected by the chip. > + * The available ratios depend on the averaging mode, > + * only normal averaging is supported for now > + */ > +static const int ad7380_normal_average_oversampling_ratios[] = { > + 1, 2, 4, 8, 16, 32, > +}; > + > static const struct ad7380_chip_info ad7380_chip_info = { > .name = "ad7380", > .channels = ad7380_channels, > @@ -231,6 +244,7 @@ static const struct ad7380_chip_info ad7384_4_chip_info = { > struct ad7380_state { > const struct ad7380_chip_info *chip_info; > struct spi_device *spi; > + unsigned int oversampling_ratio; > struct regmap *regmap; > unsigned int vref_mv; > unsigned int vcm_mv[MAX_NUM_CHANNELS]; > @@ -386,6 +400,12 @@ static int ad7380_read_direct(struct ad7380_state *st, > }; > int ret; > > + /* > + * In normal average oversampling we need to wait for multiple conversions to be done Wrap comment at 80 chars. Generally I prefer we keep to old limit of 80 unless there is a readability advantage. I don't see such an advantage in this case. > + */ > + if (st->oversampling_ratio > 1) > + xfers[0].delay.value = T_CONVERT_NS + 500 * st->oversampling_ratio; > + > + > +/** > + * check_osr - Check the oversampling ratio > + * @available_ratio: available ratios's array > + * @size: size of the available_ratio array > + * ratio: ratio to check > + * > + * Check if ratio is present in @available_ratio. Check for exact match. > + * @available_ratio is an array of the available ratios (depending on the oversampling mode). > + * The indices must correspond with the bit value expected by the chip. > + */ > +static inline int check_osr(const int *available_ratio, int size, int ratio) > +{ > + int i; > + > + for (i = 0; i < size; i++) { > + if (ratio == available_ratio[i]) > + return i; > + } > + > + return -EINVAL; > +} > + > +static int ad7380_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int val, > + int val2, long mask) > +{ > + struct ad7380_state *st = iio_priv(indio_dev); > + int ret, osr; > + > + switch (mask) { > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > + osr = check_osr(ad7380_normal_average_oversampling_ratios, Nuno already pointed out function name should be prefixed. > + ARRAY_SIZE(ad7380_normal_average_oversampling_ratios), > + val); If this is just checking, why does it return osr? Feels like name needs to be ad7380_osr_to_regval() or something like that. > + > + if (osr < 0) > + return osr; > + > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { > + ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1, > + AD7380_CONFIG1_OSR, > + FIELD_PREP(AD7380_CONFIG1_OSR, osr)); > + > + if (ret) > + return ret; > + > + st->oversampling_ratio = val; > + > + /* > + * Perform a soft reset. > + * This will flush the oversampling block and FIFO but will > + * maintain the content of the configurable registers. > + */ > + ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2, > + AD7380_CONFIG2_RESET, > + FIELD_PREP(AD7380_CONFIG2_RESET, > + AD7380_CONFIG2_RESET_SOFT)); > + } > + return 0; > default: > return -EINVAL; > } > @@ -435,6 +540,8 @@ static int ad7380_read_raw(struct iio_dev *indio_dev, > > static const struct iio_info ad7380_info = { > .read_raw = &ad7380_read_raw, > + .read_avail = &ad7380_read_avail, > + .write_raw = &ad7380_write_raw, > .debugfs_reg_access = &ad7380_debugfs_reg_access, > }; > > @@ -458,6 +565,12 @@ static int ad7380_init(struct ad7380_state *st, struct regulator *vref) > if (ret < 0) > return ret; > > + /* Disable oversampling by default. IIO comment syntax is /* * Disable oversampling by default. Also, curiously short lines that could definitely be wrapped nearer 80 chars! > + * This is the default value after reset, > + * so just initialize internal data > + */ > + st->oversampling_ratio = 1; > + > /* SPI 1-wire mode */ > return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2, > AD7380_CONFIG2_SDO, >