[PATCH 0/4] media: si4713: minor updates

[PATCH 0/4] media: si4713: minor updates

[Eduardo Valentin]

Hello Mauro and Hans,

Here are a couple of minor changes for si4713 FM transmitter driver.

These changes are also available here:
https://git.gitorious.org/si4713/si4713.git

All best,

Eduardo Valentin (4):
  media: radio: CodingStyle changes on si4713
  media: radio: correct module license (==> GPL v2)
  media: radio: add driver owner entry for radio-si4713
  media: radio: add module alias entry for radio-si4713

 drivers/media/radio/radio-si4713.c |   57 ++++++++++++++++++-----------------
 1 files changed, 29 insertions(+), 28 deletions(-)

-- 
1.7.7.1.488.ge8e1c

[PATCH 1/4] media: radio: CodingStyle changes on si4713

[Eduardo Valentin]

Minor changes to make alignment match on open parenthesis.

Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/media/radio/radio-si4713.c |   53 +++++++++++++++++------------------
 1 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/drivers/media/radio/radio-si4713.c b/drivers/media/radio/radio-si4713.c
index 1507c9d..5b5c42b 100644
--- a/drivers/media/radio/radio-si4713.c
+++ b/drivers/media/radio/radio-si4713.c
@@ -94,7 +94,7 @@ static int radio_si4713_querycap(struct file *file, void *priv,
 {
 	strlcpy(capability->driver, "radio-si4713", sizeof(capability->driver));
 	strlcpy(capability->card, "Silicon Labs Si4713 Modulator",
-				sizeof(capability->card));
+		sizeof(capability->card));
 	capability->capabilities = V4L2_CAP_MODULATOR | V4L2_CAP_RDS_OUTPUT;
 
 	return 0;
@@ -102,7 +102,7 @@ static int radio_si4713_querycap(struct file *file, void *priv,
 
 /* radio_si4713_queryctrl - enumerate control items */
 static int radio_si4713_queryctrl(struct file *file, void *priv,
-						struct v4l2_queryctrl *qc)
+				  struct v4l2_queryctrl *qc)
 {
 	/* Must be sorted from low to high control ID! */
 	static const u32 user_ctrls[] = {
@@ -152,7 +152,7 @@ static int radio_si4713_queryctrl(struct file *file, void *priv,
 		return v4l2_ctrl_query_fill(qc, 0, 0, 0, 0);
 
 	return v4l2_device_call_until_err(&rsdev->v4l2_dev, 0, core,
-						queryctrl, qc);
+					  queryctrl, qc);
 }
 
 /*
@@ -165,66 +165,66 @@ static inline struct v4l2_device *get_v4l2_dev(struct file *file)
 }
 
 static int radio_si4713_g_ext_ctrls(struct file *file, void *p,
-						struct v4l2_ext_controls *vecs)
+				    struct v4l2_ext_controls *vecs)
 {
 	return v4l2_device_call_until_err(get_v4l2_dev(file), 0, core,
-							g_ext_ctrls, vecs);
+					  g_ext_ctrls, vecs);
 }
 
 static int radio_si4713_s_ext_ctrls(struct file *file, void *p,
-						struct v4l2_ext_controls *vecs)
+				    struct v4l2_ext_controls *vecs)
 {
 	return v4l2_device_call_until_err(get_v4l2_dev(file), 0, core,
-							s_ext_ctrls, vecs);
+					  s_ext_ctrls, vecs);
 }
 
 static int radio_si4713_g_ctrl(struct file *file, void *p,
-						struct v4l2_control *vc)
+			       struct v4l2_control *vc)
 {
 	return v4l2_device_call_until_err(get_v4l2_dev(file), 0, core,
-							g_ctrl, vc);
+					  g_ctrl, vc);
 }
 
 static int radio_si4713_s_ctrl(struct file *file, void *p,
-						struct v4l2_control *vc)
+			       struct v4l2_control *vc)
 {
 	return v4l2_device_call_until_err(get_v4l2_dev(file), 0, core,
-							s_ctrl, vc);
+					  s_ctrl, vc);
 }
 
 static int radio_si4713_g_modulator(struct file *file, void *p,
-						struct v4l2_modulator *vm)
+				    struct v4l2_modulator *vm)
 {
 	return v4l2_device_call_until_err(get_v4l2_dev(file), 0, tuner,
-							g_modulator, vm);
+					  g_modulator, vm);
 }
 
 static int radio_si4713_s_modulator(struct file *file, void *p,
-						const struct v4l2_modulator *vm)
+				    const struct v4l2_modulator *vm)
 {
 	return v4l2_device_call_until_err(get_v4l2_dev(file), 0, tuner,
-							s_modulator, vm);
+					  s_modulator, vm);
 }
 
 static int radio_si4713_g_frequency(struct file *file, void *p,
-						struct v4l2_frequency *vf)
+				    struct v4l2_frequency *vf)
 {
 	return v4l2_device_call_until_err(get_v4l2_dev(file), 0, tuner,
-							g_frequency, vf);
+					  g_frequency, vf);
 }
 
 static int radio_si4713_s_frequency(struct file *file, void *p,
-						struct v4l2_frequency *vf)
+				    struct v4l2_frequency *vf)
 {
 	return v4l2_device_call_until_err(get_v4l2_dev(file), 0, tuner,
-							s_frequency, vf);
+					  s_frequency, vf);
 }
 
 static long radio_si4713_default(struct file *file, void *p,
-				bool valid_prio, int cmd, void *arg)
+				 bool valid_prio, int cmd, void *arg)
 {
 	return v4l2_device_call_until_err(get_v4l2_dev(file), 0, core,
-							ioctl, cmd, arg);
+					  ioctl, cmd, arg);
 }
 
 static struct v4l2_ioctl_ops radio_si4713_ioctl_ops = {
@@ -285,13 +285,13 @@ static int radio_si4713_pdriver_probe(struct platform_device *pdev)
 	adapter = i2c_get_adapter(pdata->i2c_bus);
 	if (!adapter) {
 		dev_err(&pdev->dev, "Cannot get i2c adapter %d\n",
-							pdata->i2c_bus);
+			pdata->i2c_bus);
 		rval = -ENODEV;
 		goto unregister_v4l2_dev;
 	}
 
 	sd = v4l2_i2c_new_subdev_board(&rsdev->v4l2_dev, adapter,
-					pdata->subdev_board_info, NULL);
+				       pdata->subdev_board_info, NULL);
 	if (!sd) {
 		dev_err(&pdev->dev, "Cannot get v4l2 subdevice\n");
 		rval = -ENODEV;
@@ -306,7 +306,7 @@ static int radio_si4713_pdriver_probe(struct platform_device *pdev)
 	}
 
 	memcpy(rsdev->radio_dev, &radio_si4713_vdev_template,
-			sizeof(radio_si4713_vdev_template));
+	       sizeof(radio_si4713_vdev_template));
 	video_set_drvdata(rsdev->radio_dev, rsdev);
 	if (video_register_device(rsdev->radio_dev, VFL_TYPE_RADIO, radio_nr)) {
 		dev_err(&pdev->dev, "Could not register video device.\n");
@@ -331,13 +331,12 @@ exit:
 static int __exit radio_si4713_pdriver_remove(struct platform_device *pdev)
 {
 	struct v4l2_device *v4l2_dev = platform_get_drvdata(pdev);
-	struct radio_si4713_device *rsdev = container_of(v4l2_dev,
-						struct radio_si4713_device,
-						v4l2_dev);
 	struct v4l2_subdev *sd = list_entry(v4l2_dev->subdevs.next,
 					    struct v4l2_subdev, list);
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct radio_si4713_device *rsdev;
 
+	rsdev = container_of(v4l2_dev, struct radio_si4713_device, v4l2_dev);
 	video_unregister_device(rsdev->radio_dev);
 	i2c_put_adapter(client->adapter);
 	v4l2_device_unregister(&rsdev->v4l2_dev);
-- 
1.7.7.1.488.ge8e1c

[PATCH 2/4] media: radio: correct module license (==> GPL v2)

[Eduardo Valentin]

As per code header comment, changing the driver license entry
to match the correct version.

Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/media/radio/radio-si4713.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/radio/radio-si4713.c b/drivers/media/radio/radio-si4713.c
index 5b5c42b..cd30a89 100644
--- a/drivers/media/radio/radio-si4713.c
+++ b/drivers/media/radio/radio-si4713.c
@@ -39,7 +39,7 @@ module_param(radio_nr, int, 0);
 MODULE_PARM_DESC(radio_nr,
 		 "Minor number for radio device (-1 ==> auto assign)");
 
-MODULE_LICENSE("GPL");
+MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Eduardo Valentin <eduardo.valentin@nokia.com>");
 MODULE_DESCRIPTION("Platform driver for Si4713 FM Radio Transmitter");
 MODULE_VERSION("0.0.1");
-- 
1.7.7.1.488.ge8e1c

[PATCH 3/4] media: radio: add driver owner entry for radio-si4713

[Eduardo Valentin]

Simple addition of platform_driver->driver->owner for radio-si4713.

Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/media/radio/radio-si4713.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/media/radio/radio-si4713.c b/drivers/media/radio/radio-si4713.c
index cd30a89..ae70930 100644
--- a/drivers/media/radio/radio-si4713.c
+++ b/drivers/media/radio/radio-si4713.c
@@ -347,6 +347,7 @@ static int __exit radio_si4713_pdriver_remove(struct platform_device *pdev)
 static struct platform_driver radio_si4713_pdriver = {
 	.driver		= {
 		.name	= "radio-si4713",
+		.owner	= THIS_MODULE,
 	},
 	.probe		= radio_si4713_pdriver_probe,
 	.remove         = __exit_p(radio_si4713_pdriver_remove),
-- 
1.7.7.1.488.ge8e1c

[PATCH 4/4] media: radio: add module alias entry for radio-si4713

[Eduardo Valentin]

Add MODULE_ALIAS entry for radio-si4713 platform driver.

Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/media/radio/radio-si4713.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/media/radio/radio-si4713.c b/drivers/media/radio/radio-si4713.c
index ae70930..9dda9c3 100644
--- a/drivers/media/radio/radio-si4713.c
+++ b/drivers/media/radio/radio-si4713.c
@@ -43,6 +43,7 @@ MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Eduardo Valentin <eduardo.valentin@nokia.com>");
 MODULE_DESCRIPTION("Platform driver for Si4713 FM Radio Transmitter");
 MODULE_VERSION("0.0.1");
+MODULE_ALIAS("platform:radio-si4713");
 
 /* Driver state struct */
 struct radio_si4713_device {
-- 
1.7.7.1.488.ge8e1c

Re: [PATCH 0/4] media: si4713: minor updates

[Hans Verkuil]

Hi Eduardo!

On Tue 19 March 2013 16:41:30 Eduardo Valentin wrote:
> Hello Mauro and Hans,
> 
> Here are a couple of minor changes for si4713 FM transmitter driver.

Thanks!

Patches 2-4 are fine, but I don't really see the point of the first patch
(except for the last chunk which is a real improvement).

The Codingstyle doesn't require such alignment, and in fact it says:

"Descendants are always substantially shorter than the parent and
are placed substantially to the right. The same applies to function headers
with a long argument list."

Unless Mauro thinks otherwise, I would leave all the alignment stuff alone
and just post a version with the last chunk.

For patches 2-4:

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Are you still able to test the si4713 driver? Because I have patches
outstanding that I would love for someone to test for me:

http://git.linuxtv.org/hverkuil/media_tree.git/shortlog/refs/heads/si4713

In particular, run the latest v4l2-compliance test over it.

Regards,

	Hans

> 
> These changes are also available here:
> https://git.gitorious.org/si4713/si4713.git
> 
> All best,
> 
> Eduardo Valentin (4):
>   media: radio: CodingStyle changes on si4713
>   media: radio: correct module license (==> GPL v2)
>   media: radio: add driver owner entry for radio-si4713
>   media: radio: add module alias entry for radio-si4713
> 
>  drivers/media/radio/radio-si4713.c |   57 ++++++++++++++++++-----------------
>  1 files changed, 29 insertions(+), 28 deletions(-)
> 
> 

Re: [PATCH 0/4] media: si4713: minor updates

[edubezval]

Hi Hans,

My last email didn't reach the list, so re-sending.

On Wed, Mar 20, 2013 at 5:04 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Hi Eduardo!
>
> On Tue 19 March 2013 16:41:30 Eduardo Valentin wrote:
> > Hello Mauro and Hans,
> >
> > Here are a couple of minor changes for si4713 FM transmitter driver.
>
> Thanks!

No problem!

>
> Patches 2-4 are fine, but I don't really see the point of the first patch
> (except for the last chunk which is a real improvement).
>
> The Codingstyle doesn't require such alignment, and in fact it says:
>
> "Descendants are always substantially shorter than the parent and
> are placed substantially to the right. The same applies to function
> headers
> with a long argument list."
>
> Unless Mauro thinks otherwise, I would leave all the alignment stuff alone
> and just post a version with the last chunk.
>

OK. The chunks on patch 01 are mostly to get rid of these outputs out
of checkpatch.pl --strict -f drivers/media/radio/radio-si4713.c:
CHECK: Alignment should match open parenthesis
#97: FILE: media/radio/radio-si4713.c:97:
+       strlcpy(capability->card, "Silicon Labs Si4713 Modulator",
+                               sizeof(capability->card));


> For patches 2-4:
>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>


Nice! I will add your Acked-by.


> Are you still able to test the si4713 driver? Because I have patches



I see. In fact that is my next step on my todo list for si4713. I
still have an n900 that I can fetch from my drobe, so just a matter of
booting it with newer kernel.

> outstanding that I would love for someone to test for me:
>
> http://git.linuxtv.org/hverkuil/media_tree.git/shortlog/refs/heads/si4713
>
> In particular, run the latest v4l2-compliance test over it.
>


OK. I will check your branch once I get my setup done and let you know.

> Regards,
>
>         Hans
>
> >
> > These changes are also available here:
> > https://git.gitorious.org/si4713/si4713.git
> >
> > All best,
> >
> > Eduardo Valentin (4):
> >   media: radio: CodingStyle changes on si4713
> >   media: radio: correct module license (==> GPL v2)
> >   media: radio: add driver owner entry for radio-si4713
> >   media: radio: add module alias entry for radio-si4713
> >
> >  drivers/media/radio/radio-si4713.c |   57
> > ++++++++++++++++++-----------------
> >  1 files changed, 29 insertions(+), 28 deletions(-)
> >
> >




--
Eduardo Bezerra Valentin

Re: [PATCH 0/4] media: si4713: minor updates

[Hans Verkuil]

On Wed 20 March 2013 12:11:11 edubezval@gmail.com wrote:
> Hi Hans,
> 
> My last email didn't reach the list, so re-sending.
> 
> On Wed, Mar 20, 2013 at 5:04 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >
> > Hi Eduardo!
> >
> > On Tue 19 March 2013 16:41:30 Eduardo Valentin wrote:
> > > Hello Mauro and Hans,
> > >
> > > Here are a couple of minor changes for si4713 FM transmitter driver.
> >
> > Thanks!
> 
> No problem!
> 
> >
> > Patches 2-4 are fine, but I don't really see the point of the first patch
> > (except for the last chunk which is a real improvement).
> >
> > The Codingstyle doesn't require such alignment, and in fact it says:
> >
> > "Descendants are always substantially shorter than the parent and
> > are placed substantially to the right. The same applies to function
> > headers
> > with a long argument list."
> >
> > Unless Mauro thinks otherwise, I would leave all the alignment stuff alone
> > and just post a version with the last chunk.
> >
> 
> OK. The chunks on patch 01 are mostly to get rid of these outputs out
> of checkpatch.pl --strict -f drivers/media/radio/radio-si4713.c:
> CHECK: Alignment should match open parenthesis
> #97: FILE: media/radio/radio-si4713.c:97:
> +       strlcpy(capability->card, "Silicon Labs Si4713 Modulator",
> +                               sizeof(capability->card));

Ah. I never use --strict, so that's why I never saw that. I have no major
problems with this so I give my Ack for this patch as well (even though
I believe it is a bit overkill :-) ).

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

> 
> 
> > For patches 2-4:
> >
> > Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> >
> 
> 
> Nice! I will add your Acked-by.
> 
> 
> > Are you still able to test the si4713 driver? Because I have patches
> 
> 
> 
> I see. In fact that is my next step on my todo list for si4713. I
> still have an n900 that I can fetch from my drobe, so just a matter of
> booting it with newer kernel.
> 
> > outstanding that I would love for someone to test for me:
> >
> > http://git.linuxtv.org/hverkuil/media_tree.git/shortlog/refs/heads/si4713
> >
> > In particular, run the latest v4l2-compliance test over it.
> >
> 
> 
> OK. I will check your branch once I get my setup done and let you know.

Great! Let me quickly rebase my tree first. I'll mail you when that's done.

Regards,

	Hans

Re: [PATCH 0/4] media: si4713: minor updates

[edubezval]

Hans,


<snip>

>> > Are you still able to test the si4713 driver? Because I have patches
>>
>>
>>
>> I see. In fact that is my next step on my todo list for si4713. I
>> still have an n900 that I can fetch from my drobe, so just a matter of
>> booting it with newer kernel.
>>
>> > outstanding that I would love for someone to test for me:
>> >
>> > http://git.linuxtv.org/hverkuil/media_tree.git/shortlog/refs/heads/si4713

So, I got my hands on my old n900 and thanks to Aaro and lo community
I could still boot it with 3.9-rc3 kernel! amazing!

I didn't have the time to look at your patches, but I could do a blind
run of v4l2-compliance -r 0 on n900. It follows the results:

#1 on my branch which is
is radio
Driver Info:
	Driver name   : radio-si4713
	Card type     : Silicon Labs Si4713 Modulator
	Bus info      :
	Driver version: 3.9.0
	Capabilities  : 0x00080800
		RDS Output
		Modulator

Compliance test for device /dev/radio0 (not using libv4l2):

Required ioctls:
		fail: v4l2-compliance.cpp(226): string empty
		fail: v4l2-compliance.cpp(278): check_ustring(vcap.bus_info,
sizeof(vcap.bus_info))
	test VIDIOC_QUERYCAP: FAIL

Allow for multiple opens:
	test second radio open: OK
		fail: v4l2-compliance.cpp(226): string empty
		fail: v4l2-compliance.cpp(278): check_ustring(vcap.bus_info,
sizeof(vcap.bus_info))
	test VIDIOC_QUERYCAP: FAIL
		fail: v4l2-compliance.cpp(336): doioctl(node, VIDIOC_G_PRIORITY, &prio)
	test VIDIOC_G/S_PRIORITY: FAIL

Debug ioctls:
	test VIDIOC_DBG_G_CHIP_IDENT: OK (Not Supported)
	test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
	test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
	test VIDIOC_G/S_TUNER: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK
	test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
	test VIDIOC_ENUMAUDIO: OK (Not Supported)
	test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDIO: OK (Not Supported)
	Inputs: 0 Audio Inputs: 0 Tuners: 0

Output ioctls:
		fail: v4l2-test-input-output.cpp(567): V4L2_TUNER_CAP_RDS set, but
not V4L2_CAP_READWRITE
		fail: v4l2-test-input-output.cpp(590): invalid modulator 0
	test VIDIOC_G/S_MODULATOR: FAIL
		fail: v4l2-test-input-output.cpp(675): could get frequency for
invalid modulator 0
	test VIDIOC_G/S_FREQUENCY: FAIL
	test VIDIOC_ENUMAUDOUT: OK (Not Supported)
	test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDOUT: OK (Not Supported)
	Outputs: 0 Audio Outputs: 0 Modulators: 0

Control ioctls:
		fail: v4l2-test-controls.cpp(145): can do querymenu on a non-menu control
		fail: v4l2-test-controls.cpp(201): invalid control 00980001
	test VIDIOC_QUERYCTRL/MENU: FAIL
		fail: v4l2-test-controls.cpp(442): g_ctrl accepted invalid control ID
	test VIDIOC_G/S_CTRL: FAIL
		fail: v4l2-test-controls.cpp(511): g_ext_ctrls does not support count == 0
	test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
	test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
	Standard Controls: 0 Private Controls: 0

Input/Output configuration ioctls:
	test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
	test VIDIOC_ENUM/G/S/QUERY_DV_PRESETS: OK (Not Supported)
	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
	test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)

Format ioctls:
	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported)
	test VIDIOC_G/S_PARM: OK (Not Supported)
	test VIDIOC_G_FBUF: OK (Not Supported)
	test VIDIOC_G_FMT: OK (Not Supported)
	test VIDIOC_TRY_FMT: OK (Not Supported)
	test VIDIOC_S_FMT: OK (Not Supported)
	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)

Codec ioctls:
	test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
	test VIDIOC_G_ENC_INDEX: OK (Not Supported)
	test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
	test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported)

Total: 38, Succeeded: 30, Failed: 8, Warnings: 0


# on your branch on the other hand I get a NULL pointer:
[    8.995758] Unable to handle kernel NULL pointer dereference at
virtual address 00000008
[    8.996093] pgd = ce5e8000
[    8.996185] [00000008] *pgd=8e532831, *pte=00000000, *ppte=00000000
[    8.996459] Internal error: Oops: 17 [#1] SMP ARM
[    8.996612] Modules linked in: si4713_i2c radio_si4713 v4l2_common videodev
[    8.996948] CPU: 0    Tainted: G        W
(3.9.0-rc1-00205-g0826407-dirty #8)
[    8.997283] PC is at v4l2_prio_open+0x10/0x58 [videodev]
[    8.997528] LR is at v4l2_fh_add+0x24/0x60 [videodev]
[    8.997680] pc : [<bf0000a8>]    lr : [<bf0060a8>]    psr: 80000013
[    8.997680] sp : ce515db0  ip : ce515d3c  fp : ce5189c0
[    8.997955] r10: ce0b5240  r9 : ce4dd1f0  r8 : 00000000
[    8.998107] r7 : ce65d4c8  r6 : ce4df640  r5 : ce65d4c8  r4 : ce4df640
[    8.998260] r3 : 00000008  r2 : ce4df684  r1 : ce4df650  r0 : 00000000
[    8.998474] Flags: Nzcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
[    8.998657] Control: 10c5387d  Table: 8e5e8019  DAC: 00000015
[    8.998809] Process v4l2-compliance (pid: 851, stack limit = 0xce514240)
[    8.998992] Stack: (0xce515db0 to 0xce516000)
[    8.999114] 5da0:                                     ce5189c0
bf006198 bf006150 ce65d4d0
[    9.001647] 5dc0: ce5189c0 bf00059c bf0004fc ce4dd0d8 ce5189c0
ce4ca840 00000000 c0120220
[    9.004211] 5de0: ce5189c0 00000000 00000000 ce5189c0 c0120190
ce4dd0d8 ce5189c8 c011ac5c
[    9.006774] 5e00: ce515ec8 ce515f78 00000002 00000000 ce515ec0
00000026 00000000 c011add4
[    9.009338] 5e20: ce515f00 c01294dc ce072800 c0126940 60000013
ce515e48 ce4dd130 cd802708
[    9.011962] 5e40: 00000000 00000000 00000000 00000006 ce4fb015
ce515f78 000efae8 c0127490
[    9.014678] 5e60: 00000000 00000000 ce4dd0d8 ce034a58 c08158f8
ce515f00 ce5189c0 ce514000
[    9.017425] 5e80: ce4fb000 ce515ec8 00000000 ce515f78 ce515ec0
c0129b90 ce515ec8 ce4fb000
[    9.020141] 5ea0: ce2a7400 ce51a104 00000000 c009a2b8 00000000
00000000 ce514000 ce02d7d0
[    9.022796] 5ec0: ce018150 cd9f8660 00000000 00000000 00000000
ce515f78 ce515f00 00000001
[    9.025451] 5ee0: ffffff9c ce4fb000 ce514000 00000000 000efae8
c012a044 00000041 ce02d7c0
[    9.028076] 5f00: ce018150 cd9f8660 b4879d71 00000006 ce4fb015
ce515f78 00000000 cd9c8858
[    9.030731] 5f20: ce4dd0d8 00000101 00000004 00000000 00000000
ce02d780 00000000 c0137724
[    9.033355] 5f40: ce02d7c0 00000002 000efae8 ce4fb000 00000002
00000000 ce4fb000 00000002
[    9.035949] 5f60: 00000003 ffffff9c 00000001 c011a8c0 00000000
ef000000 00000002 c0090000
[    9.038574] 5f80: 00000026 00000100 00000000 00000000 000ef95c
00000000 00000005 c00144a8
[    9.041198] 5fa0: 00000000 c0014300 00000000 000ef95c 000efae8
00000002 00000000 00000000
[    9.043792] 5fc0: 00000000 000ef95c 00000000 00000005 becbecc8
00000000 0014a808 000efae8
[    9.046386] 5fe0: becbeceb becbe0e8 000097d8 0001cbac 60000010
000efae8 00000000 00000000
[    9.049133] [<bf0000a8>] (v4l2_prio_open+0x10/0x58 [videodev]) from
[<bf0060a8>] (v4l2_fh_add+0x24/0x60 [videodev])
[    9.054534] [<bf0060a8>] (v4l2_fh_add+0x24/0x60 [videodev]) from
[<bf006198>] (v4l2_fh_open+0x48/0x58 [videodev])
[    9.060089] [<bf006198>] (v4l2_fh_open+0x48/0x58 [videodev]) from
[<bf00059c>] (v4l2_open+0xa0/0xe0 [videodev])
[    9.065856] [<bf00059c>] (v4l2_open+0xa0/0xe0 [videodev]) from
[<c0120220>] (chrdev_open+0x90/0x150)
[    9.071746] [<c0120220>] (chrdev_open+0x90/0x150) from [<c011ac5c>]
(do_dentry_open+0x1f8/0x280)
[    9.074890] [<c011ac5c>] (do_dentry_open+0x1f8/0x280) from
[<c011add4>] (finish_open+0x34/0x50)
[    9.078063] [<c011add4>] (finish_open+0x34/0x50) from [<c01294dc>]
(do_last+0x5b0/0xbb4)
[    9.081237] [<c01294dc>] (do_last+0x5b0/0xbb4) from [<c0129b90>]
(path_openat+0xb0/0x464)
[    9.084442] [<c0129b90>] (path_openat+0xb0/0x464) from [<c012a044>]
(do_filp_open+0x30/0x84)
[    9.087646] [<c012a044>] (do_filp_open+0x30/0x84) from [<c011a8c0>]
(do_sys_open+0xe0/0x170)
[    9.090911] [<c011a8c0>] (do_sys_open+0xe0/0x170) from [<c0014300>]
(ret_fast_syscall+0x0/0x3c)
[    9.094146] Code: e5913000 e3530002 012fff1e e2803008 (e193cf9f)
[    9.097686] ---[ end trace f9c354f7ca1aeb09 ]---
>> >
>> > In particular, run the latest v4l2-compliance test over it.
>> >
>>
>>
>> OK. I will check your branch once I get my setup done and let you know.
>
> Great! Let me quickly rebase my tree first. I'll mail you when that's done.
>
> Regards,
>
>         Hans



-- 
Eduardo Bezerra Valentin

Re: [PATCH 0/4] media: si4713: minor updates

[Hans Verkuil]

On Thu March 21 2013 19:46:03 edubezval@gmail.com wrote:
> Hans,
> 
> 
> <snip>
> 
> >> > Are you still able to test the si4713 driver? Because I have patches
> >>
> >>
> >>
> >> I see. In fact that is my next step on my todo list for si4713. I
> >> still have an n900 that I can fetch from my drobe, so just a matter of
> >> booting it with newer kernel.
> >>
> >> > outstanding that I would love for someone to test for me:
> >> >
> >> > http://git.linuxtv.org/hverkuil/media_tree.git/shortlog/refs/heads/si4713
> 
> So, I got my hands on my old n900 and thanks to Aaro and lo community
> I could still boot it with 3.9-rc3 kernel! amazing!
> 
> I didn't have the time to look at your patches, but I could do a blind
> run of v4l2-compliance -r 0 on n900. It follows the results:

... 

> 
> 
> # on your branch on the other hand I get a NULL pointer:

I've fixed that (v4l2_dev was never initialized), and I've also rebased my tree
to the latest code. Can you try again?

Regards,

	Hans

Re: [PATCH 0/4] media: si4713: minor updates

[edubezval]

Hello Hans,

On Fri, Mar 22, 2013 at 10:04 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
<snip>

>>
>> # on your branch on the other hand I get a NULL pointer:
>
> I've fixed that (v4l2_dev was never initialized), and I've also rebased my tree
> to the latest code. Can you try again?
>

This time I get a kernel crash at _power. Unfortunately I cannot fetch
the crash log as I am not having access to a serial line (using vga
console) and in my setup mtdoops is not working somehow.


Sequence is v4l2_ioctl->video_usercopy->__video_do_ioctl->v4l_s_ctrl->v4l2_s_ctrl->set_ctrl_lock->try_or_set_cluster->si4713_s_ctrl->si4713_set_power_state->mutex_lock_nested->lock_acquire.


I 'd need to spend some time on it to understand better your patches
and help you to get this working. And for that I'd prob need to spend
some time to either hack a serial line or get mtdoops to work :-)

> Regards,
>
>         Hans



-- 
Eduardo Bezerra Valentin

Re: [PATCH 0/4] media: si4713: minor updates

[Hans Verkuil]

On Fri March 22 2013 17:26:57 edubezval@gmail.com wrote:
> Hello Hans,
> 
> On Fri, Mar 22, 2013 at 10:04 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> <snip>
> 
> >>
> >> # on your branch on the other hand I get a NULL pointer:
> >
> > I've fixed that (v4l2_dev was never initialized), and I've also rebased my tree
> > to the latest code. Can you try again?
> >
> 
> This time I get a kernel crash at _power. Unfortunately I cannot fetch
> the crash log as I am not having access to a serial line (using vga
> console) and in my setup mtdoops is not working somehow.
> 
> 
> Sequence is v4l2_ioctl->video_usercopy->__video_do_ioctl->v4l_s_ctrl->v4l2_s_ctrl->set_ctrl_lock->try_or_set_cluster->si4713_s_ctrl->si4713_set_power_state->mutex_lock_nested->lock_acquire.
> 
> 
> I 'd need to spend some time on it to understand better your patches
> and help you to get this working. And for that I'd prob need to spend
> some time to either hack a serial line or get mtdoops to work :-)

You're doing fine: that was all the info I needed. Can you do a git pull and
try again?

Regards,

	Hans

Re: [PATCH 0/4] media: si4713: minor updates

[edubezval]

Hans,


On Fri, Mar 22, 2013 at 12:45 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Fri March 22 2013 17:26:57 edubezval@gmail.com wrote:
>> Hello Hans,
>>
>> On Fri, Mar 22, 2013 at 10:04 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> <snip>
>>
>> >>
>> >> # on your branch on the other hand I get a NULL pointer:
>> >
>> > I've fixed that (v4l2_dev was never initialized), and I've also rebased my tree
>> > to the latest code. Can you try again?
>> >
>>
>> This time I get a kernel crash at _power. Unfortunately I cannot fetch
>> the crash log as I am not having access to a serial line (using vga
>> console) and in my setup mtdoops is not working somehow.
>>
>>
>> Sequence is v4l2_ioctl->video_usercopy->__video_do_ioctl->v4l_s_ctrl->v4l2_s_ctrl->set_ctrl_lock->try_or_set_cluster->si4713_s_ctrl->si4713_set_power_state->mutex_lock_nested->lock_acquire.
>>
>>
>> I 'd need to spend some time on it to understand better your patches
>> and help you to get this working. And for that I'd prob need to spend
>> some time to either hack a serial line or get mtdoops to work :-)
>
> You're doing fine: that was all the info I needed. Can you do a git pull and
> try again?
>

Sure. Your patch removes the locks but I believe the set_power is used in other
places as well. And has a misspell: s/clocks/locks.

But now I still get the nested lock issue. But this time around
si4713_setup, that is called from _s_ctrl and which in turns calls
v4l2_ctrl_handler_setup, which call s_ctrl again.

> Regards,
>
>         Hans



-- 
Eduardo Bezerra Valentin

Re: [PATCH 0/4] media: si4713: minor updates

[Hans Verkuil]

On Fri March 22 2013 17:59:00 edubezval@gmail.com wrote:
> Hans,
> 
> 
> On Fri, Mar 22, 2013 at 12:45 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > On Fri March 22 2013 17:26:57 edubezval@gmail.com wrote:
> >> Hello Hans,
> >>
> >> On Fri, Mar 22, 2013 at 10:04 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >> <snip>
> >>
> >> >>
> >> >> # on your branch on the other hand I get a NULL pointer:
> >> >
> >> > I've fixed that (v4l2_dev was never initialized), and I've also rebased my tree
> >> > to the latest code. Can you try again?
> >> >
> >>
> >> This time I get a kernel crash at _power. Unfortunately I cannot fetch
> >> the crash log as I am not having access to a serial line (using vga
> >> console) and in my setup mtdoops is not working somehow.
> >>
> >>
> >> Sequence is v4l2_ioctl->video_usercopy->__video_do_ioctl->v4l_s_ctrl->v4l2_s_ctrl->set_ctrl_lock->try_or_set_cluster->si4713_s_ctrl->si4713_set_power_state->mutex_lock_nested->lock_acquire.
> >>
> >>
> >> I 'd need to spend some time on it to understand better your patches
> >> and help you to get this working. And for that I'd prob need to spend
> >> some time to either hack a serial line or get mtdoops to work :-)
> >
> > You're doing fine: that was all the info I needed. Can you do a git pull and
> > try again?
> >
> 
> Sure. Your patch removes the locks but I believe the set_power is used in other
> places as well. And has a misspell: s/clocks/locks.
> 
> But now I still get the nested lock issue. But this time around
> si4713_setup, that is called from _s_ctrl and which in turns calls
> v4l2_ctrl_handler_setup, which call s_ctrl again.

One last try: again do a git pull and see what happens.

I'm on irc at the moment, that's a bit more interactive.

It's clear by the way that I need to study the locking scheme in this
driver, but I'd really like to see the v4l2-compliance output :-)

Regards,

	Hans