Received: by 2002:ab2:7b86:0:b0:1f7:5705:b850 with SMTP id q6csp1428015lqh; Mon, 6 May 2024 07:30:37 -0700 (PDT) X-Forwarded-Encrypted: i=3; AJvYcCWCak9C7DuFm2e3lxDsPkFxEvXEUe+gp5ZA2AaHm7/Q9xRq4B8S7qh0kv9YwtM6WfvnQoneIZhhey0EFXIN/jQJPZB5PFvTLDANhSF0sQ== X-Google-Smtp-Source: AGHT+IEeA5XzgacUxpWgCBFtYETaOOXd3jSiMuDSltv/KnVplsPnbRNDx99vzq4a00d0HD6+TTPX X-Received: by 2002:ac2:4d97:0:b0:519:2c84:2405 with SMTP id g23-20020ac24d97000000b005192c842405mr5541078lfe.44.1715005837053; Mon, 06 May 2024 07:30:37 -0700 (PDT) ARC-Seal: i=2; a=rsa-sha256; t=1715005837; cv=pass; d=google.com; s=arc-20160816; b=NPlTA1Z4iMZpz27xZKdLKFXolfzAF0DooZBUrNGtJXvnHOZCm3J+eZTzFzCMDHqt7o 4rzOAnvN92fYOvp+XU0OsW653E50nixKsyFtLOVGGI/bR6+cXPtEUFe81zU7bz725eIO wOLtVRqWpZV+5ZJAgk/h/z3ifQ3SrYBojs65KvE1rwzNDZcPIreESTWiLrYxWBEqSHmn rkAHxlmNXCgjyECKiluX+pFvgR9lCMQtrGEWxbfVWsPtpLobYJiyN3glvrAGmFkk37J3 5OPhAdDyARmBaKAaqcG5dlYjPKOFtEd2acE36k2qpO9Ka46FzI7I8jm8ifcR34d4GS9C AL4g== 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=mMnCA7GeLEhPUyIUcFFUfNGWtBUU+Ce/1IP7LHlqVOI=; fh=2qYXGo5fpfJwEM8VGdvpKV9NMfKfWT+2pOyklyaTPCE=; b=lchzePV6tq+sbTFgbXKkN5R3oK9vKm68VyGPXNwIY9B27/OB8IUFZta3ZB7W4a1AWU byrJEn54rki429xigu6pDMVqa4n0j5dh0ElXO8l6UwMAw0ulzzIGh7M2PqIvHwYhfrd/ kNkdZ6CYGbD59PWfbLzWN3JzO3FoazMhjRj9fVFgYcNVcaMNkYcfZGpAiSg/t0O3VgzL HQ5bTYcbGDb8yAxKkMw6IUio9Ke5XEe9bkCxq49GdFDQhl4Hon5X4yNH87AE9WsBGJoq j2ov4MXA2cgut/NexBpp+j6bY0bx7DFhTlppX8VmcOxys4gPcAWK+kE2WMCSiyP6e6PF fGNg==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=qgSUMliR; arc=pass (i=1 dkim=pass dkdomain=kernel.org); spf=pass (google.com: domain of linux-kernel+bounces-169993-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-169993-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. [147.75.80.249]) by mx.google.com with ESMTPS id n7-20020aa7d047000000b00572ba2a782csi4753699edo.106.2024.05.06.07.30.36 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 May 2024 07:30:37 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel+bounces-169993-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=qgSUMliR; arc=pass (i=1 dkim=pass dkdomain=kernel.org); spf=pass (google.com: domain of linux-kernel+bounces-169993-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-169993-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 9996F1F21ED7 for ; Mon, 6 May 2024 14:30:36 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 58DF813D521; Mon, 6 May 2024 14:30:03 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="qgSUMliR" 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 5BA9D13D8BE; Mon, 6 May 2024 14:30:01 +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=1715005802; cv=none; b=Btp/O0Eowlnz7ZlzDIfp58C3aXUv5Kg3+q2R6bF8NAc6eFSWORmIOnqhBP5nv7Ff32vpbQV/TrISWhfbD4JQkUMr97dyUaCPhluBoXR8cXj68V3DdPTxYFs5N5DVsWUiFhT5AXQwnWv5cUeWo/CQ0S2yHWzb+ZzHiBsSmkj7who= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1715005802; c=relaxed/simple; bh=xLyyQsgBKxOS7ju/Z1jQLkPWqUuski82jXjG4sGoz8w=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=A6bYYqT74IESSui24NrkTIGh5iCj/6PGQgPSsHt3+8FAef+oMP2UHm1B85NeP1HOhoRFDnOYEF9R/9Q7SqW61vzjAFV/yxgkouYe0VVdn9UBNQoaoaZVAxZQnIlZsrDgKf2ff84Eic4WqaQQcMffIhDuJKPb+jzMMlp8cZMKwgU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=qgSUMliR; arc=none smtp.client-ip=10.30.226.201 Received: by smtp.kernel.org (Postfix) with ESMTPSA id 43365C4AF63; Mon, 6 May 2024 14:29:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1715005801; bh=xLyyQsgBKxOS7ju/Z1jQLkPWqUuski82jXjG4sGoz8w=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=qgSUMliRi/qdlo42bY09VW5dKPEjBw9ldcpgEjzTwevJIj1dfq6O6e1EAFPa+pDrY FLzUrvwfX3bfROez3MCZBCDoVObk4hzZIIa9HsNMrT8oXLPr5YCmUnox0M87In/P+b 0+dM+vPrwLdcsagU0+j71jO9yQJZ4H2xbmX6lv1AmuhtV38wZOWXuqT7+hPdajtaIq mLxkoxt2Eh58K6D0Hu7Krf8og2mK7jf7Ur1i7s+Kp2YjlQ98he9GVbRNvWamAubOYp IH9feVvYL0ZJQ320d01epzTNyiK6mDfO15IWg9oCKsv8Aom2/KqkYxez9K7/WDVxzz o9mvjAjlEMazg== Date: Mon, 6 May 2024 15:29:46 +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 10/10] iio: adc: ad7380: add support for resolution boost Message-ID: <20240506152946.74f1438c@jic23-huawei> In-Reply-To: <20240501-adding-new-ad738x-driver-v6-10-3c0741154728@baylibre.com> References: <20240501-adding-new-ad738x-driver-v6-0-3c0741154728@baylibre.com> <20240501-adding-new-ad738x-driver-v6-10-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:43 +0200 Julien Stephan wrote: > ad738x chips are able to use an additional 2 bits of resolution when > using oversampling. The 14-bits chips can have up to 16 bits of > resolution and the 16-bits chips can have up to 18 bits of resolution. > > In order to dynamically allow to enable/disable the resolution boost > feature, we have to set iio realbits/storagebits to the maximum per chips. > This means that for iio, data will always be on the higher resolution > available, and to cope with that we adjust the iio scale and iio offset > depending on the resolution boost status. > > The available scales can be displayed using the regular _scale_available > attributes and can be set by writing the regular _scale attribute. > The available scales depend on the oversampling status. > > Signed-off-by: Julien Stephan > > --- > > In order to support the resolution boost (additional 2 bits of resolution) > we need to set realbits/storagebits to the maximum value i.e : > - realbits = 16 + 2 = 18, and storagebits = 32 for 16-bits chips > - realbits = 14 + 2 = 16, and storagebits = 16 for 14-bits chips > > For 14-bits chips this does not have a major impact, but this > has the drawback of forcing 16-bits chip users to always use 32-bits > words in buffers even if they are not using oversampling and resolution > boost. Is there a better way of implementing this? For example > implementing dynamic scan_type? > > Another issue is the location of the timestamps. I understood the need > for ts to be consistent between chips, but right now I do not have a > better solution.. I was thinking of maybe adding the timestamps at the > beginning? That would imply to change the iio_chan_spec struct and maybe > add a iio_push_to_buffers_with_timestamp_first() wrapper function? Is > that an option? Questions discussed in another branch of the thread. Jonathan > > Any suggestion would be very much appreciated. > --- > drivers/iio/adc/ad7380.c | 226 ++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 194 insertions(+), 32 deletions(-) > > diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c > index 7b021bb9cf87..e240098708e9 100644 > --- a/drivers/iio/adc/ad7380.c > +++ b/drivers/iio/adc/ad7380.c > @@ -20,6 +20,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -58,6 +59,8 @@ > #define AD7380_CONFIG1_CRC_R BIT(4) > #define AD7380_CONFIG1_ALERTEN BIT(3) > #define AD7380_CONFIG1_RES BIT(2) > +#define RESOLUTION_BOOST_DISABLE 0 > +#define RESOLUTION_BOOST_ENABLE 1 If the field is defined, the values should be obvious. Also you used this as a boolean where simply passing in true or false would be less confusing. > #define AD7380_CONFIG1_REFSEL BIT(1) > #define AD7380_CONFIG1_PMODE BIT(0) > > @@ -86,6 +89,14 @@ struct ad7380_chip_info { > const struct ad7380_timing_specs *timing_specs; > }; > @@ -259,6 +271,8 @@ struct ad7380_state { > struct spi_device *spi; > unsigned int oversampling_mode; > unsigned int oversampling_ratio; > + unsigned int scales[2][2]; > + bool resolution_boost_enable; > struct regmap *regmap; > unsigned int vref_mv; > unsigned int vcm_mv[MAX_NUM_CHANNELS]; > @@ -270,7 +284,10 @@ struct ad7380_state { > * As MAX_NUM_CHANNELS is 4 the layout of the structure is the same for all parts > */ > struct { > - u16 raw[MAX_NUM_CHANNELS]; > + union { > + u16 u16[MAX_NUM_CHANNELS]; > + u32 u32[MAX_NUM_CHANNELS]; > + } raw; > > s64 ts __aligned(8); As per earlier comments, roll this timestamp into the union as well because it will move around. > } scan_data __aligned(IIO_DMA_MINALIGN); > @@ -359,23 +376,67 @@ static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg, > unreachable(); > } > > +static int ad7380_set_resolution_boost(struct iio_dev *indio_dev, bool enable) You pass 1 or 0 in here rather than true or false which would make more sense. > +{ > + struct ad7380_state *st = iio_priv(indio_dev); > + int ret; > + > + if (st->resolution_boost_enable == enable) > + return 0; > + > + ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1, > + AD7380_CONFIG1_RES, > + FIELD_PREP(AD7380_CONFIG1_RES, enable)); Mapping true / false to 1 / 0 whilst correct doesn't give particularly readable code. So useful to just have an enable ? 1 : 0 in there to make the mapping more obvious. > + > + if (ret) > + return ret; > + > + st->resolution_boost_enable = enable; Trivial: blank line here. > + return 0; > +} > > static int ad7380_init(struct ad7380_state *st, struct regulator *vref) > { > int ret; > @@ -691,12 +849,16 @@ static int ad7380_init(struct ad7380_state *st, struct regulator *vref) > if (ret < 0) > return ret; > > - /* Disable oversampling by default. > - * This is the default value after reset, > + /* Disable oversampling and resolution boost by default. Follow through from earlier. Wrong comment syntax + wrap lines nearer 80 chars. > + * This are the default values after reset, > * so just initialize internal data > */