Hi Laurent,
Thank you for the review.
On Mon, Oct 7, 2024 at 9:21 PM Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Mon, Oct 07, 2024 at 07:48:32PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > Refactor the handling of supported formats in the RZ/G2L CRU driver by
> > moving the `rzg2l_cru_ip_format` struct to the common header to allow
> > reuse across multiple files and adding pixelformat and bpp members to it.
> > This change centralizes format handling, making it easier to manage and
> > extend.
> >
> > - Moved the `rzg2l_cru_ip_format` struct to `rzg2l-cru.h` for better
> > accessibility.
> > - Added format, datatype and bpp members to `rzg2l_cru_ip_format` struct
> > - Dropped rzg2l_cru_formats
> > - Introduced helper functions `rzg2l_cru_ip_code_to_fmt()`,
> > `rzg2l_cru_ip_format_to_fmt()`, and
> > `rzg2l_cru_ip_index_to_fmt()` to streamline format lookups.
> > - Refactored the `rzg2l_cru_csi2_setup` and format alignment functions
> > to utilize the new helpers.
>
> The general rule is once change, one patch. Bundling multiple changes
> together makes review more difficult. A bullet list of changes in a
> commit message is a sign you're bundling too many changed together.
>
Agreed, I will henceforth take care.
> You can still group related changes together when it makes sensor. For
> instance moving rzg2l_cru_ip_format to rzg2l-cru.h and adding the
> rzg2l_cru_ip_code_to_fmt() & co helper functions can be one patch.
>
Agreed, I will split this up.
> > Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > .../platform/renesas/rzg2l-cru/rzg2l-cru.h | 20 +++++-
> > .../platform/renesas/rzg2l-cru/rzg2l-ip.c | 36 ++++++++--
> > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 67 ++++++-------------
> > 3 files changed, 69 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > index 4fe24bdde5b2..39296a59b3da 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > @@ -62,6 +62,20 @@ struct rzg2l_cru_ip {
> > struct v4l2_subdev *remote;
> > };
> >
> > +/**
> > + * struct rzg2l_cru_ip_format - CRU IP format
> > + * @code: Media bus code
> > + * @format: 4CC format identifier (V4L2_PIX_FMT_*)
> > + * @datatype: MIPI CSI2 data type
> > + * @bpp: bytes per pixel
> > + */
> > +struct rzg2l_cru_ip_format {
> > + u32 code;
> > + u32 format;
> > + u32 datatype;
> > + u8 bpp;
> > +};
> > +
> > /**
> > * struct rzg2l_cru_dev - Renesas CRU device structure
> > * @dev: (OF) device
> > @@ -144,10 +158,12 @@ int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru);
> > void rzg2l_cru_video_unregister(struct rzg2l_cru_dev *cru);
> > irqreturn_t rzg2l_cru_irq(int irq, void *data);
> >
> > -const struct v4l2_format_info *rzg2l_cru_format_from_pixel(u32 format);
> > -
> > int rzg2l_cru_ip_subdev_register(struct rzg2l_cru_dev *cru);
> > void rzg2l_cru_ip_subdev_unregister(struct rzg2l_cru_dev *cru);
> > struct v4l2_mbus_framefmt *rzg2l_cru_ip_get_src_fmt(struct rzg2l_cru_dev *cru);
> >
> > +const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int code);
> > +const struct rzg2l_cru_ip_format *rzg2l_cru_ip_format_to_fmt(u32 format);
> > +const struct rzg2l_cru_ip_format *rzg2l_cru_ip_index_to_fmt(u32 index);
> > +
> > #endif
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > index 7b006a0bfaae..fde6f4781cfb 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > @@ -6,17 +6,21 @@
> > */
> >
> > #include <linux/delay.h>
> > -#include "rzg2l-cru.h"
> >
> > -struct rzg2l_cru_ip_format {
> > - u32 code;
> > -};
> > +#include <media/mipi-csi2.h>
> > +
> > +#include "rzg2l-cru.h"
> >
> > static const struct rzg2l_cru_ip_format rzg2l_cru_ip_formats[] = {
> > - { .code = MEDIA_BUS_FMT_UYVY8_1X16, },
> > + {
> > + .code = MEDIA_BUS_FMT_UYVY8_1X16,
> > + .format = V4L2_PIX_FMT_UYVY,
> > + .datatype = MIPI_CSI2_DT_YUV422_8B,
> > + .bpp = 2,
> > + },
> > };
> >
> > -static const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int code)
> > +const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int code)
> > {
> > unsigned int i;
> >
> > @@ -27,6 +31,26 @@ static const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int c
> > return NULL;
> > }
> >
> > +const struct rzg2l_cru_ip_format *rzg2l_cru_ip_format_to_fmt(u32 format)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(rzg2l_cru_ip_formats); i++) {
> > + if (rzg2l_cru_ip_formats[i].format == format)
> > + return &rzg2l_cru_ip_formats[i];
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +const struct rzg2l_cru_ip_format *rzg2l_cru_ip_index_to_fmt(u32 index)
> > +{
> > + if (index >= ARRAY_SIZE(rzg2l_cru_ip_formats))
> > + return NULL;
> > +
> > + return &rzg2l_cru_ip_formats[index];
> > +}
> > +
> > struct v4l2_mbus_framefmt *rzg2l_cru_ip_get_src_fmt(struct rzg2l_cru_dev *cru)
> > {
> > struct v4l2_subdev_state *state;
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > index de88c0fab961..ceb9012c9d70 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > @@ -300,21 +300,10 @@ static void rzg2l_cru_initialize_axi(struct rzg2l_cru_dev *cru)
> > rzg2l_cru_write(cru, AMnAXIATTR, amnaxiattr);
> > }
> >
> > -static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
> > - struct v4l2_mbus_framefmt *ip_sd_fmt, u8 csi_vc)
> > +static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, u8 csi_vc,
> > + u32 csi2_datatype)
>
> I would pass the rzg2l_cru_ip_format pointer (make it const) to this
> function instead of csi2_datatype.
>
OK.
> > {
> > - u32 icnmc;
> > -
> > - switch (ip_sd_fmt->code) {
> > - case MEDIA_BUS_FMT_UYVY8_1X16:
> > - icnmc = ICnMC_INF(MIPI_CSI2_DT_YUV422_8B);
> > - *input_is_yuv = true;
> > - break;
> > - default:
> > - *input_is_yuv = false;
> > - icnmc = ICnMC_INF(MIPI_CSI2_DT_USER_DEFINED(0));
> > - break;
> > - }
> > + u32 icnmc = ICnMC_INF(csi2_datatype);
> >
> > icnmc |= (rzg2l_cru_read(cru, ICnMC) & ~ICnMC_INF_MASK);
> >
> > @@ -328,17 +317,20 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
> > struct v4l2_mbus_framefmt *ip_sd_fmt,
> > u8 csi_vc)
> > {
> > - bool output_is_yuv = false;
> > - bool input_is_yuv = false;
> > + const struct v4l2_format_info *src_finfo, *dst_finfo;
> > + const struct rzg2l_cru_ip_format *cru_ip_fmt;
> > u32 icndmr;
> >
> > - rzg2l_cru_csi2_setup(cru, &input_is_yuv, ip_sd_fmt, csi_vc);
> > + cru_ip_fmt = rzg2l_cru_ip_code_to_fmt(ip_sd_fmt->code);
> > + rzg2l_cru_csi2_setup(cru, csi_vc, cru_ip_fmt->datatype);
> > +
> > + src_finfo = v4l2_format_info(cru_ip_fmt->format);
> > + dst_finfo = v4l2_format_info(cru->format.pixelformat);
> >
> > /* Output format */
> > switch (cru->format.pixelformat) {
> > case V4L2_PIX_FMT_UYVY:
> > icndmr = ICnDMR_YCMODE_UYVY;
> > - output_is_yuv = true;
> > break;
> > default:
> > dev_err(cru->dev, "Invalid pixelformat (0x%x)\n",
> > @@ -347,7 +339,7 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
> > }
> >
> > /* If input and output use same colorspace, do bypass mode */
> > - if (output_is_yuv == input_is_yuv)
> > + if (v4l2_is_format_yuv(src_finfo) && v4l2_is_format_yuv(dst_finfo))
>
> I think this should be
>
> if (v4l2_is_format_yuv(src_finfo) == v4l2_is_format_yuv(dst_finfo))
>
Agreed.
> > rzg2l_cru_write(cru, ICnMC,
> > rzg2l_cru_read(cru, ICnMC) | ICnMC_CSCTHR);
> > else
> > @@ -810,35 +802,15 @@ int rzg2l_cru_dma_register(struct rzg2l_cru_dev *cru)
> > /* -----------------------------------------------------------------------------
> > * V4L2 stuff
> > */
> > -
> > -static const struct v4l2_format_info rzg2l_cru_formats[] = {
> > - {
> > - .format = V4L2_PIX_FMT_UYVY,
> > - .bpp[0] = 2,
> > - },
> > -};
> > -
> > -const struct v4l2_format_info *rzg2l_cru_format_from_pixel(u32 format)
> > -{
> > - unsigned int i;
> > -
> > - for (i = 0; i < ARRAY_SIZE(rzg2l_cru_formats); i++)
> > - if (rzg2l_cru_formats[i].format == format)
> > - return rzg2l_cru_formats + i;
> > -
> > - return NULL;
> > -}
> > -
> > static u32 rzg2l_cru_format_bytesperline(struct v4l2_pix_format *pix)
> > {
> > - const struct v4l2_format_info *fmt;
> > -
> > - fmt = rzg2l_cru_format_from_pixel(pix->pixelformat);
> > + const struct rzg2l_cru_ip_format *fmt;
> >
> > + fmt = rzg2l_cru_ip_format_to_fmt(pix->pixelformat);
> > if (WARN_ON(!fmt))
> > - return -EINVAL;
> > + return 0;
>
> This change isn't described in the commit message.
>
I'll make this as a separate patch.
> >
> > - return pix->width * fmt->bpp[0];
> > + return pix->width * fmt->bpp;
> > }
> >
> > static u32 rzg2l_cru_format_sizeimage(struct v4l2_pix_format *pix)
> > @@ -849,7 +821,7 @@ static u32 rzg2l_cru_format_sizeimage(struct v4l2_pix_format *pix)
> > static void rzg2l_cru_format_align(struct rzg2l_cru_dev *cru,
> > struct v4l2_pix_format *pix)
> > {
> > - if (!rzg2l_cru_format_from_pixel(pix->pixelformat))
> > + if (!rzg2l_cru_ip_format_to_fmt(pix->pixelformat))
>
> Here you're calling rzg2l_cru_ip_format_to_fmt(), and just below the
> function calls rzg2l_cru_format_bytesperline(), which calls
> rzg2l_cru_format_from_pixel() again. Store the pointer here, drop the
> rzg2l_cru_format_bytesperline() function, and just write
>
> pix->bytesperline = pix->width * fmt->bpp;
>
Agreed, I'll update it as mentioned above.
> below. I would also inline rzg2l_cru_format_sizeimage() in this function
> as there's a single caller.
>
OK, I'll update it as above.
Cheers,
Prabhakar
