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.y...@microchip.com>
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@vger.kernel.org/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

Reply via email to