| Message ID | df7c6e1b-b619-40c3-9881-838587ed15d4@moroto.mountain (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: dac: adi-axi: fix a mistake in axi_dac_ext_info_set() | expand |
Hi Dan, On Wed, 2024-04-24 at 14:45 +0300, Dan Carpenter wrote: > The last parameter of these axi_dac_(frequency|scale|phase)_set() > functions is supposed to be true for TONE_2 and false for TONE_1. The > bug is the last call where it passes "private - TONE_2". That > subtraction is going to be zero/false for TONE_2 and and -1/true for > TONE_1. Fix the bug, and re-write it as "private == TONE_2" so it's > more obvious what is happening. > > Fixes: 4e3949a192e4 ("iio: dac: add support for AXI DAC IP core") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > This is from code review. Untested. > --- Auch, good catch! Agreed that private == AXI_DAC_*_TONE_2 makes it more readable. I guess Jonathan may just squash this in the driver (was pushed this weekend). Anyways, FWIW: Reviewed-by: Nuno Sa <nuno.sa@analog.com> > drivers/iio/dac/adi-axi-dac.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c > index 9047c5aec0ff..880d83a014a1 100644 > --- a/drivers/iio/dac/adi-axi-dac.c > +++ b/drivers/iio/dac/adi-axi-dac.c > @@ -383,15 +383,15 @@ static int axi_dac_ext_info_set(struct iio_backend > *back, uintptr_t private, > case AXI_DAC_FREQ_TONE_1: > case AXI_DAC_FREQ_TONE_2: > return axi_dac_frequency_set(st, chan, buf, len, > - private - AXI_DAC_FREQ_TONE_1); > + private == AXI_DAC_FREQ_TONE_2); > case AXI_DAC_SCALE_TONE_1: > case AXI_DAC_SCALE_TONE_2: > return axi_dac_scale_set(st, chan, buf, len, > - private - AXI_DAC_SCALE_TONE_1); > + private == AXI_DAC_SCALE_TONE_2); > case AXI_DAC_PHASE_TONE_1: > case AXI_DAC_PHASE_TONE_2: > return axi_dac_phase_set(st, chan, buf, len, > - private - AXI_DAC_PHASE_TONE_2); > + private == AXI_DAC_PHASE_TONE_2); > default: > return -EOPNOTSUPP; > }
On Wed, 24 Apr 2024 14:30:02 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > Hi Dan, > > On Wed, 2024-04-24 at 14:45 +0300, Dan Carpenter wrote: > > The last parameter of these axi_dac_(frequency|scale|phase)_set() > > functions is supposed to be true for TONE_2 and false for TONE_1. The > > bug is the last call where it passes "private - TONE_2". That > > subtraction is going to be zero/false for TONE_2 and and -1/true for > > TONE_1. Fix the bug, and re-write it as "private == TONE_2" so it's > > more obvious what is happening. > > > > Fixes: 4e3949a192e4 ("iio: dac: add support for AXI DAC IP core") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > This is from code review. Untested. > > --- > > Auch, good catch! Agreed that private == AXI_DAC_*_TONE_2 makes it more > readable. > > I guess Jonathan may just squash this in the driver (was pushed this weekend). > Anyways, FWIW: > > Reviewed-by: Nuno Sa <nuno.sa@analog.com> Missed the pull request, so will have to follow it in next pull. Applied to the togreg branch of iio.git and pushed out as testing. Thanks, Jonathan > > > drivers/iio/dac/adi-axi-dac.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c > > index 9047c5aec0ff..880d83a014a1 100644 > > --- a/drivers/iio/dac/adi-axi-dac.c > > +++ b/drivers/iio/dac/adi-axi-dac.c > > @@ -383,15 +383,15 @@ static int axi_dac_ext_info_set(struct iio_backend > > *back, uintptr_t private, > > case AXI_DAC_FREQ_TONE_1: > > case AXI_DAC_FREQ_TONE_2: > > return axi_dac_frequency_set(st, chan, buf, len, > > - private - AXI_DAC_FREQ_TONE_1); > > + private == AXI_DAC_FREQ_TONE_2); > > case AXI_DAC_SCALE_TONE_1: > > case AXI_DAC_SCALE_TONE_2: > > return axi_dac_scale_set(st, chan, buf, len, > > - private - AXI_DAC_SCALE_TONE_1); > > + private == AXI_DAC_SCALE_TONE_2); > > case AXI_DAC_PHASE_TONE_1: > > case AXI_DAC_PHASE_TONE_2: > > return axi_dac_phase_set(st, chan, buf, len, > > - private - AXI_DAC_PHASE_TONE_2); > > + private == AXI_DAC_PHASE_TONE_2); > > default: > > return -EOPNOTSUPP; > > } >
diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c index 9047c5aec0ff..880d83a014a1 100644 --- a/drivers/iio/dac/adi-axi-dac.c +++ b/drivers/iio/dac/adi-axi-dac.c @@ -383,15 +383,15 @@ static int axi_dac_ext_info_set(struct iio_backend *back, uintptr_t private, case AXI_DAC_FREQ_TONE_1: case AXI_DAC_FREQ_TONE_2: return axi_dac_frequency_set(st, chan, buf, len, - private - AXI_DAC_FREQ_TONE_1); + private == AXI_DAC_FREQ_TONE_2); case AXI_DAC_SCALE_TONE_1: case AXI_DAC_SCALE_TONE_2: return axi_dac_scale_set(st, chan, buf, len, - private - AXI_DAC_SCALE_TONE_1); + private == AXI_DAC_SCALE_TONE_2); case AXI_DAC_PHASE_TONE_1: case AXI_DAC_PHASE_TONE_2: return axi_dac_phase_set(st, chan, buf, len, - private - AXI_DAC_PHASE_TONE_2); + private == AXI_DAC_PHASE_TONE_2); default: return -EOPNOTSUPP; }
The last parameter of these axi_dac_(frequency|scale|phase)_set() functions is supposed to be true for TONE_2 and false for TONE_1. The bug is the last call where it passes "private - TONE_2". That subtraction is going to be zero/false for TONE_2 and and -1/true for TONE_1. Fix the bug, and re-write it as "private == TONE_2" so it's more obvious what is happening. Fixes: 4e3949a192e4 ("iio: dac: add support for AXI DAC IP core") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- This is from code review. Untested. --- drivers/iio/dac/adi-axi-dac.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)