Re: [PATCH] mdeia: ipu3: ipu33-mmu: Replace macro IPU3_ADDR2PTE() with a function

[Dan Carpenter]

On Tue, Jan 24, 2023 at 08:55:54AM -0500, Brent Pappas wrote:
> Replace the macro IPU3_ADDR2PTE() with a static function to match
> Linux coding style standards.

When you say "Linux coding style standards" what exactly does that mean?
I've just re-read the Documentation/process/coding-style.rst section on
"Macros, Enums and RTL" and I don't see an issue with the macro.

This code is in the middle of a big section full of macros.  Why did you
pick this particular macro?  Now it doesn't mirror the IPU3_PTE2ADDR()
so this patch hurts readability.

> 
> Signed-off-by: Brent Pappas <bpappas@xxxxxxxxxxxxxxx>
> ---
>  drivers/staging/media/ipu3/ipu3-mmu.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/media/ipu3/ipu3-mmu.c b/drivers/staging/media/ipu3/ipu3-mmu.c
> index cb9bf5fb29a5..d2d603c32773 100644
> --- a/drivers/staging/media/ipu3/ipu3-mmu.c
> +++ b/drivers/staging/media/ipu3/ipu3-mmu.c
> @@ -25,7 +25,11 @@
>  #define IPU3_PT_SIZE		(IPU3_PT_PTES << 2)
>  #define IPU3_PT_ORDER		(IPU3_PT_SIZE >> PAGE_SHIFT)
>  
> -#define IPU3_ADDR2PTE(addr)	((addr) >> IPU3_PAGE_SHIFT)
> +static u32 ipu3_addr2pte(phys_addr_t addr)
> +{
> +	return addr >> IPU3_PAGE_SHIFT;
> +}

To me the original macro is fine.  The inline would also be fine if it
were done consistently.  But I guess I just don't see a lot of value in
changing the existing code.

If you were taking ownership of this driver in a more meaningful way
then I would defer to your taste...  But I just don't see a lot of value
in the patch.

regards,
dan carpenter