On Fri, May 24, 2019 at 01:19:26PM +0100, Jeremy Sowden wrote:
> kp2000_check_uio_irq contained a pair of nested ifs which each evaluated
> a bitwise operation. If both operations yielded true, the function
> returned 1; otherwise it returned 0.
>
> Replaced the whole thing with one return statement that evaluates the
> combination of both bitwise operations.
Does this really do the same thing?
>
> Signed-off-by: Jeremy Sowden <jeremy@xxxxxxxxxx>
> ---
> This applies to staging-testing.
>
> I was in two minds whether to send this patch or something less terse:
>
> + return (interrupt_active & irq_check_mask) &&
> + (interrupt_mask_inv & irq_check_mask);
>
> drivers/staging/kpc2000/kpc2000/cell_probe.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c b/drivers/staging/kpc2000/kpc2000/cell_probe.c
> index f731a97c6cac..d10f695b6673 100644
> --- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
> +++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
> @@ -240,12 +240,10 @@ int kp2000_check_uio_irq(struct kp2000_device *pcard, u32 irq_num)
> u64 interrupt_mask_inv = ~readq(pcard->sysinfo_regs_base + REG_INTERRUPT_MASK);
> u64 irq_check_mask = (1 << irq_num);
>
> - if (interrupt_active & irq_check_mask) { // if it's active (interrupt pending)
> - if (interrupt_mask_inv & irq_check_mask) { // and if it's not masked off
> - return 1;
> - }
> - }
> - return 0;
> + /*
> + * Is the IRQ active (interrupt pending) and not masked off?
> + */
> + return (interrupt_active & interrupt_mask_inv & irq_check_mask) != 0;
I have no idea what these bits really are, but try the above with the
following values:
interrupt_active = BIT(0);
interrupt_mask_inv = BIT(1);
irq_check_mask = (BIT(1) | BIT(0));
So in your new code you have:
BIT(0) & BIT(1) & (BIT(1) | BIT(0))
which reduces to:
0 & (BIT(1) | BIT(0))
which then reduces to:
0
The original if statements will return 1.
Your new one will return 0.
You can't AND interrupt_active with interrupt_mask_inv like you did
here, right?
Or am I reading this all wrong?
And what's wrong with the original code here? What is complaining about
it (other than the crazy comment style...)?
thanks,
greg k-h
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
