On 14.10.2024 16:14, David Lechner wrote:
> On 10/14/24 5:08 AM, Angelo Dureghello wrote:
> > From: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > ---
>
> 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;
> > }
> >
>
