Re: [PATCH v2 7/9] iio: dac: ad3552r: extract common code (no changes in behavior intended)

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 05 Sep 2024 17:17:37 +0200
Angelo Dureghello <adureghello@baylibre.com> wrote:

> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> 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@baylibre.com>

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);
>  
> -	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@baylibre.com>

It's all code lifted from elsewhere, so keep the original 
2021 date as well.  2021-2024 would be fine.

> + */
...