Hi Prabhakar,
Thank you for the patch.
On Fri, Sep 06, 2024 at 06:39:47PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> Add support to capture 8bit Bayer formats.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> ---
> .../platform/renesas/rzg2l-cru/rzg2l-cru.h | 5 ++
> .../platform/renesas/rzg2l-cru/rzg2l-csi2.c | 7 ++-
> .../platform/renesas/rzg2l-cru/rzg2l-ip.c | 9 +++-
> .../platform/renesas/rzg2l-cru/rzg2l-video.c | 51 ++++++++++++++++---
> 4 files changed, 61 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> index 174760239548..83c664cb0929 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> @@ -58,6 +58,11 @@ struct rzg2l_cru_ip {
> struct v4l2_subdev *remote;
> };
>
> +enum rzg2l_cru_fmt {
> + RZG2L_YUV = 0,
> + RZG2L_RAW_BAYER,
> + RZG2L_USER_DEFINED,
> +};
> /**
> * struct rzg2l_cru_dev - Renesas CRU device structure
> * @dev: (OF) device
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> index a7e4a0c109da..b14c92cd7c18 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> @@ -18,6 +18,7 @@
> #include <linux/sys_soc.h>
> #include <linux/units.h>
>
> +#include <media/mipi-csi2.h>
> #include <media/v4l2-ctrls.h>
> #include <media/v4l2-device.h>
> #include <media/v4l2-fwnode.h>
> @@ -188,7 +189,11 @@ struct rzg2l_csi2_format {
> };
>
> static const struct rzg2l_csi2_format rzg2l_csi2_formats[] = {
> - { .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = 0x1e, .bpp = 16 },
> + { .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = MIPI_CSI2_DT_YUV422_8B, .bpp = 16 },
> + { .code = MEDIA_BUS_FMT_SBGGR8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
> + { .code = MEDIA_BUS_FMT_SGBRG8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
> + { .code = MEDIA_BUS_FMT_SGRBG8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
> + { .code = MEDIA_BUS_FMT_SRGGB8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
> };
>
> static inline struct rzg2l_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> index ac8ebae4ed07..5f60be92ea85 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> @@ -6,6 +6,7 @@
> */
>
> #include <linux/delay.h>
> +#include <media/mipi-csi2.h>
I would add a blank line on both sides.
> #include "rzg2l-cru.h"
>
> struct rzg2l_cru_ip_format {
> @@ -15,7 +16,11 @@ struct rzg2l_cru_ip_format {
> };
>
> static const struct rzg2l_cru_ip_format rzg2l_cru_ip_formats[] = {
> - { .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = 0x1e, .bpp = 16 },
> + { .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = MIPI_CSI2_DT_YUV422_8B, .bpp = 16 },
> + { .code = MEDIA_BUS_FMT_SBGGR8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
> + { .code = MEDIA_BUS_FMT_SGBRG8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
> + { .code = MEDIA_BUS_FMT_SGRBG8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
> + { .code = MEDIA_BUS_FMT_SRGGB8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
> };
>
> enum rzg2l_csi2_pads {
> @@ -149,7 +154,7 @@ static int rzg2l_cru_ip_enum_frame_size(struct v4l2_subdev *sd,
> if (fse->index != 0)
> return -EINVAL;
>
> - if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16)
> + if (!rzg2l_cru_ip_code_to_fmt(fse->code))
> return -EINVAL;
>
> fse->min_width = RZG2L_CRU_MIN_INPUT_WIDTH;
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index 6101a070e785..66a0b80e8da7 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -15,6 +15,7 @@
> #include <linux/delay.h>
> #include <linux/pm_runtime.h>
>
> +#include <media/mipi-csi2.h>
> #include <media/v4l2-ioctl.h>
> #include <media/videobuf2-dma-contig.h>
>
> @@ -78,6 +79,7 @@
> #define ICnMC 0x208
> #define ICnMC_CSCTHR BIT(5)
> #define ICnMC_INF_YUV8_422 (0x1e << 16)
> +#define ICnMC_INF_RAW8 (0x2a << 16)
> #define ICnMC_INF_USER (0x30 << 16)
It looks like the INF field contains the MIPI data type. I would replace
this with
#define ICnMC_INF(x) ((x) << 16)
and use it as
ICnMC_INF(MIPI_CSI2_DT_RAW8)
> #define ICnMC_VCSEL(x) ((x) << 22)
> #define ICnMC_INF_MASK GENMASK(21, 16)
> @@ -203,6 +205,10 @@ static int rzg2l_cru_mc_validate_format(struct rzg2l_cru_dev *cru,
>
> switch (fmt.format.code) {
> case MEDIA_BUS_FMT_UYVY8_1X16:
> + case MEDIA_BUS_FMT_SBGGR8_1X8:
> + case MEDIA_BUS_FMT_SGBRG8_1X8:
> + case MEDIA_BUS_FMT_SGRBG8_1X8:
> + case MEDIA_BUS_FMT_SRGGB8_1X8:
> break;
> default:
> return -EPIPE;
> @@ -300,7 +306,7 @@ 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,
> +static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, enum rzg2l_cru_fmt *input_fmt,
> struct v4l2_mbus_framefmt *ip_sd_fmt)
> {
> u32 icnmc;
> @@ -308,11 +314,18 @@ static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
> switch (ip_sd_fmt->code) {
> case MEDIA_BUS_FMT_UYVY8_1X16:
> icnmc = ICnMC_INF_YUV8_422;
> - *input_is_yuv = true;
> + *input_fmt = RZG2L_YUV;
> + break;
> + case MEDIA_BUS_FMT_SBGGR8_1X8:
> + case MEDIA_BUS_FMT_SGBRG8_1X8:
> + case MEDIA_BUS_FMT_SGRBG8_1X8:
> + case MEDIA_BUS_FMT_SRGGB8_1X8:
> + icnmc = ICnMC_INF_RAW8;
> + *input_fmt = RZG2L_RAW_BAYER;
> break;
> default:
> - *input_is_yuv = false;
> icnmc = ICnMC_INF_USER;
> + *input_fmt = RZG2L_USER_DEFINED;
> break;
> }
>
> @@ -327,17 +340,23 @@ static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
> static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
> struct v4l2_mbus_framefmt *ip_sd_fmt)
> {
> - bool output_is_yuv = false;
> - bool input_is_yuv = false;
> + enum rzg2l_cru_fmt input_fmt, output_fmt;
> u32 icndmr;
>
> - rzg2l_cru_csi2_setup(cru, &input_is_yuv, ip_sd_fmt);
> + rzg2l_cru_csi2_setup(cru, &input_fmt, ip_sd_fmt);
>
> /* Output format */
> switch (cru->format.pixelformat) {
> case V4L2_PIX_FMT_UYVY:
> icndmr = ICnDMR_YCMODE_UYVY;
> - output_is_yuv = true;
> + output_fmt = RZG2L_YUV;
> + break;
> + case V4L2_PIX_FMT_SBGGR8:
> + case V4L2_PIX_FMT_SGBRG8:
> + case V4L2_PIX_FMT_SGRBG8:
> + case V4L2_PIX_FMT_SRGGB8:
> + icndmr = 0;
> + output_fmt = RZG2L_RAW_BAYER;
> break;
> default:
> dev_err(cru->dev, "Invalid pixelformat (0x%x)\n",
> @@ -346,7 +365,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 (input_fmt == output_fmt)
> rzg2l_cru_write(cru, ICnMC,
> rzg2l_cru_read(cru, ICnMC) | ICnMC_CSCTHR);
> else
> @@ -809,6 +828,22 @@ static const struct v4l2_format_info rzg2l_cru_formats[] = {
> .format = V4L2_PIX_FMT_UYVY,
> .bpp[0] = 2,
> },
> + {
> + .format = V4L2_PIX_FMT_SBGGR8,
> + .bpp[0] = 1,
> + },
> + {
> + .format = V4L2_PIX_FMT_SGBRG8,
> + .bpp[0] = 1,
> + },
> + {
> + .format = V4L2_PIX_FMT_SGRBG8,
> + .bpp[0] = 1,
> + },
> + {
> + .format = V4L2_PIX_FMT_SRGGB8,
> + .bpp[0] = 1,
> + },
> };
I think all of this could be simplified if you add the format to the
rzg2l_cru_ip_format structure, as well as a rzg2l_cru_fmt member, and
used rzg2l_cru_ip_format through the driver.
rzg2l_cru_initialize_image_conv() would look up the rzg2l_cru_ip_format
entry based on the code, and would pass that to rzg2l_cru_csi2_setup(),
which wouldn't have to return an input_fmt. You could even add the
icndmr value to the structure too, to avoid a switch/case.
>
> const struct v4l2_format_info *rzg2l_cru_format_from_pixel(u32 format)
--
Regards,
Laurent Pinchart
