[PATCH] iio: trigger: Remove redundant conditionals

[PATCH] iio: trigger: Remove redundant conditionals

[Gyeyoung Baek]

Checks for null initially and return early.
So there is no need to check for null later.

Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
 drivers/iio/industrialio-trigger.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index 54416a384232..6abf2a450202 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -453,36 +453,32 @@ static ssize_t current_trigger_store(struct device *dev,
 	}
 
 	trig = iio_trigger_acquire_by_name(buf);
-	if (oldtrig == trig) {
+	if (!trig || trig == oldtrig) {
 		ret = len;
 		goto out_trigger_put;
 	}
-
-	if (trig && indio_dev->info->validate_trigger) {
+	if (indio_dev->info->validate_trigger) {
 		ret = indio_dev->info->validate_trigger(indio_dev, trig);
 		if (ret)
 			goto out_trigger_put;
 	}
-
-	if (trig && trig->ops && trig->ops->validate_device) {
+	if (trig->ops && trig->ops->validate_device) {
 		ret = trig->ops->validate_device(trig, indio_dev);
 		if (ret)
 			goto out_trigger_put;
 	}
 
-	indio_dev->trig = trig;
-
 	if (oldtrig) {
 		if (indio_dev->modes & INDIO_EVENT_TRIGGERED)
 			iio_trigger_detach_poll_func(oldtrig,
 						     indio_dev->pollfunc_event);
 		iio_trigger_put(oldtrig);
 	}
-	if (indio_dev->trig) {
-		if (indio_dev->modes & INDIO_EVENT_TRIGGERED)
-			iio_trigger_attach_poll_func(indio_dev->trig,
-						     indio_dev->pollfunc_event);
-	}
+	if (indio_dev->modes & INDIO_EVENT_TRIGGERED)
+		iio_trigger_attach_poll_func(trig,
+						indio_dev->pollfunc_event);
+
+	indio_dev->trig = trig;
 
 	return len;
 
-- 
2.43.0

Re: [PATCH] iio: trigger: Remove redundant conditionals

[Jonathan Cameron]

On Sun,  4 May 2025 03:56:50 +0900
Gyeyoung Baek <gye976@gmail.com> wrote:

> Checks for null initially and return early.
> So there is no need to check for null later.
> 
> Signed-off-by: Gyeyoung Baek <gye976@gmail.com>

So the key thing here is what does this path mean.  trig == NULL
means we are clearing the current trigger.  The snag is you just jumped
over the code that removes the old trigger or sets
indio_dev->trig = NULL.

So I think the new version you have here is broken.

For changes like this it is fairly easy to test them using the
dummy driver.  Please make sure to do so and make sure you trigger
all paths.  Here that would be.

No trigger -> trigger 1
trigger 1 -> trigger 2
trigger 2 -> no trigger
 

> ---
>  drivers/iio/industrialio-trigger.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index 54416a384232..6abf2a450202 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -453,36 +453,32 @@ static ssize_t current_trigger_store(struct device *dev,
>  	}
>  
>  	trig = iio_trigger_acquire_by_name(buf);
> -	if (oldtrig == trig) {
> +	if (!trig || trig == oldtrig) {
>  		ret = len;
>  		goto out_trigger_put;
>  	}
> -
> -	if (trig && indio_dev->info->validate_trigger) {
> +	if (indio_dev->info->validate_trigger) {
>  		ret = indio_dev->info->validate_trigger(indio_dev, trig);
>  		if (ret)
>  			goto out_trigger_put;
>  	}
> -
> -	if (trig && trig->ops && trig->ops->validate_device) {
> +	if (trig->ops && trig->ops->validate_device) {
>  		ret = trig->ops->validate_device(trig, indio_dev);
>  		if (ret)
>  			goto out_trigger_put;
>  	}
>  
> -	indio_dev->trig = trig;

This and...

> -
>  	if (oldtrig) {

this need to run if oldtrig was set and trig is not.

>  		if (indio_dev->modes & INDIO_EVENT_TRIGGERED)
>  			iio_trigger_detach_poll_func(oldtrig,
>  						     indio_dev->pollfunc_event);
>  		iio_trigger_put(oldtrig);
>  	}
> -	if (indio_dev->trig) {
> -		if (indio_dev->modes & INDIO_EVENT_TRIGGERED)
> -			iio_trigger_attach_poll_func(indio_dev->trig,
> -						     indio_dev->pollfunc_event);
> -	}
> +	if (indio_dev->modes & INDIO_EVENT_TRIGGERED)
> +		iio_trigger_attach_poll_func(trig,
> +						indio_dev->pollfunc_event);
> +
> +	indio_dev->trig = trig;
>  
>  	return len;
>  

Re: [PATCH] iio: trigger: Remove redundant conditionals

[Gyeyoung Baek]

On Sun, May 4, 2025 at 11:30 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun,  4 May 2025 03:56:50 +0900
> Gyeyoung Baek <gye976@gmail.com> wrote:
>
> > Checks for null initially and return early.
> > So there is no need to check for null later.
> >
> > Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
>
> So the key thing here is what does this path mean.  trig == NULL
> means we are clearing the current trigger.  The snag is you just jumped
> over the code that removes the old trigger or sets
> indio_dev->trig = NULL.
>
> So I think the new version you have here is broken.
>
> For changes like this it is fairly easy to test them using the
> dummy driver.  Please make sure to do so and make sure you trigger
> all paths.  Here that would be.
>
> No trigger -> trigger 1
> trigger 1 -> trigger 2
> trigger 2 -> no trigger
>

Apologies, I misunderstood the logic.
I’ll make sure to verify it thoroughly before sending a patch.
Thanks for the review.

--
Regards,
Gyeyoung