[PATCH] iio: dac: ad5592r: Fix the missing return value.

[PATCH] iio: dac: ad5592r: Fix the missing return value.

[Zizhuang Deng]

The third call to `fwnode_property_read_u32` did not record
the return value, resulting in `channel_offstate` possibly
being assigned the wrong value.

Signed-off-by: Zizhuang Deng <sunsetdzz@gmail.com>
---
 drivers/iio/dac/ad5592r-base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/dac/ad5592r-base.c b/drivers/iio/dac/ad5592r-base.c
index a424b7220b61..4434c1b2a322 100644
--- a/drivers/iio/dac/ad5592r-base.c
+++ b/drivers/iio/dac/ad5592r-base.c
@@ -522,7 +522,7 @@ static int ad5592r_alloc_channels(struct iio_dev *iio_dev)
 		if (!ret)
 			st->channel_modes[reg] = tmp;
 
-		fwnode_property_read_u32(child, "adi,off-state", &tmp);
+		ret = fwnode_property_read_u32(child, "adi,off-state", &tmp);
 		if (!ret)
 			st->channel_offstate[reg] = tmp;
 	}
-- 
2.25.1

Re: [PATCH] iio: dac: ad5592r: Fix the missing return value.

[Jonathan Cameron]

On Thu, 10 Mar 2022 20:54:50 +0800
Zizhuang Deng <sunsetdzz@gmail.com> wrote:

> The third call to `fwnode_property_read_u32` did not record
> the return value, resulting in `channel_offstate` possibly
> being assigned the wrong value.
> 
> Signed-off-by: Zizhuang Deng <sunsetdzz@gmail.com>
Hi,

Definitely rather odd looking and I think your conclusion is correct.
+CC Paul for confirmation that this isn't doing something clever..

> ---
>  drivers/iio/dac/ad5592r-base.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/dac/ad5592r-base.c b/drivers/iio/dac/ad5592r-base.c
> index a424b7220b61..4434c1b2a322 100644
> --- a/drivers/iio/dac/ad5592r-base.c
> +++ b/drivers/iio/dac/ad5592r-base.c
> @@ -522,7 +522,7 @@ static int ad5592r_alloc_channels(struct iio_dev *iio_dev)
>  		if (!ret)
>  			st->channel_modes[reg] = tmp;
>  
> -		fwnode_property_read_u32(child, "adi,off-state", &tmp);
> +		ret = fwnode_property_read_u32(child, "adi,off-state", &tmp);
>  		if (!ret)
>  			st->channel_offstate[reg] = tmp;
>  	}

Re: [PATCH] iio: dac: ad5592r: Fix the missing return value.

[Paul Cercueil]

Hi,

Le dim., mars 20 2022 at 15:20:47 +0000, Jonathan Cameron 
<jic23@kernel.org> a écrit :
> On Thu, 10 Mar 2022 20:54:50 +0800
> Zizhuang Deng <sunsetdzz@gmail.com> wrote:
> 
>>  The third call to `fwnode_property_read_u32` did not record
>>  the return value, resulting in `channel_offstate` possibly
>>  being assigned the wrong value.
>> 
>>  Signed-off-by: Zizhuang Deng <sunsetdzz@gmail.com>
> Hi,
> 
> Definitely rather odd looking and I think your conclusion is correct.
> +CC Paul for confirmation that this isn't doing something clever..

It's been a while, but I don't think there was anything clever going on 
here - so the patch is fine.

Cheers,
-Paul

> 
>>  ---
>>   drivers/iio/dac/ad5592r-base.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>>  diff --git a/drivers/iio/dac/ad5592r-base.c 
>> b/drivers/iio/dac/ad5592r-base.c
>>  index a424b7220b61..4434c1b2a322 100644
>>  --- a/drivers/iio/dac/ad5592r-base.c
>>  +++ b/drivers/iio/dac/ad5592r-base.c
>>  @@ -522,7 +522,7 @@ static int ad5592r_alloc_channels(struct 
>> iio_dev *iio_dev)
>>   		if (!ret)
>>   			st->channel_modes[reg] = tmp;
>> 
>>  -		fwnode_property_read_u32(child, "adi,off-state", &tmp);
>>  +		ret = fwnode_property_read_u32(child, "adi,off-state", &tmp);
>>   		if (!ret)
>>   			st->channel_offstate[reg] = tmp;
>>   	}
> 

Re: [PATCH] iio: dac: ad5592r: Fix the missing return value.

[Jonathan Cameron]

On Mon, 21 Mar 2022 09:28:36 +0000
Paul Cercueil <paul@crapouillou.net> wrote:

> Hi,
> 
> Le dim., mars 20 2022 at 15:20:47 +0000, Jonathan Cameron 
> <jic23@kernel.org> a écrit :
> > On Thu, 10 Mar 2022 20:54:50 +0800
> > Zizhuang Deng <sunsetdzz@gmail.com> wrote:
> >   
> >>  The third call to `fwnode_property_read_u32` did not record
> >>  the return value, resulting in `channel_offstate` possibly
> >>  being assigned the wrong value.
> >> 
> >>  Signed-off-by: Zizhuang Deng <sunsetdzz@gmail.com>  
> > Hi,
> > 
> > Definitely rather odd looking and I think your conclusion is correct.
> > +CC Paul for confirmation that this isn't doing something clever..  
> 
> It's been a while, but I don't think there was anything clever going on 
> here - so the patch is fine.

Added a fixes tag (it was driver introduction) and marked for stable
given this could have some weird side effects if anyone actually
had a dt that hit this path.  Applied to the fixes-togreg branch of iio.git
but not pushed out yet as I'll be rebasing that branch on rc1 next week.

Thanks,

Jonathan

> 
> Cheers,
> -Paul
> 
> >   
> >>  ---
> >>   drivers/iio/dac/ad5592r-base.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >>  diff --git a/drivers/iio/dac/ad5592r-base.c 
> >> b/drivers/iio/dac/ad5592r-base.c
> >>  index a424b7220b61..4434c1b2a322 100644
> >>  --- a/drivers/iio/dac/ad5592r-base.c
> >>  +++ b/drivers/iio/dac/ad5592r-base.c
> >>  @@ -522,7 +522,7 @@ static int ad5592r_alloc_channels(struct 
> >> iio_dev *iio_dev)
> >>   		if (!ret)
> >>   			st->channel_modes[reg] = tmp;
> >> 
> >>  -		fwnode_property_read_u32(child, "adi,off-state", &tmp);
> >>  +		ret = fwnode_property_read_u32(child, "adi,off-state", &tmp);
> >>   		if (!ret)
> >>   			st->channel_offstate[reg] = tmp;
> >>   	}  
> >   
> 
> 

Re: [PATCH] iio: dac: ad5592r: Fix the missing return value.

[Andy Shevchenko]

On Mon, Mar 21, 2022 at 11:34 AM Paul Cercueil <paul@crapouillou.net> wrote:
> Le dim., mars 20 2022 at 15:20:47 +0000, Jonathan Cameron
> <jic23@kernel.org> a écrit :
> > On Thu, 10 Mar 2022 20:54:50 +0800
> > Zizhuang Deng <sunsetdzz@gmail.com> wrote:
> >
> >>  The third call to `fwnode_property_read_u32` did not record
> >>  the return value, resulting in `channel_offstate` possibly
> >>  being assigned the wrong value.

> > Definitely rather odd looking and I think your conclusion is correct.
> > +CC Paul for confirmation that this isn't doing something clever..
>
> It's been a while, but I don't think there was anything clever going on
> here - so the patch is fine.

Basically the question here is what value should offstate have when
there is no such property. Currently it's the same value as modes (no
seeing context other than in the patch).

> >>              if (!ret)
> >>                      st->channel_modes[reg] = tmp;
> >>
> >>  -           fwnode_property_read_u32(child, "adi,off-state", &tmp);
> >>  +           ret = fwnode_property_read_u32(child, "adi,off-state", &tmp);
> >>              if (!ret)
> >>                      st->channel_offstate[reg] = tmp;


-- 
With Best Regards,
Andy Shevchenko