Re: [PATCH v9 2/4] media: i2c: Add MAX9286 driver

[Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>]

Hi Mani,

On 14/05/2020 10:59, Manivannan Sadhasivam wrote:
> On Thu, May 14, 2020 at 10:53:00AM +0100, Kieran Bingham wrote:
>> Hi Mani,
>>>> +static int max9286_set_fmt(struct v4l2_subdev *sd,
>>>> +			   struct v4l2_subdev_pad_config *cfg,
>>>> +			   struct v4l2_subdev_format *format)
>>>> +{
>>>> +	struct max9286_priv *priv = sd_to_max9286(sd);
>>>> +	struct v4l2_mbus_framefmt *cfg_fmt;
>>>> +
>>>> +	if (format->pad >= MAX9286_SRC_PAD)
>>>> +		return -EINVAL;
>>>> +
>>>> +	/* Refuse non YUV422 formats as we hardcode DT to 8 bit YUV422 */
>>>> +	switch (format->format.code) {
>>>> +	case MEDIA_BUS_FMT_UYVY8_2X8:
>>>> +	case MEDIA_BUS_FMT_VYUY8_2X8:
>>>> +	case MEDIA_BUS_FMT_YUYV8_2X8:
>>>> +	case MEDIA_BUS_FMT_YVYU8_2X8:
>>>> +		break;
>>>> +	default:
>>>> +		format->format.code = MEDIA_BUS_FMT_YUYV8_2X8;
>>>
>>> Is there any reason for not setting default format to MEDIA_BUS_FMT_UYVY8_2X8?
>>>
>>
>>
>> No good reason no, and I see that in max9286_enum_mbus_code(), we
>> currently code to MEDIA_BUS_FMT_YUYV8_2X8, and that's the value that we
>> init to, so that would be a better default indeed.
>>
> 
> max9286_enum_mbus_code() returns MEDIA_BUS_FMT_UYVY8_2X8, no?

Argh, sorry - yet anther typo or copy/paste error.

Indeed, I have changed this locally to ...
   <double checks> MEDIA_BUS_FMT_UYVY8_2X8 </double checks>
as suggested. ;-)


> 
> Thanks,
> Mani