Hi Biju,
On Wed, Apr 2, 2025 at 9:20 AM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> > -----Original Message-----
> > From: Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx>
> > Sent: 02 April 2025 08:35
> > Subject: Re: [PATCH v5 11/17] media: rzg2l-cru: Add register mapping support
> >
> > Hi Biju,
> >
> > Thank you for the review.
> >
> > On Wed, Apr 2, 2025 at 7:31 AM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > >
> > > Hi Tommaso,
> > >
> > > Thanks for the patch.
> > >
> > > > -----Original Message-----
> > > > From: Tommaso Merciai <tommaso.merciai.xr@xxxxxxxxxxxxxx>
> > > > Sent: 28 March 2025 17:30
> > > > Subject: [PATCH v5 11/17] media: rzg2l-cru: Add register mapping
> > > > support
> > > >
> > > > 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
> > > >
> > > > Changes since v4:
> > > > - Mark __rzg2l_cru_write_constant/__rzg2l_cru_read_constant
> > > > as __always_inline
> > > >
> > > > .../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 eed9d2bd08414..abc2a979833aa 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 const u16 rzg2l_cru_regs[] = {
> > > > + [CRUnCTRL] = 0x0,
> > > > + [CRUnIE] = 0x4,
> > > > + [CRUnINTS] = 0x8,
> > > > + [CRUnRST] = 0xc,
> > > > + [AMnMB1ADDRL] = 0x100,
> > > > + [AMnMB1ADDRH] = 0x104,
> > > > + [AMnMB2ADDRL] = 0x108,
> > > > + [AMnMB2ADDRH] = 0x10c,
> > > > + [AMnMB3ADDRL] = 0x110,
> > > > + [AMnMB3ADDRH] = 0x114,
> > > > + [AMnMB4ADDRL] = 0x118,
> > > > + [AMnMB4ADDRH] = 0x11c,
> > > > + [AMnMB5ADDRL] = 0x120,
> > > > + [AMnMB5ADDRH] = 0x124,
> > > > + [AMnMB6ADDRL] = 0x128,
> > > > + [AMnMB6ADDRH] = 0x12c,
> > > > + [AMnMB7ADDRL] = 0x130,
> > > > + [AMnMB7ADDRH] = 0x134,
> > > > + [AMnMB8ADDRL] = 0x138,
> > > > + [AMnMB8ADDRH] = 0x13c,
> > > > + [AMnMBVALID] = 0x148,
> > > > + [AMnMBS] = 0x14c,
> > > > + [AMnAXIATTR] = 0x158,
> > > > + [AMnFIFOPNTR] = 0x168,
> > > > + [AMnAXISTP] = 0x174,
> > > > + [AMnAXISTPACK] = 0x178,
> > > > + [ICnEN] = 0x200,
> > > > + [ICnMC] = 0x208,
> > > > + [ICnMS] = 0x254,
> > > > + [ICnDMR] = 0x26c,
> > > > +};
> > >
> > > Do we need enum, can't we use struct instead with all these entries instead?
> > >
> > What benefit do you foresee when using struct? With the current approach being used a minimal diff is
> > generated when switched to struct there will be lots of changes.
>
> The mapping is convinient when you want to iterate throught it. Here, if
> you just want to access the offset value from its name, a structure
> looks more appropriate.
>
Thanks, as this patch has been reviewed by Laurent a couple of times
we will change this to struct If he insists.
Cheers,
Prabhakar
