Hi Laurent, Thank you for the review. On Thu, Oct 3, 2024 at 3:36 PM Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > Hi Prabhakar, > > Thank you for the patch. > > On Tue, Oct 01, 2024 at 03:09:19PM +0100, Prabhakar wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > > > Move the RZ/G2L CRU register definitions from `rzg2l-video.c` to a > > dedicated header file, `rzg2l-cru-regs.h`. Separating these definitions > > into their own file improves the readability of the code. > > > > Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > --- > > v2->v3 > > - New patch > > --- > > .../renesas/rzg2l-cru/rzg2l-cru-regs.h | 79 +++++++++++++++++++ > > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 69 +--------------- > > 2 files changed, 80 insertions(+), 68 deletions(-) > > create mode 100644 drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h > > new file mode 100644 > > index 000000000000..458f7452e5d3 > > --- /dev/null > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h > > @@ -0,0 +1,79 @@ > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > +/* > > + * rzg2l-cru-regs.h--RZ/G2L (and alike SoCs) CRU Registers Definitions > > + * > > + * Copyright (C) 2024 Renesas Electronics Corp. > > + */ > > + > > +#ifndef __RZG2L_CRU_REGS_H__ > > +#define __RZG2L_CRU_REGS_H__ > > + > > +/* HW CRU Registers Definition */ > > + > > +/* CRU Control Register */ > > +#define CRUnCTRL 0x0 > > +#define CRUnCTRL_VINSEL(x) ((x) << 0) > > + > > +/* CRU Interrupt Enable Register */ > > +#define CRUnIE 0x4 > > +#define CRUnIE_EFE BIT(17) > > + > > +/* CRU Interrupt Status Register */ > > +#define CRUnINTS 0x8 > > +#define CRUnINTS_SFS BIT(16) > > + > > +/* CRU Reset Register */ > > +#define CRUnRST 0xc > > +#define CRUnRST_VRESETN BIT(0) > > + > > +/* Memory Bank Base Address (Lower) Register for CRU Image Data */ > > +#define AMnMBxADDRL(x) (0x100 + ((x) * 8)) > > + > > +/* Memory Bank Base Address (Higher) Register for CRU Image Data */ > > +#define AMnMBxADDRH(x) (0x104 + ((x) * 8)) > > + > > +/* Memory Bank Enable Register for CRU Image Data */ > > +#define AMnMBVALID 0x148 > > +#define AMnMBVALID_MBVALID(x) GENMASK(x, 0) > > + > > +/* Memory Bank Status Register for CRU Image Data */ > > +#define AMnMBS 0x14c > > +#define AMnMBS_MBSTS 0x7 > > + > > +/* AXI Master Transfer Setting Register for CRU Image Data */ > > +#define AMnAXIATTR 0x158 > > +#define AMnAXIATTR_AXILEN_MASK GENMASK(3, 0) > > +#define AMnAXIATTR_AXILEN (0xf) > > + > > +/* AXI Master FIFO Pointer Register for CRU Image Data */ > > +#define AMnFIFOPNTR 0x168 > > +#define AMnFIFOPNTR_FIFOWPNTR GENMASK(7, 0) > > +#define AMnFIFOPNTR_FIFORPNTR_Y GENMASK(23, 16) > > + > > +/* AXI Master Transfer Stop Register for CRU Image Data */ > > +#define AMnAXISTP 0x174 > > +#define AMnAXISTP_AXI_STOP BIT(0) > > + > > +/* AXI Master Transfer Stop Status Register for CRU Image Data */ > > +#define AMnAXISTPACK 0x178 > > +#define AMnAXISTPACK_AXI_STOP_ACK BIT(0) > > + > > +/* CRU Image Processing Enable Register */ > > +#define ICnEN 0x200 > > +#define ICnEN_ICEN BIT(0) > > + > > +/* CRU Image Processing Main Control Register */ > > +#define ICnMC 0x208 > > +#define ICnMC_CSCTHR BIT(5) > > +#define ICnMC_INF(x) ((x) << 16) > > +#define ICnMC_VCSEL(x) ((x) << 22) > > +#define ICnMC_INF_MASK GENMASK(21, 16) > > + > > +/* CRU Module Status Register */ > > +#define ICnMS 0x254 > > +#define ICnMS_IA BIT(2) > > + > > +/* CRU Data Output Mode Register */ > > +#define ICnDMR 0x26c > > The ICnDMR_YCMODE_UYVY macro from > drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h should also be > moved here. With that, > Agreed, I'll move ICnDMR_YCMODE_UYVY macro here. Cheers, Prabhakar > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx> > > > + > > +#endif /* __RZG2L_CRU_REGS_H__ */ > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > index c3d10b001b7c..d7c82c7b9044 100644 > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > @@ -20,74 +20,7 @@ > > #include <media/videobuf2-dma-contig.h> > > > > #include "rzg2l-cru.h" > > - > > -/* HW CRU Registers Definition */ > > - > > -/* CRU Control Register */ > > -#define CRUnCTRL 0x0 > > -#define CRUnCTRL_VINSEL(x) ((x) << 0) > > - > > -/* CRU Interrupt Enable Register */ > > -#define CRUnIE 0x4 > > -#define CRUnIE_EFE BIT(17) > > - > > -/* CRU Interrupt Status Register */ > > -#define CRUnINTS 0x8 > > -#define CRUnINTS_SFS BIT(16) > > - > > -/* CRU Reset Register */ > > -#define CRUnRST 0xc > > -#define CRUnRST_VRESETN BIT(0) > > - > > -/* Memory Bank Base Address (Lower) Register for CRU Image Data */ > > -#define AMnMBxADDRL(x) (0x100 + ((x) * 8)) > > - > > -/* Memory Bank Base Address (Higher) Register for CRU Image Data */ > > -#define AMnMBxADDRH(x) (0x104 + ((x) * 8)) > > - > > -/* Memory Bank Enable Register for CRU Image Data */ > > -#define AMnMBVALID 0x148 > > -#define AMnMBVALID_MBVALID(x) GENMASK(x, 0) > > - > > -/* Memory Bank Status Register for CRU Image Data */ > > -#define AMnMBS 0x14c > > -#define AMnMBS_MBSTS 0x7 > > - > > -/* AXI Master Transfer Setting Register for CRU Image Data */ > > -#define AMnAXIATTR 0x158 > > -#define AMnAXIATTR_AXILEN_MASK GENMASK(3, 0) > > -#define AMnAXIATTR_AXILEN (0xf) > > - > > -/* AXI Master FIFO Pointer Register for CRU Image Data */ > > -#define AMnFIFOPNTR 0x168 > > -#define AMnFIFOPNTR_FIFOWPNTR GENMASK(7, 0) > > -#define AMnFIFOPNTR_FIFORPNTR_Y GENMASK(23, 16) > > - > > -/* AXI Master Transfer Stop Register for CRU Image Data */ > > -#define AMnAXISTP 0x174 > > -#define AMnAXISTP_AXI_STOP BIT(0) > > - > > -/* AXI Master Transfer Stop Status Register for CRU Image Data */ > > -#define AMnAXISTPACK 0x178 > > -#define AMnAXISTPACK_AXI_STOP_ACK BIT(0) > > - > > -/* CRU Image Processing Enable Register */ > > -#define ICnEN 0x200 > > -#define ICnEN_ICEN BIT(0) > > - > > -/* CRU Image Processing Main Control Register */ > > -#define ICnMC 0x208 > > -#define ICnMC_CSCTHR BIT(5) > > -#define ICnMC_INF(x) ((x) << 16) > > -#define ICnMC_VCSEL(x) ((x) << 22) > > -#define ICnMC_INF_MASK GENMASK(21, 16) > > - > > -/* CRU Module Status Register */ > > -#define ICnMS 0x254 > > -#define ICnMS_IA BIT(2) > > - > > -/* CRU Data Output Mode Register */ > > -#define ICnDMR 0x26c > > +#include "rzg2l-cru-regs.h" > > > > #define RZG2L_TIMEOUT_MS 100 > > #define RZG2L_RETRIES 10 > > -- > Regards, > > Laurent Pinchart
