The AD7173 family offer a complete integrated Sigma-Delta ADC solution
which can be used in high precision, low noise single channel applications
or higher speed multiplexed applications. The Sigma-Delta ADC is intended
primarily for measurement of signals close to DC but also delivers
outstanding performance with input bandwidths out to ~10kHz.
Signed-off-by: Dumitru Ceclan <[email protected]>
---
.../bindings/iio/adc/adi,ad7173.yaml | 139 ++++++++++++++++++
1 file changed, 139 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
new file mode 100644
index 000000000000..a0f437297a23
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
@@ -0,0 +1,139 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2023 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad7173.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD7173 ADC device driver
+
+maintainers:
+ - Ceclan Dumitru <[email protected]>
+
+description: |
+ Bindings for the Analog Devices AD717X ADC's. Datasheets for supported chips:
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD7175-2.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD7176-2.pdf
+
+properties:
+ compatible:
+ enum:
+ - adi,ad7172-2
+ - adi,ad7173-8
+ - adi,ad7175-2
+ - adi,ad7176-2
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+ spi-max-frequency:
+ maximum: 20000000
+
+ spi-cpol:
+ type: boolean
+
+ spi-cpha:
+ type: boolean
+
+ required:
+ - compatible
+ - reg
+ - interrupts
+
+patternProperties:
+ "^channel@[0-9a-f]$":
+ type: object
+ $ref: adc.yaml
+ unevaluatedProperties: false
+
+ properties:
+ reg:
+ description: Channel number
+ minimum: 0
+ maximum: 15
+
+ diff-channels:
+ description:
+ Analog input pins
+ items:
+ minimum: 0
+ maximum: 31
+
+ adi,bipolar:
+ description: Specify if the channel should measure in bipolar mode.
+ type: boolean
+
+ required:
+ - reg
+ - diff-channels
+
+allOf:
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ adc@0 {
+ compatible = "adi,ad7173-8";
+ reg = <0>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
+ interrupt-parent = <&gpio>;
+ spi-max-frequency = <5000000>;
+
+ channel@0 {
+ reg = <0>;
+ adi,bipolar;
+
+ diff-channels = <0 1>;
+ };
+
+ channel@1 {
+ reg = <1>;
+
+ diff-channels = <2 3>;
+ };
+
+ channel@2 {
+ reg = <2>;
+ adi,bipolar;
+
+ diff-channels = <4 5>;
+ };
+
+ channel@3 {
+ reg = <3>;
+ adi,bipolar;
+
+ diff-channels = <6 7>;
+ };
+
+ channel@4 {
+ reg = <4>;
+
+ diff-channels = <8 9>;
+ };
+ };
+ };
--
2.39.2
The AD7173 family offer a complete integrated Sigma-Delta ADC solution
which can be used in high precision, low noise single channel
applications or higher speed multiplexed applications. The Sigma-Delta
ADC is intended primarily for measurement of signals close to DC but also
delivers outstanding performance with input bandwidths out to ~10kHz.
Signed-off-by: Dumitru Ceclan <[email protected]>
---
drivers/iio/adc/Kconfig | 13 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ad7173.c | 850 +++++++++++++++++++++++++++++++++++++++
3 files changed, 864 insertions(+)
create mode 100644 drivers/iio/adc/ad7173.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 517b3db114b8..fe14a8b24e41 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -54,6 +54,19 @@ config AD7124
To compile this driver as a module, choose M here: the module will be
called ad7124.
+config AD7173
+ tristate "Analog Devices AD7173 driver"
+ depends on SPI_MASTER
+ select AD_SIGMA_DELTA
+ select GPIO_REGMAP
+ select REGMAP_SPI
+ help
+ Say yes here to build support for Analog Devices AD7173 and similar ADC
+ (currently supported: AD7172-2, AD7173-8, AD7175-2, AD7176-2).
+
+ To compile this driver as a module, choose M here: the module will be
+ called ad7173.
+
config AD7192
tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver"
depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 2facf979327d..8c70333ae967 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
obj-$(CONFIG_AD4130) += ad4130.o
obj-$(CONFIG_AD7091R5) += ad7091r5.o ad7091r-base.o
obj-$(CONFIG_AD7124) += ad7124.o
+obj-$(CONFIG_AD7173) += ad7173.o
obj-$(CONFIG_AD7192) += ad7192.o
obj-$(CONFIG_AD7266) += ad7266.o
obj-$(CONFIG_AD7280) += ad7280a.o
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
new file mode 100644
index 000000000000..648bcec836a5
--- /dev/null
+++ b/drivers/iio/adc/ad7173.c
@@ -0,0 +1,850 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * AD7172-2/AD7173-8/AD7175-2/AD7176-2 SPI ADC driver
+ * Copyright (C) 2023 Analog Devices, Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/gpio/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/sysfs.h>
+#include <linux/units.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#include <linux/iio/adc/ad_sigma_delta.h>
+
+#define AD7173_REG_COMMS 0x00
+#define AD7173_REG_ADC_MODE 0x01
+#define AD7173_REG_INTERFACE_MODE 0x02
+#define AD7173_REG_CRC 0x03
+#define AD7173_REG_DATA 0x04
+#define AD7173_REG_GPIO 0x06
+#define AD7173_REG_ID 0x07
+#define AD7173_REG_CH(x) (0x10 + (x))
+#define AD7173_REG_SETUP(x) (0x20 + (x))
+#define AD7173_REG_FILTER(x) (0x28 + (x))
+#define AD7173_REG_OFFSET(x) (0x30 + (x))
+#define AD7173_REG_GAIN(x) (0x38 + (x))
+
+#define AD7173_RESET_LENGTH BITS_TO_BYTES(64)
+
+#define AD7173_CH_ENABLE BIT(15)
+#define AD7173_CH_SETUP_SEL_MASK GENMASK(14, 12)
+#define AD7173_CH_SETUP_SEL(x) FIELD_PREP(AD7173_CH_SETUP_SEL_MASK, x)
+#define AD7173_CH_SETUP_AINPOS_MASK GENMASK(9, 5)
+#define AD7173_CH_SETUP_AINPOS(x) FIELD_PREP(AD7173_CH_SETUP_AINPOS_MASK, x)
+#define AD7173_CH_SETUP_AINNEG(x) (x)
+
+#define AD7173_CH_ADDRESS(pos, neg) \
+ (AD7173_CH_SETUP_AINPOS(pos) | AD7173_CH_SETUP_AINNEG(neg))
+#define AD7173_AIN_TEMP_POS 17
+#define AD7173_AIN_TEMP_NEG 18
+
+#define AD7172_ID 0x00d0
+#define AD7173_ID 0x30d0
+#define AD7175_ID 0x0cd0
+#define AD7176_ID 0x0c90
+#define AD7173_ID_MASK GENMASK(15, 4)
+
+#define AD7173_ADC_MODE_REF_EN BIT(15)
+#define AD7173_ADC_MODE_SING_CYC BIT(13)
+#define AD7173_ADC_MODE_MODE_MASK GENMASK(6, 4)
+#define AD7173_ADC_MODE_MODE(x) FIELD_PREP(AD7173_ADC_MODE_MODE_MASK, x)
+#define AD7173_ADC_MODE_CLOCKSEL_MASK GENMASK(3, 2)
+#define AD7173_ADC_MODE_CLOCKSEL(x) FIELD_PREP(AD7173_ADC_MODE_CLOCKSEL_MASK, x)
+
+#define AD7173_GPIO_PDSW BIT(14)
+#define AD7173_GPIO_OP_EN2_3 BIT(13)
+#define AD7173_GPIO_MUX_IO BIT(12)
+#define AD7173_GPIO_SYNC_EN BIT(11)
+#define AD7173_GPIO_ERR_EN BIT(10)
+#define AD7173_GPIO_ERR_DAT BIT(9)
+#define AD7173_GPIO_GP_DATA3 BIT(7)
+#define AD7173_GPIO_GP_DATA2 BIT(6)
+#define AD7173_GPIO_IP_EN1 BIT(5)
+#define AD7173_GPIO_IP_EN0 BIT(4)
+#define AD7173_GPIO_OP_EN1 BIT(3)
+#define AD7173_GPIO_OP_EN0 BIT(2)
+#define AD7173_GPIO_GP_DATA1 BIT(1)
+#define AD7173_GPIO_GP_DATA0 BIT(0)
+
+#define AD7173_GPO12_DATA(x) BIT(x)
+#define AD7173_GPO23_DATA(x) BIT(x + 4)
+#define AD7173_GPO_DATA(x) (x < 2 ? AD7173_GPO12_DATA(x) : AD7173_GPO23_DATA(x))
+
+#define AD7173_INTERFACE_DATA_STAT BIT(6)
+#define AD7173_INTERFACE_DATA_STAT_EN(x)\
+ FIELD_PREP(AD7173_INTERFACE_DATA_STAT, x)
+
+#define AD7173_SETUP_BIPOLAR BIT(12)
+#define AD7173_SETUP_AREF_BUF_MASK GENMASK(11, 10)
+#define AD7173_SETUP_AIN_BUF_MASK GENMASK(9, 8)
+
+#define AD7173_SETUP_REF_SEL_MASK GENMASK(5, 4)
+#define AD7173_SETUP_REF_SEL(x) FIELD_PREP(AD7173_SETUP_REF_SEL_MASK, x)
+#define AD7173_SETUP_REF_SEL_AVDD1_AVSS 0x3
+#define AD7173_SETUP_REF_SEL_INT_REF 0x2
+#define AD7173_SETUP_REF_SEL_EXT_REF2 0x1
+#define AD7173_SETUP_REF_SEL_EXT_REF 0x0
+
+#define AD7173_FILTER_ODR0_MASK GENMASK(5, 0)
+#define AD7173_FILTER_ODR0(x) FIELD_PREP(AD7173_FILTER_ODR0_MASK, x)
+#define AD7173_FILTER_ODR0(x) FIELD_PREP(AD7173_FILTER_ODR0_MASK, x)
+
+enum ad7173_ids {
+ ID_AD7172_2,
+ ID_AD7173_8,
+ ID_AD7175_2,
+ ID_AD7176_2,
+};
+
+struct ad7173_device_info {
+ unsigned int id;
+ unsigned int num_inputs;
+ unsigned int num_configs;
+ unsigned int num_channels;
+ unsigned char num_gpios;
+ bool has_temp;
+ unsigned int clock;
+
+ const unsigned int *sinc5_data_rates;
+ unsigned int num_sinc5_data_rates;
+};
+
+struct ad7173_channel_config {
+ bool live;
+ unsigned char cfg_slot;
+ /* Following fields are used to compare equality. Bipolar must be first */
+ bool bipolar;
+ bool input_buf;
+ unsigned char odr;
+};
+
+struct ad7173_channel {
+ unsigned int chan_reg;
+ struct ad7173_channel_config cfg;
+ unsigned int ain;
+};
+
+struct ad7173_state {
+ const struct ad7173_device_info *info;
+ struct ad_sigma_delta sd;
+ struct ad7173_channel *channels;
+ struct regulator *reg;
+ unsigned int adc_mode;
+ unsigned int interface_mode;
+ unsigned int num_channels;
+ unsigned long cfg_slots_status; /* slots usage status bitmap*/
+ unsigned long long config_usage_counter;
+ unsigned long long *config_cnts;
+#if IS_ENABLED(CONFIG_GPIOLIB)
+ struct regmap *regmap;
+ struct gpio_regmap *gpio_regmap;
+#endif
+};
+
+static const unsigned int ad7173_sinc5_data_rates[] = {
+ 6211000, 6211000, 6211000, 6211000, 6211000, 6211000, 5181000, 4444000,
+ 3115000, 2597000, 1007000, 503800, 381000, 200300, 100500, 59520,
+ 49680, 20010, 16333, 10000, 5000, 2500, 1250,
+};
+
+static const unsigned int ad7175_sinc5_data_rates[] = {
+ 50000000, 41667000, 31250000, 27778000, 20833000, 17857000, 12500000,
+ 10000000, 5000000, 2500000, 1000000, 500000, 397500, 200000,
+ 100000, 59920, 49960, 20000, 16666, 10000, 5000,
+};
+
+static const struct ad7173_device_info ad7173_device_info[] = {
+ [ID_AD7172_2] = {
+ .id = AD7172_ID,
+ .num_inputs = 5,
+ .num_channels = 4,
+ .num_configs = 4,
+ .num_gpios = 2,
+ .has_temp = true,
+ .clock = 2 * HZ_PER_MHZ,
+ .sinc5_data_rates = ad7173_sinc5_data_rates,
+ .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+ },
+ [ID_AD7173_8] = {
+ .id = AD7173_ID,
+ .num_inputs = 17,
+ .num_channels = 16,
+ .num_configs = 8,
+ .num_gpios = 4,
+ .has_temp = true,
+ .clock = 2 * HZ_PER_MHZ,
+ .sinc5_data_rates = ad7173_sinc5_data_rates,
+ .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+ },
+ [ID_AD7175_2] = {
+ .id = AD7175_ID,
+ .num_inputs = 5,
+ .num_channels = 4,
+ .num_configs = 4,
+ .num_gpios = 2,
+ .has_temp = true,
+ .clock = 16 * HZ_PER_MHZ,
+ .sinc5_data_rates = ad7175_sinc5_data_rates,
+ .num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
+ },
+ [ID_AD7176_2] = {
+ .id = AD7176_ID,
+ .num_inputs = 5,
+ .num_channels = 4,
+ .num_configs = 4,
+ .num_gpios = 2,
+ .has_temp = false,
+ .clock = 16 * HZ_PER_MHZ,
+ .sinc5_data_rates = ad7175_sinc5_data_rates,
+ .num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
+ },
+};
+
+#if IS_ENABLED(CONFIG_GPIOLIB)
+
+static const struct regmap_range ad7173_range_gpio[] = {
+ regmap_reg_range(AD7173_REG_GPIO, AD7173_REG_GPIO),
+};
+
+static const struct regmap_access_table ad7173_access_table = {
+ .yes_ranges = ad7173_range_gpio,
+ .n_yes_ranges = ARRAY_SIZE(ad7173_range_gpio),
+};
+
+static const struct regmap_config ad7173_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 16,
+ .rd_table = &ad7173_access_table,
+ .wr_table = &ad7173_access_table,
+ .read_flag_mask = BIT(6),
+};
+
+static int ad7173_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
+ unsigned int offset, unsigned int *reg,
+ unsigned int *mask)
+{
+ *mask = AD7173_GPO_DATA(offset);
+ *reg = AD7173_REG_GPIO;
+ return 0;
+}
+
+static int ad7173_gpio_init(struct iio_dev *indio_dev)
+{
+ struct ad7173_state *st = iio_priv(indio_dev);
+ struct gpio_regmap_config gpio_regmap = {};
+ struct device *dev = &st->sd.spi->dev;
+ unsigned int mask;
+
+ st->regmap = devm_regmap_init_spi(st->sd.spi, &ad7173_regmap_config);
+ if (IS_ERR(st->regmap)) {
+ return dev_err_probe(dev, PTR_ERR(st->regmap),
+ "Unable to init regmap\n");
+ }
+
+ mask = AD7173_GPIO_OP_EN0 | AD7173_GPIO_OP_EN1 | AD7173_GPIO_OP_EN2_3;
+ regmap_update_bits(st->regmap, AD7173_REG_GPIO, mask, mask);
+
+ gpio_regmap.parent = dev;
+ gpio_regmap.regmap = st->regmap;
+ gpio_regmap.ngpio = st->info->num_gpios;
+ gpio_regmap.reg_set_base = GPIO_REGMAP_ADDR_ZERO;
+ gpio_regmap.reg_mask_xlate = ad7173_mask_xlate;
+
+ st->gpio_regmap = devm_gpio_regmap_register(dev, &gpio_regmap);
+ if (IS_ERR(st->gpio_regmap)) {
+ return dev_err_probe(dev, PTR_ERR(st->gpio_regmap),
+ "Unable to init gpio-regmap\n");
+ }
+
+ return 0;
+}
+
+#endif /* CONFIG_GPIOLIB */
+
+static struct ad7173_state *ad_sigma_delta_to_ad7173(struct ad_sigma_delta *sd)
+{
+ return container_of(sd, struct ad7173_state, sd);
+}
+
+static void ad7173_reset_usage_cnts(struct ad7173_state *st)
+{
+ memset64(st->config_cnts, 0, st->info->num_configs);
+ st->config_usage_counter = 0;
+}
+
+static struct ad7173_channel_config *ad7173_find_live_config
+ (struct ad7173_state *st, struct ad7173_channel_config *cfg)
+{
+ struct ad7173_channel_config *cfg_aux;
+ ptrdiff_t cmp_size, offset;
+ int i;
+
+ offset = offsetof(struct ad7173_channel_config, cfg_slot) +
+ sizeof(cfg->cfg_slot);
+ cmp_size = sizeof(*cfg) - offset;
+
+ for (i = 0; i < st->num_channels; i++) {
+ cfg_aux = &st->channels[i].cfg;
+
+ if (cfg_aux->live && !memcmp(&cfg->bipolar, &cfg_aux->bipolar,
+ cmp_size))
+ return cfg_aux;
+ }
+ return NULL;
+}
+
+static int ad7173_free_config_slot_lru(struct ad7173_state *st)
+{
+ int i, lru_position = 0;
+
+ for (i = 1; i < st->info->num_configs; i++)
+ if (st->config_cnts[i] < st->config_cnts[lru_position])
+ lru_position = i;
+
+ for (i = 0; i < st->num_channels; i++)
+ if (st->channels[i].cfg.cfg_slot == lru_position)
+ st->channels[i].cfg.live = false;
+
+ assign_bit(lru_position, &st->cfg_slots_status, 0);
+ return lru_position;
+}
+
+static int ad7173_load_config(struct ad7173_state *st,
+ struct ad7173_channel_config *cfg)
+{
+ unsigned int config;
+ int free_cfg_slot, ret;
+
+ free_cfg_slot = find_first_zero_bit(&st->cfg_slots_status,
+ st->info->num_configs);
+ if (free_cfg_slot == st->info->num_configs)
+ free_cfg_slot = ad7173_free_config_slot_lru(st);
+
+ assign_bit(free_cfg_slot, &st->cfg_slots_status, 1);
+ cfg->cfg_slot = free_cfg_slot;
+
+ config = AD7173_SETUP_REF_SEL(AD7173_SETUP_REF_SEL_INT_REF);
+
+ if (cfg->bipolar)
+ config |= AD7173_SETUP_BIPOLAR;
+
+ if (cfg->input_buf)
+ config |= AD7173_SETUP_AIN_BUF_MASK;
+
+ ret = ad_sd_write_reg(&st->sd, AD7173_REG_SETUP(free_cfg_slot), 2, config);
+ if (ret)
+ return ret;
+
+ return ad_sd_write_reg(&st->sd, AD7173_REG_FILTER(free_cfg_slot), 2,
+ AD7173_FILTER_ODR0_MASK & cfg->odr);
+}
+
+static int ad7173_config_channel(struct ad7173_state *st, int addr)
+{
+ struct ad7173_channel_config *cfg = &st->channels[addr].cfg;
+ struct ad7173_channel_config *live_cfg;
+ int ret;
+
+ if (!cfg->live) {
+ live_cfg = ad7173_find_live_config(st, cfg);
+ if (live_cfg) {
+ cfg->cfg_slot = live_cfg->cfg_slot;
+ } else {
+ ret = ad7173_load_config(st, cfg);
+ if (ret)
+ return ret;
+ cfg->live = true;
+ }
+ }
+
+ if (st->config_usage_counter == U64_MAX)
+ ad7173_reset_usage_cnts(st);
+
+ st->config_usage_counter++;
+ st->config_cnts[cfg->cfg_slot] = st->config_usage_counter;
+
+ return 0;
+}
+
+static int ad7173_set_channel(struct ad_sigma_delta *sd, unsigned int channel)
+{
+ struct ad7173_state *st = ad_sigma_delta_to_ad7173(sd);
+ unsigned int val;
+ int ret;
+
+ ret = ad7173_config_channel(st, channel);
+ if (ret)
+ return ret;
+
+ val = AD7173_CH_ENABLE |
+ AD7173_CH_SETUP_SEL(st->channels[channel].cfg.cfg_slot) |
+ st->channels[channel].ain;
+
+ return ad_sd_write_reg(&st->sd, AD7173_REG_CH(channel), 2, val);
+}
+
+static int ad7173_set_mode(struct ad_sigma_delta *sd,
+ enum ad_sigma_delta_mode mode)
+{
+ struct ad7173_state *st = ad_sigma_delta_to_ad7173(sd);
+
+ st->adc_mode &= ~AD7173_ADC_MODE_MODE_MASK;
+ st->adc_mode |= AD7173_ADC_MODE_MODE(mode);
+
+ return ad_sd_write_reg(&st->sd, AD7173_REG_ADC_MODE, 2, st->adc_mode);
+}
+
+static int ad7173_append_status(struct ad_sigma_delta *sd, bool append)
+{
+ struct ad7173_state *st = container_of(sd, struct ad7173_state, sd);
+ unsigned int interface_mode = st->interface_mode;
+ int ret;
+
+ interface_mode |= AD7173_INTERFACE_DATA_STAT_EN(append);
+ ret = ad_sd_write_reg(&st->sd, AD7173_REG_INTERFACE_MODE, 2, interface_mode);
+ if (ret)
+ return ret;
+
+ st->interface_mode = interface_mode;
+
+ return 0;
+}
+
+static int ad7173_disable_all(struct ad_sigma_delta *sd)
+{
+ struct ad7173_state *st = container_of(sd, struct ad7173_state, sd);
+ int ret;
+ int i;
+
+ for (i = 0; i < st->num_channels; i++) {
+ ret = ad_sd_write_reg(sd, AD7173_REG_CH(i), 2, 0);
+ if (ret < 0)
+ return ret;
+ }
+
+ return 0;
+}
+
+static struct ad_sigma_delta_info ad7173_sigma_delta_info = {
+ .set_channel = ad7173_set_channel,
+ .append_status = ad7173_append_status,
+ .disable_all = ad7173_disable_all,
+ .set_mode = ad7173_set_mode,
+ .has_registers = true,
+ .addr_shift = 0,
+ .read_mask = BIT(6),
+ .status_ch_mask = GENMASK(3, 0),
+ .data_reg = AD7173_REG_DATA,
+ .irq_flags = IRQF_TRIGGER_FALLING,
+};
+
+static int ad7173_setup(struct iio_dev *indio_dev)
+{
+ struct ad7173_state *st = iio_priv(indio_dev);
+ unsigned int id;
+ u8 buf[AD7173_RESET_LENGTH];
+ int ret;
+
+ /* reset the serial interface */
+ memset(buf, 0xff, AD7173_RESET_LENGTH);
+ ret = spi_write_then_read(st->sd.spi, buf, sizeof(buf), NULL, 0);
+ if (ret < 0)
+ return ret;
+
+ /* datasheet recommends a delay of at least 500us after reset */
+ fsleep(500);
+
+ ret = ad_sd_read_reg(&st->sd, AD7173_REG_ID, 2, &id);
+ if (ret)
+ return ret;
+
+ id &= AD7173_ID_MASK;
+ if (id != st->info->id)
+ dev_warn(&st->sd.spi->dev,
+ "Unexpected device id: %x, expected: %x\n",
+ id, st->info->id);
+
+ st->adc_mode |= AD7173_ADC_MODE_REF_EN | AD7173_ADC_MODE_SING_CYC;
+ st->interface_mode = 0x0;
+
+ st->config_usage_counter = 0;
+ st->config_cnts = devm_kcalloc(indio_dev->dev.parent,
+ st->info->num_configs, sizeof(u64),
+ GFP_KERNEL);
+ if (!st->config_cnts)
+ return -ENOMEM;
+
+ /* All channels are enabled by default after a reset */
+ return ad7173_disable_all(&st->sd);
+}
+
+static int ad7173_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long info)
+{
+ struct ad7173_state *st = iio_priv(indio_dev);
+ unsigned int reg;
+ int ret;
+
+ switch (info) {
+ case IIO_CHAN_INFO_RAW:
+ ret = ad_sigma_delta_single_conversion(indio_dev, chan, val);
+ if (ret < 0)
+ return ret;
+
+ /* disable channel after single conversion */
+ ret = ad_sd_write_reg(&st->sd, AD7173_REG_CH(chan->address), 2, 0);
+ if (ret < 0)
+ return ret;
+
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+ if (chan->type == IIO_TEMP) {
+ *val = 250000000;
+ *val2 = 800273203; /* (2^24 * 477) / 10 */
+ return IIO_VAL_FRACTIONAL;
+ } else {
+ *val = 2500;
+ if (chan->differential)
+ *val2 = 23;
+ else
+ *val2 = 24;
+ return IIO_VAL_FRACTIONAL_LOG2;
+ }
+ case IIO_CHAN_INFO_OFFSET:
+ if (chan->type == IIO_TEMP) {
+ *val = -874379;
+ } else {
+ if (chan->differential)
+ *val = (chan->scan_type.realbits < 32) ?
+ -(1 << (chan->scan_type.realbits - 1)) :
+ INT_MIN;
+ else
+ *val = 0;
+ }
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ reg = st->channels[chan->address].cfg.odr;
+
+ *val = st->info->sinc5_data_rates[reg] / MILLI;
+ *val2 = (st->info->sinc5_data_rates[reg] % MILLI) * MILLI;
+
+ return IIO_VAL_INT_PLUS_MICRO;
+ }
+ return -EINVAL;
+}
+
+static int ad7173_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long info)
+{
+ struct ad7173_state *st = iio_priv(indio_dev);
+ struct ad7173_channel_config *cfg;
+ unsigned int freq, i, reg;
+ int ret = 0;
+
+ iio_device_claim_direct_mode(indio_dev);
+ if (iio_buffer_enabled(indio_dev)) {
+ iio_device_release_direct_mode(indio_dev);
+ return -EBUSY;
+ }
+
+ switch (info) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ freq = val * MILLI + val2 / MILLI;
+
+ for (i = 0; i < st->info->num_sinc5_data_rates - 1; i++) {
+ if (freq >= st->info->sinc5_data_rates[i])
+ break;
+ }
+
+ cfg = &st->channels[chan->address].cfg;
+ cfg->odr = i;
+
+ if (cfg->live) {
+ ret = ad_sd_read_reg(&st->sd, AD7173_REG_FILTER(cfg->cfg_slot), 2, ®);
+ if (ret)
+ break;
+ reg &= ~AD7173_FILTER_ODR0_MASK;
+ reg |= AD7173_FILTER_ODR0(i);
+ ret = ad_sd_write_reg(&st->sd, AD7173_REG_FILTER(cfg->cfg_slot), 2, reg);
+ }
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ iio_device_release_direct_mode(indio_dev);
+ return ret;
+}
+
+static int ad7173_update_scan_mode(struct iio_dev *indio_dev,
+ const unsigned long *scan_mask)
+{
+ struct ad7173_state *st = iio_priv(indio_dev);
+ int i, ret = 0;
+
+ iio_device_claim_direct_mode(indio_dev);
+ for (i = 0; i < indio_dev->num_channels; i++) {
+ if (test_bit(i, scan_mask))
+ ret = ad7173_set_channel(&st->sd, i);
+ else
+ ret = ad_sd_write_reg(&st->sd, AD7173_REG_CH(i), 2, 0);
+
+ if (ret < 0)
+ goto out;
+ }
+
+out:
+ iio_device_release_direct_mode(indio_dev);
+ return ret;
+}
+
+static int ad7173_debug(struct iio_dev *indio_dev, unsigned int reg,
+ unsigned int writeval, unsigned int *readval)
+{
+ struct ad7173_state *st = iio_priv(indio_dev);
+ u8 reg_size;
+
+ if (reg == 0)
+ reg_size = 1;
+ else if (reg == AD7173_REG_CRC || reg == AD7173_REG_DATA ||
+ reg >= AD7173_REG_OFFSET(0))
+ reg_size = 3;
+ else
+ reg_size = 2;
+
+ if (readval)
+ return ad_sd_read_reg(&st->sd, reg, reg_size, readval);
+
+ return ad_sd_write_reg(&st->sd, reg, reg_size, writeval);
+}
+
+static const struct iio_info ad7173_info = {
+ .read_raw = &ad7173_read_raw,
+ .write_raw = &ad7173_write_raw,
+ .debugfs_reg_access = &ad7173_debug,
+ .validate_trigger = ad_sd_validate_trigger,
+ .update_scan_mode = ad7173_update_scan_mode,
+};
+
+static const struct iio_chan_spec ad7173_channel_template = {
+ .type = IIO_VOLTAGE,
+ .indexed = 1,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE),
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 24,
+ .storagebits = 32,
+ .endianness = IIO_BE,
+ },
+};
+
+static const struct iio_chan_spec ad7173_temp_iio_channel_template = {
+ .type = IIO_TEMP,
+ .indexed = 1,
+ .channel = AD7173_AIN_TEMP_POS,
+ .channel2 = AD7173_AIN_TEMP_NEG,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET),
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 24,
+ .storagebits = 32,
+ .endianness = IIO_BE,
+ },
+};
+
+static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
+{
+ struct ad7173_state *st = iio_priv(indio_dev);
+ struct ad7173_channel *channels_st_priv;
+ struct fwnode_handle *child;
+ struct device *dev = indio_dev->dev.parent;
+ struct iio_chan_spec *chan;
+ unsigned int num_channels;
+ unsigned int ain[2], chan_index = 0;
+ int ret;
+
+ num_channels = device_get_child_node_count(dev);
+
+ if (st->info->has_temp)
+ num_channels++;
+
+ if (num_channels == 0)
+ return 0;
+ st->num_channels = num_channels;
+
+ chan = devm_kcalloc(dev, sizeof(*chan), num_channels,
+ GFP_KERNEL);
+ if (!chan)
+ return -ENOMEM;
+
+ channels_st_priv = devm_kcalloc(dev, num_channels,
+ sizeof(*channels_st_priv), GFP_KERNEL);
+ if (!channels_st_priv)
+ return -ENOMEM;
+
+ indio_dev->channels = chan;
+ indio_dev->num_channels = num_channels;
+ st->channels = channels_st_priv;
+
+ if (st->info->has_temp) {
+ chan[chan_index] = ad7173_temp_iio_channel_template;
+ channels_st_priv[chan_index].ain =
+ AD7173_CH_ADDRESS(chan[chan_index].channel, chan[chan_index].channel2);
+ channels_st_priv[chan_index].cfg.bipolar = false;
+ channels_st_priv[chan_index].cfg.input_buf = true;
+ chan_index++;
+ }
+
+ device_for_each_child_node(dev, child) {
+ ret = fwnode_property_read_u32_array(child, "diff-channels",
+ ain, ARRAY_SIZE(ain));
+ if (ret) {
+ fwnode_handle_put(child);
+ return ret;
+ }
+
+ if (ain[0] >= st->info->num_inputs ||
+ ain[1] >= st->info->num_inputs) {
+ fwnode_handle_put(child);
+ return dev_err_probe(dev, -EINVAL,
+ "Input pin number out of range for pair (%d %d).", ain[0], ain[1]);
+ }
+
+ chan[chan_index] = ad7173_channel_template;
+ chan[chan_index].address = chan_index;
+ chan[chan_index].scan_index = chan_index;
+ chan[chan_index].channel = ain[0];
+ chan[chan_index].channel2 = ain[1];
+
+ channels_st_priv[chan_index].ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
+ channels_st_priv[chan_index].chan_reg = chan_index;
+ channels_st_priv[chan_index].cfg.input_buf = true;
+ channels_st_priv[chan_index].cfg.odr = 0;
+
+ chan[chan_index].differential = fwnode_property_read_bool(child, "adi,bipolar");
+ if (chan[chan_index].differential) {
+ chan[chan_index].info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET);
+ channels_st_priv[chan_index].cfg.bipolar = true;
+ }
+
+ chan_index++;
+ }
+
+ return 0;
+}
+
+static int ad7173_probe(struct spi_device *spi)
+{
+ struct ad7173_state *st;
+ struct iio_dev *indio_dev;
+ struct device *dev = &spi->dev;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ st = iio_priv(indio_dev);
+ st->info = device_get_match_data(dev);
+ if (!st->info)
+ return -ENODEV;
+
+ indio_dev->name = spi_get_device_id(spi)->name;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->info = &ad7173_info;
+
+ spi->mode = SPI_MODE_3;
+
+ ad7173_sigma_delta_info.num_slots = st->info->num_configs;
+ ret = ad_sd_init(&st->sd, indio_dev, spi, &ad7173_sigma_delta_info);
+ if (ret)
+ return ret;
+
+ spi_set_drvdata(spi, indio_dev);
+
+ ret = ad7173_fw_parse_channel_config(indio_dev);
+ if (ret)
+ return ret;
+
+ ret = devm_ad_sd_setup_buffer_and_trigger(dev, indio_dev);
+ if (ret)
+ return ret;
+
+ ret = ad7173_setup(indio_dev);
+ if (ret)
+ return ret;
+
+ ret = devm_iio_device_register(dev, indio_dev);
+ if (ret)
+ return ret;
+
+ if (IS_ENABLED(CONFIG_GPIOLIB))
+ return ad7173_gpio_init(indio_dev);
+ else
+ return 0;
+}
+
+static const struct of_device_id ad7173_of_match[] = {
+ { .compatible = "adi,ad7172-2",
+ .data = &ad7173_device_info[ID_AD7172_2], },
+ { .compatible = "adi,ad7173-8",
+ .data = &ad7173_device_info[ID_AD7173_8], },
+ { .compatible = "adi,ad7175-2",
+ .data = &ad7173_device_info[ID_AD7175_2], },
+ { .compatible = "adi,ad7176-2",
+ .data = &ad7173_device_info[ID_AD7176_2], },
+ { },
+};
+MODULE_DEVICE_TABLE(of, ad7173_of_match);
+
+static const struct spi_device_id ad7173_id_table[] = {
+ { "ad7172-2", &ad7173_device_info[ID_AD7172_2], },
+ { "ad7173-8", &ad7173_device_info[ID_AD7173_8], },
+ { "ad7175-2", &ad7173_device_info[ID_AD7175_2], },
+ { "ad7176-2", &ad7173_device_info[ID_AD7176_2], },
+ { },
+};
+MODULE_DEVICE_TABLE(spi, ad7173_id_table);
+
+static struct spi_driver ad7173_driver = {
+ .driver = {
+ .name = "ad7173",
+ .of_match_table = ad7173_of_match,
+ },
+ .probe = ad7173_probe,
+ .id_table = ad7173_id_table,
+};
+module_spi_driver(ad7173_driver);
+
+MODULE_AUTHOR("Lars-Peter Clausen <[email protected]>");
+MODULE_AUTHOR("Dumitru Ceclan <[email protected]>");
+MODULE_DESCRIPTION("Analog Devices AD7172/AD7173/AD7175/AD7176 ADC driver");
+MODULE_LICENSE("GPL");
--
2.39.2
Hi Dumitru,
kernel test robot noticed the following build warnings:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v6.6-rc3 next-20230928]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Dumitru-Ceclan/iio-adc-ad7173-add-AD7173-driver/20230928-205802
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20230928125443.615006-2-mitrutzceclan%40gmail.com
patch subject: [PATCH v2 2/2] iio: adc: ad7173: add AD7173 driver
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230929/[email protected]/config)
compiler: sparc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230929/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/iio/adc/ad7173.c:829:23: warning: initialization of 'long unsigned int' from 'const struct ad7173_device_info *' makes integer from pointer without a cast [-Wint-conversion]
829 | { "ad7172-2", &ad7173_device_info[ID_AD7172_2], },
| ^
drivers/iio/adc/ad7173.c:829:23: note: (near initialization for 'ad7173_id_table[0].driver_data')
drivers/iio/adc/ad7173.c:830:23: warning: initialization of 'long unsigned int' from 'const struct ad7173_device_info *' makes integer from pointer without a cast [-Wint-conversion]
830 | { "ad7173-8", &ad7173_device_info[ID_AD7173_8], },
| ^
drivers/iio/adc/ad7173.c:830:23: note: (near initialization for 'ad7173_id_table[1].driver_data')
drivers/iio/adc/ad7173.c:831:23: warning: initialization of 'long unsigned int' from 'const struct ad7173_device_info *' makes integer from pointer without a cast [-Wint-conversion]
831 | { "ad7175-2", &ad7173_device_info[ID_AD7175_2], },
| ^
drivers/iio/adc/ad7173.c:831:23: note: (near initialization for 'ad7173_id_table[2].driver_data')
drivers/iio/adc/ad7173.c:832:23: warning: initialization of 'long unsigned int' from 'const struct ad7173_device_info *' makes integer from pointer without a cast [-Wint-conversion]
832 | { "ad7176-2", &ad7173_device_info[ID_AD7176_2], },
| ^
drivers/iio/adc/ad7173.c:832:23: note: (near initialization for 'ad7173_id_table[3].driver_data')
vim +829 drivers/iio/adc/ad7173.c
827
828 static const struct spi_device_id ad7173_id_table[] = {
> 829 { "ad7172-2", &ad7173_device_info[ID_AD7172_2], },
830 { "ad7173-8", &ad7173_device_info[ID_AD7173_8], },
831 { "ad7175-2", &ad7173_device_info[ID_AD7175_2], },
832 { "ad7176-2", &ad7173_device_info[ID_AD7176_2], },
833 { },
834 };
835 MODULE_DEVICE_TABLE(spi, ad7173_id_table);
836
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hey,
On Thu, Sep 28, 2023 at 03:54:42PM +0300, Dumitru Ceclan wrote:
> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
> which can be used in high precision, low noise single channel applications
> or higher speed multiplexed applications. The Sigma-Delta ADC is intended
> primarily for measurement of signals close to DC but also delivers
> outstanding performance with input bandwidths out to ~10kHz.
>
> Signed-off-by: Dumitru Ceclan <[email protected]>
> ---
> .../bindings/iio/adc/adi,ad7173.yaml | 139 ++++++++++++++++++
> 1 file changed, 139 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> new file mode 100644
> index 000000000000..a0f437297a23
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> @@ -0,0 +1,139 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad7173.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD7173 ADC device driver
> +
> +maintainers:
> + - Ceclan Dumitru <[email protected]>
> +
> +description: |
> + Bindings for the Analog Devices AD717X ADC's. Datasheets for supported chips:
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7175-2.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7176-2.pdf
> +
> +properties:
> + compatible:
> + enum:
> + - adi,ad7172-2
> + - adi,ad7173-8
> + - adi,ad7175-2
> + - adi,ad7176-2
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 0
> +
> + spi-max-frequency:
> + maximum: 20000000
> +
> + spi-cpol:
> + type: boolean
> +
> + spi-cpha:
> + type: boolean
> +
> + required:
> + - compatible
> + - reg
> + - interrupts
> +
> +patternProperties:
> + "^channel@[0-9a-f]$":
> + type: object
> + $ref: adc.yaml
> + unevaluatedProperties: false
> +
> + properties:
> + reg:
> + description: Channel number
> + minimum: 0
> + maximum: 15
> +
> + diff-channels:
> + description:
> + Analog input pins
These descriptions are redundant AFAICT, since youre just constraining
properties defined in adc.yaml.
> + items:
> + minimum: 0
> + maximum: 31
> +
> + adi,bipolar:
> + description: Specify if the channel should measure in bipolar mode.
> + type: boolean
You have a ref here to adc.yaml, but do not appear to be making use of
"bipolar" property:
bipolar:
$ref: /schemas/types.yaml#/definitions/flag
description: If provided, the channel is to be used in bipolar mode.
> +
> + required:
> + - reg
> + - diff-channels
> +
> +allOf:
> + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + adc@0 {
> + compatible = "adi,ad7173-8";
> + reg = <0>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
> + interrupt-parent = <&gpio>;
> + spi-max-frequency = <5000000>;
> +
> + channel@0 {
> + reg = <0>;
> + adi,bipolar;
> +
Why the odd newlines?
Mostly looks good to me, cheers,
Conor.
> + diff-channels = <0 1>;
> + };
> +
> + channel@1 {
> + reg = <1>;
> +
> + diff-channels = <2 3>;
> + };
> +
> + channel@2 {
> + reg = <2>;
> + adi,bipolar;
> +
> + diff-channels = <4 5>;
> + };
> +
> + channel@3 {
> + reg = <3>;
> + adi,bipolar;
> +
> + diff-channels = <6 7>;
> + };
> +
> + channel@4 {
> + reg = <4>;
> +
> + diff-channels = <8 9>;
> + };
> + };
> + };
> --
> 2.39.2
>
On Fri, 29 Sep 2023 07:29:01 +0800
kernel test robot <[email protected]> wrote:
> Hi Dumitru,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on jic23-iio/togreg]
> [also build test WARNING on linus/master v6.6-rc3 next-20230928]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Dumitru-Ceclan/iio-adc-ad7173-add-AD7173-driver/20230928-205802
> base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> patch link: https://lore.kernel.org/r/20230928125443.615006-2-mitrutzceclan%40gmail.com
> patch subject: [PATCH v2 2/2] iio: adc: ad7173: add AD7173 driver
> config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230929/[email protected]/config)
> compiler: sparc64-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230929/[email protected]/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <[email protected]>
> | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> All warnings (new ones prefixed by >>):
>
> >> drivers/iio/adc/ad7173.c:829:23: warning: initialization of 'long unsigned int' from 'const struct ad7173_device_info *' makes integer from pointer without a cast [-Wint-conversion]
> 829 | { "ad7172-2", &ad7173_device_info[ID_AD7172_2], },
> | ^
> drivers/iio/adc/ad7173.c:829:23: note: (near initialization for 'ad7173_id_table[0].driver_data')
> drivers/iio/adc/ad7173.c:830:23: warning: initialization of 'long unsigned int' from 'const struct ad7173_device_info *' makes integer from pointer without a cast [-Wint-conversion]
> 830 | { "ad7173-8", &ad7173_device_info[ID_AD7173_8], },
> | ^
> drivers/iio/adc/ad7173.c:830:23: note: (near initialization for 'ad7173_id_table[1].driver_data')
> drivers/iio/adc/ad7173.c:831:23: warning: initialization of 'long unsigned int' from 'const struct ad7173_device_info *' makes integer from pointer without a cast [-Wint-conversion]
> 831 | { "ad7175-2", &ad7173_device_info[ID_AD7175_2], },
> | ^
> drivers/iio/adc/ad7173.c:831:23: note: (near initialization for 'ad7173_id_table[2].driver_data')
> drivers/iio/adc/ad7173.c:832:23: warning: initialization of 'long unsigned int' from 'const struct ad7173_device_info *' makes integer from pointer without a cast [-Wint-conversion]
> 832 | { "ad7176-2", &ad7173_device_info[ID_AD7176_2], },
> | ^
> drivers/iio/adc/ad7173.c:832:23: note: (near initialization for 'ad7173_id_table[3].driver_data')
>
>
> vim +829 drivers/iio/adc/ad7173.c
>
> 827
> 828 static const struct spi_device_id ad7173_id_table[] = {
> > 829 { "ad7172-2", &ad7173_device_info[ID_AD7172_2], },
> 830 { "ad7173-8", &ad7173_device_info[ID_AD7173_8], },
> 831 { "ad7175-2", &ad7173_device_info[ID_AD7175_2], },
> 832 { "ad7176-2", &ad7173_device_info[ID_AD7176_2], },
Cast these to kernel_ulong_t as done in similar drivers like the ad7192
Jonathan
> 833 { },
> 834 };
> 835 MODULE_DEVICE_TABLE(spi, ad7173_id_table);
> 836
>
On Thu, 28 Sep 2023 15:54:43 +0300
Dumitru Ceclan <[email protected]> wrote:
> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
> which can be used in high precision, low noise single channel
> applications or higher speed multiplexed applications. The Sigma-Delta
> ADC is intended primarily for measurement of signals close to DC but also
> delivers outstanding performance with input bandwidths out to ~10kHz.
>
> Signed-off-by: Dumitru Ceclan <[email protected]>
Hi Dumitru,
A few more things came up on a fresh look. All small stuff so fingers
crossed for v3.
Jonathan
> ---
> drivers/iio/adc/Kconfig | 13 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ad7173.c | 850 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 864 insertions(+)
> create mode 100644 drivers/iio/adc/ad7173.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 517b3db114b8..fe14a8b24e41 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -54,6 +54,19 @@ config AD7124
> To compile this driver as a module, choose M here: the module will be
> called ad7124.
>
> +config AD7173
> + tristate "Analog Devices AD7173 driver"
> + depends on SPI_MASTER
> + select AD_SIGMA_DELTA
> + select GPIO_REGMAP
If you are selecting it, why does it have if guards in the driver.
I prefer the select here, so drop this if guards.
> + select REGMAP_SPI
> + help
> + Say yes here to build support for Analog Devices AD7173 and similar ADC
> + (currently supported: AD7172-2, AD7173-8, AD7175-2, AD7176-2).
> +
> + To compile this driver as a module, choose M here: the module will be
> + called ad7173.
> +
> config AD7192
> tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver"
> depends on SPI
...
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> new file mode 100644
> index 000000000000..648bcec836a5
> --- /dev/null
> +++ b/drivers/iio/adc/ad7173.c
> @@ -0,0 +1,850 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * AD7172-2/AD7173-8/AD7175-2/AD7176-2 SPI ADC driver
> + * Copyright (C) 2023 Analog Devices, Inc.
As you list Lars as an author and he's not be at ADI for 'a while'
I'm guessing this should probably include a date range going back
a few years. Always good to reflect that history even if it's
been out of tree.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/gpio/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
> +#include <linux/sysfs.h>
> +#include <linux/units.h>
> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
sysfs.h is only needed if you are defining IIO attrs without
doing it via the IIO core channel descriptions. I don't see you doing
that so shouldn't be needed.
> +#include <linux/iio/trigger.h>
Whilst the driver provides a trigger, it's via the ad_sigma_delta
library so I don't think this driver needs to include trigger.h
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +#include <linux/iio/adc/ad_sigma_delta.h>
> +
> +#define AD7173_REG_COMMS 0x00
> +#define AD7173_REG_ADC_MODE 0x01
> +#define AD7173_REG_INTERFACE_MODE 0x02
> +#define AD7173_REG_CRC 0x03
> +#define AD7173_REG_DATA 0x04
> +#define AD7173_REG_GPIO 0x06
> +#define AD7173_REG_ID 0x07
> +#define AD7173_REG_CH(x) (0x10 + (x))
> +#define AD7173_REG_SETUP(x) (0x20 + (x))
> +#define AD7173_REG_FILTER(x) (0x28 + (x))
> +#define AD7173_REG_OFFSET(x) (0x30 + (x))
> +#define AD7173_REG_GAIN(x) (0x38 + (x))
> +
> +#define AD7173_RESET_LENGTH BITS_TO_BYTES(64)
> +
> +#define AD7173_CH_ENABLE BIT(15)
> +#define AD7173_CH_SETUP_SEL_MASK GENMASK(14, 12)
> +#define AD7173_CH_SETUP_SEL(x) FIELD_PREP(AD7173_CH_SETUP_SEL_MASK, x)
> +#define AD7173_CH_SETUP_AINPOS_MASK GENMASK(9, 5)
> +#define AD7173_CH_SETUP_AINPOS(x) FIELD_PREP(AD7173_CH_SETUP_AINPOS_MASK, x)
> +#define AD7173_CH_SETUP_AINNEG(x) (x)
> +
> +#define AD7173_CH_ADDRESS(pos, neg) \
> + (AD7173_CH_SETUP_AINPOS(pos) | AD7173_CH_SETUP_AINNEG(neg))
> +#define AD7173_AIN_TEMP_POS 17
> +#define AD7173_AIN_TEMP_NEG 18
> +
> +#define AD7172_ID 0x00d0
> +#define AD7173_ID 0x30d0
> +#define AD7175_ID 0x0cd0
> +#define AD7176_ID 0x0c90
> +#define AD7173_ID_MASK GENMASK(15, 4)
> +
> +#define AD7173_ADC_MODE_REF_EN BIT(15)
> +#define AD7173_ADC_MODE_SING_CYC BIT(13)
> +#define AD7173_ADC_MODE_MODE_MASK GENMASK(6, 4)
> +#define AD7173_ADC_MODE_MODE(x) FIELD_PREP(AD7173_ADC_MODE_MODE_MASK, x)
> +#define AD7173_ADC_MODE_CLOCKSEL_MASK GENMASK(3, 2)
> +#define AD7173_ADC_MODE_CLOCKSEL(x) FIELD_PREP(AD7173_ADC_MODE_CLOCKSEL_MASK, x)
> +
> +#define AD7173_GPIO_PDSW BIT(14)
> +#define AD7173_GPIO_OP_EN2_3 BIT(13)
> +#define AD7173_GPIO_MUX_IO BIT(12)
> +#define AD7173_GPIO_SYNC_EN BIT(11)
> +#define AD7173_GPIO_ERR_EN BIT(10)
> +#define AD7173_GPIO_ERR_DAT BIT(9)
> +#define AD7173_GPIO_GP_DATA3 BIT(7)
> +#define AD7173_GPIO_GP_DATA2 BIT(6)
> +#define AD7173_GPIO_IP_EN1 BIT(5)
> +#define AD7173_GPIO_IP_EN0 BIT(4)
> +#define AD7173_GPIO_OP_EN1 BIT(3)
> +#define AD7173_GPIO_OP_EN0 BIT(2)
> +#define AD7173_GPIO_GP_DATA1 BIT(1)
> +#define AD7173_GPIO_GP_DATA0 BIT(0)
> +
> +#define AD7173_GPO12_DATA(x) BIT(x)
> +#define AD7173_GPO23_DATA(x) BIT(x + 4)
> +#define AD7173_GPO_DATA(x) (x < 2 ? AD7173_GPO12_DATA(x) : AD7173_GPO23_DATA(x))
> +
> +#define AD7173_INTERFACE_DATA_STAT BIT(6)
> +#define AD7173_INTERFACE_DATA_STAT_EN(x)\
> + FIELD_PREP(AD7173_INTERFACE_DATA_STAT, x)
> +
> +#define AD7173_SETUP_BIPOLAR BIT(12)
> +#define AD7173_SETUP_AREF_BUF_MASK GENMASK(11, 10)
> +#define AD7173_SETUP_AIN_BUF_MASK GENMASK(9, 8)
> +
> +#define AD7173_SETUP_REF_SEL_MASK GENMASK(5, 4)
> +#define AD7173_SETUP_REF_SEL(x) FIELD_PREP(AD7173_SETUP_REF_SEL_MASK, x)
> +#define AD7173_SETUP_REF_SEL_AVDD1_AVSS 0x3
> +#define AD7173_SETUP_REF_SEL_INT_REF 0x2
> +#define AD7173_SETUP_REF_SEL_EXT_REF2 0x1
> +#define AD7173_SETUP_REF_SEL_EXT_REF 0x0
> +
> +#define AD7173_FILTER_ODR0_MASK GENMASK(5, 0)
> +#define AD7173_FILTER_ODR0(x) FIELD_PREP(AD7173_FILTER_ODR0_MASK, x)
> +#define AD7173_FILTER_ODR0(x) FIELD_PREP(AD7173_FILTER_ODR0_MASK, x)
Firstly why is it duplicated?
Secondly why not just use the FIELD_PREP() inline as it's obvious
what it's doing and the extra macro, if anything makes it less clear.
Same is true for all the similar macros.
> +
> +enum ad7173_ids {
> + ID_AD7172_2,
> + ID_AD7173_8,
> + ID_AD7175_2,
> + ID_AD7176_2,
> +};
> +
> +struct ad7173_device_info {
> + unsigned int id;
> + unsigned int num_inputs;
> + unsigned int num_configs;
> + unsigned int num_channels;
> + unsigned char num_gpios;
> + bool has_temp;
> + unsigned int clock;
> +
> + const unsigned int *sinc5_data_rates;
> + unsigned int num_sinc5_data_rates;
> +};
> +
> +struct ad7173_channel_config {
> + bool live;
> + unsigned char cfg_slot;
> + /* Following fields are used to compare equality. Bipolar must be first */
> + bool bipolar;
> + bool input_buf;
> + unsigned char odr;
> +};
> +
> +struct ad7173_channel {
> + unsigned int chan_reg;
> + struct ad7173_channel_config cfg;
> + unsigned int ain;
> +};
> +
> +struct ad7173_state {
> + const struct ad7173_device_info *info;
> + struct ad_sigma_delta sd;
> + struct ad7173_channel *channels;
> + struct regulator *reg;
> + unsigned int adc_mode;
> + unsigned int interface_mode;
> + unsigned int num_channels;
> + unsigned long cfg_slots_status; /* slots usage status bitmap*/
Whilst you might not need to as it fits in one, DECLARE_BITMAP()
is a good way of making it clear something is a bitmap.
I wondered if you used this a bitmap, and you do. Though another
question there is why assign_bit() when value is known.
Just use set_bit() or clear_bit() directly
> + unsigned long long config_usage_counter;
> + unsigned long long *config_cnts;
> +#if IS_ENABLED(CONFIG_GPIOLIB)
> + struct regmap *regmap;
> + struct gpio_regmap *gpio_regmap;
> +#endif
> +};
> +#if IS_ENABLED(CONFIG_GPIOLIB)
Seems odd to have these guards. I don't think we mind obliging
people to build with GPIOLIB support as very unlikely they won't
have it anyway for other reasons. Looking at the Kconfig I see
you select this anyway, so just drop the if guards.
> +
> +static const struct regmap_range ad7173_range_gpio[] = {
> + regmap_reg_range(AD7173_REG_GPIO, AD7173_REG_GPIO),
> +};
> +
> +static const struct regmap_access_table ad7173_access_table = {
> + .yes_ranges = ad7173_range_gpio,
> + .n_yes_ranges = ARRAY_SIZE(ad7173_range_gpio),
> +};
> +
> +static const struct regmap_config ad7173_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 16,
> + .rd_table = &ad7173_access_table,
> + .wr_table = &ad7173_access_table,
> + .read_flag_mask = BIT(6),
> +};
> +
> +static int ad7173_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
> + unsigned int offset, unsigned int *reg,
> + unsigned int *mask)
> +{
> + *mask = AD7173_GPO_DATA(offset);
> + *reg = AD7173_REG_GPIO;
> + return 0;
> +}
> +
> +static int ad7173_gpio_init(struct iio_dev *indio_dev)
> +{
> + struct ad7173_state *st = iio_priv(indio_dev);
> + struct gpio_regmap_config gpio_regmap = {};
> + struct device *dev = &st->sd.spi->dev;
> + unsigned int mask;
> +
> + st->regmap = devm_regmap_init_spi(st->sd.spi, &ad7173_regmap_config);
If the regmap config is only used for the gpio parts, good to name
it to make that clear.
> + if (IS_ERR(st->regmap)) {
> + return dev_err_probe(dev, PTR_ERR(st->regmap),
> + "Unable to init regmap\n");
> + }
> +
> + mask = AD7173_GPIO_OP_EN0 | AD7173_GPIO_OP_EN1 | AD7173_GPIO_OP_EN2_3;
> + regmap_update_bits(st->regmap, AD7173_REG_GPIO, mask, mask);
> +
> + gpio_regmap.parent = dev;
> + gpio_regmap.regmap = st->regmap;
> + gpio_regmap.ngpio = st->info->num_gpios;
> + gpio_regmap.reg_set_base = GPIO_REGMAP_ADDR_ZERO;
> + gpio_regmap.reg_mask_xlate = ad7173_mask_xlate;
> +
> + st->gpio_regmap = devm_gpio_regmap_register(dev, &gpio_regmap);
> + if (IS_ERR(st->gpio_regmap)) {
> + return dev_err_probe(dev, PTR_ERR(st->gpio_regmap),
> + "Unable to init gpio-regmap\n");
> + }
> +
> + return 0;
> +}
> +
> +#endif /* CONFIG_GPIOLIB */
> +
> +static struct ad7173_state *ad_sigma_delta_to_ad7173(struct ad_sigma_delta *sd)
> +{
> + return container_of(sd, struct ad7173_state, sd);
At least one more place you can use this... ( I deleted it whilst reviewing but
remembered the pattern). It's in ad7173_append_status()
> +}
> +
> +static void ad7173_reset_usage_cnts(struct ad7173_state *st)
> +{
> + memset64(st->config_cnts, 0, st->info->num_configs);
> + st->config_usage_counter = 0;
> +}
> +
> +static struct ad7173_channel_config *ad7173_find_live_config
> + (struct ad7173_state *st, struct ad7173_channel_config *cfg)
> +{
> + struct ad7173_channel_config *cfg_aux;
> + ptrdiff_t cmp_size, offset;
> + int i;
> +
> + offset = offsetof(struct ad7173_channel_config, cfg_slot) +
> + sizeof(cfg->cfg_slot);
> + cmp_size = sizeof(*cfg) - offset;
> +
> + for (i = 0; i < st->num_channels; i++) {
> + cfg_aux = &st->channels[i].cfg;
> +
> + if (cfg_aux->live && !memcmp(&cfg->bipolar, &cfg_aux->bipolar,
> + cmp_size))
> + return cfg_aux;
> + }
> + return NULL;
> +}
> +
> +static int ad7173_set_mode(struct ad_sigma_delta *sd,
> + enum ad_sigma_delta_mode mode)
> +{
> + struct ad7173_state *st = ad_sigma_delta_to_ad7173(sd);
> +
> + st->adc_mode &= ~AD7173_ADC_MODE_MODE_MASK;
> + st->adc_mode |= AD7173_ADC_MODE_MODE(mode);
Another one I'd find easier to read if it were just
FIELD_PREP(AD71...)
as then it would be obvious we masked the field out then replaced it.
It's fairly obvious anyway, but the macro doesn't make it more than
just calling FIELD_PREP directly.
> +
> + return ad_sd_write_reg(&st->sd, AD7173_REG_ADC_MODE, 2, st->adc_mode);
> +}
> +static int ad7173_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long info)
> +{
> + struct ad7173_state *st = iio_priv(indio_dev);
> + unsigned int reg;
> + int ret;
> +
> + switch (info) {
> + case IIO_CHAN_INFO_RAW:
> + ret = ad_sigma_delta_single_conversion(indio_dev, chan, val);
> + if (ret < 0)
> + return ret;
> +
> + /* disable channel after single conversion */
> + ret = ad_sd_write_reg(&st->sd, AD7173_REG_CH(chan->address), 2, 0);
> + if (ret < 0)
> + return ret;
> +
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + if (chan->type == IIO_TEMP) {
> + *val = 250000000;
> + *val2 = 800273203; /* (2^24 * 477) / 10 */
> + return IIO_VAL_FRACTIONAL;
> + } else {
> + *val = 2500;
> + if (chan->differential)
> + *val2 = 23;
> + else
> + *val2 = 24;
> + return IIO_VAL_FRACTIONAL_LOG2;
> + }
> + case IIO_CHAN_INFO_OFFSET:
> + if (chan->type == IIO_TEMP) {
> + *val = -874379;
> + } else {
> + if (chan->differential)
> + *val = (chan->scan_type.realbits < 32) ?
> + -(1 << (chan->scan_type.realbits - 1)) :
> + INT_MIN;
when is realbits >= 32? I'm not seeing such a case, so perhaps a comment here
if there is one.
> + else
> + *val = 0;
> + }
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + reg = st->channels[chan->address].cfg.odr;
> +
> + *val = st->info->sinc5_data_rates[reg] / MILLI;
> + *val2 = (st->info->sinc5_data_rates[reg] % MILLI) * MILLI;
* MICRO/MILLI
probably better expresses what is going on here as first bit ends up in
milli and you want it in micro. Maths the same though - just a slight
boost in readability I think.
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> + }
> + return -EINVAL;
> +}
> +
> +static int ad7173_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long info)
> +{
> + struct ad7173_state *st = iio_priv(indio_dev);
> + struct ad7173_channel_config *cfg;
> + unsigned int freq, i, reg;
> + int ret = 0;
> +
> + iio_device_claim_direct_mode(indio_dev);
> + if (iio_buffer_enabled(indio_dev)) {
Take a closer look at what iio_device_claim_direct_mode does and
check it's return value. For reference if the buffer is enabled
this is a double unlock of the mutex and a nice big error splat.
> + iio_device_release_direct_mode(indio_dev);
> + return -EBUSY;
> + }
> +
> + switch (info) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + freq = val * MILLI + val2 / MILLI;
> +
> + for (i = 0; i < st->info->num_sinc5_data_rates - 1; i++) {
> + if (freq >= st->info->sinc5_data_rates[i])
> + break;
> + }
> +
> + cfg = &st->channels[chan->address].cfg;
> + cfg->odr = i;
> +
> + if (cfg->live) {
> + ret = ad_sd_read_reg(&st->sd, AD7173_REG_FILTER(cfg->cfg_slot), 2, ®);
> + if (ret)
> + break;
> + reg &= ~AD7173_FILTER_ODR0_MASK;
> + reg |= AD7173_FILTER_ODR0(i);
As mentioned above, I think this is less clear than
reg &= ~AD7173_FILTER_ODR0_MASK;
reg |= FIELD_PREP(AD7173_FILTER_ODR0_MASK, i);
as you can then see it's all about the same file specified as a mask.
> + ret = ad_sd_write_reg(&st->sd, AD7173_REG_FILTER(cfg->cfg_slot), 2, reg);
Line is very long. If you can keep closer to 80 chars and below that's alway
preferred. Maybe flip the logic to exit early if nothing to do..
if(!cfg->live) /* Nothing to do yet */
break;
ret = ...
> + }
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + iio_device_release_direct_mode(indio_dev);
> + return ret;
> +}
> +static int ad7173_probe(struct spi_device *spi)
> +{
> + struct ad7173_state *st;
> + struct iio_dev *indio_dev;
> + struct device *dev = &spi->dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->info = device_get_match_data(dev);
> + if (!st->info)
> + return -ENODEV;
> +
> + indio_dev->name = spi_get_device_id(spi)->name;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &ad7173_info;
> +
> + spi->mode = SPI_MODE_3;
> +
> + ad7173_sigma_delta_info.num_slots = st->info->num_configs;
> + ret = ad_sd_init(&st->sd, indio_dev, spi, &ad7173_sigma_delta_info);
> + if (ret)
> + return ret;
> +
> + spi_set_drvdata(spi, indio_dev);
> +
> + ret = ad7173_fw_parse_channel_config(indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = devm_ad_sd_setup_buffer_and_trigger(dev, indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = ad7173_setup(indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret)
> + return ret;
> +
> + if (IS_ENABLED(CONFIG_GPIOLIB))
> + return ad7173_gpio_init(indio_dev);
As nothing to do with IIO part of driver, I'd just pass in st instead
so there is no implication that this is IIO related.
> + else
Either drop the else (one of the bots will moan about this otherwise)
or I'd go for as style more consistent with earlier code.
if (IS_ENABLED(COHNFIG_GPIOLIB)) {
ret = ad7173_gpio_init(st);
if (ret)
return ret;
}
return 0;
}
> + return 0;
> +}
> +
> +static const struct of_device_id ad7173_of_match[] = {
> + { .compatible = "adi,ad7172-2",
> + .data = &ad7173_device_info[ID_AD7172_2], },
> + { .compatible = "adi,ad7173-8",
> + .data = &ad7173_device_info[ID_AD7173_8], },
> + { .compatible = "adi,ad7175-2",
> + .data = &ad7173_device_info[ID_AD7175_2], },
> + { .compatible = "adi,ad7176-2",
> + .data = &ad7173_device_info[ID_AD7176_2], },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, ad7173_of_match);
> +
> +static const struct spi_device_id ad7173_id_table[] = {
> + { "ad7172-2", &ad7173_device_info[ID_AD7172_2], },
> + { "ad7173-8", &ad7173_device_info[ID_AD7173_8], },
> + { "ad7175-2", &ad7173_device_info[ID_AD7175_2], },
> + { "ad7176-2", &ad7173_device_info[ID_AD7176_2], },
Cast to kernel_ulong_t as per the build bot warning you got.
> + { },
trivial, but little point in a comma after an element acting as
a list terminator.
> +};
On 9/30/23 17:05, Jonathan Cameron wrote:
> On Thu, 28 Sep 2023 15:54:43 +0300
> Dumitru Ceclan <[email protected]> wrote>> +config AD7173
>> + tristate "Analog Devices AD7173 driver"
>> + depends on SPI_MASTER
>> + select AD_SIGMA_DELTA
>> + select GPIO_REGMAP
> If you are selecting it, why does it have if guards in the driver.
> I prefer the select here, so drop this if guards.
From what i checked, selecting GPIO_REGMAP does not select GPIOLIB but only REGMAP.
Also, in the thread from V1 Arnd Bergmann suggested:
" I think the best way to handle these is to remove both
the 'select' and the #ifdef in the driver and instead use
'if (IS_ENABLED(CONFIG_GPIOLIB))' to handle optional gpio
providers in the code. "
On Tue, Oct 03, 2023 at 01:33:36PM +0300, Ceclan Dumitru-Ioan wrote:
> On 9/30/23 17:05, Jonathan Cameron wrote:
> > On Thu, 28 Sep 2023 15:54:43 +0300
> > Dumitru Ceclan <[email protected]> wrote>> +config AD7173
> >> + tristate "Analog Devices AD7173 driver"
> >> + depends on SPI_MASTER
> >> + select AD_SIGMA_DELTA
> >> + select GPIO_REGMAP
> > If you are selecting it, why does it have if guards in the driver.
> > I prefer the select here, so drop this if guards.
>
> From what i checked, selecting GPIO_REGMAP does not select GPIOLIB but only REGMAP.
>
> Also, in the thread from V1 Arnd Bergmann suggested:
> " I think the best way to handle these is to remove both
> the 'select' and the #ifdef in the driver and instead use
> 'if (IS_ENABLED(CONFIG_GPIOLIB))' to handle optional gpio
> providers in the code. "
Why not simply to be dependent on GPIOLIB like other drivers do in this folder?
--
With Best Regards,
Andy Shevchenko
On 10/3/23 13:45, Andy Shevchenko wrote:
> Subject:
> Re: [PATCH v2 2/2] iio: adc: ad7173: add AD7173 driver
> From:
> Andy Shevchenko <[email protected]>
> Date:
> 10/3/23, 13:45
>
> To:
> Ceclan Dumitru-Ioan <[email protected]>
> CC:
> Jonathan Cameron <[email protected]>, [email protected], [email protected], [email protected], Lars-Peter Clausen <[email protected]>, Rob Herring <[email protected]>, Krzysztof Kozlowski <[email protected]>, Conor Dooley <[email protected]>, Michael Walle <[email protected]>, Arnd Bergmann <[email protected]>, ChiaEn Wu <[email protected]>, Niklas Schnelle <[email protected]>, Leonard Göhrs <[email protected]>, Mike Looijmans <[email protected]>, Haibo Chen <[email protected]>, Hugo Villeneuve <[email protected]>, Ceclan Dumitru <[email protected]>, [email protected], [email protected], [email protected]
>
>
> On Tue, Oct 03, 2023 at 01:33:36PM +0300, Ceclan Dumitru-Ioan wrote:
>> On 9/30/23 17:05, Jonathan Cameron wrote:
>>> On Thu, 28 Sep 2023 15:54:43 +0300
>>> Dumitru Ceclan <[email protected]> wrote>> +config AD7173
>>>> + tristate "Analog Devices AD7173 driver"
>>>> + depends on SPI_MASTER
>>>> + select AD_SIGMA_DELTA
>>>> + select GPIO_REGMAP
>>> If you are selecting it, why does it have if guards in the driver.
>>> I prefer the select here, so drop this if guards.
>> From what i checked, selecting GPIO_REGMAP does not select GPIOLIB but only REGMAP.
>>
>> Also, in the thread from V1 Arnd Bergmann suggested:
>> " I think the best way to handle these is to remove both
>> the 'select' and the #ifdef in the driver and instead use
>> 'if (IS_ENABLED(CONFIG_GPIOLIB))' to handle optional gpio
>> providers in the code. "
> Why not simply to be dependent on GPIOLIB like other drivers do in this folder?
I followed the suggestion given by Arnd. The full argument:
"From a Kconfig perspective, any user-visible symbol ideally only uses
'depends on', while hidden symbols usually use 'select'.
For the GPIOLIB symbol specifically, we have a mix of both, but the
overall usage is that gpio consumers only use 'depends on',
while some of the providers use 'select'. This risks causing build
breakage from a dependency loop when combined with other symbols
that have the same problem (e.g. I2C), but it tends to work out
as long as a strong hierarchy is kept. In particular, using 'select'
from an arch/*/Kconfig platform option is generally harmless as
long as those don't depend on anything else.
The new driver is a gpio provider and at least ad4130 and
ad5592r uses 'select' here, but then again ad74115 and
ad74113 use 'depends on' and ads7950 uses neither.
I think the best way to handle these is to remove both
the 'select' and the #ifdef in the driver and instead use
'if (IS_ENABLED(CONFIG_GPIOLIB))' to handle optional gpio
providers in the code."
I do not have a lot of experience with this subject.
As such, if you consider the argument invalid, mention it and i will
change to 'depends on'.
On Tue, Oct 3, 2023 at 1:57 PM Ceclan Dumitru-Ioan
<[email protected]> wrote:
> On 10/3/23 13:45, Andy Shevchenko wrote:
> > On Tue, Oct 03, 2023 at 01:33:36PM +0300, Ceclan Dumitru-Ioan wrote:
> >> On 9/30/23 17:05, Jonathan Cameron wrote:
> >>> On Thu, 28 Sep 2023 15:54:43 +0300
> >>> Dumitru Ceclan <[email protected]> wrote>> +config AD7173
...
> >>>> + select GPIO_REGMAP
> >>> If you are selecting it, why does it have if guards in the driver.
> >>> I prefer the select here, so drop this if guards.
> >> From what i checked, selecting GPIO_REGMAP does not select GPIOLIB but only REGMAP.
> >>
> >> Also, in the thread from V1 Arnd Bergmann suggested:
> >> " I think the best way to handle these is to remove both
> >> the 'select' and the #ifdef in the driver and instead use
> >> 'if (IS_ENABLED(CONFIG_GPIOLIB))' to handle optional gpio
> >> providers in the code. "
> > Why not simply to be dependent on GPIOLIB like other drivers do in this folder?
>
> I followed the suggestion given by Arnd. The full argument:
>
> "From a Kconfig perspective, any user-visible symbol ideally only uses
> 'depends on', while hidden symbols usually use 'select'.
>
> For the GPIOLIB symbol specifically, we have a mix of both, but the
> overall usage is that gpio consumers only use 'depends on',
> while some of the providers use 'select'. This risks causing build
> breakage from a dependency loop when combined with other symbols
> that have the same problem (e.g. I2C), but it tends to work out
> as long as a strong hierarchy is kept. In particular, using 'select'
> from an arch/*/Kconfig platform option is generally harmless as
> long as those don't depend on anything else.
>
> The new driver is a gpio provider and at least ad4130 and
> ad5592r uses 'select' here, but then again ad74115 and
> ad74113 use 'depends on' and ads7950 uses neither.
>
> I think the best way to handle these is to remove both
> the 'select' and the #ifdef in the driver and instead use
> 'if (IS_ENABLED(CONFIG_GPIOLIB))' to handle optional gpio
> providers in the code."
>
> I do not have a lot of experience with this subject.
> As such, if you consider the argument invalid, mention it and i will
> change to 'depends on'.
I see, I would ask GPIOLIB maintainers about this.
I don't know if there is any plan to fix this through the entire
kernel and which way has been chosen for that.
--
With Best Regards,
Andy Shevchenko
On Tue, Oct 3, 2023, at 13:02, Andy Shevchenko wrote:
> On Tue, Oct 3, 2023 at 1:57 PM Ceclan Dumitru-Ioan
> <[email protected]> wrote:
>> On 10/3/23 13:45, Andy Shevchenko wrote:
>> > On Tue, Oct 03, 2023 at 01:33:36PM +0300, Ceclan Dumitru-Ioan wrote:
>> >> On 9/30/23 17:05, Jonathan Cameron wrote:
>> >>>>
>> >>>> + select GPIO_REGMAP
>> >>> If you are selecting it, why does it have if guards in the driver.
>> >>> I prefer the select here, so drop this if guards.
>> >> From what i checked, selecting GPIO_REGMAP does not select GPIOLIB but only REGMAP.
I think the correct solution for this is to use
select GPIO_REGMAP if GPIOLIB
which matches what the driver does.
>> >> Also, in the thread from V1 Arnd Bergmann suggested:
>> >> " I think the best way to handle these is to remove both
>> >> the 'select' and the #ifdef in the driver and instead use
>> >> 'if (IS_ENABLED(CONFIG_GPIOLIB))' to handle optional gpio
>> >> providers in the code. "
>> > Why not simply to be dependent on GPIOLIB like other drivers do in this folder?
Some (possibly all) of the other drivers are gpiolib users that do not
function without gpiolib. This one is only a provider and not a consumer,
and the gpiolib functionality in it is optional, so I think there is
technically no dependency, even if in practice gpiolib is always there.
>> I followed the suggestion given by Arnd. The full argument:
>>
>> "From a Kconfig perspective, any user-visible symbol ideally only uses
>> 'depends on', while hidden symbols usually use 'select'.
>>
>> For the GPIOLIB symbol specifically, we have a mix of both, but the
>> overall usage is that gpio consumers only use 'depends on',
>> while some of the providers use 'select'. This risks causing build
>> breakage from a dependency loop when combined with other symbols
>> that have the same problem (e.g. I2C), but it tends to work out
>> as long as a strong hierarchy is kept. In particular, using 'select'
>> from an arch/*/Kconfig platform option is generally harmless as
>> long as those don't depend on anything else.
>>
>> The new driver is a gpio provider and at least ad4130 and
>> ad5592r uses 'select' here, but then again ad74115 and
>> ad74113 use 'depends on' and ads7950 uses neither.
>>
>> I think the best way to handle these is to remove both
>> the 'select' and the #ifdef in the driver and instead use
>> 'if (IS_ENABLED(CONFIG_GPIOLIB))' to handle optional gpio
>> providers in the code."
>>
>> I do not have a lot of experience with this subject.
>> As such, if you consider the argument invalid, mention it and i will
>> change to 'depends on'.
>
> I see, I would ask GPIOLIB maintainers about this.
> I don't know if there is any plan to fix this through the entire
> kernel and which way has been chosen for that.
I think the way we have handled it in the past is to not touch
existing drivers unless there is a problem with circular dependencies,
and then we change a bunch of 'select' to 'depends on' to make
it work again.
Changing all the wrong ones at once would be nice, but likely
cause introduce other dependency problems, but circular dependencies
and incorrect defaults in defconfig builds.
In the long run, we might decide to make gpiolib unconditional
all architectures, but for the moment that causes bloat on
small memory configurations like most m68k or armv7-m.
Arnd
Hi,
I've just had a look at the gpio-regmap part.
> +static int ad7173_mask_xlate(struct gpio_regmap *gpio, unsigned int
> base,
> + unsigned int offset, unsigned int *reg,
> + unsigned int *mask)
> +{
> + *mask = AD7173_GPO_DATA(offset);
> + *reg = AD7173_REG_GPIO;
*reg = base;
See also below.
> + return 0;
> +}
> +
> +static int ad7173_gpio_init(struct iio_dev *indio_dev)
> +{
> + struct ad7173_state *st = iio_priv(indio_dev);
> + struct gpio_regmap_config gpio_regmap = {};
> + struct device *dev = &st->sd.spi->dev;
> + unsigned int mask;
> +
> + st->regmap = devm_regmap_init_spi(st->sd.spi, &ad7173_regmap_config);
> + if (IS_ERR(st->regmap)) {
> + return dev_err_probe(dev, PTR_ERR(st->regmap),
> + "Unable to init regmap\n");
> + }
> +
> + mask = AD7173_GPIO_OP_EN0 | AD7173_GPIO_OP_EN1 |
> AD7173_GPIO_OP_EN2_3;
> + regmap_update_bits(st->regmap, AD7173_REG_GPIO, mask, mask);
> +
> + gpio_regmap.parent = dev;
> + gpio_regmap.regmap = st->regmap;
> + gpio_regmap.ngpio = st->info->num_gpios;
> + gpio_regmap.reg_set_base = GPIO_REGMAP_ADDR_ZERO;
Why don't you set it to AD7173_REG_GPIO? Register 0 seems wrong, it
looks like you're using that to say this is a output-only I/O.
> + gpio_regmap.reg_mask_xlate = ad7173_mask_xlate;
> +
> + st->gpio_regmap = devm_gpio_regmap_register(dev, &gpio_regmap);
> + if (IS_ERR(st->gpio_regmap)) {
> + return dev_err_probe(dev, PTR_ERR(st->gpio_regmap),
> + "Unable to init gpio-regmap\n");
> + }
> +
> + return 0;
> +}
> +
> +#endif /* CONFIG_GPIOLIB */
Otherwise looks good. But I've noticed that the chip can actually
also do input on gpio0 and gpio1. If you ever want to support that,
it seems like you need two gpio-regmaps.
-michael
Hi Dumitru,
kernel test robot noticed the following build warnings:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v6.6-rc4 next-20231004]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Dumitru-Ceclan/iio-adc-ad7173-add-AD7173-driver/20230928-205802
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20230928125443.615006-2-mitrutzceclan%40gmail.com
patch subject: [PATCH v2 2/2] iio: adc: ad7173: add AD7173 driver
config: i386-randconfig-063-20231005 (https://download.01.org/0day-ci/archive/20231005/[email protected]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231005/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
sparse warnings: (new ones prefixed by >>)
>> drivers/iio/adc/ad7173.c:829:42: sparse: sparse: incorrect type in initializer (different base types) @@ expected unsigned long [usertype] driver_data @@ got struct ad7173_device_info const * @@
drivers/iio/adc/ad7173.c:829:42: sparse: expected unsigned long [usertype] driver_data
drivers/iio/adc/ad7173.c:829:42: sparse: got struct ad7173_device_info const *
drivers/iio/adc/ad7173.c:830:42: sparse: sparse: incorrect type in initializer (different base types) @@ expected unsigned long [usertype] driver_data @@ got struct ad7173_device_info const * @@
drivers/iio/adc/ad7173.c:830:42: sparse: expected unsigned long [usertype] driver_data
drivers/iio/adc/ad7173.c:830:42: sparse: got struct ad7173_device_info const *
drivers/iio/adc/ad7173.c:831:42: sparse: sparse: incorrect type in initializer (different base types) @@ expected unsigned long [usertype] driver_data @@ got struct ad7173_device_info const * @@
drivers/iio/adc/ad7173.c:831:42: sparse: expected unsigned long [usertype] driver_data
drivers/iio/adc/ad7173.c:831:42: sparse: got struct ad7173_device_info const *
drivers/iio/adc/ad7173.c:832:42: sparse: sparse: incorrect type in initializer (different base types) @@ expected unsigned long [usertype] driver_data @@ got struct ad7173_device_info const * @@
drivers/iio/adc/ad7173.c:832:42: sparse: expected unsigned long [usertype] driver_data
drivers/iio/adc/ad7173.c:832:42: sparse: got struct ad7173_device_info const *
vim +829 drivers/iio/adc/ad7173.c
827
828 static const struct spi_device_id ad7173_id_table[] = {
> 829 { "ad7172-2", &ad7173_device_info[ID_AD7172_2], },
830 { "ad7173-8", &ad7173_device_info[ID_AD7173_8], },
831 { "ad7175-2", &ad7173_device_info[ID_AD7175_2], },
832 { "ad7176-2", &ad7173_device_info[ID_AD7176_2], },
833 { },
834 };
835 MODULE_DEVICE_TABLE(spi, ad7173_id_table);
836
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki