Re: [PATCH v2 5/5] media: atmel-isc: Rework the format list
From: Yang, Wenyou
Date: Thu Sep 28 2017 - 01:32:54 EST
On 2017/9/27 16:03, Hans Verkuil wrote:
On 09/27/2017 07:15 AM, Yang, Wenyou wrote:
Hi Hans,
Thank you very much for your review.
On 2017/9/25 21:24, Hans Verkuil wrote:
Hi Wenyou,
On 18/09/17 08:39, Wenyou Yang wrote:
To improve the readability of code, split the format array into two,
one for the format description, other for the register configuration.
Meanwhile, add the flag member to indicate the format can be achieved
from the sensor or be produced by the controller, and rename members
related to the register configuration.
Also add more formats support: GREY, ARGB444, ARGB555 and ARGB32.
Signed-off-by: Wenyou Yang <wenyou.yang@xxxxxxxxxxxxx>
This looks better. Just a few comments, see below.
---
Changes in v2:
- Add the new patch to remove the unnecessary member from
isc_subdev_entity struct.
- Rebase on the patch set,
[PATCH 0/6] [media] Atmel: Adjustments for seven function implementations
https://www.mail-archive.com/linux-media@xxxxxxxxxxxxxxx/msg118342.html
drivers/media/platform/atmel/atmel-isc.c | 524 ++++++++++++++++++++++++-------
1 file changed, 405 insertions(+), 119 deletions(-)
diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
index 2d876903da71..90bd0b28a975 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -89,34 +89,56 @@ struct isc_subdev_entity {
struct list_head list;
};
+#define FMT_FLAG_FROM_SENSOR BIT(0)
+#define FMT_FLAG_FROM_CONTROLLER BIT(1)
Document the meaning of these flags.
Will add it in next version.
+
/*
* struct isc_format - ISC media bus format information
* @fourcc: Fourcc code for this format
* @mbus_code: V4L2 media bus format code.
+ * flags: Indicate format from sensor or converted by controller
* @bpp: Bits per pixel (when stored in memory)
- * @reg_bps: reg value for bits per sample
* (when transferred over a bus)
- * @pipeline: pipeline switch
* @sd_support: Subdev supports this format
* @isc_support: ISC can convert raw format to this format
*/
+
struct isc_format {
u32 fourcc;
u32 mbus_code;
+ u32 flags;
u8 bpp;
- u32 reg_bps;
- u32 reg_bay_cfg;
- u32 reg_rlp_mode;
- u32 reg_dcfg_imode;
- u32 reg_dctrl_dview;
-
- u32 pipeline;
-
bool sd_support;
bool isc_support;
};
+/* Pipeline bitmap */
+#define WB_ENABLE BIT(0)
+#define CFA_ENABLE BIT(1)
+#define CC_ENABLE BIT(2)
+#define GAM_ENABLE BIT(3)
+#define GAM_BENABLE BIT(4)
+#define GAM_GENABLE BIT(5)
+#define GAM_RENABLE BIT(6)
+#define CSC_ENABLE BIT(7)
+#define CBC_ENABLE BIT(8)
+#define SUB422_ENABLE BIT(9)
+#define SUB420_ENABLE BIT(10)
+
+#define GAM_ENABLES (GAM_RENABLE | GAM_GENABLE | GAM_BENABLE | GAM_ENABLE)
+
+struct fmt_config {
+ u32 fourcc;
+
+ u32 pfe_cfg0_bps;
+ u32 cfa_baycfg;
+ u32 rlp_cfg_mode;
+ u32 dcfg_imode;
+ u32 dctrl_dview;
+
+ u32 bits_pipeline;
+};
#define HIST_ENTRIES 512
#define HIST_BAYER (ISC_HIS_CFG_MODE_B + 1)
@@ -181,80 +203,321 @@ struct isc_device {
struct list_head subdev_entities;
};
-#define RAW_FMT_IND_START 0
-#define RAW_FMT_IND_END 11
-#define ISC_FMT_IND_START 12
-#define ISC_FMT_IND_END 14
-
-static struct isc_format isc_formats[] = {
- { V4L2_PIX_FMT_SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8, 8,
- ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_DAT8,
- ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
- { V4L2_PIX_FMT_SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8, 8,
- ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_GBGB, ISC_RLP_CFG_MODE_DAT8,
- ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
- { V4L2_PIX_FMT_SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8, 8,
- ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_GRGR, ISC_RLP_CFG_MODE_DAT8,
- ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
- { V4L2_PIX_FMT_SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8, 8,
- ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_RGRG, ISC_RLP_CFG_MODE_DAT8,
- ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
-
- { V4L2_PIX_FMT_SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10, 16,
- ISC_PFG_CFG0_BPS_TEN, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_DAT10,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
- { V4L2_PIX_FMT_SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10, 16,
- ISC_PFG_CFG0_BPS_TEN, ISC_BAY_CFG_GBGB, ISC_RLP_CFG_MODE_DAT10,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
- { V4L2_PIX_FMT_SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10, 16,
- ISC_PFG_CFG0_BPS_TEN, ISC_BAY_CFG_GRGR, ISC_RLP_CFG_MODE_DAT10,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
- { V4L2_PIX_FMT_SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10, 16,
- ISC_PFG_CFG0_BPS_TEN, ISC_BAY_CFG_RGRG, ISC_RLP_CFG_MODE_DAT10,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
-
- { V4L2_PIX_FMT_SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12, 16,
- ISC_PFG_CFG0_BPS_TWELVE, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_DAT12,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
- { V4L2_PIX_FMT_SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12, 16,
- ISC_PFG_CFG0_BPS_TWELVE, ISC_BAY_CFG_GBGB, ISC_RLP_CFG_MODE_DAT12,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
- { V4L2_PIX_FMT_SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12, 16,
- ISC_PFG_CFG0_BPS_TWELVE, ISC_BAY_CFG_GRGR, ISC_RLP_CFG_MODE_DAT12,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
- { V4L2_PIX_FMT_SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12, 16,
- ISC_PFG_CFG0_BPS_TWELVE, ISC_BAY_CFG_RGRG, ISC_RLP_CFG_MODE_DAT12,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
-
- { V4L2_PIX_FMT_YUV420, 0x0, 12,
- ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_YYCC,
- ISC_DCFG_IMODE_YC420P, ISC_DCTRL_DVIEW_PLANAR, 0x7fb,
- false, false },
- { V4L2_PIX_FMT_YUV422P, 0x0, 16,
- ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_YYCC,
- ISC_DCFG_IMODE_YC422P, ISC_DCTRL_DVIEW_PLANAR, 0x3fb,
- false, false },
- { V4L2_PIX_FMT_RGB565, MEDIA_BUS_FMT_RGB565_2X8_LE, 16,
- ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_RGB565,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x7b,
- false, false },
-
- { V4L2_PIX_FMT_YUYV, MEDIA_BUS_FMT_YUYV8_2X8, 16,
- ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_DAT8,
- ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
+#define MAX_RAW_FMT_INDEX 11
Do you still need this? The FMT_FLAG_FROM_SENSOR already tells you if it
is a raw format or not.
As far as I can tell you can drop this define.
The MAX_RAW_FMT_INDEX is used to get the raw format supported by the sensor.
Some sensor provide more formats other than the raw format, so the
FMT_FLAG_FROM_SENSOR is not enough.
So, add a new flag. The problem with a define like that is that is easily
can get out-of-sync with the array. It's a fragile coding approach.
Yes, adding a new flag is better.
Thanks.
Best Regards,
Wenyou Yang