Re: [PATCH v2 10/16] media: platform: rzg2l-cru: Simplify handling of supported formats

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux