[PATCH 01/10] staging: solo6x10: Fix TODO file with proper maintainer

[PATCH 01/10] staging: solo6x10: Fix TODO file with proper maintainer

[Ezequiel Garcia]

Mauro Carvalho Chehab is the current maintainer of staging/media.

Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 drivers/staging/media/solo6x10/TODO |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/media/solo6x10/TODO b/drivers/staging/media/solo6x10/TODO
index 7e6c4fa..539f739 100644
--- a/drivers/staging/media/solo6x10/TODO
+++ b/drivers/staging/media/solo6x10/TODO
@@ -20,5 +20,5 @@ TODO (general):
 	  - implement loopback of external sound jack with incoming audio?
 	  - implement pause/resume
 
-Plase send patches to Greg Kroah-Hartman <greg@kroah.com> and Cc Ben Collins
+Plase send patches to Mauro Carvalho Chehab <mchehab@redhat.com> and Cc Ben Collins
 <bcollins@bluecherry.net>
-- 
1.7.4.4

[PATCH 02/10] staging: solo6x10: Use linux/{io,uaccess}.h instead of asm/{io,uaccess}.h

[Ezequiel Garcia]

Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 drivers/staging/media/solo6x10/gpio.c     |    2 +-
 drivers/staging/media/solo6x10/solo6x10.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/solo6x10/gpio.c b/drivers/staging/media/solo6x10/gpio.c
index 0925e6f..6129111 100644
--- a/drivers/staging/media/solo6x10/gpio.c
+++ b/drivers/staging/media/solo6x10/gpio.c
@@ -19,7 +19,7 @@
 
 #include <linux/kernel.h>
 #include <linux/fs.h>
-#include <asm/uaccess.h>
+#include <linux/uaccess.h>
 #include "solo6x10.h"
 
 static void solo_gpio_mode(struct solo_dev *solo_dev,
diff --git a/drivers/staging/media/solo6x10/solo6x10.h b/drivers/staging/media/solo6x10/solo6x10.h
index abee721..5e9b399 100644
--- a/drivers/staging/media/solo6x10/solo6x10.h
+++ b/drivers/staging/media/solo6x10/solo6x10.h
@@ -29,7 +29,7 @@
 #include <linux/wait.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
-#include <asm/io.h>
+#include <linux/io.h>
 #include <linux/atomic.h>
 #include <linux/videodev2.h>
 #include <media/v4l2-dev.h>
-- 
1.7.4.4

[PATCH 03/10] staging: solo6x10: Replace C++ style comment with C style

[Ezequiel Garcia]

Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 drivers/staging/media/solo6x10/v4l2-enc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/media/solo6x10/v4l2-enc.c b/drivers/staging/media/solo6x10/v4l2-enc.c
index f8f0da9..9333a00 100644
--- a/drivers/staging/media/solo6x10/v4l2-enc.c
+++ b/drivers/staging/media/solo6x10/v4l2-enc.c
@@ -466,7 +466,7 @@ static void write_bytes(u8 **out, unsigned *bits, const u8 *src, unsigned count)
 static void write_bits(u8 **out, unsigned *bits, u32 value, unsigned count)
 {
 
-	value <<= 32 - count; // shift to the right
+	value <<= 32 - count; /* shift to the right */
 
 	while (count--) {
 		**out <<= 1;
-- 
1.7.4.4

[PATCH 04/10] staging: solo6x10: Use DEFINE_PCI_DEVICE_TABLE for struct pci_device_id

[Ezequiel Garcia]

Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 drivers/staging/media/solo6x10/core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/media/solo6x10/core.c b/drivers/staging/media/solo6x10/core.c
index d2fd842..8c4f5cf 100644
--- a/drivers/staging/media/solo6x10/core.c
+++ b/drivers/staging/media/solo6x10/core.c
@@ -291,7 +291,7 @@ static void __devexit solo_pci_remove(struct pci_dev *pdev)
 	free_solo_dev(solo_dev);
 }
 
-static struct pci_device_id solo_id_table[] = {
+static DEFINE_PCI_DEVICE_TABLE(solo_id_table) = {
 	/* 6010 based cards */
 	{PCI_DEVICE(PCI_VENDOR_ID_SOFTLOGIC, PCI_DEVICE_ID_SOLO6010)},
 	{PCI_DEVICE(PCI_VENDOR_ID_SOFTLOGIC, PCI_DEVICE_ID_SOLO6110),
-- 
1.7.4.4

[PATCH 05/10] staging: solo6x10: Remove format type mismatch warning

[Ezequiel Garcia]

The patch removes this warning:
warning: format ‘%d’ expects type ‘int’,
but argument 2 has type ‘long unsigned int’

Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 drivers/staging/media/solo6x10/tw28.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/solo6x10/tw28.c b/drivers/staging/media/solo6x10/tw28.c
index db56b42..f946f68 100644
--- a/drivers/staging/media/solo6x10/tw28.c
+++ b/drivers/staging/media/solo6x10/tw28.c
@@ -586,11 +586,11 @@ int solo_tw28_init(struct solo_dev *solo_dev)
 		 solo_dev->tw28_cnt, solo_dev->tw28_cnt == 1 ? "" : "s");
 
 	if (solo_dev->tw2865)
-		printk(" tw2865[%d]", hweight32(solo_dev->tw2865));
+		printk(" tw2865[%lu]", hweight32(solo_dev->tw2865));
 	if (solo_dev->tw2864)
-		printk(" tw2864[%d]", hweight32(solo_dev->tw2864));
+		printk(" tw2864[%lu]", hweight32(solo_dev->tw2864));
 	if (solo_dev->tw2815)
-		printk(" tw2815[%d]", hweight32(solo_dev->tw2815));
+		printk(" tw2815[%lu]", hweight32(solo_dev->tw2815));
 	printk("\n");
 
 	return 0;
-- 
1.7.4.4

[PATCH 06/10] staging: solo6x10: Replace printk(KERN_WARNING with dev_warn

[Ezequiel Garcia]

Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 drivers/staging/media/solo6x10/p2m.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/solo6x10/p2m.c b/drivers/staging/media/solo6x10/p2m.c
index 56210f0..1eb9fb3 100644
--- a/drivers/staging/media/solo6x10/p2m.c
+++ b/drivers/staging/media/solo6x10/p2m.c
@@ -231,15 +231,15 @@ static void run_p2m_test(struct solo_dev *solo_dev)
 	u32 size = SOLO_JPEG_EXT_ADDR(solo_dev) + SOLO_JPEG_EXT_SIZE(solo_dev);
 	int i, d;
 
-	printk(KERN_WARNING "%s: Testing %u bytes of external ram\n",
-	       SOLO6X10_NAME, size);
+	dev_warn(&solo_dev->pdev->dev, "Testing %u bytes of external ram\n",
+		 size);
 
 	for (i = 0; i < size; i += TEST_CHUNK_SIZE)
 		for (d = 0; d < 4; d++)
 			errs += p2m_test(solo_dev, d, i, TEST_CHUNK_SIZE);
 
-	printk(KERN_WARNING "%s: Found %llu errors during p2m test\n",
-	       SOLO6X10_NAME, errs);
+	dev_warn(&solo_dev->pdev->dev, "Found %llu errors during p2m test\n",
+	       errs);
 
 	return;
 }
-- 
1.7.4.4

[PATCH 07/10] staging: solo6x10: Merge quoted string split across lines

[Ezequiel Garcia]

Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 drivers/staging/media/solo6x10/core.c |    4 ++--
 drivers/staging/media/solo6x10/v4l2.c |    6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/solo6x10/core.c b/drivers/staging/media/solo6x10/core.c
index 8c4f5cf..2cfe906 100644
--- a/drivers/staging/media/solo6x10/core.c
+++ b/drivers/staging/media/solo6x10/core.c
@@ -174,8 +174,8 @@ static int __devinit solo_pci_probe(struct pci_dev *pdev,
 		solo_dev->nr_ext = 2;
 		break;
 	default:
-		dev_warn(&pdev->dev, "Invalid chip_id 0x%02x, "
-			 "defaulting to 4 channels\n",
+		dev_warn(&pdev->dev,
+			 "Invalid chip_id 0x%02x, defaulting to 4 channels\n",
 			 chip_id);
 	case 5:
 		solo_dev->nr_chans = 4;
diff --git a/drivers/staging/media/solo6x10/v4l2.c b/drivers/staging/media/solo6x10/v4l2.c
index 571c3a3..90e1379 100644
--- a/drivers/staging/media/solo6x10/v4l2.c
+++ b/drivers/staging/media/solo6x10/v4l2.c
@@ -933,9 +933,9 @@ int solo_v4l2_init(struct solo_dev *solo_dev)
 	if (video_nr != -1)
 		video_nr++;
 
-	dev_info(&solo_dev->pdev->dev, "Display as /dev/video%d with "
-		 "%d inputs (%d extended)\n", solo_dev->vfd->num,
-		 solo_dev->nr_chans, solo_dev->nr_ext);
+	dev_info(&solo_dev->pdev->dev,
+		"Display as /dev/video%d with %d inputs (%d extended)\n",
+		 solo_dev->vfd->num, solo_dev->nr_chans, solo_dev->nr_ext);
 
 	/* Cycle all the channels and clear */
 	for (i = 0; i < solo_dev->nr_chans; i++) {
-- 
1.7.4.4

[PATCH 08/10] staging: solo6x10: Declare static const array properly

[Ezequiel Garcia]

Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 drivers/staging/media/solo6x10/v4l2.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/solo6x10/v4l2.c b/drivers/staging/media/solo6x10/v4l2.c
index 90e1379..1f896b9 100644
--- a/drivers/staging/media/solo6x10/v4l2.c
+++ b/drivers/staging/media/solo6x10/v4l2.c
@@ -588,12 +588,12 @@ static int solo_querycap(struct file *file, void  *priv,
 static int solo_enum_ext_input(struct solo_dev *solo_dev,
 			       struct v4l2_input *input)
 {
-	static const char *dispnames_1[] = { "4UP" };
-	static const char *dispnames_2[] = { "4UP-1", "4UP-2" };
-	static const char *dispnames_5[] = {
+	static const char * const dispnames_1[] = { "4UP" };
+	static const char * const dispnames_2[] = { "4UP-1", "4UP-2" };
+	static const char * const dispnames_5[] = {
 		"4UP-1", "4UP-2", "4UP-3", "4UP-4", "16UP"
 	};
-	const char **dispnames;
+	const char * const *dispnames;
 
 	if (input->index >= (solo_dev->nr_chans + solo_dev->nr_ext))
 		return -EINVAL;
-- 
1.7.4.4

[PATCH 09/10] staging: solo6x10: Fix several over 80 character lines

[Ezequiel Garcia]

Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 drivers/staging/media/solo6x10/i2c.c      |    3 ++-
 drivers/staging/media/solo6x10/v4l2-enc.c |   28 ++++++++++++++--------------
 drivers/staging/media/solo6x10/v4l2.c     |    5 ++++-
 3 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/media/solo6x10/i2c.c b/drivers/staging/media/solo6x10/i2c.c
index ef95a50..795818b 100644
--- a/drivers/staging/media/solo6x10/i2c.c
+++ b/drivers/staging/media/solo6x10/i2c.c
@@ -288,7 +288,8 @@ int solo_i2c_init(struct solo_dev *solo_dev)
 	for (i = 0; i < SOLO_I2C_ADAPTERS; i++) {
 		struct i2c_adapter *adap = &solo_dev->i2c_adap[i];
 
-		snprintf(adap->name, I2C_NAME_SIZE, "%s I2C %d", SOLO6X10_NAME, i);
+		snprintf(adap->name, I2C_NAME_SIZE,
+			"%s I2C %d", SOLO6X10_NAME, i);
 		adap->algo = &solo_i2c_algo;
 		adap->algo_data = solo_dev;
 		adap->retries = 1;
diff --git a/drivers/staging/media/solo6x10/v4l2-enc.c b/drivers/staging/media/solo6x10/v4l2-enc.c
index 9333a00..fd52a42 100644
--- a/drivers/staging/media/solo6x10/v4l2-enc.c
+++ b/drivers/staging/media/solo6x10/v4l2-enc.c
@@ -297,19 +297,19 @@ static int enc_get_mpeg_dma_sg(struct solo_dev *solo_dev,
 
 	if (off + size <= SOLO_MP4E_EXT_SIZE(solo_dev)) {
 		return solo_p2m_dma_sg(solo_dev, SOLO_P2M_DMA_ID_MP4E,
-				       desc, 0, sglist, skip,
-				       SOLO_MP4E_EXT_ADDR(solo_dev) + off, size);
+				desc, 0, sglist, skip,
+				SOLO_MP4E_EXT_ADDR(solo_dev) + off, size);
 	}
 
 	/* Buffer wrap */
 	ret = solo_p2m_dma_sg(solo_dev, SOLO_P2M_DMA_ID_MP4E, desc, 0,
-			      sglist, skip, SOLO_MP4E_EXT_ADDR(solo_dev) + off,
-			      SOLO_MP4E_EXT_SIZE(solo_dev) - off);
+			sglist, skip, SOLO_MP4E_EXT_ADDR(solo_dev) + off,
+			SOLO_MP4E_EXT_SIZE(solo_dev) - off);
 
 	ret |= solo_p2m_dma_sg(solo_dev, SOLO_P2M_DMA_ID_MP4E, desc, 0,
-			       sglist, skip + SOLO_MP4E_EXT_SIZE(solo_dev) - off,
-			       SOLO_MP4E_EXT_ADDR(solo_dev),
-			       size + off - SOLO_MP4E_EXT_SIZE(solo_dev));
+			sglist, skip + SOLO_MP4E_EXT_SIZE(solo_dev) - off,
+			SOLO_MP4E_EXT_ADDR(solo_dev),
+			size + off - SOLO_MP4E_EXT_SIZE(solo_dev));
 
 	return ret;
 }
@@ -366,19 +366,19 @@ static int enc_get_jpeg_dma_sg(struct solo_dev *solo_dev,
 
 	if (off + size <= SOLO_JPEG_EXT_SIZE(solo_dev)) {
 		return solo_p2m_dma_sg(solo_dev, SOLO_P2M_DMA_ID_JPEG,
-				       desc, 0, sglist, skip,
-				       SOLO_JPEG_EXT_ADDR(solo_dev) + off, size);
+			       desc, 0, sglist, skip,
+			       SOLO_JPEG_EXT_ADDR(solo_dev) + off, size);
 	}
 
 	/* Buffer wrap */
 	ret = solo_p2m_dma_sg(solo_dev, SOLO_P2M_DMA_ID_JPEG, desc, 0,
-			      sglist, skip, SOLO_JPEG_EXT_ADDR(solo_dev) + off,
-			      SOLO_JPEG_EXT_SIZE(solo_dev) - off);
+		      sglist, skip, SOLO_JPEG_EXT_ADDR(solo_dev) + off,
+		      SOLO_JPEG_EXT_SIZE(solo_dev) - off);
 
 	ret |= solo_p2m_dma_sg(solo_dev, SOLO_P2M_DMA_ID_JPEG, desc, 0,
-			       sglist, skip + SOLO_JPEG_EXT_SIZE(solo_dev) - off,
-			       SOLO_JPEG_EXT_ADDR(solo_dev),
-			       size + off - SOLO_JPEG_EXT_SIZE(solo_dev));
+		       sglist, skip + SOLO_JPEG_EXT_SIZE(solo_dev) - off,
+		       SOLO_JPEG_EXT_ADDR(solo_dev),
+		       size + off - SOLO_JPEG_EXT_SIZE(solo_dev));
 
 	return ret;
 }
diff --git a/drivers/staging/media/solo6x10/v4l2.c b/drivers/staging/media/solo6x10/v4l2.c
index 1f896b9..acc0ca0 100644
--- a/drivers/staging/media/solo6x10/v4l2.c
+++ b/drivers/staging/media/solo6x10/v4l2.c
@@ -324,7 +324,10 @@ static void solo_fillbuf(struct solo_filehandle *fh,
 			continue;
 		}
 
-		/* Shove as many lines into a repeating descriptor as possible */
+		/*
+		 * Shove as many lines into a repeating
+		 * descriptor as possible
+		 */
 		lines = min(sg_size_left / line_len,
 			    solo_vlines(solo_dev) - i);
 
-- 
1.7.4.4

[PATCH 10/10] staging: solo6x10: Avoid extern declaration by reworking module parameter

[Ezequiel Garcia]

This patch moves video_nr module parameter to core.c
and then passes that parameter as an argument to functions
that need it.
This way we avoid the extern declaration and parameter
dependencies are better exposed.

Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 drivers/staging/media/solo6x10/core.c     |    8 ++++++--
 drivers/staging/media/solo6x10/solo6x10.h |    4 ++--
 drivers/staging/media/solo6x10/v4l2-enc.c |    9 ++++-----
 drivers/staging/media/solo6x10/v4l2.c     |    6 +-----
 4 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/media/solo6x10/core.c b/drivers/staging/media/solo6x10/core.c
index 2cfe906..c2e3932 100644
--- a/drivers/staging/media/solo6x10/core.c
+++ b/drivers/staging/media/solo6x10/core.c
@@ -31,6 +31,10 @@ MODULE_AUTHOR("Ben Collins <bcollins@bluecherry.net>");
 MODULE_VERSION(SOLO6X10_VERSION);
 MODULE_LICENSE("GPL");
 
+unsigned int video_nr = -1;
+module_param(video_nr, uint, 0644);
+MODULE_PARM_DESC(video_nr, "videoX start number, -1 is autodetect (default)");
+
 void solo_irq_on(struct solo_dev *solo_dev, u32 mask)
 {
 	solo_dev->irq_mask |= mask;
@@ -261,7 +265,7 @@ static int __devinit solo_pci_probe(struct pci_dev *pdev,
 	if (ret)
 		goto fail_probe;
 
-	ret = solo_v4l2_init(solo_dev);
+	ret = solo_v4l2_init(solo_dev, video_nr);
 	if (ret)
 		goto fail_probe;
 
@@ -269,7 +273,7 @@ static int __devinit solo_pci_probe(struct pci_dev *pdev,
 	if (ret)
 		goto fail_probe;
 
-	ret = solo_enc_v4l2_init(solo_dev);
+	ret = solo_enc_v4l2_init(solo_dev, video_nr);
 	if (ret)
 		goto fail_probe;
 
diff --git a/drivers/staging/media/solo6x10/solo6x10.h b/drivers/staging/media/solo6x10/solo6x10.h
index 5e9b399..7837cc2 100644
--- a/drivers/staging/media/solo6x10/solo6x10.h
+++ b/drivers/staging/media/solo6x10/solo6x10.h
@@ -286,13 +286,13 @@ void solo_i2c_exit(struct solo_dev *solo_dev);
 int solo_p2m_init(struct solo_dev *solo_dev);
 void solo_p2m_exit(struct solo_dev *solo_dev);
 
-int solo_v4l2_init(struct solo_dev *solo_dev);
+int solo_v4l2_init(struct solo_dev *solo_dev, unsigned int video_nr);
 void solo_v4l2_exit(struct solo_dev *solo_dev);
 
 int solo_enc_init(struct solo_dev *solo_dev);
 void solo_enc_exit(struct solo_dev *solo_dev);
 
-int solo_enc_v4l2_init(struct solo_dev *solo_dev);
+int solo_enc_v4l2_init(struct solo_dev *solo_dev, unsigned int video_nr);
 void solo_enc_v4l2_exit(struct solo_dev *solo_dev);
 
 int solo_g723_init(struct solo_dev *solo_dev);
diff --git a/drivers/staging/media/solo6x10/v4l2-enc.c b/drivers/staging/media/solo6x10/v4l2-enc.c
index fd52a42..bf5c390 100644
--- a/drivers/staging/media/solo6x10/v4l2-enc.c
+++ b/drivers/staging/media/solo6x10/v4l2-enc.c
@@ -34,8 +34,6 @@
 
 static int solo_enc_thread(void *data);
 
-extern unsigned video_nr;
-
 struct solo_enc_fh {
 	struct			solo_enc_dev *enc;
 	u32			fmt;
@@ -1724,7 +1722,8 @@ static struct video_device solo_enc_template = {
 	.current_norm		= V4L2_STD_NTSC_M,
 };
 
-static struct solo_enc_dev *solo_enc_alloc(struct solo_dev *solo_dev, u8 ch)
+static struct solo_enc_dev *
+solo_enc_alloc(struct solo_dev *solo_dev, u8 ch, unsigned int video_nr)
 {
 	struct solo_enc_dev *solo_enc;
 	int ret;
@@ -1787,12 +1786,12 @@ static void solo_enc_free(struct solo_enc_dev *solo_enc)
 	kfree(solo_enc);
 }
 
-int solo_enc_v4l2_init(struct solo_dev *solo_dev)
+int solo_enc_v4l2_init(struct solo_dev *solo_dev, unsigned int video_nr)
 {
 	int i;
 
 	for (i = 0; i < solo_dev->nr_chans; i++) {
-		solo_dev->v4l2_enc[i] = solo_enc_alloc(solo_dev, i);
+		solo_dev->v4l2_enc[i] = solo_enc_alloc(solo_dev, i, video_nr);
 		if (IS_ERR(solo_dev->v4l2_enc[i]))
 			break;
 	}
diff --git a/drivers/staging/media/solo6x10/v4l2.c b/drivers/staging/media/solo6x10/v4l2.c
index acc0ca0..3669d85 100644
--- a/drivers/staging/media/solo6x10/v4l2.c
+++ b/drivers/staging/media/solo6x10/v4l2.c
@@ -50,10 +50,6 @@ struct solo_filehandle {
 	int			desc_idx;
 };
 
-unsigned video_nr = -1;
-module_param(video_nr, uint, 0644);
-MODULE_PARM_DESC(video_nr, "videoX start number, -1 is autodetect (default)");
-
 static void erase_on(struct solo_dev *solo_dev)
 {
 	solo_reg_write(solo_dev, SOLO_VO_DISP_ERASE, SOLO_VO_DISP_ERASE_ON);
@@ -907,7 +903,7 @@ static struct video_device solo_v4l2_template = {
 	.current_norm		= V4L2_STD_NTSC_M,
 };
 
-int solo_v4l2_init(struct solo_dev *solo_dev)
+int solo_v4l2_init(struct solo_dev *solo_dev, unsigned int video_nr)
 {
 	int ret;
 	int i;
-- 
1.7.4.4

Re: [PATCH 10/10] staging: solo6x10: Avoid extern declaration by reworking module parameter

[Ismael Luceno]

On Thu, Jun 21, 2012 at 4:52 PM, Ezequiel Garcia <elezegarcia@gmail.com> wrote:
> This patch moves video_nr module parameter to core.c
> and then passes that parameter as an argument to functions
> that need it.
> This way we avoid the extern declaration and parameter
> dependencies are better exposed.
<...>

NACK.

The changes to video_nr are supposed to be preserved.

Re: [PATCH 10/10] staging: solo6x10: Avoid extern declaration by reworking module parameter

[Ezequiel Garcia]

On Wed, Jul 18, 2012 at 7:26 PM, Ismael Luceno <ismael.luceno@gmail.com> wrote:
> On Thu, Jun 21, 2012 at 4:52 PM, Ezequiel Garcia <elezegarcia@gmail.com> wrote:
>> This patch moves video_nr module parameter to core.c
>> and then passes that parameter as an argument to functions
>> that need it.
>> This way we avoid the extern declaration and parameter
>> dependencies are better exposed.
> <...>
>
> NACK.
>
> The changes to video_nr are supposed to be preserved.

Mmm, I'm sorry but I don't see any functionality change in this patch,
just a cleanup.

What do you mean by "changes to video_nr are supposed to be preserved"?

Re: [PATCH 10/10] staging: solo6x10: Avoid extern declaration by reworking module parameter

[Ismael Luceno]

On Thu, 19 Jul 2012 10:25:09 -0300
Ezequiel Garcia <elezegarcia@gmail.com> wrote:
> On Wed, Jul 18, 2012 at 7:26 PM, Ismael Luceno
> <ismael.luceno@gmail.com> wrote:
> > On Thu, Jun 21, 2012 at 4:52 PM, Ezequiel Garcia
> > <elezegarcia@gmail.com> wrote:
> >> This patch moves video_nr module parameter to core.c
> >> and then passes that parameter as an argument to functions
> >> that need it.
> >> This way we avoid the extern declaration and parameter
> >> dependencies are better exposed.
> > <...>
> >
> > NACK.
> >
> > The changes to video_nr are supposed to be preserved.
> 
> Mmm, I'm sorry but I don't see any functionality change in this patch,
> just a cleanup.
> 
> What do you mean by "changes to video_nr are supposed to be
> preserved"?

It is modified by solo_enc_alloc, which is called multiple times by
solo_enc_v4l2_init.

Re: [PATCH 10/10] staging: solo6x10: Avoid extern declaration by reworking module parameter

[Ezequiel Garcia]

On Thu, Jul 19, 2012 at 3:41 PM, Ismael Luceno <ismael.luceno@gmail.com> wrote:
> On Thu, 19 Jul 2012 10:25:09 -0300
> Ezequiel Garcia <elezegarcia@gmail.com> wrote:
>> On Wed, Jul 18, 2012 at 7:26 PM, Ismael Luceno
>> <ismael.luceno@gmail.com> wrote:
>> > On Thu, Jun 21, 2012 at 4:52 PM, Ezequiel Garcia
>> > <elezegarcia@gmail.com> wrote:
>> >> This patch moves video_nr module parameter to core.c
>> >> and then passes that parameter as an argument to functions
>> >> that need it.
>> >> This way we avoid the extern declaration and parameter
>> >> dependencies are better exposed.
>> > <...>
>> >
>> > NACK.
>> >
>> > The changes to video_nr are supposed to be preserved.
>>
>> Mmm, I'm sorry but I don't see any functionality change in this patch,
>> just a cleanup.
>>
>> What do you mean by "changes to video_nr are supposed to be
>> preserved"?
>
> It is modified by solo_enc_alloc, which is called multiple times by
> solo_enc_v4l2_init.

Mmm, I see what you mean. Sorry for not noticing that :-(

However, I still think that extern int is really not needed and should
be cleaned up.
Using global variables is not a nice practice, and using this extern
hides dependencies, i.e. hides who is using video_nr.
(For instance, I failed to see such dependency)

Perhaps, you could consider passing the int as a pointer, so that
solo_enc_alloc()
can modify it.

However, since it's your driver, you have access to the hardware, etc.
I won't push any further this issue. Plus, this little issue is not the
one preventing from moving out of staging.

Feel free to nack any other of the patches,
though I think this one was the only one non-trivial.

Thanks for reviewing,
Ezequiel.

Re: [PATCH 10/10] staging: solo6x10: Avoid extern declaration by reworking module parameter

[Hans Verkuil]

On Thu July 19 2012 20:41:11 Ismael Luceno wrote:
> On Thu, 19 Jul 2012 10:25:09 -0300
> Ezequiel Garcia <elezegarcia@gmail.com> wrote:
> > On Wed, Jul 18, 2012 at 7:26 PM, Ismael Luceno
> > <ismael.luceno@gmail.com> wrote:
> > > On Thu, Jun 21, 2012 at 4:52 PM, Ezequiel Garcia
> > > <elezegarcia@gmail.com> wrote:
> > >> This patch moves video_nr module parameter to core.c
> > >> and then passes that parameter as an argument to functions
> > >> that need it.
> > >> This way we avoid the extern declaration and parameter
> > >> dependencies are better exposed.
> > > <...>
> > >
> > > NACK.
> > >
> > > The changes to video_nr are supposed to be preserved.
> > 
> > Mmm, I'm sorry but I don't see any functionality change in this patch,
> > just a cleanup.
> > 
> > What do you mean by "changes to video_nr are supposed to be
> > preserved"?
> 
> It is modified by solo_enc_alloc, which is called multiple times by
> solo_enc_v4l2_init.

You don't need to modify it at all. video_register_device() will start
looking for a free video node number starting at video_nr and counting
upwards, so increasing video_nr has no purpose. Leaving it out will give
you exactly the same result.

The only driver that does the same thing is vivi, and I think it should
be removed from there as well. The solo driver probably copied it from
vivi :-(

Regards,

	Hans

Re: [PATCH 10/10] staging: solo6x10: Avoid extern declaration by reworking module parameter

[Ezequiel Garcia]

On Thu, Jul 19, 2012 at 4:48 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Thu July 19 2012 20:41:11 Ismael Luceno wrote:
>> On Thu, 19 Jul 2012 10:25:09 -0300
>> Ezequiel Garcia <elezegarcia@gmail.com> wrote:
>> > On Wed, Jul 18, 2012 at 7:26 PM, Ismael Luceno
>> > <ismael.luceno@gmail.com> wrote:
>> > > On Thu, Jun 21, 2012 at 4:52 PM, Ezequiel Garcia
>> > > <elezegarcia@gmail.com> wrote:
>> > >> This patch moves video_nr module parameter to core.c
>> > >> and then passes that parameter as an argument to functions
>> > >> that need it.
>> > >> This way we avoid the extern declaration and parameter
>> > >> dependencies are better exposed.
>> > > <...>
>> > >
>> > > NACK.
>> > >
>> > > The changes to video_nr are supposed to be preserved.
>> >
>> > Mmm, I'm sorry but I don't see any functionality change in this patch,
>> > just a cleanup.
>> >
>> > What do you mean by "changes to video_nr are supposed to be
>> > preserved"?
>>
>> It is modified by solo_enc_alloc, which is called multiple times by
>> solo_enc_v4l2_init.
>
> You don't need to modify it at all. video_register_device() will start
> looking for a free video node number starting at video_nr and counting
> upwards, so increasing video_nr has no purpose. Leaving it out will give
> you exactly the same result.
>

Yes, but perhaps the module author wanted to force a device
/dev/videoX *start* number,
as it's documented in the parameter usage string:

  MODULE_PARM_DESC(video_nr, "videoX start number, -1 is autodetect (default)");

Now, I don't now why would one want to do that or if it makes sense at all.
In any case, it seems it's the intended behavior.

Re: [PATCH 10/10] staging: solo6x10: Avoid extern declaration by reworking module parameter

[Hans Verkuil]

On Thu July 19 2012 21:55:20 Ezequiel Garcia wrote:
> On Thu, Jul 19, 2012 at 4:48 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > On Thu July 19 2012 20:41:11 Ismael Luceno wrote:
> >> On Thu, 19 Jul 2012 10:25:09 -0300
> >> Ezequiel Garcia <elezegarcia@gmail.com> wrote:
> >> > On Wed, Jul 18, 2012 at 7:26 PM, Ismael Luceno
> >> > <ismael.luceno@gmail.com> wrote:
> >> > > On Thu, Jun 21, 2012 at 4:52 PM, Ezequiel Garcia
> >> > > <elezegarcia@gmail.com> wrote:
> >> > >> This patch moves video_nr module parameter to core.c
> >> > >> and then passes that parameter as an argument to functions
> >> > >> that need it.
> >> > >> This way we avoid the extern declaration and parameter
> >> > >> dependencies are better exposed.
> >> > > <...>
> >> > >
> >> > > NACK.
> >> > >
> >> > > The changes to video_nr are supposed to be preserved.
> >> >
> >> > Mmm, I'm sorry but I don't see any functionality change in this patch,
> >> > just a cleanup.
> >> >
> >> > What do you mean by "changes to video_nr are supposed to be
> >> > preserved"?
> >>
> >> It is modified by solo_enc_alloc, which is called multiple times by
> >> solo_enc_v4l2_init.
> >
> > You don't need to modify it at all. video_register_device() will start
> > looking for a free video node number starting at video_nr and counting
> > upwards, so increasing video_nr has no purpose. Leaving it out will give
> > you exactly the same result.
> >
> 
> Yes, but perhaps the module author wanted to force a device
> /dev/videoX *start* number,
> as it's documented in the parameter usage string:
> 
>   MODULE_PARM_DESC(video_nr, "videoX start number, -1 is autodetect (default)");
> 
> Now, I don't now why would one want to do that or if it makes sense at all.
> In any case, it seems it's the intended behavior.
> 

But doing video_nr++ is pointless and will not have an effect.

Example: if video_nr is specified as 2, then video_register_device() will attempt
to make a /dev/video2 node, if that's already in use, then it will try /dev/video3,
etc.

So doing video_nr++ after a video_register_device() will not change the outcome
for the next video_register_device() that is called, and has the somewhat odd
side-effect of changing the video_nr module parameter from what the user passed in.
So cat /sys/module/vivi/parameters/video_nr returns 3 instead of 2.

Just don't change it and let video_register_device() do its magic.

Regards,

	Hans

Re: [PATCH 10/10] staging: solo6x10: Avoid extern declaration by reworking module parameter

[Ezequiel Garcia]

On Thu, Jul 19, 2012 at 5:41 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Thu July 19 2012 21:55:20 Ezequiel Garcia wrote:
>> On Thu, Jul 19, 2012 at 4:48 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> > On Thu July 19 2012 20:41:11 Ismael Luceno wrote:
>> >> On Thu, 19 Jul 2012 10:25:09 -0300
>> >> Ezequiel Garcia <elezegarcia@gmail.com> wrote:
>> >> > On Wed, Jul 18, 2012 at 7:26 PM, Ismael Luceno
>> >> > <ismael.luceno@gmail.com> wrote:
>> >> > > On Thu, Jun 21, 2012 at 4:52 PM, Ezequiel Garcia
>> >> > > <elezegarcia@gmail.com> wrote:
>> >> > >> This patch moves video_nr module parameter to core.c
>> >> > >> and then passes that parameter as an argument to functions
>> >> > >> that need it.
>> >> > >> This way we avoid the extern declaration and parameter
>> >> > >> dependencies are better exposed.
>> >> > > <...>
>> >> > >
>> >> > > NACK.
>> >> > >
>> >> > > The changes to video_nr are supposed to be preserved.
>> >> >
>> >> > Mmm, I'm sorry but I don't see any functionality change in this patch,
>> >> > just a cleanup.
>> >> >
>> >> > What do you mean by "changes to video_nr are supposed to be
>> >> > preserved"?
>> >>
>> >> It is modified by solo_enc_alloc, which is called multiple times by
>> >> solo_enc_v4l2_init.
>> >
>> > You don't need to modify it at all. video_register_device() will start
>> > looking for a free video node number starting at video_nr and counting
>> > upwards, so increasing video_nr has no purpose. Leaving it out will give
>> > you exactly the same result.
>> >
>>
>> Yes, but perhaps the module author wanted to force a device
>> /dev/videoX *start* number,
>> as it's documented in the parameter usage string:
>>
>>   MODULE_PARM_DESC(video_nr, "videoX start number, -1 is autodetect (default)");
>>
>> Now, I don't now why would one want to do that or if it makes sense at all.
>> In any case, it seems it's the intended behavior.
>>
>
> But doing video_nr++ is pointless and will not have an effect.
>
> Example: if video_nr is specified as 2, then video_register_device() will attempt
> to make a /dev/video2 node, if that's already in use, then it will try /dev/video3,
> etc.
>
> So doing video_nr++ after a video_register_device() will not change the outcome
> for the next video_register_device() that is called, and has the somewhat odd
> side-effect of changing the video_nr module parameter from what the user passed in.
> So cat /sys/module/vivi/parameters/video_nr returns 3 instead of 2.
>
> Just don't change it and let video_register_device() do its magic.
>

Very true.

I did a quick grep through video drivers and it seems no one should be
using this
video_register_device parameter with anything but -1.

Is there any reason to keep it?

Thanks,
Ezequiel.

Re: [PATCH 10/10] staging: solo6x10: Avoid extern declaration by reworking module parameter

[Ismael Luceno]

On Thu, 19 Jul 2012 21:48:33 +0200
Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Thu July 19 2012 20:41:11 Ismael Luceno wrote:
> > On Thu, 19 Jul 2012 10:25:09 -0300
> > Ezequiel Garcia <elezegarcia@gmail.com> wrote:
> > > On Wed, Jul 18, 2012 at 7:26 PM, Ismael Luceno
> > > <ismael.luceno@gmail.com> wrote:
> > > > On Thu, Jun 21, 2012 at 4:52 PM, Ezequiel Garcia
> > > > <elezegarcia@gmail.com> wrote:
> > > >> This patch moves video_nr module parameter to core.c
> > > >> and then passes that parameter as an argument to functions
> > > >> that need it.
> > > >> This way we avoid the extern declaration and parameter
> > > >> dependencies are better exposed.
> > > > <...>
> > > >
> > > > NACK.
> > > >
> > > > The changes to video_nr are supposed to be preserved.
> > > 
> > > Mmm, I'm sorry but I don't see any functionality change in this
> > > patch, just a cleanup.
> > > 
> > > What do you mean by "changes to video_nr are supposed to be
> > > preserved"?
> > 
> > It is modified by solo_enc_alloc, which is called multiple times by
> > solo_enc_v4l2_init.
> 
> You don't need to modify it at all. video_register_device() will start
> looking for a free video node number starting at video_nr and counting
> upwards, so increasing video_nr has no purpose. Leaving it out will
> give you exactly the same result.

Oh, didn't know that. Interesting.