[PATCH] media: ov5640: fix get_/set_fmt colorspace related fields

[PATCH] media: ov5640: fix get_/set_fmt colorspace related fields

[Hugues Fruchet]

Fix set of missing colorspace related fields in get_/set_fmt.
Detected by v4l2-compliance tool.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/i2c/ov5640.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 03940f0..676f635 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1874,7 +1874,13 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
 		if (ov5640_formats[i].code == fmt->code)
 			break;
 	if (i >= ARRAY_SIZE(ov5640_formats))
-		fmt->code = ov5640_formats[0].code;
+		i = 0;
+
+	fmt->code = ov5640_formats[i].code;
+	fmt->colorspace = ov5640_formats[i].colorspace;
+	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
+	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
 
 	return 0;
 }
@@ -1885,6 +1891,7 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
 {
 	struct ov5640_dev *sensor = to_ov5640_dev(sd);
 	const struct ov5640_mode_info *new_mode;
+	struct v4l2_mbus_framefmt *mbus_fmt = &format->format;
 	int ret;
 
 	if (format->pad != 0)
@@ -1897,7 +1904,7 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
 		goto out;
 	}
 
-	ret = ov5640_try_fmt_internal(sd, &format->format,
+	ret = ov5640_try_fmt_internal(sd, mbus_fmt,
 				      sensor->current_fr, &new_mode);
 	if (ret)
 		goto out;
@@ -1906,12 +1913,12 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
 		struct v4l2_mbus_framefmt *fmt =
 			v4l2_subdev_get_try_format(sd, cfg, 0);
 
-		*fmt = format->format;
+		*fmt = *mbus_fmt;
 		goto out;
 	}
 
 	sensor->current_mode = new_mode;
-	sensor->fmt = format->format;
+	sensor->fmt = *mbus_fmt;
 	sensor->pending_mode_change = true;
 out:
 	mutex_unlock(&sensor->lock);
@@ -2497,16 +2504,22 @@ static int ov5640_probe(struct i2c_client *client,
 	struct fwnode_handle *endpoint;
 	struct ov5640_dev *sensor;
 	int ret;
+	struct v4l2_mbus_framefmt *fmt;
 
 	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
 	if (!sensor)
 		return -ENOMEM;
 
 	sensor->i2c_client = client;
-	sensor->fmt.code = MEDIA_BUS_FMT_UYVY8_2X8;
-	sensor->fmt.width = 640;
-	sensor->fmt.height = 480;
-	sensor->fmt.field = V4L2_FIELD_NONE;
+	fmt = &sensor->fmt;
+	fmt->code = ov5640_formats[0].code;
+	fmt->colorspace = ov5640_formats[0].colorspace;
+	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
+	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
+	fmt->width = 640;
+	fmt->height = 480;
+	fmt->field = V4L2_FIELD_NONE;
 	sensor->frame_interval.numerator = 1;
 	sensor->frame_interval.denominator = ov5640_framerates[OV5640_30_FPS];
 	sensor->current_fr = OV5640_30_FPS;
-- 
1.9.1

Re: [PATCH] media: ov5640: fix get_/set_fmt colorspace related fields

[Sakari Ailus]

Hi Hugues,

On Tue, Mar 06, 2018 at 06:04:39PM +0100, Hugues Fruchet wrote:
> Fix set of missing colorspace related fields in get_/set_fmt.
> Detected by v4l2-compliance tool.
> 
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>

Could you confirm this is the one you intended to send? There are two
others with similar content.

...

> @@ -2497,16 +2504,22 @@ static int ov5640_probe(struct i2c_client *client,
>  	struct fwnode_handle *endpoint;
>  	struct ov5640_dev *sensor;
>  	int ret;
> +	struct v4l2_mbus_framefmt *fmt;

This one I'd arrange before ret. The local variable declarations should
generally look like a Christmas tree but upside down.

If you're happy with that, I can swap the two lines as well (no need for
v2).

>  
>  	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
>  	if (!sensor)
>  		return -ENOMEM;
>  
>  	sensor->i2c_client = client;
> -	sensor->fmt.code = MEDIA_BUS_FMT_UYVY8_2X8;
> -	sensor->fmt.width = 640;
> -	sensor->fmt.height = 480;
> -	sensor->fmt.field = V4L2_FIELD_NONE;
> +	fmt = &sensor->fmt;
> +	fmt->code = ov5640_formats[0].code;
> +	fmt->colorspace = ov5640_formats[0].colorspace;
> +	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> +	fmt->width = 640;
> +	fmt->height = 480;
> +	fmt->field = V4L2_FIELD_NONE;
>  	sensor->frame_interval.numerator = 1;
>  	sensor->frame_interval.denominator = ov5640_framerates[OV5640_30_FPS];
>  	sensor->current_fr = OV5640_30_FPS;

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

[PATCH] media: dvbdev: fix building on ia64

[Mauro Carvalho Chehab]

Not sure why, but, on ia64, with Linaro's gcc 7.3 compiler,
using #ifdef (CONFIG_I2C) is not OK.

So, replace it by IS_ENABLED(CONFIG_I2C), in order to fix the
builds there.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/dvb-core/dvbdev.c | 2 +-
 include/media/dvbdev.h          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index a840133feacb..cf747d753a79 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -942,7 +942,7 @@ int dvb_usercopy(struct file *file,
 	return err;
 }
 
-#ifdef CONFIG_I2C
+#if IS_ENABLED(CONFIG_I2C)
 struct i2c_client *dvb_module_probe(const char *module_name,
 				    const char *name,
 				    struct i2c_adapter *adap,
diff --git a/include/media/dvbdev.h b/include/media/dvbdev.h
index 2d2897508590..ee91516ad074 100644
--- a/include/media/dvbdev.h
+++ b/include/media/dvbdev.h
@@ -358,7 +358,7 @@ long dvb_generic_ioctl(struct file *file,
 int dvb_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 		 int (*func)(struct file *file, unsigned int cmd, void *arg));
 
-#ifdef CONFIG_I2C
+#if IS_ENABLED(CONFIG_I2C)
 
 struct i2c_adapter;
 struct i2c_client;
-- 
2.14.3

Re: [PATCH] media: dvbdev: fix building on ia64

[Fabio Estevam]

Hi Mauro,

On Wed, Mar 7, 2018 at 6:14 AM, Mauro Carvalho Chehab
<mchehab@s-opensource.com> wrote:
> Not sure why, but, on ia64, with Linaro's gcc 7.3 compiler,
> using #ifdef (CONFIG_I2C) is not OK.

Looking at the kbuild report the failure happens when CONFIG_I2C=m.

IS_ENABLED() macro takes care of both built-in and module as it will expand to:

#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)

and that's the reason why IS_ENABLE() fixes the build problem.

Regards,

Fabio Estevam

Re: [PATCH] media: ov5640: fix get_/set_fmt colorspace related fields

[Fabio Estevam]

Hi Sakari,

On Wed, Mar 7, 2018 at 5:13 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:

>> @@ -2497,16 +2504,22 @@ static int ov5640_probe(struct i2c_client *client,
>>       struct fwnode_handle *endpoint;
>>       struct ov5640_dev *sensor;
>>       int ret;
>> +     struct v4l2_mbus_framefmt *fmt;
>
> This one I'd arrange before ret. The local variable declarations should
> generally look like a Christmas tree but upside down.

It seems Mauro is not happy with reverse Christmas tree ordering:
https://www.mail-archive.com/linux-media@vger.kernel.org/msg127221.html

Re: [PATCH] media: dvbdev: fix building on ia64

[Mauro Carvalho Chehab]

Em Wed, 7 Mar 2018 06:47:14 -0300
Fabio Estevam <festevam@gmail.com> escreveu:

> Hi Mauro,
> 
> On Wed, Mar 7, 2018 at 6:14 AM, Mauro Carvalho Chehab
> <mchehab@s-opensource.com> wrote:
> > Not sure why, but, on ia64, with Linaro's gcc 7.3 compiler,
> > using #ifdef (CONFIG_I2C) is not OK.  
> 
> Looking at the kbuild report the failure happens when CONFIG_I2C=m.
> 
> IS_ENABLED() macro takes care of both built-in and module as it will expand to:
> 
> #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)

Ah, true! Yeah, I forgot that for tri-state vars, it should test for
_MODULE variant.

> 
> and that's the reason why IS_ENABLE() fixes the build problem.

Thanks for reminding me.

Regards,
Mauro

Re: [PATCH] media: ov5640: fix get_/set_fmt colorspace related fields

[Sakari Ailus]

Hi Fabio,

On Wed, Mar 07, 2018 at 06:51:26AM -0300, Fabio Estevam wrote:
> Hi Sakari,
> 
> On Wed, Mar 7, 2018 at 5:13 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> 
> >> @@ -2497,16 +2504,22 @@ static int ov5640_probe(struct i2c_client *client,
> >>       struct fwnode_handle *endpoint;
> >>       struct ov5640_dev *sensor;
> >>       int ret;
> >> +     struct v4l2_mbus_framefmt *fmt;
> >
> > This one I'd arrange before ret. The local variable declarations should
> > generally look like a Christmas tree but upside down.
> 
> It seems Mauro is not happy with reverse Christmas tree ordering:
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg127221.html

There are other arguments supporting the change such as:

- alignment with the rest of the driver and
- putting similar definitions together (return value vs. pointers somewhere
  else).

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH] media: ov5640: fix get_/set_fmt colorspace related fields

[Hugues FRUCHET]

Hi Sakari,

This is the right one and it's OK to swap the lines for local variables, 
I'll keep this in mind for next changes.

Best regards,
Hugues.

On 03/07/2018 09:13 AM, Sakari Ailus wrote:
> Hi Hugues,
> 
> On Tue, Mar 06, 2018 at 06:04:39PM +0100, Hugues Fruchet wrote:
>> Fix set of missing colorspace related fields in get_/set_fmt.
>> Detected by v4l2-compliance tool.
>>
>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> 
> Could you confirm this is the one you intended to send? There are two
> others with similar content.
> 
> ...
> 
>> @@ -2497,16 +2504,22 @@ static int ov5640_probe(struct i2c_client *client,
>>   	struct fwnode_handle *endpoint;
>>   	struct ov5640_dev *sensor;
>>   	int ret;
>> +	struct v4l2_mbus_framefmt *fmt;
> 
> This one I'd arrange before ret. The local variable declarations should
> generally look like a Christmas tree but upside down.
> 
> If you're happy with that, I can swap the two lines as well (no need for
> v2).
> 
>>   
>>   	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
>>   	if (!sensor)
>>   		return -ENOMEM;
>>   
>>   	sensor->i2c_client = client;
>> -	sensor->fmt.code = MEDIA_BUS_FMT_UYVY8_2X8;
>> -	sensor->fmt.width = 640;
>> -	sensor->fmt.height = 480;
>> -	sensor->fmt.field = V4L2_FIELD_NONE;
>> +	fmt = &sensor->fmt;
>> +	fmt->code = ov5640_formats[0].code;
>> +	fmt->colorspace = ov5640_formats[0].colorspace;
>> +	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
>> +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>> +	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
>> +	fmt->width = 640;
>> +	fmt->height = 480;
>> +	fmt->field = V4L2_FIELD_NONE;
>>   	sensor->frame_interval.numerator = 1;
>>   	sensor->frame_interval.denominator = ov5640_framerates[OV5640_30_FPS];
>>   	sensor->current_fr = OV5640_30_FPS;
>