From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (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 3AAFE274670; Sun, 6 Sep 2026 02:25:12 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1788661514; cv=none; b=r19GWSI1ffuGfPzFaEuls9OsJ3KAHKsBKvRWKL9rLM2T6dg0g5NLCC3L2CDUMWjdTJY98irW/w727wlyJr5G7Tc06zMHKAT5LeETLAyv+F6ZzaIdIFYAHcDVRjnV+Cl5shuAhTvMlwLSN4J6yoZH1SgTUcUFVaG2uL0OlI0fh1w= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1788661514; c=relaxed/simple; bh=XNFUTg+F6BlOgy01nFlTdDiQ8Svks7lggXl/P8UNOQU=; h=MIME-Version:Content-Type:Subject:From:To:Cc:In-Reply-To: References:Date:Message-Id; b=l0Bt6mK0fEUz/BQhCnjfZA58S8CYZ75XtDjnl9fPPPKAyyAgWmJ14WQtq0AOBRB8l0VZPsSOm8uxofKeYiuHf5kNm0k32VePLOJvc6oBptOekceOFik79hKQlVK8IQ8pHl2Kbloo6gILNDl/Jxzjl84+CmhY8GwDIb5VlMeX2kc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=P6xNwBA/; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="P6xNwBA/" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 742921F00A3A; Sun, 6 Sep 2026 02:25:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1788661512; bh=8rw9FA+u/7iuctnN0TV1UFU1IECUzyEEaBgVbPgV7TE=; h=Subject:From:To:Cc:In-Reply-To:References:Date; b=P6xNwBA/5MzpXYEm8cbywGMKDhVByCFzGwmx9IkLjeaVSGeU/fxtXvsXkI+EFHGlh mGlNbeNpLG57kInd/NISCWakCMlVpNoi+MAgy7zuxkqwR2eGSv+pwLNv4VBEG5oz2n lRE0arFwyQzhEE3lmoqWsY16QsBm/XvVeKUcPigSLQBQrsmDYP8zVQQqrpAvG9cVKW 1hN9de8pVtjlGshh+sYDQKFRQu4CIhEyBFwNalmZQe4+9SMhXylDi1G3lVL35hYqYU l5PhKpHqmvCTSsQIq/IryTyUN5Z3Z/v+DvOp4PU1BzKjp6sdV/rrPn8bJVXsiXuVda Q3OWJkRcx1YVQ== Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Subject: Re: [PATCH v6 05/17] iio: adc: Add AD7768 and AD7768-4 core support From: Jonathan Cameron To: sashiko-reviews@lists.linux.dev Cc: Janani Sunil , conor+dt@kernel.org, robh@kernel.org, devicetree@vger.kernel.org In-Reply-To: <20260904143153.9AF1A1F00A3E@smtp.kernel.org> References: <20260904-ad7768-driver-v6-0-e4378f946bfb@analog.com> <20260904-ad7768-driver-v6-5-e4378f946bfb@analog.com> <20260904143153.9AF1A1F00A3E@smtp.kernel.org> Date: Sun, 06 Sep 2026 03:24:54 +0100 Message-Id: <178866149408.3402141.12120124355772738970.b4-reply@b4> X-Mailer: b4 0.16.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=9689; i=jonathan.cameron@oss.qualcomm.com; h=from:subject:message-id; bh=XNFUTg+F6BlOgy01nFlTdDiQ8Svks7lggXl/P8UNOQU=; b=owEBfwKA/ZANAwAKAVSFNJnE9BaIAcsmYgBqnM73ZJx87z1GrIZ8tZQ0x58u7rOdyz5erdcIP MvkG1UdE4GJAkUEAAEKAC8WIQRuKWazh4QGUpEmgbFUhTSZxPQWiAUCapzO9xEcamljMjNAa2Vy bmVsLm9yZwAKCRBUhTSZxPQWiGCRD/41v2nDbncpKcMmsGB7orqKAugrDe28tJmBB1EamA7eQsT 8sSGpMXR4W7FuqY7dkmd5pbjBAxFnNLTt2GQx2n6OXp5KA93iv+NdTke/woPlAthuEtsPVTxe0t W2YgF7ekymU0XAWb1lEy1uyCsIov0/P2t3IU0N0HtrGGQ7LYBUjsQGl2xket0owcnnQgHYi5WWK XwlMwL4gJtywhVP01q/A7wxGDS0OLVbq+4Z9PJYBFS34brUwAuWQjuDmfBYv9exmm8uQZDFVNST vVBiGlShwjK6NUUDdURPJ/Cq4d/36nWLCi7IJQDYdiTAuKk7vuR0ljUQj3j/TzNstpuhIEn3SHE 4/jsEY4MNV6PfHt3yV65Nf8DZZtO5cfBwtIlUk7hVB7+Ni8tPy5gvTtPqJWFwZhk5obBJ/XsObg jFp2Htx+QPD/FBmmUxbh6luVKoNWvlaU+gHK0UtuWKEz4h4skenGK/W8h7bz9K/+UYtZ9ydfRlS y7r1/H6+z3Aw8Neb3Q9LMyYMxKOotlKxDOJb4T/e0rDDI95Ilc60XvBxRTuG+MMVVN5dPPT5Ia2 YTGdsanhrjJj7MBO8YfpPh/ktnJeVjRsfNwcKonTWrtOnGZidPi7QJc8Q0j+IBSkwclcNsyWbEC hYjpSs0sbp8x90zG0lkEJ6f4+VmPDWfVdXURiKQ== X-Developer-Key: i=jonathan.cameron@oss.qualcomm.com; a=openpgp; fpr=6E2966B387840652912681B154853499C4F41688 > > commit cad3d6d13491f167da3c31d541a31ac850d8d3b5 > Author: Janani Sunil > > iio: adc: Add AD7768 and AD7768-4 core support > > Add core support for the AD7768 and AD7768-4 simultaneous sampling ADCs. > Configure supplies, clock and reset, use a custom regmap bus for the SPI > protocol, and parse the enabled channels and input buffer settings from > devicetree. There was quite a bit of sashiko feedback on this one. I took a look as getting close to merging the series. Please check my logic. > > > diff --git a/drivers/iio/adc/ad7768.c b/drivers/iio/adc/ad7768.c > > new file mode 100644 > > index 0000000000000..c80056a9edd8c > > --- /dev/null > > +++ b/drivers/iio/adc/ad7768.c > > @@ -0,0 +1,1000 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > [ ... ] > > > +#define AD7768_REG_PRECHARGE_BUF1 0x11 > > +#define AD7768_REG_PRECHARGE_BUF2 0x12 > > +#define AD7768_PREBUF_POS_EN(ch) BIT((ch) * 2) > > +#define AD7768_PREBUF_NEG_EN(ch) BIT(((ch) * 2) + 1) > > [Severity: Medium] > Are these bit assignments for the precharge buffers swapped? According to the > AD7768 datasheet, the positive (POS) precharge buffers are assigned to odd bits, > and the negative (NEG) precharge buffers are assigned to even bits. No they aren't. I think sashiko didn't realize table starts at bit 7 and works down to bit 0. > > > + > > +#define AD7768_REG_REFP_BUF 0x13 > > +#define AD7768_REG_REFN_BUF 0x14 > > [ ... ] > > > +static u8 ad7768_all_standby_mask(const struct ad7768_state *st) > > +{ > > + return GENMASK(st->chip_info->num_channels - 1, 0); > > +} > > [Severity: High] > Does this generate the correct physical mask for the AD7768-4? The AD7768-4 > maps its 4 logical channels to physical channels 0, 1, 4, and 5. This returns > GENMASK(3, 0) which is 0x0F, writing 0 to physical bits 4, 5, 6, and 7, > leaving them fully active. > > [ ... ] This one looks plausible. There is handling of the bits written to deal with the mapping but not the mask I think. Please take a look. > > > +static int ad7768_regmap_read(void *context, const void *reg_buf, > > + size_t reg_size, void *val_buf, size_t val_size) > > +{ > > + struct ad7768_state *st = spi_get_drvdata(context); > > + struct spi_device *spi = context; > > + struct spi_transfer t[] = { > > + { > > + .tx_buf = &st->d16, > > + .len = sizeof(st->d16), > > + .cs_change = 1, > > + }, { > > + /* > > + * Register responses are delayed by one CS frame. While > > + * receiving the response to this read, the device also > > + * decodes another command on SDI. Repeat the read > > + * command to avoid sending an unspecified dummy > > + * command. > > + */ > > + .tx_buf = &st->d16, > > + .rx_buf = &st->d16, > > + .len = sizeof(st->d16), > > + }, > > + }; > > [Severity: High] > Can this violate the DMA API? Both transfers use the same memory address > (&st->d16) for their buffers. When the SPI subsystem maps all transfers for > DMA concurrently, attempting to map the same memory region twice concurrently > (DMA_TO_DEVICE and DMA_BIDIRECTIONAL) can cause cache coherency issues on > non-coherent architectures. So this one I'm fairly sure on, but feel free to check. There is nothing stopping SPI using one buffer for both directions. If it's a problem for a particular setup I think they have to bounce it. > > [ ... ] > > > +static int ad7768_update_scan_mode(struct iio_dev *indio_dev, > > + const unsigned long *scan_mask) > > +{ > > + struct ad7768_state *st = iio_priv(indio_dev); > > + unsigned long channel_mask; > > + unsigned long standby_mask; > > + int ret; > > + > > + channel_mask = ad7768_all_standby_mask(st); > > + standby_mask = channel_mask & ~*scan_mask; > > + > > + /* > > + * Crystal excitation requires channel 4 on AD7768 or channel 2 on > > + * AD7768-4 to remain active. > > + */ > > + if (st->clock_source == AD7768_CLOCK_SOURCE_XTAL) > > + __clear_bit(st->chip_info->num_channels / 2, &standby_mask); > > + > > + ret = regmap_update_bits(st->regmap, AD7768_REG_CH_STANDBY, > > + channel_mask, standby_mask); > > + if (ret) > > + return ret; > > + > > + for (unsigned int c = 0; c < st->chip_info->num_channels; c++) { > > + if (test_bit(c, scan_mask)) > > + ret = iio_backend_chan_enable(st->back, c); > > + else > > + ret = iio_backend_chan_disable(st->back, c); > > + if (ret) > > + return ret; > > + } > > [Severity: Medium] > Will this leave the backend hardware out of sync if an error occurs? If one > operation fails, it returns the error immediately without unwinding the > previously modified channels. This is one of those error cases where unwinding may just be impractical. Real question is whether there is anything that means trying again won't end up in a sane state or that anything particularly bad will happen as a result of the bad state in the meantime. I think this is fine but please check. > > > + > > + return 0; > > +} > > [ ... ] > > > +static int ad7768_buffer_postdisable(struct iio_dev *indio_dev) > > +{ > > + struct ad7768_state *st = iio_priv(indio_dev); > > + > > + pm_runtime_put_autosuspend(regmap_get_device(st->regmap)); > > + return 0; > > +} > > [Severity: Medium] > Could this leave the hardware powered on after device removal? > > If the driver is unbound while the IIO buffer is active, > devm_iio_device_register unwinds first, calling iio_device_unregister(), > which disables the buffer and triggers ad7768_buffer_postdisable(). > > Immediately after, devm_pm_runtime_set_active_enabled unwinds, calling > pm_runtime_disable() which cancels the pending autosuspend work. The > driver should ensure synchronous suspend if the buffer is active during > removal. Ah. Interesting corner case. There are probably lots of other paths to the power happening to be on at shut down but I can see maybe we ultimately want a flag to say we are going down in that buffer disable path. Anyhow, you should be careful to ensure it doesn't matter what state we are in wrt to runtime pm, after we've turned that off we should make sure we turn off the device by hand. Afterall maybe runtime pm was never enabled in the first place (or even built!) > > [ ... ] > > > +static int ad7768_configure_precharge_buffers(struct iio_dev *indio_dev, > > + struct ad7768_precharge_config *precharge_cfg) > > +{ > > + struct ad7768_state *st = iio_priv(indio_dev); > > + u8 prebuf1_val, prebuf2_val; > > + u16 prebuf_mask = 0; > > + u8 refbufp_val = 0; > > + u8 refbufn_val = 0; > > + int ret; > > + > > + for (unsigned int ch = 0; ch < indio_dev->num_channels; ch++) { > > + u8 channel = indio_dev->channels[ch].channel; > > + > > + if (precharge_cfg[channel].prebufp_en) > > + prebuf_mask |= AD7768_PREBUF_POS_EN(channel); > > + > > + if (precharge_cfg[channel].prebufn_en) > > + prebuf_mask |= AD7768_PREBUF_NEG_EN(channel); > > + > > + if (precharge_cfg[channel].refbufp) > > + refbufp_val |= ad7768_channel_mask(st, channel); > > + > > + if (precharge_cfg[channel].refbufn) > > + refbufn_val |= ad7768_channel_mask(st, channel); > > + } > > [Severity: Medium] > Does this apply the incorrect polarity for the reference buffer configuration? > When adi,refbuf-pos-enable or adi,refbuf-neg-enable are provided in the device > tree, the driver sets the corresponding bit to 1. According to the AD7768 > datasheet, setting a bit to 1 in the REFP_BUF and REFN_BUF registers powers down > the buffer. I'm not sure what datasheet sashiko has. Mine says 1 is power up. I'm going to guess it's assuming layout of values that aren't true. > > [ ... ] > > > + device_for_each_named_child_node_scoped(dev, child, "channel") { > > + u32 channel; > > + > > + ret = fwnode_property_read_u32(child, "reg", &channel); > > + if (ret) > > + return dev_err_probe(dev, ret, > > + "Failed to parse reg of %pfwP\n", > > + child); > > + > > + if (channel >= st->chip_info->num_channels) > > + return dev_err_probe(dev, -ECHRNG, > > + "Invalid channel %u in firmware\n", > > + channel); > > + > > + ret = regmap_clear_bits(st->regmap, AD7768_REG_CH_STANDBY, > > + BIT(channel)); > > [Severity: High] > Does this use logical indices instead of physical bits for the AD7768-4? The > AD7768-4 uses physical channels 0, 1, 4, and 5. Using BIT(channel) will update > physical bits 0-3 instead of the mapped hardware bits. This seems plausible. Please check. > > > + if (ret) > > + return ret; > > + > > [ ... ] > > > + pm_runtime_set_autosuspend_delay(dev, 2000); > > + pm_runtime_use_autosuspend(dev); > > + ret = devm_pm_runtime_set_active_enabled(dev); > > + if (ret) > > + return ret; > > + > > + return devm_iio_device_register(dev, indio_dev); > > +} > > [Severity: Medium] > Does this leave the device permanently active? This configures autosuspend and > enables runtime PM, which transitions the device to RPM_ACTIVE with a usage > count of 0, but fails to call pm_request_idle(dev). As a result, the autosuspend > timer is never triggered, leaving the device fully powered indefinitely until > a user explicitly accesses it. Nope. This is usual thing of sashiko failing to notice the stuff in base/dd.c that always checks if the reference count is 0 and hence it can put it to sleep. This happens just after probe. So a definite mixed bag for sashiko on this one. Jonathan