lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <wfp4hao3zbmm7tfkcur6ximzsuzhp2e2jeyq4emybhaxfhtyfk@r4x26gmik6v4>
Date: Tue, 15 Oct 2024 12:19:50 +0200
From: Angelo Dureghello <adureghello@...libre.com>
To: David Lechner <dlechner@...libre.com>
Cc: Nuno Sá <nuno.sa@...log.com>, 
	Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, 
	Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Olivier Moysan <olivier.moysan@...s.st.com>, linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH v6 5/8] iio: dac: ad3552r: changes to use FIELD_PREP

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ