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. > +#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. > + 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); > + 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. > + 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
