Re: [PATCH] staging: media: atomisp: Use BIT macro instead of left shifting

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Sat, Jan 29, 2022 at 12:38:04PM +0100, Moses Christopher Bollavarapu wrote:

It's a bit too many people in Cc list (I dropped more than a half in this reply).
I would suggest to use my script [1] or look into it for a hint how to reduce it.

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

> There is a BIT(nr) macro available in Linux Kernel,
> which does the same thing.

In some cases it might fix a (theoretical) UB issue.

> Example: BIT(7) = (1UL << 7)

...

> +		s_config->source.tpg.x_mask = BIT(4) - 1;

> +		s_config->source.tpg.y_mask = BIT(4) - 1;

> +		s_config->source.tpg.xy_mask = BIT(8) - 1;

For masks you may use GENMASK().

...

> -		irq = irq & 1 << INTR_IIR;
> +		irq = irq & BIT(INTR_IIR);

It may be:

	irq &= BIT(...);

...

> -	irq = irq & 1 << INTR_IIR;
> +	irq = irq & BIT(INTR_IIR);

Ditto.

...

> -		virt += (1 << PAGE_SHIFT);
> +		virt += BIT(PAGE_SHIFT);

This is weird. Shouldn't be as simple as

	virt += PAGE_SIZE;

?

...

>  enum ia_css_sp_sleep_mode {
>  	SP_DISABLE_SLEEP_MODE = 0,
> -	SP_SLEEP_AFTER_FRAME = 1 << 0,
> -	SP_SLEEP_AFTER_IRQ = 1 << 1
> +	SP_SLEEP_AFTER_FRAME  = BIT(0),

> +	SP_SLEEP_AFTER_IRQ    = BIT(1)

While at it, add a comma here.

>  };

...

> +	IA_CSS_DEBUG_DUMP_ALL = BIT(14)  /** Dump all device parameters */

Ditto.

...

On top of this don't forget to add

	#include <linux/bits.h>

to the modified header files.

-- 
With Best Regards,
Andy Shevchenko