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
