From: Angelo Dureghello <adureghello@baylibre.com>
Changes to use FIELD_PREP, so that driver-specific ad3552r_field_prep
is removed. Variables (arrays) that was used to call ad3552r_field_prep
are removed too.
Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
drivers/iio/dac/ad3552r.c | 166 ++++++++++++++--------------------------------
1 file changed, 49 insertions(+), 117 deletions(-)
diff --git a/drivers/iio/dac/ad3552r.c b/drivers/iio/dac/ad3552r.c
index 7d61b2fe6624..6fb06e2e3193 100644
--- a/drivers/iio/dac/ad3552r.c
+++ b/drivers/iio/dac/ad3552r.c
@@ -210,46 +210,6 @@ static const s32 gains_scaling_table[] = {
[AD3552R_CH_GAIN_SCALING_0_125] = 125
};
-enum ad3552r_dev_attributes {
- /* - Direct register values */
- /* From 0-3 */
- AD3552R_SDO_DRIVE_STRENGTH,
- /*
- * 0 -> Internal Vref, vref_io pin floating (default)
- * 1 -> Internal Vref, vref_io driven by internal vref
- * 2 or 3 -> External Vref
- */
- AD3552R_VREF_SELECT,
- /* Read registers in ascending order if set. Else descending */
- AD3552R_ADDR_ASCENSION,
-};
-
-enum ad3552r_ch_attributes {
- /* DAC powerdown */
- AD3552R_CH_DAC_POWERDOWN,
- /* DAC amplifier powerdown */
- AD3552R_CH_AMPLIFIER_POWERDOWN,
- /* Select the output range. Select from enum ad3552r_ch_output_range */
- AD3552R_CH_OUTPUT_RANGE_SEL,
- /*
- * Over-rider the range selector in order to manually set the output
- * voltage range
- */
- AD3552R_CH_RANGE_OVERRIDE,
- /* Manually set the offset voltage */
- AD3552R_CH_GAIN_OFFSET,
- /* Sets the polarity of the offset. */
- AD3552R_CH_GAIN_OFFSET_POLARITY,
- /* PDAC gain scaling */
- AD3552R_CH_GAIN_SCALING_P,
- /* NDAC gain scaling */
- AD3552R_CH_GAIN_SCALING_N,
- /* Rfb value */
- AD3552R_CH_RFB,
- /* Channel select. When set allow Input -> DAC and Mask -> DAC */
- AD3552R_CH_SELECT,
-};
-
struct ad3552r_ch_data {
s32 scale_int;
s32 scale_dec;
@@ -285,45 +245,6 @@ struct ad3552r_desc {
unsigned int num_ch;
};
-static const u16 addr_mask_map[][2] = {
- [AD3552R_ADDR_ASCENSION] = {
- AD3552R_REG_ADDR_INTERFACE_CONFIG_A,
- AD3552R_MASK_ADDR_ASCENSION
- },
- [AD3552R_SDO_DRIVE_STRENGTH] = {
- AD3552R_REG_ADDR_INTERFACE_CONFIG_D,
- AD3552R_MASK_SDO_DRIVE_STRENGTH
- },
- [AD3552R_VREF_SELECT] = {
- AD3552R_REG_ADDR_SH_REFERENCE_CONFIG,
- AD3552R_MASK_REFERENCE_VOLTAGE_SEL
- },
-};
-
-/* 0 -> reg addr, 1->ch0 mask, 2->ch1 mask */
-static const u16 addr_mask_map_ch[][3] = {
- [AD3552R_CH_DAC_POWERDOWN] = {
- AD3552R_REG_ADDR_POWERDOWN_CONFIG,
- AD3552R_MASK_CH_DAC_POWERDOWN(0),
- AD3552R_MASK_CH_DAC_POWERDOWN(1)
- },
- [AD3552R_CH_AMPLIFIER_POWERDOWN] = {
- AD3552R_REG_ADDR_POWERDOWN_CONFIG,
- AD3552R_MASK_CH_AMPLIFIER_POWERDOWN(0),
- AD3552R_MASK_CH_AMPLIFIER_POWERDOWN(1)
- },
- [AD3552R_CH_OUTPUT_RANGE_SEL] = {
- AD3552R_REG_ADDR_CH0_CH1_OUTPUT_RANGE,
- AD3552R_MASK_CH_OUTPUT_RANGE_SEL(0),
- AD3552R_MASK_CH_OUTPUT_RANGE_SEL(1)
- },
- [AD3552R_CH_SELECT] = {
- AD3552R_REG_ADDR_CH_SELECT_16B,
- AD3552R_MASK_CH(0),
- AD3552R_MASK_CH(1)
- }
-};
-
static u8 _ad3552r_reg_len(u8 addr)
{
switch (addr) {
@@ -399,11 +320,6 @@ static int ad3552r_read_reg(struct ad3552r_desc *dac, u8 addr, u16 *val)
return 0;
}
-static u16 ad3552r_field_prep(u16 val, u16 mask)
-{
- return (val << __ffs(mask)) & mask;
-}
-
/* Update field of a register, shift val if needed */
static int ad3552r_update_reg_field(struct ad3552r_desc *dac, u8 addr, u16 mask,
u16 val)
@@ -416,21 +332,11 @@ static int ad3552r_update_reg_field(struct ad3552r_desc *dac, u8 addr, u16 mask,
return ret;
reg &= ~mask;
- reg |= ad3552r_field_prep(val, mask);
+ reg |= val;
return ad3552r_write_reg(dac, addr, reg);
}
-static int ad3552r_set_ch_value(struct ad3552r_desc *dac,
- enum ad3552r_ch_attributes attr,
- u8 ch,
- u16 val)
-{
- /* Update register related to attributes in chip */
- return ad3552r_update_reg_field(dac, addr_mask_map_ch[attr][0],
- addr_mask_map_ch[attr][ch + 1], val);
-}
-
#define AD3552R_CH_DAC(_idx) ((struct iio_chan_spec) { \
.type = IIO_VOLTAGE, \
.output = true, \
@@ -510,8 +416,14 @@ static int ad3552r_write_raw(struct iio_dev *indio_dev,
val);
break;
case IIO_CHAN_INFO_ENABLE:
- err = ad3552r_set_ch_value(dac, AD3552R_CH_DAC_POWERDOWN,
- chan->channel, !val);
+ if (chan->channel == 0)
+ val = FIELD_PREP(AD3552R_MASK_CH_DAC_POWERDOWN(0), !val);
+ else
+ val = FIELD_PREP(AD3552R_MASK_CH_DAC_POWERDOWN(1), !val);
+
+ err = ad3552r_update_reg_field(dac, AD3552R_REG_ADDR_POWERDOWN_CONFIG,
+ AD3552R_MASK_CH_DAC_POWERDOWN(chan->channel),
+ val);
break;
default:
err = -EINVAL;
@@ -715,9 +627,9 @@ static int ad3552r_reset(struct ad3552r_desc *dac)
}
return ad3552r_update_reg_field(dac,
- addr_mask_map[AD3552R_ADDR_ASCENSION][0],
- addr_mask_map[AD3552R_ADDR_ASCENSION][1],
- val);
+ AD3552R_REG_ADDR_INTERFACE_CONFIG_A,
+ AD3552R_MASK_ADDR_ASCENSION,
+ FIELD_PREP(AD3552R_MASK_ADDR_ASCENSION, val));
}
static void ad3552r_get_custom_range(struct ad3552r_desc *dac, s32 i, s32 *v_min,
@@ -812,20 +724,20 @@ static int ad3552r_configure_custom_gain(struct ad3552r_desc *dac,
"mandatory custom-output-range-config property missing\n");
dac->ch_data[ch].range_override = 1;
- reg |= ad3552r_field_prep(1, AD3552R_MASK_CH_RANGE_OVERRIDE);
+ 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 |= ad3552r_field_prep(val, AD3552R_MASK_CH_GAIN_SCALING_P);
+ reg |= FIELD_PREP(AD3552R_MASK_CH_GAIN_SCALING_P, val);
dac->ch_data[ch].p = val;
err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-n", &val);
if (err)
return dev_err_probe(dev, err,
"mandatory adi,gain-scaling-n property missing\n");
- reg |= ad3552r_field_prep(val, AD3552R_MASK_CH_GAIN_SCALING_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);
@@ -841,9 +753,9 @@ static int ad3552r_configure_custom_gain(struct ad3552r_desc *dac,
dac->ch_data[ch].gain_offset = val;
offset = abs((s32)val);
- reg |= ad3552r_field_prep((offset >> 8), AD3552R_MASK_CH_OFFSET_BIT_8);
+ reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_BIT_8, (offset >> 8));
- reg |= ad3552r_field_prep((s32)val < 0, AD3552R_MASK_CH_OFFSET_POLARITY);
+ reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_POLARITY, (s32)val < 0);
addr = AD3552R_REG_ADDR_CH_GAIN(ch);
err = ad3552r_write_reg(dac, addr,
offset & AD3552R_MASK_CH_OFFSET_BITS_0_7);
@@ -886,9 +798,9 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
}
err = ad3552r_update_reg_field(dac,
- addr_mask_map[AD3552R_VREF_SELECT][0],
- addr_mask_map[AD3552R_VREF_SELECT][1],
- val);
+ AD3552R_REG_ADDR_SH_REFERENCE_CONFIG,
+ AD3552R_MASK_REFERENCE_VOLTAGE_SEL,
+ FIELD_PREP(AD3552R_MASK_REFERENCE_VOLTAGE_SEL, val));
if (err)
return err;
@@ -900,9 +812,9 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
}
err = ad3552r_update_reg_field(dac,
- addr_mask_map[AD3552R_SDO_DRIVE_STRENGTH][0],
- addr_mask_map[AD3552R_SDO_DRIVE_STRENGTH][1],
- val);
+ AD3552R_REG_ADDR_INTERFACE_CONFIG_D,
+ AD3552R_MASK_SDO_DRIVE_STRENGTH,
+ FIELD_PREP(AD3552R_MASK_SDO_DRIVE_STRENGTH, val));
if (err)
return err;
}
@@ -938,9 +850,15 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
"Invalid adi,output-range-microvolt value\n");
val = err;
- err = ad3552r_set_ch_value(dac,
- AD3552R_CH_OUTPUT_RANGE_SEL,
- ch, val);
+ if (ch == 0)
+ val = FIELD_PREP(AD3552R_MASK_CH_OUTPUT_RANGE_SEL(0), val);
+ else
+ val = FIELD_PREP(AD3552R_MASK_CH_OUTPUT_RANGE_SEL(1), val);
+
+ err = ad3552r_update_reg_field(dac,
+ AD3552R_REG_ADDR_CH0_CH1_OUTPUT_RANGE,
+ AD3552R_MASK_CH_OUTPUT_RANGE_SEL(ch),
+ val);
if (err)
return err;
@@ -958,7 +876,14 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
ad3552r_calc_gain_and_offset(dac, ch);
dac->enabled_ch |= BIT(ch);
- err = ad3552r_set_ch_value(dac, AD3552R_CH_SELECT, ch, 1);
+ if (ch == 0)
+ val = FIELD_PREP(AD3552R_MASK_CH(0), 1);
+ else
+ val = FIELD_PREP(AD3552R_MASK_CH(1), 1);
+
+ err = ad3552r_update_reg_field(dac,
+ AD3552R_REG_ADDR_CH_SELECT_16B,
+ AD3552R_MASK_CH(ch), val);
if (err < 0)
return err;
@@ -970,8 +895,15 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
/* Disable unused channels */
for_each_clear_bit(ch, &dac->enabled_ch,
dac->model_data->num_hw_channels) {
- err = ad3552r_set_ch_value(dac, AD3552R_CH_AMPLIFIER_POWERDOWN,
- ch, 1);
+ if (ch == 0)
+ val = FIELD_PREP(AD3552R_MASK_CH_OUTPUT_RANGE_SEL(0), 1);
+ else
+ val = FIELD_PREP(AD3552R_MASK_CH_OUTPUT_RANGE_SEL(1), 1);
+
+ err = ad3552r_update_reg_field(dac,
+ AD3552R_REG_ADDR_POWERDOWN_CONFIG,
+ AD3552R_MASK_CH_OUTPUT_RANGE_SEL(ch),
+ val);
if (err)
return err;
}
--
2.45.0.rc1On 10/14/24 5:08 AM, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@baylibre.com>
>
> Changes to use FIELD_PREP, so that driver-specific ad3552r_field_prep
> is removed. Variables (arrays) that was used to call ad3552r_field_prep
> are removed too.
>
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
Found one likely bug. The rest are suggestions to keep the static
analyzers happy.
\
> @@ -510,8 +416,14 @@ static int ad3552r_write_raw(struct iio_dev *indio_dev,
> val);
> break;
> case IIO_CHAN_INFO_ENABLE:
> - err = ad3552r_set_ch_value(dac, AD3552R_CH_DAC_POWERDOWN,
> - chan->channel, !val);
> + if (chan->channel == 0)
> + val = FIELD_PREP(AD3552R_MASK_CH_DAC_POWERDOWN(0), !val);
> + else
> + val = FIELD_PREP(AD3552R_MASK_CH_DAC_POWERDOWN(1), !val);
In the past, I've had bots (Sparse, IIRC) complain about using !val
with FIELD_PREP. Alternative is to write it as val ? 1 : 0.
> +
> + err = ad3552r_update_reg_field(dac, AD3552R_REG_ADDR_POWERDOWN_CONFIG,
> + AD3552R_MASK_CH_DAC_POWERDOWN(chan->channel),
> + val);
> break;
> default:
> err = -EINVAL;
> @@ -715,9 +627,9 @@ static int ad3552r_reset(struct ad3552r_desc *dac)
> }
>
> return ad3552r_update_reg_field(dac,
> - addr_mask_map[AD3552R_ADDR_ASCENSION][0],
> - addr_mask_map[AD3552R_ADDR_ASCENSION][1],
> - val);
> + AD3552R_REG_ADDR_INTERFACE_CONFIG_A,
> + AD3552R_MASK_ADDR_ASCENSION,
> + FIELD_PREP(AD3552R_MASK_ADDR_ASCENSION, val));
> }
>
> static void ad3552r_get_custom_range(struct ad3552r_desc *dac, s32 i, s32 *v_min,
> @@ -812,20 +724,20 @@ static int ad3552r_configure_custom_gain(struct ad3552r_desc *dac,
> "mandatory custom-output-range-config property missing\n");
>
> dac->ch_data[ch].range_override = 1;
> - reg |= ad3552r_field_prep(1, AD3552R_MASK_CH_RANGE_OVERRIDE);
> + 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 |= ad3552r_field_prep(val, AD3552R_MASK_CH_GAIN_SCALING_P);
> + reg |= FIELD_PREP(AD3552R_MASK_CH_GAIN_SCALING_P, val);
> dac->ch_data[ch].p = val;
>
> err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-n", &val);
> if (err)
> return dev_err_probe(dev, err,
> "mandatory adi,gain-scaling-n property missing\n");
> - reg |= ad3552r_field_prep(val, AD3552R_MASK_CH_GAIN_SCALING_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);
> @@ -841,9 +753,9 @@ static int ad3552r_configure_custom_gain(struct ad3552r_desc *dac,
> dac->ch_data[ch].gain_offset = val;
>
> offset = abs((s32)val);
> - reg |= ad3552r_field_prep((offset >> 8), AD3552R_MASK_CH_OFFSET_BIT_8);
> + reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_BIT_8, (offset >> 8));
Can drop () from (offset >> 8).
>
> - reg |= ad3552r_field_prep((s32)val < 0, AD3552R_MASK_CH_OFFSET_POLARITY);
> + reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_POLARITY, (s32)val < 0);
Instead of (s32) cast, could write val < 0 : 1 : 0 (to be consistent with
suggestion above for replacing !val).
> addr = AD3552R_REG_ADDR_CH_GAIN(ch);
> err = ad3552r_write_reg(dac, addr,
> offset & AD3552R_MASK_CH_OFFSET_BITS_0_7);
> @@ -886,9 +798,9 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
> }
>
> err = ad3552r_update_reg_field(dac,
> - addr_mask_map[AD3552R_VREF_SELECT][0],
> - addr_mask_map[AD3552R_VREF_SELECT][1],
> - val);
> + AD3552R_REG_ADDR_SH_REFERENCE_CONFIG,
> + AD3552R_MASK_REFERENCE_VOLTAGE_SEL,
> + FIELD_PREP(AD3552R_MASK_REFERENCE_VOLTAGE_SEL, val));
> if (err)
> return err;
>
> @@ -900,9 +812,9 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
> }
>
> err = ad3552r_update_reg_field(dac,
> - addr_mask_map[AD3552R_SDO_DRIVE_STRENGTH][0],
> - addr_mask_map[AD3552R_SDO_DRIVE_STRENGTH][1],
> - val);
> + AD3552R_REG_ADDR_INTERFACE_CONFIG_D,
> + AD3552R_MASK_SDO_DRIVE_STRENGTH,
> + FIELD_PREP(AD3552R_MASK_SDO_DRIVE_STRENGTH, val));
> if (err)
> return err;
> }
> @@ -938,9 +850,15 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
> "Invalid adi,output-range-microvolt value\n");
>
> val = err;
> - err = ad3552r_set_ch_value(dac,
> - AD3552R_CH_OUTPUT_RANGE_SEL,
> - ch, val);
> + if (ch == 0)
> + val = FIELD_PREP(AD3552R_MASK_CH_OUTPUT_RANGE_SEL(0), val);
> + else
> + val = FIELD_PREP(AD3552R_MASK_CH_OUTPUT_RANGE_SEL(1), val);
> +
> + err = ad3552r_update_reg_field(dac,
> + AD3552R_REG_ADDR_CH0_CH1_OUTPUT_RANGE,
> + AD3552R_MASK_CH_OUTPUT_RANGE_SEL(ch),
> + val);
> if (err)
> return err;
>
> @@ -958,7 +876,14 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
> ad3552r_calc_gain_and_offset(dac, ch);
> dac->enabled_ch |= BIT(ch);
>
> - err = ad3552r_set_ch_value(dac, AD3552R_CH_SELECT, ch, 1);
> + if (ch == 0)
> + val = FIELD_PREP(AD3552R_MASK_CH(0), 1);
> + else
> + val = FIELD_PREP(AD3552R_MASK_CH(1), 1);
> +
> + err = ad3552r_update_reg_field(dac,
> + AD3552R_REG_ADDR_CH_SELECT_16B,
> + AD3552R_MASK_CH(ch), val);
> if (err < 0)
> return err;
>
> @@ -970,8 +895,15 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
> /* Disable unused channels */
> for_each_clear_bit(ch, &dac->enabled_ch,
> dac->model_data->num_hw_channels) {
> - err = ad3552r_set_ch_value(dac, AD3552R_CH_AMPLIFIER_POWERDOWN,
> - ch, 1);
> + if (ch == 0)
> + val = FIELD_PREP(AD3552R_MASK_CH_OUTPUT_RANGE_SEL(0), 1);
> + else
> + val = FIELD_PREP(AD3552R_MASK_CH_OUTPUT_RANGE_SEL(1), 1);
Should these be AD3552R_MASK_CH_AMPLIFIER_POWERDOWN instead of
AD3552R_MASK_CH_OUTPUT_RANGE_SEL? (2 above and 1 below.)
> +
> + err = ad3552r_update_reg_field(dac,
> + AD3552R_REG_ADDR_POWERDOWN_CONFIG,
> + AD3552R_MASK_CH_OUTPUT_RANGE_SEL(ch),
> + val);
> if (err)
> return err;
> }
>
On 14.10.2024 16:14, David Lechner wrote:
> On 10/14/24 5:08 AM, Angelo Dureghello wrote:
> > From: Angelo Dureghello <adureghello@baylibre.com>
> >
> > Changes to use FIELD_PREP, so that driver-specific ad3552r_field_prep
> > is removed. Variables (arrays) that was used to call ad3552r_field_prep
> > are removed too.
> >
> > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > ---
>
> Found one likely bug. The rest are suggestions to keep the static
> analyzers happy.
>
> \
> > @@ -510,8 +416,14 @@ static int ad3552r_write_raw(struct iio_dev *indio_dev,
> > val);
> > break;
> > case IIO_CHAN_INFO_ENABLE:
> > - err = ad3552r_set_ch_value(dac, AD3552R_CH_DAC_POWERDOWN,
> > - chan->channel, !val);
> > + if (chan->channel == 0)
> > + val = FIELD_PREP(AD3552R_MASK_CH_DAC_POWERDOWN(0), !val);
> > + else
> > + val = FIELD_PREP(AD3552R_MASK_CH_DAC_POWERDOWN(1), !val);
>
> In the past, I've had bots (Sparse, IIRC) complain about using !val
> with FIELD_PREP. Alternative is to write it as val ? 1 : 0.
>
> > +
> > + err = ad3552r_update_reg_field(dac, AD3552R_REG_ADDR_POWERDOWN_CONFIG,
> > + AD3552R_MASK_CH_DAC_POWERDOWN(chan->channel),
> > + val);
> > break;
> > default:
> > err = -EINVAL;
> > @@ -715,9 +627,9 @@ static int ad3552r_reset(struct ad3552r_desc *dac)
> > }
> >
> > return ad3552r_update_reg_field(dac,
> > - addr_mask_map[AD3552R_ADDR_ASCENSION][0],
> > - addr_mask_map[AD3552R_ADDR_ASCENSION][1],
> > - val);
> > + AD3552R_REG_ADDR_INTERFACE_CONFIG_A,
> > + AD3552R_MASK_ADDR_ASCENSION,
> > + FIELD_PREP(AD3552R_MASK_ADDR_ASCENSION, val));
> > }
> >
> > static void ad3552r_get_custom_range(struct ad3552r_desc *dac, s32 i, s32 *v_min,
> > @@ -812,20 +724,20 @@ static int ad3552r_configure_custom_gain(struct ad3552r_desc *dac,
> > "mandatory custom-output-range-config property missing\n");
> >
> > dac->ch_data[ch].range_override = 1;
> > - reg |= ad3552r_field_prep(1, AD3552R_MASK_CH_RANGE_OVERRIDE);
> > + 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 |= ad3552r_field_prep(val, AD3552R_MASK_CH_GAIN_SCALING_P);
> > + reg |= FIELD_PREP(AD3552R_MASK_CH_GAIN_SCALING_P, val);
> > dac->ch_data[ch].p = val;
> >
> > err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-n", &val);
> > if (err)
> > return dev_err_probe(dev, err,
> > "mandatory adi,gain-scaling-n property missing\n");
> > - reg |= ad3552r_field_prep(val, AD3552R_MASK_CH_GAIN_SCALING_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);
> > @@ -841,9 +753,9 @@ static int ad3552r_configure_custom_gain(struct ad3552r_desc *dac,
> > dac->ch_data[ch].gain_offset = val;
> >
> > offset = abs((s32)val);
> > - reg |= ad3552r_field_prep((offset >> 8), AD3552R_MASK_CH_OFFSET_BIT_8);
> > + reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_BIT_8, (offset >> 8));
>
> Can drop () from (offset >> 8).
>
> >
> > - reg |= ad3552r_field_prep((s32)val < 0, AD3552R_MASK_CH_OFFSET_POLARITY);
> > + reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_POLARITY, (s32)val < 0);
>
> Instead of (s32) cast, could write val < 0 : 1 : 0 (to be consistent with
> suggestion above for replacing !val).
>
> > addr = AD3552R_REG_ADDR_CH_GAIN(ch);
> > err = ad3552r_write_reg(dac, addr,
> > offset & AD3552R_MASK_CH_OFFSET_BITS_0_7);
> > @@ -886,9 +798,9 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
> > }
> >
> > err = ad3552r_update_reg_field(dac,
> > - addr_mask_map[AD3552R_VREF_SELECT][0],
> > - addr_mask_map[AD3552R_VREF_SELECT][1],
> > - val);
> > + AD3552R_REG_ADDR_SH_REFERENCE_CONFIG,
> > + AD3552R_MASK_REFERENCE_VOLTAGE_SEL,
> > + FIELD_PREP(AD3552R_MASK_REFERENCE_VOLTAGE_SEL, val));
> > if (err)
> > return err;
> >
> > @@ -900,9 +812,9 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
> > }
> >
> > err = ad3552r_update_reg_field(dac,
> > - addr_mask_map[AD3552R_SDO_DRIVE_STRENGTH][0],
> > - addr_mask_map[AD3552R_SDO_DRIVE_STRENGTH][1],
> > - val);
> > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D,
> > + AD3552R_MASK_SDO_DRIVE_STRENGTH,
> > + FIELD_PREP(AD3552R_MASK_SDO_DRIVE_STRENGTH, val));
> > if (err)
> > return err;
> > }
> > @@ -938,9 +850,15 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
> > "Invalid adi,output-range-microvolt value\n");
> >
> > val = err;
> > - err = ad3552r_set_ch_value(dac,
> > - AD3552R_CH_OUTPUT_RANGE_SEL,
> > - ch, val);
> > + if (ch == 0)
> > + val = FIELD_PREP(AD3552R_MASK_CH_OUTPUT_RANGE_SEL(0), val);
> > + else
> > + val = FIELD_PREP(AD3552R_MASK_CH_OUTPUT_RANGE_SEL(1), val);
> > +
> > + err = ad3552r_update_reg_field(dac,
> > + AD3552R_REG_ADDR_CH0_CH1_OUTPUT_RANGE,
> > + AD3552R_MASK_CH_OUTPUT_RANGE_SEL(ch),
> > + val);
> > if (err)
> > return err;
> >
> > @@ -958,7 +876,14 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
> > ad3552r_calc_gain_and_offset(dac, ch);
> > dac->enabled_ch |= BIT(ch);
> >
> > - err = ad3552r_set_ch_value(dac, AD3552R_CH_SELECT, ch, 1);
> > + if (ch == 0)
> > + val = FIELD_PREP(AD3552R_MASK_CH(0), 1);
> > + else
> > + val = FIELD_PREP(AD3552R_MASK_CH(1), 1);
> > +
> > + err = ad3552r_update_reg_field(dac,
> > + AD3552R_REG_ADDR_CH_SELECT_16B,
> > + AD3552R_MASK_CH(ch), val);
> > if (err < 0)
> > return err;
> >
> > @@ -970,8 +895,15 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
> > /* Disable unused channels */
> > for_each_clear_bit(ch, &dac->enabled_ch,
> > dac->model_data->num_hw_channels) {
> > - err = ad3552r_set_ch_value(dac, AD3552R_CH_AMPLIFIER_POWERDOWN,
> > - ch, 1);
> > + if (ch == 0)
> > + val = FIELD_PREP(AD3552R_MASK_CH_OUTPUT_RANGE_SEL(0), 1);
> > + else
> > + val = FIELD_PREP(AD3552R_MASK_CH_OUTPUT_RANGE_SEL(1), 1);
>
> Should these be AD3552R_MASK_CH_AMPLIFIER_POWERDOWN instead of
> AD3552R_MASK_CH_OUTPUT_RANGE_SEL? (2 above and 1 below.)
>
Hi David,
thanks, probably a copy and paste issue.
Fixed.
Not changing anything else since otherwise an additional patch
for style changes should be needed.
> > +
> > + err = ad3552r_update_reg_field(dac,
> > + AD3552R_REG_ADDR_POWERDOWN_CONFIG,
> > + AD3552R_MASK_CH_OUTPUT_RANGE_SEL(ch),
> > + val);
> > if (err)
> > return err;
> > }
> >
>
On Mon, 2024-10-14 at 16:14 -0500, David Lechner wrote: > On 10/14/24 5:08 AM, Angelo Dureghello wrote: > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > Changes to use FIELD_PREP, so that driver-specific ad3552r_field_prep > > is removed. Variables (arrays) that was used to call ad3552r_field_prep > > are removed too. > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > --- > > Found one likely bug. The rest are suggestions to keep the static > analyzers happy. > > \ > > @@ -510,8 +416,14 @@ static int ad3552r_write_raw(struct iio_dev *indio_dev, > > val); > > break; > > case IIO_CHAN_INFO_ENABLE: > > - err = ad3552r_set_ch_value(dac, AD3552R_CH_DAC_POWERDOWN, > > - chan->channel, !val); > > + if (chan->channel == 0) > > + val = FIELD_PREP(AD3552R_MASK_CH_DAC_POWERDOWN(0), > > !val); > > + else > > + val = FIELD_PREP(AD3552R_MASK_CH_DAC_POWERDOWN(1), > > !val); > > In the past, I've had bots (Sparse, IIRC) complain about using !val > with FIELD_PREP. Alternative is to write it as val ? 1 : 0. > Hmm, I'm fairly sure I also suffered from that warning. AFAICT, there's nothing wrong with the code so I would not make it less readable just to keep the tool happy (it seems to me that the tool is the one that needs to make this right). But this is just me - yeah, not a fan of the ternary operator :) Anyways, no strong feelings so if you go with the above, I won't really complain... just my 2 cents. - Nuno Sá >
© 2016 - 2024 Red Hat, Inc.