| Message ID | 20241007184839.190519-11-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: platform: rzg2l-cru: CSI-2 and CRU enhancements | expand |
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@bp.renesas.com> > > 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. 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. > Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > .../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. > { > - 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)) > 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. > > - 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; below. I would also inline rzg2l_cru_format_sizeimage() in this function as there's a single caller. > pix->pixelformat = RZG2L_CRU_DEFAULT_FORMAT; > > switch (pix->field) { > @@ -941,10 +913,13 @@ static int rzg2l_cru_g_fmt_vid_cap(struct file *file, void *priv, > static int rzg2l_cru_enum_fmt_vid_cap(struct file *file, void *priv, > struct v4l2_fmtdesc *f) > { > - if (f->index >= ARRAY_SIZE(rzg2l_cru_formats)) > + const struct rzg2l_cru_ip_format *fmt; > + > + fmt = rzg2l_cru_ip_index_to_fmt(f->index); > + if (!fmt) > return -EINVAL; > > - f->pixelformat = rzg2l_cru_formats[f->index].format; > + f->pixelformat = fmt->format; > > return 0; > }
Hi Laurent, Thank you for the review. On Mon, Oct 7, 2024 at 9:21 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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@bp.renesas.com> > > > > 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@ideasonboard.com> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > .../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
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) { - 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)) 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; - 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)) pix->pixelformat = RZG2L_CRU_DEFAULT_FORMAT; switch (pix->field) { @@ -941,10 +913,13 @@ static int rzg2l_cru_g_fmt_vid_cap(struct file *file, void *priv, static int rzg2l_cru_enum_fmt_vid_cap(struct file *file, void *priv, struct v4l2_fmtdesc *f) { - if (f->index >= ARRAY_SIZE(rzg2l_cru_formats)) + const struct rzg2l_cru_ip_format *fmt; + + fmt = rzg2l_cru_ip_index_to_fmt(f->index); + if (!fmt) return -EINVAL; - f->pixelformat = rzg2l_cru_formats[f->index].format; + f->pixelformat = fmt->format; return 0; }