Re: [PATCH v3 08/10] iio: dac: ad3552r: extract common code (no changes in behavior intended)

[Angelo Dureghello]

Hi Jonathan,

On 29.09.2024 12:57, Jonathan Cameron wrote:
> On Thu, 19 Sep 2024 11:20:04 +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>
> For these, main request is to move them to a namespace + GPL is
> probably the appropriate choice here.
> 
> 
> > ---
> >  drivers/iio/dac/Makefile         |   2 +-
> >  drivers/iio/dac/ad3552r-common.c | 173 +++++++++++++++++++++++
> >  drivers/iio/dac/ad3552r.c        | 293 ++++-----------------------------------
> >  drivers/iio/dac/ad3552r.h        | 190 +++++++++++++++++++++++++
> >  4 files changed, 390 insertions(+), 268 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..624f3f97cdea
> > --- /dev/null
> > +++ b/drivers/iio/dac/ad3552r-common.c
> > @@ -0,0 +1,173 @@
> > +// 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"
> > +
> > +const s32 ad3552r_ch_ranges[AD3552R_MAX_RANGES][2] = {
> > +	[AD3552R_CH_OUTPUT_RANGE_0__2P5V]	= { 0, 2500 },
> > +	[AD3552R_CH_OUTPUT_RANGE_0__5V]		= { 0, 5000 },
> > +	[AD3552R_CH_OUTPUT_RANGE_0__10V]	= { 0, 10000 },
> > +	[AD3552R_CH_OUTPUT_RANGE_NEG_5__5V]	= { -5000, 5000 },
> > +	[AD3552R_CH_OUTPUT_RANGE_NEG_10__10V]	= { -10000, 10000 }
> > +};
> > +EXPORT_SYMBOL(ad3552r_ch_ranges);
> 
> GPL and namespace them to avoid poluting the general namespace with driver
> specific exports.
> 
> EXPORT_SYMBOL_NS_GPL() etc.
> 
> 
> > +
> > +u16 ad3552r_calc_custom_gain(u8 p, u8 n, s16 goffs)
> > +{
> > +	u16 reg;
> > +
> > +	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);
> Hmm. Not sure the s32 case does anything useful here.
> Also this is a little messy from local view of code. It is not obvious
> that only BIT(0) can be set here.  I'd be tempted to mask that
> before passing to FIELD_PREP()
> 
> > +	reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_POLARITY, (s32)goffs < 0);
> 
> Why do you need the s32 cast for this last line?
> 
> > +
> > +	return reg;
> > +}
> > +
> > +int ad3552r_get_ref_voltage(struct device *dev)
> > +{
> > +	int voltage;
> > +	int delta = 100000;
> > +
> > +	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"))
> > +			return AD3552R_INTERNAL_VREF_PIN_2P5V;
> > +		else
> > +			return AD3552R_INTERNAL_VREF_PIN_FLOATING;
> > +	}
> > +
> > +	if (voltage > 2500000 + delta || voltage < 2500000 - delta) {
> > +		dev_warn(dev, "vref-supply must be 2.5V");
> > +		return -EINVAL;
> > +	}
> 
> Obviously this is legacy code, but why do we care in the driver?
> If someone has circuitry or configuration that is wrong, do we need to check
> that?  I guess it does little harm though.
> 
> > +
> > +	return AD3552R_EXTERNAL_VREF_PIN_INPUT;
> > +}
> > +
> > +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)
> > +		return err;
> > +
> > +	if (*val > 3) {
>


 
> Usually we avoid setting values passed back on error if it is easy to do so.
> I'd bounce via a local variable and only set *val = drive_strength
> after you know it is in range.
> 
> > +		dev_err(dev,
> > +			"adi,sdo-drive-strength must be less than 4\n");
> > +		return -EINVAL;
> Is dev_err_probe() appropriate here?  I haven't checked if this is called
> from non probe paths so ignore this comment if it is.
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int ad3552r_get_custom_gain(struct device *dev, struct fwnode_handle *child,
> > +			    u8 *gs_p, u8 *gs_n, u16 *rfb, s16 *goffs)
> > +{
> > +	int err;
> > +	u32 val;
> > +	struct fwnode_handle *gain_child __free(fwnode_handle) =
> > +				fwnode_get_named_child_node(child,
> One tab more than the line above is fine for cases like this and makes for
> more readable code.
>
Aligning with c then line comes to long. 
I can offer, as in other drivers:

int ad3552r_get_custom_gain(struct device *dev, struct fwnode_handle *child,
			    u8 *gs_p, u8 *gs_n, u16 *rfb, s16 *goffs)
{
	int err;
	u32 val;
	struct fwnode_handle *gain_child __free(fwnode_handle) =
		fwnode_get_named_child_node(child,
					    "custom-output-range-config");

Also, do you prefer 80 or 100 as eol limit ?

 
> > +				"custom-output-range-config");
> 
> Align this final parameter with c of child.
> 
> > +
> > +	if (!gain_child)
> > +		return dev_err_probe(dev, -EINVAL,
> > +				     "custom-output-range-config mandatory\n");
> > +
> > +	err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-p", &val);
> > +	if (err)
> > +		return dev_err_probe(dev, err,
> > +				     "adi,gain-scaling-p mandatory\n");
> > +	*gs_p = val;
> > +
> > +	err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-n", &val);
> > +	if (err)
> > +		return dev_err_probe(dev, err,
> > +				     "adi,gain-scaling-n property mandatory\n");
> > +	*gs_n = val;
> > +
> > +	err = fwnode_property_read_u32(gain_child, "adi,rfb-ohms", &val);
> > +	if (err)
> > +		return dev_err_probe(dev, err,
> > +				     "adi,rfb-ohms mandatory\n");
> > +	*rfb = val;
> > +
> > +	err = fwnode_property_read_u32(gain_child, "adi,gain-offset", &val);
> > +	if (err)
> > +		return dev_err_probe(dev, err,
> > +				     "adi,gain-offset mandatory\n");
> > +	*goffs = val;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ad3552r_find_range(u16 id, s32 *vals)
> > +{
> > +	int i, len;
> > +	const s32 (*ranges)[2];
> > +
> > +	if (id == AD3542R_ID) {
> 
> This is already in your model_data. Use that not another lookup via
> an ID enum.  The ID enum approach doesn't scale as we add more parts
> as it scatters device specific code through the driver.
>

This function is only used internally to this common part.
 
> 
> > +		len = ARRAY_SIZE(ad3542r_ch_ranges);
> > +		ranges = ad3542r_ch_ranges;
> > +	} else {
> > +		len = ARRAY_SIZE(ad3552r_ch_ranges);
> > +		ranges = ad3552r_ch_ranges;
> > +	}
> > +
> > +	for (i = 0; i < len; i++)
> > +		if (vals[0] == ranges[i][0] * 1000 &&
> > +		    vals[1] == ranges[i][1] * 1000)
> > +			return i;
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +int ad3552r_get_output_range(struct device *dev, enum ad3552r_id chip_id,
> > +			     struct fwnode_handle *child, u32 *val)
> As above, don't pass the enum. Either pass the model_data or pass the
> actual stuff you need which is the ranges array and size of that array.
>

Cannot pass model data, structures of the 2 drviers are different.
If i pass arrays, the logic of deciding what array (checking the id)
must be done in the drivers, but in this way, there will be less
common code.
 
> > +{
> > +	int ret;
> > +	s32 vals[2];
> > +
> > +	/* This property is optional, so returning -ENOENT if missing */
> > +	if (!fwnode_property_present(child, "adi,output-range-microvolt"))
> > +		return -ENOENT;
> > +
> > +	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;
> > +}
> 
> Thanks,
> 
> Jonathan
> 
> 

Thanks,

-- 
Regards,
  Angelo