Re: [PATCH] staging: media: atomisp: Drop unnecessary else block after return/break

[Andy Shevchenko]

On Mon, Jul 06, 2026 at 04:08:10PM +0530, Dileep Sankhla wrote:
> Remove redundant else blocks following return or break statements. As
> control flow exits in these cases, the else branch is unnecessary.
> Dropping it improves code readability.
> 
> No functional change.

...

> +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> static int atomisp_mrfld_pre_power_down(struct atomisp_device *isp)

>  		spin_unlock_irqrestore(&isp->lock, flags);
>  		return -EAGAIN;
> -	} else {
> -		pci_read_config_dword(pdev, PCI_INTERRUPT_CTRL, &irq);
> -		irq &= BIT(INTR_IIR);
> -		pci_write_config_dword(pdev, PCI_INTERRUPT_CTRL, irq);
> -
> -		pci_read_config_dword(pdev, PCI_INTERRUPT_CTRL, &irq);
> -		if (!(irq & BIT(INTR_IIR))) {
> -			atomisp_css2_hw_store_32(MRFLD_INTR_ENABLE_REG, 0x0);
> -			goto done;
> -		}
> -		dev_err(isp->dev,
> -			"%s: error in iunit interrupt. status reg=0x%x\n",
> -			__func__, irq);
> -		spin_unlock_irqrestore(&isp->lock, flags);
> -		return -EAGAIN;
>  	}
> +
> +	pci_read_config_dword(pdev, PCI_INTERRUPT_CTRL, &irq);
> +	irq &= BIT(INTR_IIR);
> +	pci_write_config_dword(pdev, PCI_INTERRUPT_CTRL, irq);
> +
> +	pci_read_config_dword(pdev, PCI_INTERRUPT_CTRL, &irq);
> +	if (!(irq & BIT(INTR_IIR))) {
> +		atomisp_css2_hw_store_32(MRFLD_INTR_ENABLE_REG, 0x0);
> +		goto done;
> +	}
> +	dev_err(isp->dev,
> +		"%s: error in iunit interrupt. status reg=0x%x\n", __func__,
> +		irq);
> +	spin_unlock_irqrestore(&isp->lock, flags);
> +	return -EAGAIN;

I would really avoid touching this for now. This is non-straight workaround for
some platforms and it needs real care on what's going on and how to make it
look better. Since your patch does not targeting that, it's doubtful that this
change is helpful.

...

>  	if ((*flags) & INPUT_SYSTEM_CFG_FLAG_SET) {
>  		// Check for consistency with already set value.
> -		if ((*lhs) == (rhs)) {
> +		if ((*lhs) == (rhs))

Unneeded parentheses, also see below.

>  			return INPUT_SYSTEM_ERR_NO_ERROR;
> -		} else {
> -			*flags |= INPUT_SYSTEM_CFG_FLAG_CONFLICT;
> -			return INPUT_SYSTEM_ERR_CONFLICT_ON_RESOURCE;
> -		}
> +
> +		*flags |= INPUT_SYSTEM_CFG_FLAG_CONFLICT;
> +		return INPUT_SYSTEM_ERR_CONFLICT_ON_RESOURCE;
>  	}

In this case it's better to toggle the conditional to follow the pattern to
check for errors first.

		if (*lhs != rhs) {
			*flags |= INPUT_SYSTEM_CFG_FLAG_CONFLICT;
			return INPUT_SYSTEM_ERR_CONFLICT_ON_RESOURCE;
		}

		return INPUT_SYSTEM_ERR_NO_ERROR;

And yeah, looking at the below, you might want to have a common helper for
this. So, perhaps don't touch these three cases for now. Or we can leave them
as in your patch, it's up to Sakari and you.

...

> +		/* TMP: check discrepancy between nr of enqueued
> +		 * parameter sets and dequeued sets
> +		 */

When moving comments with a wrong style, fix the style at the same time.

		/*
		 * TMP: check discrepancy between nr of enqueued parameter sets
		 * and dequeued sets.
		 */

...

> +		assert(g_param_buffer_enqueue_count < g_param_buffer_dequeue_count + 50);

What will this do in the kernel environment? Perhaps first you need to check that and
most likely change the assert():s to something else?

...

> +		ia_css_bufq_enqueue_psys_event(
> +				IA_CSS_PSYS_SW_EVENT_BUFFER_ENQUEUED,
> +				(uint8_t)thread_id, (uint8_t)queue_id, 0);

Why do we need the castings?

...

Half of the patch is good to go, and the other needs more work and real work on
the driver.

-- 
With Best Regards,
Andy Shevchenko