linux-kernel - RE: [PATCH v5 11/17] media: rzg2l-cru: Add register mapping support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <TY3PR01MB113462DC897E0DB681B1C020F86AF2@TY3PR01MB11346.jpnprd01.prod.outlook.com>
Date: Wed, 2 Apr 2025 08:19:54 +0000
From: Biju Das <biju.das.jz@...renesas.com>
To: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
CC: Tommaso Merciai <tommaso.merciai.xr@...renesas.com>, Tommaso Merciai
	<tomm.merciai@...il.com>, "linux-renesas-soc@...r.kernel.org"
	<linux-renesas-soc@...r.kernel.org>, "linux-media@...r.kernel.org"
	<linux-media@...r.kernel.org>, Prabhakar Mahadev Lad
	<prabhakar.mahadev-lad.rj@...renesas.com>, Mauro Carvalho Chehab
	<mchehab@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
	<krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Geert Uytterhoeven
	<geert+renesas@...der.be>, Magnus Damm <magnus.damm@...il.com>, Laurent
 Pinchart <laurent.pinchart+renesas@...asonboard.com>, Hans Verkuil
	<hverkuil@...all.nl>, Uwe Kleine-König
	<u.kleine-koenig@...libre.com>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v5 11/17] media: rzg2l-cru: Add register mapping support

Hi Prabhakar,

> -----Original Message-----
> From: Lad, Prabhakar <prabhakar.csengg@...il.com>
> 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@...renesas.com> wrote:
> >
> > Hi Tommaso,
> >
> > Thanks for the patch.
> >
> > > -----Original Message-----
> > > From: Tommaso Merciai <tommaso.merciai.xr@...renesas.com>
> > > 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@...renesas.com>
> > >
> > > 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@...renesas.com>
> > > Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@...renesas.com>
> > > ---
> > > 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.

See [1]

[1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20250326122003.122976-15-biju.das.jz@bp.renesas.com/

Cheers,
Biju

> 
> > > +
> > > +static const struct rzg2l_cru_info rzgl2_cru_info = {
> > > +     .regs = rzg2l_cru_regs,
> > > +};
> >
> > For a single entry, why you need struct?
> >
> This struct will grow further, see the later patches.
> 
> Cheers,
> Prabhakar