From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.5 required=3.0 tests=BAYES_00,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E269FC4320A for ; Mon, 30 Aug 2021 16:50:58 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id C745660F92 for ; Mon, 30 Aug 2021 16:50:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237912AbhH3Qvu (ORCPT ); Mon, 30 Aug 2021 12:51:50 -0400 Received: from mail.kernel.org ([198.145.29.99]:35318 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237939AbhH3Qvq (ORCPT ); Mon, 30 Aug 2021 12:51:46 -0400 Received: from jic23-huawei (cpc108967-cmbg20-2-0-cust86.5-4.cable.virginm.net [81.101.6.87]) (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 802C860F5E; Mon, 30 Aug 2021 16:50:49 +0000 (UTC) Date: Mon, 30 Aug 2021 17:54:00 +0100 From: Jonathan Cameron To: Mihail Chindris Cc: , , , , , , Subject: Re: [PATCH v4 6/6] drivers:iio:dac: Add AD3552R driver support Message-ID: <20210830175349.2db56ecd@jic23-huawei> In-Reply-To: <20210820165927.4524-7-mihail.chindris@analog.com> References: <20210820165927.4524-1-mihail.chindris@analog.com> <20210820165927.4524-7-mihail.chindris@analog.com> X-Mailer: Claws Mail 4.0.0 (GTK+ 3.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 20 Aug 2021 16:59:27 +0000 Mihail Chindris wrote: > The AD3552R-16 is a low drift ultrafast, 16-bit accuracy, > current output digital-to-analog converter (DAC) designed > to generate multiple output voltage span ranges. > The AD3552R-16 operates with a fixed 2.5V reference. The following is a fairly quick review. Lots here so I'll take a closer look at a later version. Only so much stamina, though I'm almost caught up with what came in whilst I was on vacation! > > analog.com/media/en/technical-documentation/data-sheets/ad3552r.pdf We have a datasheet tag these days. > > Signed-off-by: Mihail Chindris > --- > drivers/iio/dac/Kconfig | 10 + > drivers/iio/dac/Makefile | 1 + > drivers/iio/dac/ad3552r.c | 1419 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 1430 insertions(+) > create mode 100644 drivers/iio/dac/ad3552r.c > > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig > index 75e1f2b48638..ced6428f2c92 100644 > --- a/drivers/iio/dac/Kconfig > +++ b/drivers/iio/dac/Kconfig > @@ -6,6 +6,16 @@ > > menu "Digital to analog converters" > > +config AD3552R > + tristate "Analog Devices AD3552R DAC driver" > + depends on SPI_MASTER > + help > + Say yes here to build support for Analog Devices AD3552R > + Digital to Analog Converter. > + > + To compile this driver as a module, choose M here: the > + module will be called ad3552r. > + > config AD5064 > tristate "Analog Devices AD5064 and similar multi-channel DAC driver" > depends on (SPI_MASTER && I2C!=m) || I2C > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile > index 33e16f14902a..dffe36efd8ff 100644 > --- a/drivers/iio/dac/Makefile > +++ b/drivers/iio/dac/Makefile > @@ -4,6 +4,7 @@ > # > > # When adding new entries keep the list in alphabetical order > +obj-$(CONFIG_AD3552R) += ad3552r.o > obj-$(CONFIG_AD5360) += ad5360.o > obj-$(CONFIG_AD5380) += ad5380.o > obj-$(CONFIG_AD5421) += ad5421.o > diff --git a/drivers/iio/dac/ad3552r.c b/drivers/iio/dac/ad3552r.c > new file mode 100644 > index 000000000000..89993dd87522 > --- /dev/null > +++ b/drivers/iio/dac/ad3552r.c > @@ -0,0 +1,1419 @@ > +// SPDX-License-Identifier: GPL-2.0-only > + No blank line here. > +/* > + * Analog Devices AD3552R > + * Digital to Analog converter driver > + * > + * Copyright 2021 Analog Devices Inc. > + */ > +#include Alphabetical order. If you prefer you can pull the subsystem headers out afterwards in their own block. > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* Register addresses */ > +/* Primary address space */ > +#define AD3552R_REG_ADDR_INTERFACE_CONFIG_A 0x00 > +#define AD3552R_REG_ADDR_INTERFACE_CONFIG_B 0x01 > +#define AD3552R_REG_ADDR_DEVICE_CONFIG 0x02 > +#define AD3552R_REG_ADDR_CHIP_TYPE 0x03 > +#define AD3552R_REG_ADDR_PRODUCT_ID_L 0x04 > +#define AD3552R_REG_ADDR_PRODUCT_ID_H 0x05 > +#define AD3552R_REG_ADDR_CHIP_GRADE 0x06 > +#define AD3552R_REG_ADDR_SCRATCH_PAD 0x0A > +#define AD3552R_REG_ADDR_SPI_REVISION 0x0B > +#define AD3552R_REG_ADDR_VENDOR_L 0x0C > +#define AD3552R_REG_ADDR_VENDOR_H 0x0D > +#define AD3552R_REG_ADDR_STREAM_MODE 0x0E > +#define AD3552R_REG_ADDR_TRANSFER_REGISTER 0x0F > +#define AD3552R_REG_ADDR_INTERFACE_CONFIG_C 0x10 > +#define AD3552R_REG_ADDR_INTERFACE_STATUS_A 0x11 > +#define AD3552R_REG_ADDR_INTERFACE_CONFIG_D 0x14 > +#define AD3552R_REG_ADDR_SH_REFERENCE_CONFIG 0x15 > +#define AD3552R_REG_ADDR_ERR_ALARM_MASK 0x16 > +#define AD3552R_REG_ADDR_ERR_STATUS 0x17 > +#define AD3552R_REG_ADDR_POWERDOWN_CONFIG 0x18 > +#define AD3552R_REG_ADDR_CH0_CH1_OUTPUT_RANGE 0x19 > +#define AD3552R_REG_ADDR_CH_OFFSET(ch) (0x1B + (ch) * 2) > +#define AD3552R_REG_ADDR_CH_GAIN(ch) (0x1C + (ch) * 2) > +/* > + * Secondary region > + * For multibyte registers specify the highest address because the access is > + * done in descending order > + */ > +#define AD3552R_SECONDARY_REGION_START 0x28 > +#define AD3552R_REG_ADDR_HW_LDAC_16B 0x28 > +#define AD3552R_REG_ADDR_CH_DAC_16B(ch) (0x2C - (1 - ch) * 2) > +#define AD3552R_REG_ADDR_DAC_PAGE_MASK_16B 0x2E > +#define AD3552R_REG_ADDR_CH_SELECT_16B 0x2F > +#define AD3552R_REG_ADDR_INPUT_PAGE_MASK_16B 0x31 > +#define AD3552R_REG_ADDR_SW_LDAC_16B 0x32 > +#define AD3552R_REG_ADDR_CH_INPUT_16B(ch) (0x36 - (1 - ch) * 2) > +/* 3 bytes registers */ > +#define AD3552R_REG_START_24B 0x37 > +#define AD3552R_REG_ADDR_HW_LDAC_24B 0x37 > +#define AD3552R_REG_ADDR_CH_DAC_24B(ch) (0x3D - (1 - ch) * 3) > +#define AD3552R_REG_ADDR_DAC_PAGE_MASK_24B 0x40 > +#define AD3552R_REG_ADDR_CH_SELECT_24B 0x41 > +#define AD3552R_REG_ADDR_INPUT_PAGE_MASK_24B 0x44 > +#define AD3552R_REG_ADDR_SW_LDAC_24B 0x45 > +#define AD3552R_REG_ADDR_CH_INPUT_24B(ch) (0x4B - (1 - ch) * 3) > + > +#define AD3552R_REG_ADDR_MAX 0x4B > + > +/* AD3552R_REG_ADDR_INTERFACE_CONFIG_A */ > +#define AD3552R_MASK_SOFTWARE_RESET (BIT(7) | BIT(0)) > +#define AD3552R_MASK_ADDR_ASCENSION BIT(5) > +#define AD3552R_MASK_SDO_ACTIVE BIT(4) > +/* AD3552R_REG_ADDR_INTERFACE_CONFIG_B */ > +#define AD3552R_MASK_SINGLE_INST BIT(7) > +#define AD3552R_MASK_SHORT_INSTRUCTION BIT(3) > +/* AD3552R_REG_ADDR_DEVICE_CONFIG */ > +#define AD3552R_MASK_DEVICE_STATUS(n) BIT(4 + (n)) > +#define AD3552R_MASK_CUSTOM_MODES (BIT(3) | BIT(2)) > +#define AD3552R_MASK_OPERATING_MODES (BIT(1) | BIT(0)) > +/* AD3552R_REG_ADDR_CHIP_TYPE */ > +#define AD3552R_MASK_CLASS 0x0F > +/* AD3552R_REG_ADDR_CHIP_GRADE */ > +#define AD3552R_MASK_GRADE 0xF0 > +#define AD3552R_MASK_DEVICE_REVISION 0x0F > +/* AD3552R_REG_ADDR_STREAM_MODE */ > +#define AD3552R_MASK_LENGTH 0x0F > +/* AD3552R_REG_ADDR_TRANSFER_REGISTER */ > +#define AD3552R_MASK_MULTI_IO_MODE (BIT(7) | BIT(6)) > +#define AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE BIT(2) > +/* AD3552R_REG_ADDR_INTERFACE_CONFIG_C */ > +#define AD3552R_MASK_CRC_ENABLE (BIT(7) | BIT(6) |\ > + BIT(1) | BIT(0)) > +#define AD3552R_MASK_STRICT_REGISTER_ACCESS BIT(5) > +/* AD3552R_REG_ADDR_INTERFACE_STATUS_A */ > +#define AD3552R_MASK_INTERFACE_NOT_READY BIT(7) > +#define AD3552R_MASK_CLOCK_COUNTING_ERROR BIT(5) > +#define AD3552R_MASK_INVALID_OR_NO_CRC BIT(3) > +#define AD3552R_MASK_WRITE_TO_READ_ONLY_REGISTER BIT(2) > +#define AD3552R_MASK_PARTIAL_REGISTER_ACCESS BIT(1) > +#define AD3552R_MASK_REGISTER_ADDRESS_INVALID BIT(0) > +/* AD3552R_REG_ADDR_INTERFACE_CONFIG_D */ > +#define AD3552R_MASK_ALERT_ENABLE_PULLUP BIT(6) > +#define AD3552R_MASK_MEM_CRC_EN BIT(4) > +#define AD3552R_MASK_SDO_DRIVE_STRENGTH (BIT(3) | BIT(2)) > +#define AD3552R_MASK_DUAL_SPI_SYNCHROUNOUS_EN BIT(1) > +#define AD3552R_MASK_SPI_CONFIG_DDR BIT(0) > +/* AD3552R_REG_ADDR_SH_REFERENCE_CONFIG */ > +#define AD3552R_MASK_IDUMP_FAST_MODE BIT(6) > +#define AD3552R_MASK_SAMPLE_HOLD_DIFFERENTIAL_USER_EN BIT(5) > +#define AD3552R_MASK_SAMPLE_HOLD_USER_TRIM (BIT(4) | BIT(3)) > +#define AD3552R_MASK_SAMPLE_HOLD_USER_ENABLE BIT(2) > +#define AD3552R_MASK_REFERENCE_VOLTAGE_SEL (BIT(1) | BIT(0)) > +/* AD3552R_REG_ADDR_ERR_ALARM_MASK */ > +#define AD3552R_MASK_REF_RANGE_ALARM BIT(6) > +#define AD3552R_MASK_CLOCK_COUNT_ERR_ALARM BIT(5) > +#define AD3552R_MASK_MEM_CRC_ERR_ALARM BIT(4) > +#define AD3552R_MASK_SPI_CRC_ERR_ALARM BIT(3) > +#define AD3552R_MASK_WRITE_TO_READ_ONLY_ALARM BIT(2) > +#define AD3552R_MASK_PARTIAL_REGISTER_ACCESS_ALARM BIT(1) > +#define AD3552R_MASK_REGISTER_ADDRESS_INVALID_ALARM BIT(0) > +/* AD3552R_REG_ADDR_ERR_STATUS */ > +#define AD3552R_MASK_REF_RANGE_ERR_STATUS BIT(6) > +#define AD3552R_MASK_DUAL_SPI_STREAM_EXCEEDS_DAC_ERR_STATUS BIT(5) > +#define AD3552R_MASK_MEM_CRC_ERR_STATUS BIT(4) > +#define AD3552R_MASK_RESET_STATUS BIT(0) > +/* AD3552R_REG_ADDR_POWERDOWN_CONFIG */ > +#define AD3552R_MASK_CH_DAC_POWERDOWN(ch) BIT(4 + (ch)) > +#define AD3552R_MASK_CH_AMPLIFIER_POWERDOWN(ch) BIT(ch) > +/* AD3552R_REG_ADDR_CH0_CH1_OUTPUT_RANGE */ > +#define AD3552R_MASK_CH_OUTPUT_RANGE_SEL(ch) ((ch) ? 0xF0 : 0x0F) > +/* AD3552R_REG_ADDR_CH_GAIN */ With appropriate naming the defines can make this clear without the comment. Also good to have them next to the reg addre define. #define AD3552R_CH_GAIN_REG WHATEVER #define AD3552R_CH_GAIN_RANGE_OVERRIDE BIT(7) etc works well. Subtle indenting is enough to make it easy to read. > +#define AD3552R_MASK_CH_RANGE_OVERRIDE BIT(7) > +#define AD3552R_MASK_CH_GAIN_SCALING_N (BIT(6) | BIT(5)) > +#define AD3552R_MASK_CH_GAIN_SCALING_P (BIT(4) | BIT(3)) GENMASK or BIT() for all masks (GENMASK for all multibit ones that are contiguous) > +#define AD3552R_MASK_CH_OFFSET_POLARITY BIT(2) > +#define AD3552R_MASK_CH_OFFSET_BIT_8 BIT(0) > +/* AD3552R_REG_ADDR_CH_OFFSET */ > +#define AD3552R_MASK_CH_OFFSET_BITS_0_7 0xFF GENMASK. > + > +/* Useful defines */ > +#define AD3552R_NUM_CH 2 > +#define AD3552R_MASK_CH(ch) BIT(ch) > +#define AD3552R_PAGE_CH 2 > +#define AD3552R_MAX_REG_SIZE 3 > +#define AD3552R_READ_BIT (1 << 7) > +#define AD3552R_ADDR_MASK (~AD3552R_READ_BIT) GENMASK, rather than this unusual approach. > +#define AD3552R_CRC_ENABLE_VALUE (BIT(6) | BIT(1)) > +#define AD3552R_CRC_DISABLE_VALUE (BIT(1) | BIT(0)) > +#define AD3552R_CRC_POLY 0x07 > +#define AD3552R_CRC_SEED 0xA5 > +#define AD3552R_MASK_DAC_12B 0xFFF0 GENMASK. > +#define AD3552R_DEFAULT_CONFIG_B_VALUE 0x8 > +#define SCRATCH_PAD_TEST_VAL1 0x34 > +#define SCRATCH_PAD_TEST_VAL2 0xB2 > +#define TO_MICROS 1000000 Should be a standard definition you can use for this. Check units.h > +#define GAIN_SCALE 1000 > +#define AD3552R_READ true > +#define AD3552R_WRITE false If these provide value, then renaming your parameter to convey the same thing without adding a macro that just evaluates to a boolean. > +#define LDAC_PULSE_US 10 prefix to avoid potential naming clashes. > + > +enum ad3552r_ch_output_range { > + /* Range from 0 V to 2.5 V. Requires Rfb1x connection */ > + AD3552R_CH_OUTPUT_RANGE_0__2_5V, > + /* Range from 0 V to 5 V. Requires Rfb1x connection */ > + AD3552R_CH_OUTPUT_RANGE_0__5V, > + /* Range from 0 V to 10 V. Requires Rfb2x connection */ > + AD3552R_CH_OUTPUT_RANGE_0__10V, > + /* Range from -2.5 V to 7.5 V. Requires Rfb2x connection */ > + AD3552R_CH_OUTPUT_RANGE_NEG_5__5V, > + /* Range from -6.5 V to 3.5 V. Requires Rfb4x connection */ > + AD3552R_CH_OUTPUT_RANGE_NEG_10__10V, > +}; > + > +static const s32 ch_ranges[][2] = { > + [AD3552R_CH_OUTPUT_RANGE_0__2_5V] = {0, 2500}, > + [AD3552R_CH_OUTPUT_RANGE_0__5V] = {0, 5000}, > + [AD3552R_CH_OUTPUT_RANGE_0__10V] = {0, 10000}, > + [AD3552R_CH_OUTPUT_RANGE_NEG_5__5V] = {-5000, 5000}, > + [AD3552R_CH_OUTPUT_RANGE_NEG_10__10V] = {-10000, 10000} > +}; > + > +enum ad3552r_ch_gain_scaling { > + /* Gain scaling of 1 */ > + AD3552R_CH_GAIN_SCALING_1, > + /* Gain scaling of 0.5 */ > + AD3552R_CH_GAIN_SCALING_0_5, > + /* Gain scaling of 0.25 */ > + AD3552R_CH_GAIN_SCALING_0_25, > + /* Gain scaling of 0.125 */ > + AD3552R_CH_GAIN_SCALING_0_125, > +}; > + > +/* Gain * GAIN_SCALE */ > +static const s32 gains_scaling_table[] = { > + [AD3552R_CH_GAIN_SCALING_1] = 1000, > + [AD3552R_CH_GAIN_SCALING_0_5] = 500, > + [AD3552R_CH_GAIN_SCALING_0_25] = 250, > + [AD3552R_CH_GAIN_SCALING_0_125] = 125 > +}; > + > +enum ad3552r_dev_attributes { > + /* - Direct register values */ > + /* From 0-3 */ > + AD3552R_SDO_DRIVE_STRENGTH, > + /* > + * 0 -> Internal Vref, vref_io pin floating (default) > + * 1 -> Internal Vref, vref_io driven by internal vref > + * 2 or 3 -> External Vref > + */ > + AD3552R_VREF_SELECT, > + /* Enable / Disable CRC */ > + AD3552R_CRC_ENABLE, > + /* Spi mode: Strandard, Dual or Quad */ > + AD3552R_SPI_MULTI_IO_MODE, > + /* Spi data rate: Single or dual */ > + AD3552R_SPI_DATA_RATE, > + /* Dual spi synchronous mode */ > + AD3552R_SPI_SYNCHRONOUS_ENABLE, > + > + /* - Direct register values (Private) */ > + /* Read registers in ascending order if set. Else descending */ > + AD3552R_ADDR_ASCENSION, > + /* Single instruction mode if set. Else, stream mode */ > + AD3552R_SINGLE_INST, > + /* Number of addresses to loop on when stream writing. */ > + AD3552R_STREAM_MODE, > + /* Keep stream value if set. */ > + AD3552R_STREAM_LENGTH_KEEP_VALUE, > +}; > + > +enum ad3552r_ch_attributes { > + /* DAC powerdown */ > + AD3552R_CH_DAC_POWERDOWN, > + /* DAC amplifier powerdown */ > + AD3552R_CH_AMPLIFIER_POWERDOWN, > + /* Select the output range. Select from enum ad3552r_ch_output_range */ > + AD3552R_CH_OUTPUT_RANGE_SEL, > + /* > + * Over-rider the range selector in order to manually set the output > + * voltage range > + */ > + AD3552R_CH_RANGE_OVERRIDE, > + /* Manually set the offset voltage */ > + AD3552R_CH_GAIN_OFFSET, > + /* Sets the polarity of the offset. */ > + AD3552R_CH_GAIN_OFFSET_POLARITY, > + /* PDAC gain scaling */ > + AD3552R_CH_GAIN_SCALING_P, > + /* NDAC gain scaling */ > + AD3552R_CH_GAIN_SCALING_N, > + /* Trigger a software LDAC */ > + AD3552R_CH_TRIGGER_SOFTWARE_LDAC, > + /* Hardware LDAC Mask */ > + AD3552R_CH_HW_LDAC_MASK, > + /* Rfb value */ > + AD3552R_CH_RFB, > + /* Channel select. When set allow Input -> DAC and Mask -> DAC */ > + AD3552R_CH_SELECT, > + /* Raw value to be set to dac */ > + AD3552R_CH_CODE > +}; > + > +struct ad3552r_ch_data { > + u16 gain_offset : 9; > + u16 range_override : 1; > + u16 n : 2; > + u16 p : 2; > + u16 offset_polarity : 1; I'd be tempted to not use bitfields here and instead just used fairly small types and m ake the various booleans actual booleans. Structure will end up a bit bigger but that shouldn't matter too much if at all given how large it is anyway. > + u16 rfb; > + u8 range; > + s32 scale_int; > + s32 scale_dec; > + s32 offset_int; > + s32 offset_dec; > + bool prec_en; > +}; > + > +struct ad3552r_desc { > + struct iio_dev *indio_dev; As below, get rid of this. > + struct mutex lock; What is the scope of the lock. All locks need comments stating the scope. > + struct gpio_desc *gpio_reset; > + struct gpio_desc *gpio_ldac; > + struct spi_device *spi; > + struct ad3552r_ch_data ch_data[AD3552R_NUM_CH]; > + struct iio_chan_spec channels[AD3552R_NUM_CH + 1]; > + unsigned long enabled_ch; > + unsigned int num_ch; > + bool use_input_regs; > + u8 buf_data[2 * (AD3552R_MAX_REG_SIZE + 2)] ____cacheline_aligned; Comment needed to explain the size of this. > +}; > + > +static const u16 addr_mask_map[][2] = { > + [AD3552R_ADDR_ASCENSION] = { > + AD3552R_REG_ADDR_INTERFACE_CONFIG_A, > + AD3552R_MASK_ADDR_ASCENSION > + }, > + [AD3552R_SINGLE_INST] = { > + AD3552R_REG_ADDR_INTERFACE_CONFIG_B, > + AD3552R_MASK_SINGLE_INST > + }, > + [AD3552R_STREAM_MODE] = { > + AD3552R_REG_ADDR_STREAM_MODE, > + AD3552R_MASK_LENGTH > + }, > + [AD3552R_STREAM_LENGTH_KEEP_VALUE] = { > + AD3552R_REG_ADDR_TRANSFER_REGISTER, > + AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE > + }, > + [AD3552R_SDO_DRIVE_STRENGTH] = { > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SDO_DRIVE_STRENGTH > + }, > + [AD3552R_VREF_SELECT] = { > + AD3552R_REG_ADDR_SH_REFERENCE_CONFIG, > + AD3552R_MASK_REFERENCE_VOLTAGE_SEL > + }, > + [AD3552R_CRC_ENABLE] = { > + AD3552R_REG_ADDR_INTERFACE_CONFIG_C, > + AD3552R_MASK_CRC_ENABLE > + }, > + [AD3552R_SPI_MULTI_IO_MODE] = { > + AD3552R_REG_ADDR_TRANSFER_REGISTER, > + AD3552R_MASK_MULTI_IO_MODE > + }, > + [AD3552R_SPI_DATA_RATE] = { > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SPI_CONFIG_DDR > + }, > + [AD3552R_SPI_SYNCHRONOUS_ENABLE] = { > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_DUAL_SPI_SYNCHROUNOUS_EN > + }, > +}; > + > +/* 0 -> reg addr, 1->ch0 mask, 2->ch1 mask */ > +static const u16 addr_mask_map_ch[][3] = { > + [AD3552R_CH_DAC_POWERDOWN] = { > + AD3552R_REG_ADDR_POWERDOWN_CONFIG, > + AD3552R_MASK_CH_DAC_POWERDOWN(0), > + AD3552R_MASK_CH_DAC_POWERDOWN(1) > + }, > + [AD3552R_CH_AMPLIFIER_POWERDOWN] = { > + AD3552R_REG_ADDR_POWERDOWN_CONFIG, > + AD3552R_MASK_CH_AMPLIFIER_POWERDOWN(0), > + AD3552R_MASK_CH_AMPLIFIER_POWERDOWN(1) > + }, > + [AD3552R_CH_OUTPUT_RANGE_SEL] = { > + AD3552R_REG_ADDR_CH0_CH1_OUTPUT_RANGE, > + AD3552R_MASK_CH_OUTPUT_RANGE_SEL(0), > + AD3552R_MASK_CH_OUTPUT_RANGE_SEL(1) > + }, > + /* > + * This attributes are update by the chip on 16B and 24B no matter to > + * what register the write is done > + */ > + [AD3552R_CH_TRIGGER_SOFTWARE_LDAC] = { > + AD3552R_REG_ADDR_SW_LDAC_16B, > + AD3552R_MASK_CH(0), > + AD3552R_MASK_CH(1) > + }, > + [AD3552R_CH_HW_LDAC_MASK] = { > + AD3552R_REG_ADDR_HW_LDAC_16B, > + AD3552R_MASK_CH(0), > + AD3552R_MASK_CH(1) > + }, > + [AD3552R_CH_SELECT] = { > + AD3552R_REG_ADDR_CH_SELECT_16B, > + AD3552R_MASK_CH(0), > + AD3552R_MASK_CH(1) > + } > +}; > + > +static u8 _ad3552r_reg_len(u8 addr) > +{ > + if (addr > AD3552R_REG_ADDR_MAX) > + return 0; > + > + switch (addr) { > + case AD3552R_REG_ADDR_HW_LDAC_16B: > + case AD3552R_REG_ADDR_CH_SELECT_16B: > + case AD3552R_REG_ADDR_SW_LDAC_16B: > + case AD3552R_REG_ADDR_HW_LDAC_24B: > + case AD3552R_REG_ADDR_CH_SELECT_24B: > + case AD3552R_REG_ADDR_SW_LDAC_24B: > + return 1; > + default: > + break; > + } > + > + if (addr > AD3552R_REG_ADDR_HW_LDAC_24B) > + return 3; > + if (addr > AD3552R_REG_ADDR_HW_LDAC_16B) > + return 2; > + > + return 1; > +} > + > +/* SPI transfer to device */ > +static int ad3552r_transfer(struct ad3552r_desc *dac, u8 addr, u32 len, > + u8 *data, bool is_read) > +{ > + int err; > + u8 instr; > + > + instr = addr & AD3552R_ADDR_MASK; > + instr |= is_read ? AD3552R_READ_BIT : 0; > + dac->buf_data[0] = instr; > + if (is_read) { > + err = spi_write_then_read(dac->spi, dac->buf_data, 1, > + dac->buf_data + 1, len); > + if (err) > + return err; > + > + memcpy(data, dac->buf_data + 1, len); > + > + return 0; > + } > + > + memcpy(dac->buf_data + 1, data, len); > + return spi_write(dac->spi, dac->buf_data, len + 1); > +} > + > +static int ad3552r_write_reg(struct ad3552r_desc *dac, u8 addr, u16 val) > +{ > + u8 reg_len, buf[AD3552R_MAX_REG_SIZE] = { 0 }; > + > + reg_len = _ad3552r_reg_len(addr); > + if (!reg_len) > + return -EINVAL; > + > + if (reg_len == 2) > + /* Only DAC register are 2 bytes wide */ > + val &= AD3552R_MASK_DAC_12B; > + if (reg_len == 1) > + buf[0] = val & 0xFF; > + else > + /* reg_len can be 2 or 3, but 3rd bytes needs to be set to 0 */ > + *((u16 *)buf) = cpu_to_be16(val); put_unaligned_be16() no one said buf is 2 byte aligned... > + > + return ad3552r_transfer(dac, addr, reg_len, buf, > + AD3552R_WRITE); Single line. > +} > + > +static int ad3552r_read_reg(struct ad3552r_desc *dac, u8 addr, u16 *val) > +{ > + int err; > + u8 reg_len, buf[AD3552R_MAX_REG_SIZE] = { 0 }; > + > + reg_len = _ad3552r_reg_len(addr); > + if (!reg_len) > + return -EINVAL; > + > + err = ad3552r_transfer(dac, addr, reg_len, buf, AD3552R_READ); > + if (err) > + return err; > + > + if (reg_len == 1) > + *val = buf[0]; > + else > + /* reg_len can be 2 or 3, but only first 2 bytes are relevant */ > + *val = be16_to_cpu(*((u16 *)buf)); get_unaligned_be16() > + > + return 0; > +} > + > +/* Update field of a register, shift val if needed */ > +static int ad3552r_update_reg_field(struct ad3552r_desc *dac, u8 addr, u16 mask, > + u16 val) > +{ > + int ret; > + u16 reg; > + > + ret = ad3552r_read_reg(dac, addr, ®); > + if (ret < 0) > + return ret; > + > + reg = (reg & ~mask) | (val << __ffs(mask)); > + > + return ad3552r_write_reg(dac, addr, reg); > +} > + > +static int ad3552r_set_dev_value(struct ad3552r_desc *dac, > + enum ad3552r_dev_attributes attr, > + u16 val) > +{ > + switch (attr) { > + case AD3552R_SPI_MULTI_IO_MODE: > + case AD3552R_SPI_DATA_RATE: > + case AD3552R_SPI_SYNCHRONOUS_ENABLE: > + case AD3552R_CRC_ENABLE: > + /* Not implemented */ > + return -EINVAL; > + default: > + return ad3552r_update_reg_field(dac, addr_mask_map[attr][0], > + addr_mask_map[attr][1], val); > + } > + > + return 0; > +} > + > +static int ad3552r_set_offset_value(struct ad3552r_desc *dac, u8 ch, int val) > +{ > + int err; > + > + err = ad3552r_write_reg(dac, AD3552R_REG_ADDR_CH_OFFSET(ch), > + val & AD3552R_MASK_CH_OFFSET_BITS_0_7); > + if (err) > + return err; > + > + err = ad3552r_update_reg_field(dac, > + AD3552R_REG_ADDR_CH_GAIN(ch), > + AD3552R_MASK_CH_OFFSET_BIT_8, > + (val >> 8) & AD3552R_MASK_CH_OFFSET_BIT_8); > + if (err) > + return err; > + > + dac->ch_data[ch].gain_offset = val; > + > + return 0; > +} > + > +static int ad3552r_set_gain_value(struct ad3552r_desc *dac, > + enum ad3552r_ch_attributes attr, > + u8 ch, > + int val) > +{ > + int reg_mask, err; > + > + if (attr == AD3552R_CH_GAIN_OFFSET) > + return ad3552r_set_offset_value(dac, ch, val); > + > + switch (attr) { > + case AD3552R_CH_RANGE_OVERRIDE: > + val = !!val; > + reg_mask = AD3552R_MASK_CH_RANGE_OVERRIDE; > + break; > + case AD3552R_CH_GAIN_OFFSET_POLARITY: > + val = !!val; > + reg_mask = AD3552R_MASK_CH_OFFSET_POLARITY; > + break; > + case AD3552R_CH_GAIN_SCALING_P: > + reg_mask = AD3552R_MASK_CH_GAIN_SCALING_P; > + break; > + case AD3552R_CH_GAIN_SCALING_N: > + reg_mask = AD3552R_MASK_CH_GAIN_SCALING_N; > + break; > + default: > + return -EINVAL; > + } > + > + err = ad3552r_update_reg_field(dac, AD3552R_REG_ADDR_CH_GAIN(ch), > + reg_mask, val); > + if (err) > + return err; > + > + switch (attr) { > + case AD3552R_CH_RANGE_OVERRIDE: > + dac->ch_data[ch].range_override = val; > + break; > + case AD3552R_CH_GAIN_OFFSET_POLARITY: > + dac->ch_data[ch].offset_polarity = val; > + break; > + case AD3552R_CH_GAIN_SCALING_P: > + dac->ch_data[ch].p = val; > + break; > + case AD3552R_CH_GAIN_SCALING_N: > + dac->ch_data[ch].n = val; > + break; > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +/* Iterate over mask and write required bytes */ > +static int ad3552r_write_codes(struct ad3552r_desc *dac, u32 mask, u8 *vals) > +{ > + int err, i, reg_len, k = 0; > + unsigned long lmask = mask; > + u8 addr, buff[AD3552R_NUM_CH * AD3552R_MAX_REG_SIZE]; > + u16 val; > + > + /* If writing to consecutive registers do just one transfer */ > + > + if (mask == (AD3552R_MASK_CH(0) | AD3552R_MASK_CH(1)) && > + dac->ch_data[0].prec_en == dac->ch_data[1].prec_en) { > + if (dac->use_input_regs) { > + if (dac->ch_data[0].prec_en) > + addr = AD3552R_REG_ADDR_CH_INPUT_24B(1); > + else > + addr = AD3552R_REG_ADDR_CH_INPUT_16B(1); > + } else { > + if (dac->ch_data[0].prec_en) > + addr = AD3552R_REG_ADDR_CH_DAC_24B(1); > + else > + addr = AD3552R_REG_ADDR_CH_DAC_16B(1); > + } > + > + reg_len = _ad3552r_reg_len(addr); > + buff[0] = vals[0]; > + buff[reg_len] = vals[2]; > + if (dac->ch_data[0].prec_en) { > + /* Reg_len is 3 here */ > + buff[1] = vals[1]; > + buff[2] = 0; > + buff[4] = vals[3]; > + buff[5] = 0; > + } else { > + buff[1] = vals[1] & 0xf0; > + buff[3] = vals[3] & 0xf0; > + } > + > + err = ad3552r_transfer(dac, addr, reg_len * 2, buff, > + AD3552R_WRITE); > + if (err) > + return err; > + } else { > + > + k = 0; > + for_each_set_bit(i, &lmask, AD3552R_NUM_CH + 1) { > + /* Writing to mask CH */ > + if (i == AD3552R_PAGE_CH) > + addr = dac->ch_data[0].prec_en ? > + AD3552R_REG_ADDR_INPUT_PAGE_MASK_24B : > + AD3552R_REG_ADDR_INPUT_PAGE_MASK_16B; > + else > + addr = dac->ch_data[i].prec_en ? > + AD3552R_REG_ADDR_CH_INPUT_24B(i) : > + AD3552R_REG_ADDR_CH_INPUT_16B(i); > + > + reg_len = _ad3552r_reg_len(addr); > + val = be16_to_cpu(*((u16 *)(vals + k))); > + > + k += 2; > + err = ad3552r_write_reg(dac, addr, val); > + if (err) > + return err; > + } > + } > + > + if (dac->gpio_ldac) { > + gpiod_set_value_cansleep(dac->gpio_ldac, 0); > + usleep_range(LDAC_PULSE_US, LDAC_PULSE_US + 10); > + gpiod_set_value_cansleep(dac->gpio_ldac, 1); > + } > + > + return 0; > +} > + > +static int ad3552r_get_ch_value(struct ad3552r_desc *dac, > + enum ad3552r_ch_attributes attr, > + u8 ch, > + u16 *val) > +{ > + int ret; > + u16 reg; > + u8 addr; > + u16 mask; > + > + /* Attributes not defined in addr_mask_map_ch */ > + switch (attr) { > + case AD3552R_CH_CODE: > + return ad3552r_read_reg(dac, AD3552R_REG_ADDR_CH_DAC_24B(ch), > + val); > + case AD3552R_CH_RFB: > + *val = dac->ch_data[ch].rfb; > + return 0; > + default: > + break; > + } > + > + if (attr >= AD3552R_CH_RANGE_OVERRIDE && > + attr <= AD3552R_CH_GAIN_SCALING_N) I'd assume this couldn't happen or am I missing a path by which it can? Don't bother checking for things we know never happen. > + return -EINVAL; > + > + addr = addr_mask_map_ch[attr][0]; > + if (addr == AD3552R_REG_ADDR_SW_LDAC_24B || > + addr == AD3552R_REG_ADDR_SW_LDAC_16B) { Any plausible way you can actually hit this? If so I'd expect it to be closed down earlier than this point. > + dev_err(&dac->indio_dev->dev, "Write only registers\n"); > + /* LDAC are write only registers */ > + return -EINVAL; > + } > + > + ret = ad3552r_read_reg(dac, addr, ®); > + if (ret < 0) > + return ret; > + > + mask = addr_mask_map_ch[attr][ch + 1]; > + *val = (reg & mask) >> __ffs(mask); FIELD_GET might do the job here for you though with everything dynamic perhaps not. > + > + return 0; > +} > + > +static int ad3552r_set_ch_value(struct ad3552r_desc *dac, > + enum ad3552r_ch_attributes attr, > + u8 ch, > + u16 val) > +{ > + int ret; > + > + /* Attributes not defined in addr_mask_map_ch */ > + switch (attr) { > + case AD3552R_CH_CODE: > + return ad3552r_write_reg(dac, AD3552R_REG_ADDR_CH_DAC_24B(ch), > + val); > + case AD3552R_CH_RFB: > + dac->ch_data[ch].rfb = val; > + return 0; > + default: > + break; > + } > + > + if (attr >= AD3552R_CH_RANGE_OVERRIDE && > + attr <= AD3552R_CH_GAIN_SCALING_N) > + return ad3552r_set_gain_value(dac, attr, ch, val); > + > + /* Update register related to attributes in chip */ > + ret = ad3552r_update_reg_field(dac, addr_mask_map_ch[attr][0], > + addr_mask_map_ch[attr][ch + 1], val); > + if (ret < 0) > + return ret; > + > + /* Update software structures */ > + if (attr == AD3552R_CH_OUTPUT_RANGE_SEL) { > + val %= AD3552R_CH_OUTPUT_RANGE_NEG_10__10V + 1; > + dac->ch_data[ch].range = val; > + } > + > + return ret; > +} > + > +static ssize_t ad3552r_write_ext(struct iio_dev *indio_dev, > + uintptr_t private, > + const struct iio_chan_spec *chan, > + const char *buf, size_t len) > +{ > + struct ad3552r_desc *dac = iio_priv(indio_dev); > + int val, frac, err; > + > + err = iio_str_to_fixpoint(buf, 0, &val, &frac); > + if (err < 0) > + return err; > + > + dac->ch_data[chan->channel].prec_en = !!val; > + > + return len; > +} > + > +static ssize_t ad3552r_read_ext(struct iio_dev *indio_dev, > + uintptr_t private, > + const struct iio_chan_spec *chan, > + char *buf) > +{ > + struct ad3552r_desc *dac = iio_priv(indio_dev); > + int val; > + > + if (private != 0) > + return -EINVAL; > + > + val = dac->ch_data[chan->channel].prec_en; > + > + return iio_format_value(buf, IIO_VAL_INT, 1, &val); > +} > + > +#define AD3552R_CH_ATTR(_name, _what) { \ > + .name = _name, \ > + .read = ad3552r_read_ext, \ > + .write = ad3552r_write_ext, \ > + .private = _what, \ > + .shared = IIO_SEPARATE, \ > +} > + > +static const struct iio_chan_spec_ext_info ad3552r_ext_info[] = { > + AD3552R_CH_ATTR("precision_mode_en", 0), > + {}, > +}; > + > +#define AD3552R_CH_DAC(_idx) ((struct iio_chan_spec) { \ > + .type = IIO_VOLTAGE, \ > + .output = true, \ > + .indexed = true, \ > + .channel = _idx, \ > + .scan_index = _idx, \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = 16, \ > + .storagebits = 16, \ > + .endianness = IIO_BE, \ > + }, \ > + .ext_info = ad3552r_ext_info, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_ENABLE) | \ > + BIT(IIO_CHAN_INFO_OFFSET), \ > +}) > + > +#define AD3552R_CH_DAC_PAGE(_idx) ((struct iio_chan_spec) { \ > + .type = IIO_VOLTAGE, \ > + .output = true, \ > + .indexed = true, \ > + .channel = _idx, \ > + .scan_index = _idx, \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = 16, \ > + .storagebits = 16, \ > + .endianness = IIO_BE, \ > + }, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .modified = 1, \ > + .channel2 = IIO_MOD_X_AND_Z, \ This needs a comment. What is this channel? MOD_X_AND_Z is a pretty non obvious for a DAC! > +}) > + > +static int ad3552r_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, > + int *val2, > + long mask) > +{ > + struct ad3552r_desc *dac = iio_priv(indio_dev); > + u16 tmp_val; > + int err; > + u8 ch = chan->channel; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + mutex_lock(&dac->lock); > + if (chan->channel == AD3552R_PAGE_CH) > + err = ad3552r_read_reg(dac, > + AD3552R_REG_ADDR_DAC_PAGE_MASK_24B, > + &tmp_val); > + else > + err = ad3552r_get_ch_value(dac, AD3552R_CH_CODE, ch, > + &tmp_val); > + if (err < 0) { > + mutex_unlock(&dac->lock); > + return err; > + } > + > + *val = tmp_val; > + mutex_unlock(&dac->lock); > + break; > + case IIO_CHAN_INFO_ENABLE: > + mutex_lock(&dac->lock); > + err = ad3552r_get_ch_value(dac, AD3552R_CH_DAC_POWERDOWN, > + ch, &tmp_val); > + if (err < 0) { > + mutex_unlock(&dac->lock); > + return err; > + } > + *val = !tmp_val; > + mutex_unlock(&dac->lock); > + break; > + case IIO_CHAN_INFO_SCALE: > + *val = dac->ch_data[ch].scale_int; > + *val2 = dac->ch_data[ch].scale_dec; > + return IIO_VAL_INT_PLUS_MICRO; > + case IIO_CHAN_INFO_OFFSET: > + *val = dac->ch_data[ch].offset_int; > + *val2 = dac->ch_data[ch].offset_dec; > + return IIO_VAL_INT_PLUS_MICRO; > + default: > + return -EINVAL; > + } > + > + return IIO_VAL_INT; > +} > + > +static int ad3552r_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, > + int val2, > + long mask) > +{ > + struct ad3552r_desc *dac = iio_priv(indio_dev); > + enum ad3552r_ch_attributes attr; > + int err = 0; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + if (chan->channel == AD3552R_PAGE_CH) { > + mutex_lock(&dac->lock); > + err = ad3552r_write_reg(dac, > + AD3552R_REG_ADDR_DAC_PAGE_MASK_24B, > + val); > + mutex_unlock(&dac->lock); > + > + return err; > + } > + > + attr = AD3552R_CH_CODE; > + break; > + case IIO_CHAN_INFO_ENABLE: > + attr = AD3552R_CH_DAC_POWERDOWN; > + val = !val; > + break; > + default: > + return -EINVAL; > + } > + > + mutex_lock(&dac->lock); > + err = ad3552r_set_ch_value(dac, attr, chan->channel, val); > + mutex_unlock(&dac->lock); > + > + return err; > +} > + > +static int ad3552r_update_scan_mode(struct iio_dev *indio_dev, > + const unsigned long *scan_mask) > +{ > + u32 mask; > + > + mask = *scan_mask; > + /* If writing to mask, can't write to other channels */ > + if ((mask & AD3552R_MASK_CH(AD3552R_PAGE_CH)) && > + (mask & (~AD3552R_MASK_CH(AD3552R_PAGE_CH)))) > + return -EINVAL; > + > + return 0; > +} > + > +/* > + * Device type specific information. > + */ > +static const struct iio_info ad3552r_iio_info = { > + .read_raw = ad3552r_read_raw, > + .write_raw = ad3552r_write_raw, > + .update_scan_mode = ad3552r_update_scan_mode > +}; > + > +static irqreturn_t ad3552r_trigger_handler(int irq, void *p) > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *indio_dev = pf->indio_dev; > + struct iio_buffer *buf = indio_dev->buffer; > + struct ad3552r_desc *dac = iio_priv(indio_dev); > + char buff[AD3552R_NUM_CH * AD3552R_MAX_REG_SIZE]; > + int err; > + > + memset(buff, 0, sizeof(buff)); > + mutex_lock(&dac->lock); > + err = iio_buffer_remove_sample(buf, buff); > + if (err) > + goto end; > + > + err = ad3552r_write_codes(dac, *indio_dev->active_scan_mask, buff); > + if (err) > + goto end; > + > +end: > + iio_trigger_notify_done(indio_dev->trig); > + mutex_unlock(&dac->lock); > + > + return IRQ_HANDLED; > +} > + > +static int ad3552r_setup_trigger_buffer(struct device *dev, > + struct iio_dev *indio_dev, int irq) > +{ > + struct ad3552r_desc *dac = iio_priv(indio_dev); This sort of careful indenting tends to cause noise in the long run because future changes require the indenting to be updated to be consistent. I would encourage you to drop it and just have a single space for all definitions. > + struct iio_trigger *hwtrig; > + int err; > + > + /* Configure trigger buffer */ Function is fairly self explanatory. I would drop the comment. Comments rot so best not have them if they don't provide substantial information not available from a quick glance at the code. Often the "why" not the "what". > + err = devm_iio_triggered_buffer_setup_ext(dev, indio_dev, NULL, > + &ad3552r_trigger_handler, > + IIO_BUFFER_DIRECTION_OUT, > + NULL, > + NULL); > + if (err) > + return err; > + > + if (!irq) > + return 0; > + > + hwtrig = devm_iio_trigger_alloc(dev, "%s-ldac-dev%d", > + indio_dev->name, > + iio_device_id(indio_dev)); > + if (!hwtrig) > + return -ENOMEM; > + > + hwtrig->dev.parent = dev; > + iio_trigger_set_drvdata(hwtrig, dac); > + err = devm_iio_trigger_register(dev, hwtrig); > + if (err < 0) > + return err; > + > + return devm_request_threaded_irq(dev, irq, > + iio_trigger_generic_data_rdy_poll, What is this trigger? Already been raised in other review... > + NULL, > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > + indio_dev->name, > + hwtrig); > +} > + > +static int ad3552r_check_scratch_pad(struct ad3552r_desc *dac) > +{ > + const u16 val1 = SCRATCH_PAD_TEST_VAL1; > + const u16 val2 = SCRATCH_PAD_TEST_VAL2; > + u16 val; > + int err; > + > + err = ad3552r_write_reg(dac, AD3552R_REG_ADDR_SCRATCH_PAD, val1); > + if (err < 0) > + return err; > + > + err = ad3552r_read_reg(dac, AD3552R_REG_ADDR_SCRATCH_PAD, &val); > + if (err < 0) > + return err; > + > + if (val1 != val) > + return -ENODEV; > + > + err = ad3552r_write_reg(dac, AD3552R_REG_ADDR_SCRATCH_PAD, val2); > + if (err < 0) > + return err; > + > + err = ad3552r_read_reg(dac, AD3552R_REG_ADDR_SCRATCH_PAD, &val); > + if (err < 0) > + return err; > + > + if (val2 != val) > + return -ENODEV; > + > + return 0; > +} > + > +struct reg_addr_pool { > + struct ad3552r_desc *dac; > + u8 addr; > +}; > + > +static u16 ad3552r_read_reg_pool(struct reg_addr_pool *addr) pool? > +{ > + u16 val = 0; > + > + ad3552r_read_reg(addr->dac, addr->addr, &val); Why not have ad3552r_read_reg return an int that is val? Seems that would make life easier for you. > + > + return val; > +} > + > +static int ad3552r_reset(struct ad3552r_desc *dac) > +{ > + struct reg_addr_pool addr; > + int ret; > + u16 val; > + > + dac->gpio_reset = devm_gpiod_get_optional(&dac->spi->dev, "reset", > + GPIOD_OUT_LOW); > + if (IS_ERR(dac->gpio_reset)) > + return PTR_ERR(dac->gpio_reset); > + > + if (dac->gpio_reset) { > + /* Perform hardware reset */ > + usleep_range(10, 20); > + gpiod_set_value_cansleep(dac->gpio_reset, 1); > + } else { > + /* Perform software reset if no GPIO provided */ > + ret = ad3552r_update_reg_field(dac, AD3552R_REG_ADDR_INTERFACE_CONFIG_A, > + AD3552R_MASK_SOFTWARE_RESET, > + AD3552R_MASK_SOFTWARE_RESET); > + if (ret < 0) > + return ret; > + > + } > + > + addr.dac = dac; > + addr.addr = AD3552R_REG_ADDR_INTERFACE_CONFIG_B; > + ret = readx_poll_timeout(ad3552r_read_reg_pool, > + &addr, > + val, Where it doesn't greatly hurt readability, try to keep multiple parameters on one line as long as you are below 80 chars. > + (val == AD3552R_DEFAULT_CONFIG_B_VALUE), why the brackets? cond is appropriately bracketed in the macro definitions so you shouldn't need them here. > + 5000, > + 50000); > + if (ret) { > + dev_err(&dac->spi->dev, "Err: %d\n", ret); > + return ret; > + } > + > + ret = readx_poll_timeout(ad3552r_read_reg_pool, > + &addr, > + val, > + (!(val & AD3552R_MASK_INTERFACE_NOT_READY)), Again, too many brackets and lines. > + 5000, > + 50000); > + if (ret) { > + dev_err(&dac->spi->dev, "Err: %d\n", ret); > + return ret; > + } > + > + ret = ad3552r_set_dev_value(dac, AD3552R_ADDR_ASCENSION, 0); > + if (ret < 0) I think this can only return <= 0 to so you should be fine with return ad3552r_set_dev_value() If I'm wrong you might want to push any forcing of no positive returns into a lower level function. > + return ret; > + > + return 0; > +} > + > +static void ad3552r_get_custom_range(struct ad3552r_desc *dac, s32 i, s32 *v_min, > + s32 *v_max) > +{ > + s64 vref, tmp, common, offset, gn, gp; > + /* > + * From datasheet formula (In Volts): > + * Vmin = 2.5 + [(GainN + Offset / 1024) * 2.5 * Rfb * 1.03] > + * Vmax = 2.5 - [(GainP + Offset / 1024) * 2.5 * Rfb * 1.03] > + * Calculus are converted to milivolts > + */ > + vref = 2500; > + /* 2.5 * 1.03 * 1000 (To mV) */ > + common = 2575 * dac->ch_data[i].rfb; > + offset = dac->ch_data[i].gain_offset; > + if (dac->ch_data[i].offset_polarity) > + offset *= -1; > + > + gn = gains_scaling_table[dac->ch_data[i].n]; > + tmp = (1024 * gn + GAIN_SCALE * offset) * common; > + tmp = div_s64(tmp, 1024 * GAIN_SCALE); > + *v_max = vref + tmp; > + > + gp = gains_scaling_table[dac->ch_data[i].p]; > + tmp = (1024 * gp - GAIN_SCALE * offset) * common; > + tmp = div_s64(tmp, 1024 * GAIN_SCALE); > + *v_min = vref - tmp; > +} > + > +static void ad3552r_set_gain_and_offset(struct ad3552r_desc *dac, s32 ch) Would expect something called 'set' to be writing to hardware. Perhaps calc_gain_and_offset? > +{ > + s32 idx, v_max, v_min, span, rem; > + s64 tmp; > + > + if (dac->ch_data[ch].range_override) { > + ad3552r_get_custom_range(dac, ch, &v_min, &v_max); > + } else { > + /* Normal range */ > + idx = dac->ch_data[ch].range; > + v_max = ch_ranges[idx][1]; > + v_min = ch_ranges[idx][0]; > + } > + > + /* > + * From datasheet formula: > + * Vout = Span * (D / 65536) + Vmin > + * Converted to scale and offset: > + * Scale = Span / 65536 > + * Offset = 65536 * Vmin / Span > + * > + * Reminders are in micros in order to be printed as > + * IIO_VAL_INT_PLUS_MICRO > + */ > + span = v_max - v_min; > + dac->ch_data[ch].scale_int = div_s64_rem(span, 65536, &rem); > + dac->ch_data[ch].scale_dec = DIV_ROUND_CLOSEST((s64)rem * TO_MICROS, > + 65536); > + > + dac->ch_data[ch].offset_int = div_s64_rem(v_min * 65536, span, > + &rem); > + tmp = (s64)rem * TO_MICROS; > + dac->ch_data[ch].offset_dec = div_s64(tmp, span); > +} > + > +static const char * const gain_dts_names[] = { > + "adi,gain-scaling-p", > + "adi,gain-scaling-n", > + "adi,rfb" > +}; > + > +static int ad3552r_configure_device(struct ad3552r_desc *dac) > +{ > + static const enum ad3552r_ch_attributes gain_attrs[] = { > + AD3552R_CH_GAIN_SCALING_P, > + AD3552R_CH_GAIN_SCALING_N, > + AD3552R_CH_RFB > + }; > + struct fwnode_handle *child, *custom_gain_child = NULL; Odd spacing. Also easier to read as two separate lines. As a general rule don't share lines if setting the variables to anything. > + int i, err, cnt = 0; > + u32 val, ch; > + bool is_custom; > + > + dac->gpio_ldac = devm_gpiod_get_optional(&dac->spi->dev, "ldac", As you have a bunch of accesses to &dac->spi->dev in here, I'd have a local struct device *dev; > + GPIOD_OUT_HIGH); > + if (IS_ERR(dac->gpio_ldac)) > + return PTR_ERR(dac->gpio_ldac); > + > + dac->use_input_regs = device_property_read_bool(&dac->spi->dev, > + "adi,synch_channels"); > + > + err = device_property_read_u32(&dac->spi->dev, "adi,vref-select", &val); > + if (!err) { > + if (val > 2) { > + dev_err(&dac->spi->dev, "%s must be less than 3\n", > + "adi,vref-select"); > + return -EINVAL; > + } > + err = ad3552r_set_dev_value(dac, AD3552R_VREF_SELECT, val); > + if (err) > + return err; I gave comments on this stuff in the review of the binding patch. Will follow through to changes in here. > + } > + > + err = device_property_read_u32(&dac->spi->dev, "adi,sdo-drive-strength", > + &val); > + if (!err) { > + if (val > 3) { > + dev_err(&dac->spi->dev, "%s must be less than 4\n", > + "adi,sdo-drive-strength"); > + return -EINVAL; > + } > + err = ad3552r_set_dev_value(dac, AD3552R_SDO_DRIVE_STRENGTH, > + val); > + if (err) > + return err; > + } > + > + dac->num_ch = device_get_child_node_count(&dac->spi->dev); > + if (!dac->num_ch) { > + dev_err(&dac->spi->dev, "No channels defined\n"); > + return -ENODEV; > + } > + > + device_for_each_child_node(&dac->spi->dev, child) { > + err = fwnode_property_read_u32(child, "reg", &ch); > + if (err) { > + dev_err(&dac->spi->dev, "Mandory reg property missing\n"); > + goto put_child; > + } > + if (ch >= AD3552R_NUM_CH) { > + dev_err(&dac->spi->dev, "reg must be less than %d\n", > + AD3552R_NUM_CH); > + err = -EINVAL; > + goto put_child; > + } > + > + if (fwnode_property_present(child, "adi,output-range")) { > + is_custom = false; > + err = fwnode_property_read_u32(child, > + "adi,output-range", > + &val); > + if (err) { > + dev_err(&dac->spi->dev, > + "Mandory adi,output-range property missing\n"); > + goto put_child; > + } > + > + if (val > AD3552R_CH_OUTPUT_RANGE_NEG_10__10V) { > + dev_err(&dac->spi->dev, > + "adi,output-range must be less or equal than %d\n", > + AD3552R_CH_OUTPUT_RANGE_NEG_10__10V + 1); > + err = -EINVAL; > + goto put_child; > + } > + > + err = ad3552r_set_ch_value(dac, > + AD3552R_CH_OUTPUT_RANGE_SEL, > + ch, val); > + if (err) > + goto put_child; > + } else { > + is_custom = true; > + custom_gain_child = > + fwnode_get_named_child_node(child, > + "custom-output-range-config"); > + if (IS_ERR(custom_gain_child)) { > + err = PTR_ERR(custom_gain_child); > + dev_err(&dac->spi->dev, > + "Mandory custom-output-range-config property missing\n"); > + goto put_child; > + } > + > + err = fwnode_property_read_u32(custom_gain_child, > + "adi,gain-offset", &val); > + if (err) { > + dev_err(&dac->spi->dev, > + "Mandory adi,gain-offset property missing\n"); > + goto put_child; > + } > + > + err = ad3552r_set_ch_value(dac, > + AD3552R_CH_GAIN_OFFSET, > + ch, abs((s32)val)); > + if (err) > + goto put_child; > + > + err = ad3552r_set_ch_value(dac, AD3552R_CH_GAIN_OFFSET_POLARITY, > + ch, (s32)val < 0); > + if (err) > + goto put_child; > + > + for (i = 0; i < ARRAY_SIZE(gain_attrs); ++i) { > + err = fwnode_property_read_u32(custom_gain_child, > + gain_dts_names[i], > + &val); > + if (err) { > + dev_err(&dac->spi->dev, > + "Mandory %s property missing\n", > + gain_dts_names[i]); > + goto put_child; > + } > + > + err = ad3552r_set_ch_value(dac, gain_attrs[i], > + ch, val); > + if (err) > + goto put_child; > + } > + } > + > + ad3552r_set_gain_and_offset(dac, ch); > + err = ad3552r_set_ch_value(dac, AD3552R_CH_RANGE_OVERRIDE, ch, > + is_custom); > + if (err) > + goto put_child; > + > + dac->enabled_ch |= BIT(ch); > + > + err = ad3552r_set_ch_value(dac, AD3552R_CH_SELECT, ch, 1); > + if (err < 0) > + return err; > + > + dac->channels[cnt] = AD3552R_CH_DAC(ch); > + ++cnt; > + > + } > + > + if (cnt == AD3552R_NUM_CH) { > + dac->channels[cnt] = AD3552R_CH_DAC_PAGE(AD3552R_PAGE_CH); > + ++cnt; > + } else { > + /* Disable unused channels */ > + for_each_clear_bit(ch, &dac->enabled_ch, AD3552R_PAGE_CH) { > + err = ad3552r_set_ch_value(dac, > + AD3552R_CH_AMPLIFIER_POWERDOWN, > + ch, > + 0); > + if (err) > + goto put_child; > + } > + } > + > + dac->num_ch = cnt; > + > +put_child: > + if (!IS_ERR_OR_NULL(custom_gain_child)) > + fwnode_handle_put(custom_gain_child); I would think about factoring out the custom_gain bit as a separate function and then you can have this error handling in there. > + fwnode_handle_put(child); > + > + return err; > +} > + > +static int ad3552r_init(struct ad3552r_desc *dac) > +{ > + int err; > + > + err = ad3552r_reset(dac); > + if (err) { > + dev_err(&dac->spi->dev, "Reset failed\n"); > + return err; > + } > + > + err = ad3552r_check_scratch_pad(dac); > + if (err) { > + dev_err(&dac->spi->dev, "Scratch pad test failed\n"); > + return err; > + } > + > + return ad3552r_configure_device(dac); > +} > + > +static int ad3552r_probe(struct spi_device *spi) > +{ > + struct ad3552r_desc *dac; > + struct iio_dev *indio_dev; > + int err; > + > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*dac)); > + if (!indio_dev) > + return -ENOMEM; > + > + dac = iio_priv(indio_dev); > + dac->indio_dev = indio_dev; This is usually a bad sign. It's normally possible and sensible to architect a driver to only navigate a heriarchy in one direction. This is adding a pointer the other way. Not much used anyway so please clear it out entirely. > + dac->spi = spi; > + > + mutex_init(&dac->lock); > + > + err = ad3552r_init(dac); > + if (err) > + return err; > + > + /* Config triggered buffer device */ > + indio_dev->dev.parent = &spi->dev; > + indio_dev->name = "ad3552r"; > + indio_dev->info = &ad3552r_iio_info; > + indio_dev->num_channels = dac->num_ch; > + indio_dev->channels = dac->channels; > + indio_dev->modes = INDIO_DIRECT_MODE; > + > + err = ad3552r_setup_trigger_buffer(&spi->dev, indio_dev, spi->irq); > + if (err) > + return err; > + > + return devm_iio_device_register(&spi->dev, indio_dev); > +} > + > +static const struct of_device_id ad3552r_of_match[] = { > + { .compatible = "adi,ad3552r" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, ad3552r_of_match); > + > +static struct spi_driver ad3552r_driver = { > + .driver = { > + .name = "ad3552r", > + .of_match_table = ad3552r_of_match, > + }, > + .probe = ad3552r_probe > +}; > +module_spi_driver(ad3552r_driver); > + > +MODULE_AUTHOR("Mihail Chindris "); > +MODULE_DESCRIPTION("Analog Device AD3552R DAC"); > +MODULE_LICENSE("GPL v2");