Hi Laurent,
Thanks for the feedback.
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Sent: 27 March 2025 10:16
> Subject: Re: [PATCH v4 11/17] media: rzg2l-cru: Add register mapping support
>
> Hi Tommaso,
>
> Thanks for being patient (and reminding me about this). Apparently, Embedded World is bad for e-mail
> backlogs.
>
> On Thu, Mar 13, 2025 at 01:01:24PM +0100, Tommaso Merciai wrote:
> > On Wed, Mar 12, 2025 at 01:37:25PM +0000, Biju Das wrote:
> > > On 03 March 2025 16:08, Tommaso Merciai wrote:
> > > >
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > > >
> > > > Prepare for adding support for RZ/G3E and RZ/V2HP SoCs, which have
> > > > a CRU-IP that is mostly identical to RZ/G2L but with different
> > > > register offsets and additional registers. Introduce a flexible register mapping mechanism to
> handle these variations.
> > > >
> > > > Define the `rzg2l_cru_info` structure to store register mappings
> > > > and pass it as part of the OF match data. Update the read/write
> > > > functions to check out-of-bound accesses and use indexed register offsets from `rzg2l_cru_info`,
> ensuring compatibility across different SoC variants.
> > > >
> > > > Signed-off-by: Lad Prabhakar
> > > > <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > > > Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@xxxxxxxxxxxxxx>
> > > > ---
> > > > Changes since v2:
> > > > - Implemented new rzg2l_cru_write/read() that now are checking out-of-bound
> > > > accesses as suggested by LPinchart.
> > > > - Fixed AMnMBxADDRL() and AMnMBxADDRH() as suggested by LPinchart.
> > > > - Update commit body
> > > >
> > > > .../platform/renesas/rzg2l-cru/rzg2l-core.c | 46 ++++++++++++-
> > > > .../renesas/rzg2l-cru/rzg2l-cru-regs.h | 66 ++++++++++---------
> > > > .../platform/renesas/rzg2l-cru/rzg2l-cru.h | 4 ++
> > > > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 58
> > > > ++++++++++++++--
> > > > 4 files changed, 139 insertions(+), 35 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > > index eed9d2bd0841..abc2a979833a 100644
> > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > > @@ -22,6 +22,7 @@
> > > > #include <media/v4l2-mc.h>
> > > >
> > > > #include "rzg2l-cru.h"
> > > > +#include "rzg2l-cru-regs.h"
> > > >
> > > > static inline struct rzg2l_cru_dev *notifier_to_cru(struct
> > > > v4l2_async_notifier *n) { @@ -269,6
> > > > +270,9 @@ static int rzg2l_cru_probe(struct platform_device *pdev)
> > > >
> > > > cru->dev = dev;
> > > > cru->info = of_device_get_match_data(dev);
> > > > + if (!cru->info)
> > > > + return dev_err_probe(dev, -EINVAL,
> > > > + "Failed to get OF match data\n");
> > > >
> > > > irq = platform_get_irq(pdev, 0);
> > > > if (irq < 0)
> > > > @@ -317,8 +321,48 @@ static void rzg2l_cru_remove(struct platform_device *pdev)
> > > > rzg2l_cru_dma_unregister(cru);
> > > > }
> > > >
> > > >
> > > > -static u32 rzg2l_cru_read(struct rzg2l_cru_dev *cru, u32 offset)
> > > > +static inline void
> > > > +__rzg2l_cru_write_constant(struct rzg2l_cru_dev *cru, u32 offset,
> > > > +u32 value)
> > > > {
> > > > - return ioread32(cru->base + offset);
> > > > + const u16 *regs = cru->info->regs;
> > > > +
> > > > + BUILD_BUG_ON(offset >= RZG2L_CRU_MAX_REG);
> > > > +
> > > > + iowrite32(value, cru->base + regs[offset]);
> > >
> > > Do you need this code as the purpose is to test compile time
> > > constant and It won't execute at run time?
>
> Biju, I'm not sure to understan this comment.
> __rzg2l_cru_write_constant() is called at runtime, with a compile-time constant offset. The
> BUILD_BUG_ON() verifies at compile time that the offset is valid, causing compilation errors if it
> isn't.
Thanks for explanation. Now got it. I don't see any drivers using this way. Hence the confusion.
>
> __rzg2l_cru_write(), on the other hand, is called when the offset is not known at compile time,
> because it's computed dynamically. That's a small subset of the calls. It needs to check the offset at
> runtime for overflows.
OK.
>
> What do you mean by "won't execute at runtime", and what code do you think is not needed ?
I got confused with BUILD_BUG_ON() on a frequently called function and the one without __rzg2l_cru_write()
Cheers,
Biju
