From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ot1-f44.google.com (mail-ot1-f44.google.com [209.85.210.44]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A0541262FFC for ; Mon, 6 Jul 2026 16:31:02 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.44 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783355466; cv=none; b=Wr3+YJs8AXHYIdAeFCzMotNs7Eu5W/dOMgKRP4XDCZGhjoFyl12719tiBpFmba5ZMgUD1CjV+VKTJ9tzkz8LuG2yvB+dFs0SOIMZKctoScG5aLhw9V+HLRldonZUn1BRPRpAaAcVk7GUa1FeL9I/fni5vhV/hLuc0ypD6kUjdNc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783355466; c=relaxed/simple; bh=I8QYJk3LG6/dJj76RH/oHg/uN7TFDxpmKPIrctEJj+A=; h=Message-ID:Date:MIME-Version:Subject:To:References:From: In-Reply-To:Content-Type; b=mXcjD4EODKwf5+Hd+Rur9JifXcS9H0V4LP9vLoN2P44cUhPLrkvZXHXVLYLTL5EpnsouWt1L72x6bDb3ZSZcT3vcLpz6LpL/zqOnN6vbmXpR4dgjAToc2jgupXjWhFVSdaa209QSgrPrtbnsDtuB9Kt/xVgs3H7R0sircUCQQ9U= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=baylibre.com; spf=pass smtp.mailfrom=baylibre.com; dkim=pass (2048-bit key) header.d=baylibre.com header.i=@baylibre.com header.b=ZMqCTUbM; arc=none smtp.client-ip=209.85.210.44 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=baylibre.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=baylibre.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=baylibre.com header.i=@baylibre.com header.b="ZMqCTUbM" Received: by mail-ot1-f44.google.com with SMTP id 46e09a7af769-7eb787dec99so2059012a34.0 for ; Mon, 06 Jul 2026 09:31:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre.com; s=google; t=1783355461; x=1783960261; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:user-agent:mime-version:date:message-id:from :to:cc:subject:date:message-id:reply-to; bh=W2JVlXDnAv223HzjjuMC/qjZn6HcPUW70uXnl4N1Zws=; b=ZMqCTUbMOaV9/yNUminb8pAAYomP4PUQH080VGIXy2jAGJsZds8v4yIUGT7thmbvRZ H3kYlLsmN6/k4AkkYAN0SIj3085//Cjd8Yk/iPAkwPRE6PgWCLZk16qaxvfM6vBHZyrm zjXl9Nga1F2N1MKg51WOs2gYawpZKGLG7xCvGoMhQy+lAV6PRAay2KN4x8p2rcz2EzaM 7AxbV3eesuCds5goDz+uoeVuw1c2vXIpRMoU6fc3tcHe67kzHvOiGuUwe6sJxvX0+ACf /kasvDgIQsIRwExXMLuKfU2UIEqJx2SW95+SJ6VqEh2lqzRUpj3j9v+wCT0WxdThxfWh os/g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1783355461; x=1783960261; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=W2JVlXDnAv223HzjjuMC/qjZn6HcPUW70uXnl4N1Zws=; b=rS5Y9ap4gBywH9eL5GTdutds6Em0v/ReLkZB8pZ8wrOX3QXRecdI0HOJm+s0cd62xR FZtb8xWe3a6raAkFT6K0qRPx645hjCaQsQBTW4XL3eETgc7yHXvgrJQ2RPEzxVk3kL2q q3IMp2BXoD8qR3qxA29fjXZi2mGvhRkJg/MIEiphCmLS7lQlzRku23VK5W5JCvlYaCF+ AtjGRDvmeKPjCSFJPf6NNDOxeSKED2HlEipARMv2FX8pZmIxBNPj/5QX7iJzEMKCqGAQ Epe2rmHBTrTLk5LY1ACF4RbfLt4yzBqUzWnyz/THBn/QmYco1JeSH4mdSzNVg5DvOrPI gbaA== X-Forwarded-Encrypted: i=1; AFNElJ/49i1lbdpD/jUJ60F1cRbibfpFfVWH075QrrIgMt9bxvCyZKGiyKFgzpvYRyoLi0n9ZIuKjoJeVGTg54I=@vger.kernel.org X-Gm-Message-State: AOJu0YxUvTlKTZTAkKxZA7P5X54EEnTbh4J+rANPuCUe4XzO0alpkbFj k2Mo/T+t181qpVu9WaMgGOSqTD9Z2fVlGL8iqgCmu+u4rzA6bZZBSPJ4lnJJAbdwjnQ= X-Gm-Gg: AfdE7cky/Gck1bP4grg3erontTFYx7UeTIQ9IXilnBGHL+ZuwLAyaAfcjgD0sd5Sft8 pw+0gEoIwvsX1GMgi/pL1FMkPteWni7TbmVvfIjpFJfS50XQiqb0bUYwuGjAlyIYt7iHdP3ZL5J 0iU0GVMYBVd7mVGKlnAU64UTPg2vkYG5WJXid8cORE1a+88MjNQkUfA/stBzuB2hVkQ4UaRAGzC 7pVJL5LvD/k3RK1PvVEZW/dzTHTqz9WjOe9GgRMNRlt+9qz3fab3EOeg+ReyoAszlveVShlAwLW qZPEU6I1wfG0j2neYpDdGZ/OuQg29wlEN3/2CNkchNH1M3DPL7VThZVzx3wnbcnDgKj9XhHlz4M XbLjNoocamLKNi3KsjXojZMkWItBnLHyXqxFA9KgHYRF7QPO3mViYhVaRQlYg7wzqnY0KcPXhVt Xcfmy2XeQINeYUwOhBepAdENfPQNlSUCyoNTqU6TV2k2OtnO6fXq8buSuuAN9Ddic= X-Received: by 2002:a05:6830:8284:b0:7e6:efb8:fe66 with SMTP id 46e09a7af769-7ebb2240ca0mr792160a34.12.1783355461431; Mon, 06 Jul 2026 09:31:01 -0700 (PDT) Received: from ?IPV6:2600:8803:e7e4:500:a38a:a0af:ed1d:5c77? ([2600:8803:e7e4:500:a38a:a0af:ed1d:5c77]) by smtp.gmail.com with ESMTPSA id 46e09a7af769-7eb544a2706sm11448740a34.17.2026.07.06.09.30.59 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 06 Jul 2026 09:30:59 -0700 (PDT) Message-ID: <867d2cb8-a1f3-49e9-a8a2-04e232f404ab@baylibre.com> Date: Mon, 6 Jul 2026 11:30:59 -0500 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4] iio: ti-ads7138: Disable STATS_EN bit while reading conversion results To: Paul Geurts , jic23@kernel.org, nuno.sa@analog.com, andy@kernel.org, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, tobias.sperling@softing.com References: <20260706074803.13624-1-paul.geurts@prodrive-technologies.com> Content-Language: en-US From: David Lechner In-Reply-To: <20260706074803.13624-1-paul.geurts@prodrive-technologies.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 7/6/26 2:48 AM, Paul Geurts wrote: > There is a data race in reading the STATS registers, resulting in wrong > data being read. When the data in the RECENT register switches between > 0x24F0 and 0x2500, occasionally value 0x2400 or 0x25F0 is read. This > happens when the value is updated inbetween reading MSB and LSB. > > The datasheet says: "Until a new conversion result is available, > previous values can be read from the statistics registers. Before > reading the statistics registers, set STATS_EN to 0 to prevent any > updates to this register block." As the STATS_EN is currently not > cleared, the values of the stats registers might change mid read, > giving faulty values. > > Disable the STATS_EN bit before reading one of the stistics registers to s/stistics/statistics/ > make sure the device does not update the register mid read. This is > applicable to registers MAX_CHn_xSB, MIN_CHn_xSB and RECENT_CHn_xSB. > > This means reading one of the statistics registers resets the MAX and > MIN registers. This is unfortunate, but necessary to get correct data > from the device. > > Signed-off-by: Paul Geurts > Fixes: 93a39542d3c3 ("iio: adc: Add driver for ADS7128 / ADS7138") > --- > > V1 -> V2: Checked return values and prefixed iio: in commit msg > V2 -> V3: > - Clarified commit msg > - Disable STATS_EN for MIN and MAX too > V3 -> V4: > - Clarified commit msg more > - Reduced code duplication by creating a statistics read wrapper > around read_block > > v1: https://lore.kernel.org/all/20260619075646.4100193-1-paul.geurts@prodrive-technologies.com/ > v2: https://lore.kernel.org/all/20260619090004.355053-1-paul.geurts@prodrive-technologies.com/ > v3: https://lore.kernel.org/all/20260624080131.3669357-1-paul.geurts@prodrive-technologies.com/ > --- > drivers/iio/adc/ti-ads7138.c | 40 ++++++++++++++++++++++++++++-------- > 1 file changed, 31 insertions(+), 9 deletions(-) > > diff --git a/drivers/iio/adc/ti-ads7138.c b/drivers/iio/adc/ti-ads7138.c > index af87f5f19a0f..14096bf3373a 100644 > --- a/drivers/iio/adc/ti-ads7138.c > +++ b/drivers/iio/adc/ti-ads7138.c > @@ -227,6 +227,25 @@ static int ads7138_osr_to_bits(int osr) > return -EINVAL; > } > > +static int ads7138_read_statistics(const struct i2c_client *client, u8 reg, > + u8 *out_values, u8 length) > +{ > + int ret; > + > + /* Disable statistics update so the value is not updated mid read */ > + ret = ads7138_i2c_clear_bit(client, ADS7138_REG_GENERAL_CFG, > + ADS7138_GENERAL_CFG_STATS_EN); > + if (ret) > + return ret; Nice to have a blank line here. > + ret = ads7138_i2c_read_block(client, reg, out_values, length); > + if (ret) > + return ret; And blank line here too. And should probably restore ADS7138_GENERAL_CFG_STATS_EN on error here, but if I2C read doesn't work, we probably have bigger problems, so maybe OK to keep it simple. I don't remember if this came up in previous discussions. > + /* Enable statistics update after read */ > + ret = ads7138_i2c_set_bit(client, ADS7138_REG_GENERAL_CFG, > + ADS7138_GENERAL_CFG_STATS_EN); Would be simpler to just return directly. > + return ret; > +} > + > static int ads7138_read_raw(struct iio_dev *indio_dev, > struct iio_chan_spec const *chan, int *val, > int *val2, long mask) > @@ -236,28 +255,31 @@ static int ads7138_read_raw(struct iio_dev *indio_dev, > u8 values[2]; > > switch (mask) { IIO style is to have /* on separate line for multi-line comments. > + /* Reading the statistics registers reinitializes them. This is unfortunate > + * but necessary to prevent data races > + */ > case IIO_CHAN_INFO_RAW: > - ret = ads7138_i2c_read_block(data->client, > - ADS7138_REG_RECENT_LSB_CH(chan->channel), > - values, ARRAY_SIZE(values)); > + ret = ads7138_read_statistics(data->client, > + ADS7138_REG_RECENT_LSB_CH(chan->channel), > + values, ARRAY_SIZE(values)); > if (ret) > return ret; > > *val = get_unaligned_le16(values); > return IIO_VAL_INT; > case IIO_CHAN_INFO_PEAK: > - ret = ads7138_i2c_read_block(data->client, > - ADS7138_REG_MAX_LSB_CH(chan->channel), > - values, ARRAY_SIZE(values)); > + ret = ads7138_read_statistics(data->client, > + ADS7138_REG_MAX_LSB_CH(chan->channel), > + values, ARRAY_SIZE(values)); > if (ret) > return ret; > > *val = get_unaligned_le16(values); > return IIO_VAL_INT; > case IIO_CHAN_INFO_TROUGH: > - ret = ads7138_i2c_read_block(data->client, > - ADS7138_REG_MIN_LSB_CH(chan->channel), > - values, ARRAY_SIZE(values)); > + ret = ads7138_read_statistics(data->client, > + ADS7138_REG_MIN_LSB_CH(chan->channel), > + values, ARRAY_SIZE(values)); > if (ret) > return ret; > Reviewed-by: David Lechner If you are lucky, Jonathan might pick this up and tweak it, so wait a bit before more feedback before sending a v5.