Re: [PATCH v3 17/17] media: rzg2l-cru: Move register definitions to a separate file

[Lad, Prabhakar]

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