Re: [PATCH v3 10/17] media: rzg2l-cru: Simplify handling of supported formats

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Thu, Oct 3, 2024 at 3:25 PM Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> Just one minor comment below.
>
Missed Reviewed-by?

> On Tue, Oct 01, 2024 at 03:09:12PM +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.
> >
> > Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > v2->v3
> > - Updated subject line and commit message
> > - Implemented rzg2l_cru_ip_format_to_fmt() and rzg2l_cru_ip_index_to_fmt()
> > - Dropped checking fmt in rzg2l_cru_initialize_image_conv()
> >
> > v1->v2
> > - New patch
> > ---
> >  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    | 20 +++++-
> >  .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 35 ++++++++--
> >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 67 ++++++-------------
> >  3 files changed, 68 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..12aac9d6cb4b 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;
> >  }
> >
> > +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];
>
>         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];
>         }
>
> Sakari can probably handle this when applying the series to his tree.
>
As there will be v4 anyway for fixing the link_validate(), I'll
address it for v4.

Cheers,
Prabhakar