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