Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 24 Jan 2023 17:42:21 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH] mdeia: ipu3: ipu33-mmu: Replace macro IPU3_ADDR2PTE() with a function |
| |
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@pappasbrent.com> > --- > 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
| ![\](/images/icornerr.gif) |