Re: [PATCH v2 2/6] dmaengine: sh: rz_dmac: make register_dma_req() chip-specific

[Geert Uytterhoeven]

Hi Cosmin,

On Tue, 23 Dec 2025 at 15:08, Cosmin-Gabriel Tanislav
<cosmin-gabriel.tanislav.xa@xxxxxxxxxxx> wrote:
> > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > On Mon, 1 Dec 2025 at 13:52, Cosmin Tanislav
> > <cosmin-gabriel.tanislav.xa@xxxxxxxxxxx> 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@xxxxxxxxxxx>

> > > @@ -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.

I have no strong feelings about this.

If it is really more of an internal ICU thingy, an alternative would
be to remove all public *_ICU_DMAC_REQ_NO_DEFAULT definitions, and
just pass -1.  Then the ICU drivers become responsible for filling in
the appropriate default value.

> > > +};
> > > +
> > > +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.

Fine for me, too.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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