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: <CA+V-a8sJQnyJb_uq9yEcjHRW7ZFOw3g2XQyygcozWTgMjrYxRQ@mail.gmail.com>
Date: Wed, 2 Apr 2025 07:35:25 +0000
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Biju Das <biju.das.jz@...renesas.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 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.

> > +
> > +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