Re: [PATCH v2 1/2] staging: iio: ad9834: Correct phase range check

[Jonathan Cameron]

On Thu, 7 Nov 2024 13:32:42 +0300
Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:

> On Thu, Nov 07, 2024 at 01:10:14AM +0000, Zicheng Qu wrote:
> > User Perspective:
> > When a user sets the phase value, the ad9834_write_phase() is called.
> > The phase register has a 12-bit resolution, so the valid range is 0 to
> > 4095. If the phase offset value of 4096 is input, it effectively exactly
> > equals 0 in the lower 12 bits, meaning no offset.
> > 
> > Reasons for the Change:
> > 1) Original Condition (phase > BIT(AD9834_PHASE_BITS)):
> > This condition allows a phase value equal to 2^12, which is 4096.
> > However, this value exceeds the valid 12-bit range, as the maximum valid
> > phase value should be 4095.
> > 2) Modified Condition (phase >= BIT(AD9834_PHASE_BITS)):
> > Ensures that the phase value is within the valid range, preventing
> > invalid datafrom being written.
> > 
> > Impact on Subsequent Logic: st->data = cpu_to_be16(addr | phase):
> > If the phase value is 2^12, i.e., 4096 (0001 0000 0000 0000), and addr
> > is AD9834_REG_PHASE0 (1100 0000 0000 0000), then addr | phase results in
> > 1101 0000 0000 0000, occupying DB12. According to the section of WRITING
> > TO A PHASE REGISTER in the datasheet, the MSB 12 PHASE0 bits should be
> > DB11. The original condition leads to incorrect DB12 usage, which
> > contradicts the datasheet and could pose potential issues for future
> > updates if DB12 is used in such related cases.
> > 
> > Fixes: 12b9d5bf76bf ("Staging: IIO: DDS: AD9833 / AD9834 driver")
> > Cc: <stable@xxxxxxxxxxxxxxx>
> > Signed-off-by: Zicheng Qu <quzicheng@xxxxxxxxxx>
> > ---  
> 
> Fair enough.  Thanks.
> 
> Reviewed-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
Applied both patches to the fixes-togreg branch of iio.git.

Note they probably won't go upstream now until after rc1.

Thanks,

Jonathan

> 
> regards,
> dan carpenter
>