Re: [PATCH v2 5/5] media: atmel-isc: Rework the format list

["Yang, Wenyou" <Wenyou.Yang@microchip.com>]

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@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