Hi Thomas, Thank you for your feedback! > From: Thomas Gleixner <tglx@xxxxxxxxxxxxx> > Sent: 07 February 2025 07:49 > Subject: Re: [PATCH 4/7] irqchip/renesas-rzv2h: Add rzv2h_icu_register_dma_req_ack > > On Thu, Feb 06 2025 at 22:03, Fabrizio Castro wrote: > > https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#function-references-in-changelogs > > > On the Renesas RZ/V2H(P) family of SoCs, DMAC IPs are connected > > to the Interrupt Control Unit (ICU). > > +#define ICU_DMkSELy(k, y) (0x420 + (k) * 0x20 + (y) * 4) > > +#define ICU_DMACKSELk(k) (0x500 + (k) * 4) > > > > /* NMI */ > > #define ICU_NMI_EDGE_FALLING 0 > > @@ -80,6 +83,19 @@ > > #define ICU_TINT_EXTRACT_GPIOINT(x) FIELD_GET(GENMASK(31, 16), (x)) > > #define ICU_PB5_TINT 0x55 > > > > +/* DMAC */ > > +#define ICU_DMAC_DkSEL_CLRON_MASK BIT(15) > > +#define ICU_DMAC_DkRQ_SEL_MASK GENMASK(9, 0) > > +#define ICU_DMAC_DMAREQ_MASK (ICU_DMAC_DkRQ_SEL_MASK | \ > > + ICU_DMAC_DkSEL_CLRON_MASK) > > + > > +#define ICU_DMAC_PREP_DkSEL_CLRON(x) FIELD_PREP(ICU_DMAC_DkSEL_CLRON_MASK, (x)) > > +#define ICU_DMAC_PREP_DkRQ_SEL(x) FIELD_PREP(ICU_DMAC_DkRQ_SEL_MASK, (x)) > > +#define ICU_DMAC_PREP_DMAREQ(sel, clr) (ICU_DMAC_PREP_DkRQ_SEL(sel) | \ > > + ICU_DMAC_PREP_DkSEL_CLRON(clr)) > > That's a pretty convoluted way to create a mask whihc has the CLRON bit > always set to 0 according to the only usage site. Indeed, it can be simplified. > > > +#define ICU_DMAC_DACK_SEL_MASK GENMASK(6, 0) > > > +void rzv2h_icu_register_dma_req_ack(struct platform_device *icu_dev, u8 dmac_index, u8 > dmac_channel, > > + u16 req_no, u8 ack_no) > > +{ > > + struct rzv2h_icu_priv *priv = platform_get_drvdata(icu_dev); > > + u32 icu_dmackselk, dmaack, dmaack_mask; > > + u32 icu_dmksely, dmareq, dmareq_mask; > > + u8 k, field_no; > > + u8 y, upper; > > + > > + if (req_no >= 0x1b5) > > In the DMA part you use proper defines for this, but here you put magic > numbers into the code. Please share the defines and use them consistently. Thanks for pointing it out, I'll fix it in v2. > > > + req_no = RZV2H_ICU_DMAC_REQ_NO_DEFAULT; > > + > > + if (ack_no >= 0x50) > > + ack_no = RZV2H_ICU_DMAC_ACK_NO_DEFAULT; > > + > > + y = dmac_channel / 2; > > + upper = dmac_channel % 2; > > + > > + dmareq = ICU_DMAC_PREP_DMAREQ(req_no, 0); > > + dmareq_mask = ICU_DMAC_DMAREQ_MASK; > > + > > + if (upper) { > > + dmareq <<= 16; > > + dmareq_mask <<= 16; > > + } > > You already have macros for this, so the obvious thing to do is to put > the shift magic into them: > > /* Two 16 bit fields per register */ > #define ICU_DMAC_DMAREQ_SHIFT(ch) ((ch & 0x01) * 16) > > #define ICU_DMAC_PREP_DMAREQ(sel, ch) (ICU_DMAC_PREP_DkRQ_SEL(sel) \ > << ICU_DMAC_DMAREQ_SHIFT(ch)) > #define ICU_DMAC_DMAREQ_MASK(ch) (ICU_DMAC_DkRQ_SEL_MASK \ > << ICU_DMAC_DMAREQ_SHIFT(ch)) > > dmareq = ICU_DMAC_PREP_DMAREQ(req_no, ch); > dmareq_mask = ICU_DMAC_DMAREQ_MASK(ch); Thank you, I'll adjust accordingly. > > > + k = ack_no / 4; > > + field_no = ack_no % 4; > > + > > + dmaack_mask = ICU_DMAC_DACK_SEL_MASK << (field_no * 8); > > + dmaack = ack_no << (field_no * 8); > > Same here. Will do. Cheers, Fab > > > + guard(raw_spinlock_irqsave)(&priv->lock); > > + > > + icu_dmksely = readl(priv->base + ICU_DMkSELy(dmac_index, y)); > > + icu_dmksely = (icu_dmksely & ~dmareq_mask) | dmareq; > > + writel(icu_dmksely, priv->base + ICU_DMkSELy(dmac_index, y)); > > + > > + icu_dmackselk = readl(priv->base + ICU_DMACKSELk(k)); > > + icu_dmackselk = (icu_dmackselk & ~dmaack_mask) | dmaack; > > + writel(icu_dmackselk, priv->base + ICU_DMACKSELk(k)); > > Thanks, > > tglx
