[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+V-a8vNs22ittNMha5uv0ybw5eusqSVsFQ3qPCwF=D6RcktNg@mail.gmail.com>
Date: Thu, 3 Oct 2024 21:23:29 +0100
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>, Hans Verkuil <hverkuil-cisco@...all.nl>,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-renesas-soc@...r.kernel.org, Biju Das <biju.das.jz@...renesas.com>,
Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH v3 17/17] media: rzg2l-cru: Move register definitions to a
separate file
Hi Laurent,
Thank you for the review.
On Thu, Oct 3, 2024 at 3:36 PM Laurent Pinchart
<laurent.pinchart@...asonboard.com> 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@...renesas.com>
> >
> > 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@...asonboard.com>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> > ---
> > 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@...asonboard.com>
>
> > +
> > +#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
Powered by blists - more mailing lists