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. Regards, Hans
