Re: [PATCH 2/8] media: vidc: adding core part and helper functions

[Stanimir Varbanov]

Hi Hans,

Thanks for the express comments!

<cut>

>> +
>> +struct vidc_core {
>> +	struct list_head list;
>> +	void __iomem *base;
>> +	int irq;
>> +	struct clk *clks[VIDC_CLKS_NUM_MAX];
>> +	struct mutex lock;
>> +	struct hfi_core hfi;
>> +	struct video_device vdev_dec;
>> +	struct video_device vdev_enc;
> 
> I know that many drivers embed struct video_device, but this can cause subtle
> refcounting problems. I recommend changing this to a pointer and using video_device_alloc().
> 
> I have plans to reorganize the way video_devices are allocated and registered in
> the near future, and you might just as well prepare this driver for that by switching
> to a pointer.

OK, thanks for the info, I will change to pointers.

<cut>

>> +
>> +int vidc_set_color_format(struct vidc_inst *inst, u32 type, u32 pixfmt)
>> +{
>> +	struct hfi_uncompressed_format_select fmt;
>> +	struct hfi_core *hfi = &inst->core->hfi;
>> +	u32 ptype = HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SELECT;
>> +	int ret;
>> +
>> +	fmt.buffer_type = type;
>> +
>> +	switch (pixfmt) {
>> +	case V4L2_PIX_FMT_NV12:
>> +		fmt.format = HFI_COLOR_FORMAT_NV12;
>> +		break;
>> +	case V4L2_PIX_FMT_NV21:
>> +		fmt.format = HFI_COLOR_FORMAT_NV21;
>> +		break;
>> +	default:
>> +		return -ENOTSUPP;
> 
> I'm not really sure how this error code is used, but normally -EINVAL is returned
> for invalid pixel formats. -ENOTSUPP is not used by V4L2.
> 

you are right, I need to change this to EINVAL.

-- 
regards,
Stan