On Thu, 05 Sep 2024 17:17:37 +0200 Angelo Dureghello <adureghello@xxxxxxxxxxxx> wrote: > From: Angelo Dureghello <adureghello@xxxxxxxxxxxx> > > Extracting common code, to share common code to be used later > by the AXI driver version (ad3552r-axi.c). > > Signed-off-by: Angelo Dureghello <adureghello@xxxxxxxxxxxx> Looks like a few arrays are now duplicated that probably shouldn't be. Otherwise, just trivial comments inline. Jonathan > --- > drivers/iio/dac/Makefile | 2 +- > drivers/iio/dac/ad3552r-common.c | 163 +++++++++++++++++++++++ > drivers/iio/dac/ad3552r.c | 276 ++++----------------------------------- > drivers/iio/dac/ad3552r.h | 199 ++++++++++++++++++++++++++++ > 4 files changed, 389 insertions(+), 251 deletions(-) > > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile > index 2cf148f16306..56a125f56284 100644 > --- a/drivers/iio/dac/Makefile > +++ b/drivers/iio/dac/Makefile > @@ -4,7 +4,7 @@ > # > > # When adding new entries keep the list in alphabetical order > -obj-$(CONFIG_AD3552R) += ad3552r.o > +obj-$(CONFIG_AD3552R) += ad3552r.o ad3552r-common.o > obj-$(CONFIG_AD5360) += ad5360.o > obj-$(CONFIG_AD5380) += ad5380.o > obj-$(CONFIG_AD5421) += ad5421.o > diff --git a/drivers/iio/dac/ad3552r-common.c b/drivers/iio/dac/ad3552r-common.c > new file mode 100644 > index 000000000000..c8ccfbe2e95e > --- /dev/null > +++ b/drivers/iio/dac/ad3552r-common.c > @@ -0,0 +1,163 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +// > +// Copyright (c) 2010-2024 Analog Devices Inc. > +// Copyright (c) 2024 Baylibre, SAS > + > +#include <linux/device.h> > +#include <linux/module.h> > +#include <linux/property.h> > +#include <linux/regulator/consumer.h> > + > +#include "ad3552r.h" > + > +static const s32 ad3552r_ch_ranges[][2] = { > + [AD3552R_CH_OUTPUT_RANGE_0__2P5V] = {0, 2500}, Please add spaces after { and before } given the code is moving anyway and that's my preferred style (I'm getting fussier about it over time :) > + [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} > +}; > + > +static const s32 ad3542r_ch_ranges[][2] = { > + [AD3542R_CH_OUTPUT_RANGE_0__2P5V] = {0, 2500}, > + [AD3542R_CH_OUTPUT_RANGE_0__3V] = {0, 3000}, > + [AD3542R_CH_OUTPUT_RANGE_0__5V] = {0, 5000}, > + [AD3542R_CH_OUTPUT_RANGE_0__10V] = {0, 10000}, > + [AD3542R_CH_OUTPUT_RANGE_NEG_2P5__7P5V] = {-2500, 7500}, > + [AD3542R_CH_OUTPUT_RANGE_NEG_5__5V] = {-5000, 5000} > +}; > + > +void ad3552r_calc_custom_gain(u8 p, u8 n, s16 goffs, u16 *reg) return a u16 instead of passing it in as a pointer. Fits better with how it is used I think. > +{ > + *reg = FIELD_PREP(AD3552R_MASK_CH_RANGE_OVERRIDE, 1); > + *reg |= FIELD_PREP(AD3552R_MASK_CH_GAIN_SCALING_P, p); > + *reg |= FIELD_PREP(AD3552R_MASK_CH_GAIN_SCALING_N, n); > + *reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_BIT_8, abs((s32)goffs) >> 8); > + *reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_POLARITY, (s32)goffs < 0); > +} > + > +int ad3552r_get_ref_voltage(struct device *dev, u32 *val) > +{ > + int voltage, delta = 100000; Trivial: Don't mix declarations with assignment and ones with out on same line. It's slightly trickier to read. This driver currently does this a lot but lets not introduce more cases. > + > + voltage = devm_regulator_get_enable_read_voltage(dev, "vref"); > + if (voltage < 0 && voltage != -ENODEV) > + return dev_err_probe(dev, voltage, > + "Error getting vref voltage\n"); > + > + if (voltage == -ENODEV) { > + if (device_property_read_bool(dev, "adi,vref-out-en")) > + *val = AD3552R_INTERNAL_VREF_PIN_2P5V; If val fits nicely into 31 bits and hence the positive values, I'd return it as then these all become return AD... > + else > + *val = AD3552R_INTERNAL_VREF_PIN_FLOATING; > + } else { and this else is then unnecessary. > + if (voltage > 2500000 + delta || voltage < 2500000 - delta) { > + dev_warn(dev, "vref-supply must be 2.5V"); > + return -EINVAL; > + } > + *val = AD3552R_EXTERNAL_VREF_PIN_INPUT; > + } > + > + return 0; > +} > + > +int ad3552r_get_drive_strength(struct device *dev, u32 *val) > +{ > + int err; > + > + err = device_property_read_u32(dev, "adi,sdo-drive-strength", val); > + if (!err && *val > 3) { > + dev_err(dev, > + "adi,sdo-drive-strength must be less than 4\n"); > + return -EINVAL; if (err) return err; if (*val > 3) { dev_err(dev, "adi,sdo-drive-strength must be less than 4\n"); return -EINVAL; } return 0; is a bit longer but keeps the error handling well separated from the good path. > + } > + > + return err; > +} > + > +int ad3552r_get_output_range(struct device *dev, enum ad3552r_id chip_id, > + struct fwnode_handle *child, u32 *val) > +{ > + int ret; > + s32 vals[2]; > + > + if (!fwnode_property_present(child, "adi,output-range-microvolt")) > + return -ENOENT; Maybe add a comment that this property is optional, so we want to distinguish this particular reason for failure. Took me a few mins to figure out why we needed this check. > + > + ret = fwnode_property_read_u32_array(child, > + "adi,output-range-microvolt", > + vals, 2); > + if (ret) > + return dev_err_probe(dev, ret, > + "invalid adi,output-range-microvolt\n"); > + > + ret = ad3552r_find_range(chip_id, vals); > + if (ret < 0) > + return dev_err_probe(dev, ret, > + "invalid adi,output-range-microvolt value\n"); > + > + *val = ret; > + > + return 0; > +} > diff --git a/drivers/iio/dac/ad3552r.c b/drivers/iio/dac/ad3552r.c > index d867de7c90d1..c149be9c8c7d 100644 > --- a/drivers/iio/dac/ad3552r.c > +++ b/drivers/iio/dac/ad3552r.c > @@ -11,153 +11,9 @@ > -enum ad3552r_ch_output_range { > - /* Range from 0 V to 2.5 V. Requires Rfb1x connection */ > - AD3552R_CH_OUTPUT_RANGE_0__2P5V, > - /* 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 -5 V to 5 V. Requires Rfb2x connection */ > - AD3552R_CH_OUTPUT_RANGE_NEG_5__5V, > - /* Range from -10 V to 10 V. Requires Rfb4x connection */ > - AD3552R_CH_OUTPUT_RANGE_NEG_10__10V, > -}; > +#include "ad3552r.h" > > static const s32 ad3552r_ch_ranges[][2] = { This exists now in ad3552-common.c as well as here which seems unlikely to be what you want. > [AD3552R_CH_OUTPUT_RANGE_0__2P5V] = {0, 2500}, > @@ -167,21 +23,6 @@ static const s32 ad3552r_ch_ranges[][2] = { > [AD3552R_CH_OUTPUT_RANGE_NEG_10__10V] = {-10000, 10000} > }; > > -enum ad3542r_ch_output_range { > - /* Range from 0 V to 2.5 V. Requires Rfb1x connection */ > - AD3542R_CH_OUTPUT_RANGE_0__2P5V, > - /* Range from 0 V to 3 V. Requires Rfb1x connection */ > - AD3542R_CH_OUTPUT_RANGE_0__3V, > - /* Range from 0 V to 5 V. Requires Rfb1x connection */ > - AD3542R_CH_OUTPUT_RANGE_0__5V, > - /* Range from 0 V to 10 V. Requires Rfb2x connection */ > - AD3542R_CH_OUTPUT_RANGE_0__10V, > - /* Range from -2.5 V to 7.5 V. Requires Rfb2x connection */ > - AD3542R_CH_OUTPUT_RANGE_NEG_2P5__7P5V, > - /* Range from -5 V to 5 V. Requires Rfb2x connection */ > - AD3542R_CH_OUTPUT_RANGE_NEG_5__5V, > -}; > - > static const s32 ad3542r_ch_ranges[][2] = { Isn't this in the new location above? > [AD3542R_CH_OUTPUT_RANGE_0__2P5V] = {0, 2500}, > [AD3542R_CH_OUTPUT_RANGE_0__3V] = {0, 3000}, > @@ -733,72 +574,32 @@ static void ad3552r_calc_gain_and_offset(struct ad3552r_desc *dac, s32 ch) > dac->ch_data[ch].offset_dec = div_s64(tmp, span); > } > > static int ad3552r_configure_custom_gain(struct ad3552r_desc *dac, > struct fwnode_handle *child, > u32 ch) > { > struct device *dev = &dac->spi->dev; > - u32 val; > int err; > u8 addr; > - u16 reg = 0, offset; > - > - struct fwnode_handle *gain_child __free(fwnode_handle) > - = fwnode_get_named_child_node(child, > - "custom-output-range-config"); > - if (!gain_child) > - return dev_err_probe(dev, -EINVAL, > - "mandatory custom-output-range-config property missing\n"); > - > - dac->ch_data[ch].range_override = 1; > - reg |= FIELD_PREP(AD3552R_MASK_CH_RANGE_OVERRIDE, 1); > - > - err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-p", &val); > - if (err) > - return dev_err_probe(dev, err, > - "mandatory adi,gain-scaling-p property missing\n"); > - reg |= FIELD_PREP(AD3552R_MASK_CH_GAIN_SCALING_P, val); > - dac->ch_data[ch].p = val; > + u16 reg = 0; > > - err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-n", &val); > + err = ad3552r_get_custom_gain(dev, child, > + &dac->ch_data[ch].p, > + &dac->ch_data[ch].n, > + &dac->ch_data[ch].rfb, > + &dac->ch_data[ch].gain_offset); > if (err) > - return dev_err_probe(dev, err, > - "mandatory adi,gain-scaling-n property missing\n"); > - reg |= FIELD_PREP(AD3552R_MASK_CH_GAIN_SCALING_N, val); > - dac->ch_data[ch].n = val; > - > - err = fwnode_property_read_u32(gain_child, "adi,rfb-ohms", &val); > - if (err) > - return dev_err_probe(dev, err, > - "mandatory adi,rfb-ohms property missing\n"); > - dac->ch_data[ch].rfb = val; > + return err; > > - err = fwnode_property_read_u32(gain_child, "adi,gain-offset", &val); > - if (err) > - return dev_err_probe(dev, err, > - "mandatory adi,gain-offset property missing\n"); > - dac->ch_data[ch].gain_offset = val; > + dac->ch_data[ch].range_override = 1; > > - offset = abs((s32)val); > - reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_BIT_8, (offset >> 8)); > + ad3552r_calc_custom_gain(dac->ch_data[ch].p, dac->ch_data[ch].n, > + dac->ch_data[ch].gain_offset, ®); > > - reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_POLARITY, (s32)val < 0); > addr = AD3552R_REG_ADDR_CH_GAIN(ch); > err = ad3552r_write_reg(dac, addr, Given you now have the calculation of reg nice and compact, I'd move that after this call. It's odd to calculate a value first then not use it whilst doing some other stuff. > - offset & AD3552R_MASK_CH_OFFSET_BITS_0_7); > + abs((s32)dac->ch_data[ch].gain_offset) & > + AD3552R_MASK_CH_OFFSET_BITS_0_7); > if (err) > return dev_err_probe(dev, err, "Error writing register\n"); > > @@ -812,30 +613,17 @@ static int ad3552r_configure_custom_gain(struct ad3552r_desc *dac, > static int ad3552r_configure_device(struct ad3552r_desc *dac) > { > struct device *dev = &dac->spi->dev; > - int err, cnt = 0, voltage, delta = 100000; > - u32 vals[2], val, ch; > + int err, cnt = 0; > + u32 val, ch; > > dac->gpio_ldac = devm_gpiod_get_optional(dev, "ldac", GPIOD_OUT_HIGH); > if (IS_ERR(dac->gpio_ldac)) > return dev_err_probe(dev, PTR_ERR(dac->gpio_ldac), > "Error getting gpio ldac"); > > - voltage = devm_regulator_get_enable_read_voltage(dev, "vref"); > - if (voltage < 0 && voltage != -ENODEV) > - return dev_err_probe(dev, voltage, "Error getting vref voltage\n"); > - > - if (voltage == -ENODEV) { > - if (device_property_read_bool(dev, "adi,vref-out-en")) > - val = AD3552R_INTERNAL_VREF_PIN_2P5V; > - else > - val = AD3552R_INTERNAL_VREF_PIN_FLOATING; > - } else { > - if (voltage > 2500000 + delta || voltage < 2500000 - delta) { > - dev_warn(dev, "vref-supply must be 2.5V"); > - return -EINVAL; > - } > - val = AD3552R_EXTERNAL_VREF_PIN_INPUT; > - } > + err = ad3552r_get_ref_voltage(dev, &val); > + if (err) > + return err; With suggestion above, err = ad3552_get_ref_voltage(dev); if (err) return err; val = err; > > err = ad3552r_update_reg_field(dac, > AD3552R_REG_ADDR_SH_REFERENCE_CONFIG, ... > diff --git a/drivers/iio/dac/ad3552r.h b/drivers/iio/dac/ad3552r.h > new file mode 100644 > index 000000000000..cada1f12f000 > --- /dev/null > +++ b/drivers/iio/dac/ad3552r.h > @@ -0,0 +1,199 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * AD3552R Digital <-> Analog converters common header > + * > + * Copyright 2024 Analog Devices Inc. > + * Author: Angelo Dureghello <adureghello@xxxxxxxxxxxx> It's all code lifted from elsewhere, so keep the original 2021 date as well. 2021-2024 would be fine. > + */ ...