linux-kernel - RE: [PATCH v2 2/6] dmaengine: sh: rz_dmac: make register_dma

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:
 <TYYPR01MB1395515AF65F8522AED6B591885B5A@TYYPR01MB13955.jpnprd01.prod.outlook.com>
Date: Tue, 23 Dec 2025 14:08:29 +0000
From: Cosmin-Gabriel Tanislav <cosmin-gabriel.tanislav.xa@...esas.com>
To: geert <geert@...ux-m68k.org>
CC: Vinod Koul <vkoul@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
	magnus.damm <magnus.damm@...il.com>, Fabrizio Castro
	<fabrizio.castro.jz@...esas.com>, Prabhakar Mahadev Lad
	<prabhakar.mahadev-lad.rj@...renesas.com>, Johan Hovold <johan@...nel.org>,
	Biju Das <biju.das.jz@...renesas.com>, "dmaengine@...r.kernel.org"
	<dmaengine@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-renesas-soc@...r.kernel.org"
	<linux-renesas-soc@...r.kernel.org>
Subject: RE: [PATCH v2 2/6] dmaengine: sh: rz_dmac: make register_dma_req()
 chip-specific

> From: Geert Uytterhoeven <geert@...ux-m68k.org>
> Sent: Tuesday, December 23, 2025 3:45 PM
> 
> Hi Cosmin,
> 
> On Mon, 1 Dec 2025 at 13:52, Cosmin Tanislav
> <cosmin-gabriel.tanislav.xa@...esas.com> wrote:
> > The Renesas RZ/T2H (R9A09G077) and RZ/N2H (R9A09G087) SoCs use a
> > completely different ICU unit compared to RZ/V2H, which requires a
> > separate implementation.
> >
> > To prepare for adding support for these SoCs, add a chip-specific
> > structure and put a pointer to the rzv2h_icu_register_dma_req() function
> > in the .register_dma_req field of the chip-specific structure to allow
> > for other implementations. Do the same for the default request value,
> > RZV2H_ICU_DMAC_REQ_NO_DEFAULT.
> >
> > While at it, factor out the logic that calls .register_dma_req() or
> > rz_dmac_set_dmars_register() into a separate function to remove some
> > code duplication. Since the default values are different between the
> > two, use -1 for designating that the default value should be used.
> >
> > Signed-off-by: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@...esas.com>
> 
> Thanks for your patch!
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@...der.be>
> 
> You can find a few non-functional nits below...
> 
> > --- a/drivers/dma/sh/rz-dmac.c
> > +++ b/drivers/dma/sh/rz-dmac.c
> > @@ -95,9 +95,16 @@ struct rz_dmac_icu {
> >         u8 dmac_index;
> >  };
> >
> > +struct rz_dmac_info {
> > +       void (*register_dma_req)(struct platform_device *icu_dev, u8 dmac_index,
> > +                                u8 dmac_channel, u16 req_no);
> 
> icu_register_dma_req, to make clear this is ICU-specific?
> 

Ack.

> > +       u16 dma_req_no_default;
> 
> default_dma_req_no, to avoid confusion between literal "no" and an
> abbreviation of "number"?
> 

Ack.

> > +};
> > +
> 
> > @@ -1067,9 +1068,18 @@ static void rz_dmac_remove(struct platform_device *pdev)
> >         pm_runtime_disable(&pdev->dev);
> >  }
> >
> > +static const struct rz_dmac_info rz_dmac_v2h_info = {
> > +       .register_dma_req = rzv2h_icu_register_dma_req,
> > +       .dma_req_no_default = RZV2H_ICU_DMAC_REQ_NO_DEFAULT,
> 
> Since this is the only remaining user of RZV2H_ICU_DMAC_REQ_NO_DEFAULT,
> and this structure does specify hardware, perhaps just hardcode 0x3ff?
> 

In my opinion we should let the macro live in the ICU header as the
value is more tied to the ICU block than to the DMAC block, even if
the DMAC driver is the only actual user. But if you think this is
worth changing, I will change it.

> > +};
> > +
> > +static const struct rz_dmac_info rz_dmac_common_info = {
> 
> rz_dmac_classic_info, as this is not really common to all variants?
> I am open for a different name ;-)
> 

rz_dmac_generic_info? I don't have a strong opinion, but I agree that
common denotes that it would be shared across all variants, which is
not the case.

> > +       .dma_req_no_default = 0,
> > +};
> > +
> >  static const struct of_device_id of_rz_dmac_match[] = {
> > -       { .compatible = "renesas,r9a09g057-dmac", },
> > -       { .compatible = "renesas,rz-dmac", },
> > +       { .compatible = "renesas,r9a09g057-dmac", .data = &rz_dmac_v2h_info },
> > +       { .compatible = "renesas,rz-dmac", .data = &rz_dmac_common_info },
> >         { /* Sentinel */ }
> >  };
> >  MODULE_DEVICE_TABLE(of, of_rz_dmac_match);
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds