Hi Laurent,
Thank you for the review.
On Fri, Sep 27, 2024 at 11:59 PM Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Tue, Sep 10, 2024 at 06:53:51PM +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_pix_fmt_to_bpp()`, and
> > `rzg2l_cru_ip_index_to_pix_fmt()` to streamline format lookups.
> > - Refactored the `rzg2l_cru_csi2_setup` and format alignment functions
> > to utilize the new helpers.
> >
> > 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 | 35 +++++++--
> > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 71 +++++++------------
> > 3 files changed, 72 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..24097df14881 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);
> > +u8 rzg2l_cru_ip_pix_fmt_to_bpp(u32 format);
> > +int rzg2l_cru_ip_index_to_pix_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 cc297e137f3e..2d3b985b7b0d 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,25 @@ static const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int c
> > return NULL;
> > }
> >
> > +u8 rzg2l_cru_ip_pix_fmt_to_bpp(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].bpp;
> > +
> > + return 0;
> > +}
>
> Instead of making this a ad-hoc 4cc -> bpp conversion, I would rename
> the function to rzg2l_cru_ip_format_to_fmt() (or something similar) and
> return a const struct rzg2l_cru_ip_format *. The caller can use the .bpp
> field.
>
OK, I'll introduce rzg2l_cru_ip_format_to_fmt().
> > +
> > +int rzg2l_cru_ip_index_to_pix_fmt(u32 index)
> > +{
> > + if (index >= ARRAY_SIZE(rzg2l_cru_ip_formats))
> > + return -EINVAL;
> > +
> > + return rzg2l_cru_ip_formats[index].format;
>
> There's no guarantee the 4CC won't map to a negative 32-bit integer. I
> would return a onst struct rzg2l_cru_ip_format * from this function, and
> rename it accordingly. The call can then use the .format field.
>
OK, I'll introduce rzg2l_cru_ip_index_to_fmt().
> > +}
> > +
> > 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..014c0ff2721b 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,23 @@ 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);
> > + if (!cru_ip_fmt)
> > + return -EINVAL;
>
> I think you can drop this check, as the code is guaranteed to be valid.
>
Agreed, I will drop this check.
Cheers,
Prabhakar
