[PATCH 0/3] saa7115: add detection code for gm7113c

[PATCH 0/3] saa7115: add detection code for gm7113c

[Jon Arne Jørgensen]

This is the second version of a patch-set previously posted by Mauro,
the first verseon was posted on 26 April, and can be found here:
http://www.spinics.net/lists/linux-media/msg63079.html

The purpose of this patch is to add support for the gm7113c chip in the saa7115 driver.
The gm7113c chip is a chinese clone of the Philips/NXP saa7113 chip.
The chip is found in several cheap usb video capture devices.

 drivers/media/i2c/saa7115.c     | 207 +++++++++++++++++++++++++++++-----------
 include/media/v4l2-chip-ident.h |   2 +
 2 files changed, 155 insertions(+), 54 deletions(-)

[PATCH V2 1/3] saa7115: move the autodetection code out of the probe function

[Jon Arne Jørgensen]

As we're now seeing other variants from chinese clones, like
gm1113c, we'll need to add more bits at the detection code.

So, move it into a separate function.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
Signed-off-by: Jon Arne Jørgensen <jonarne@jonarne.no>
---
 drivers/media/i2c/saa7115.c | 133 +++++++++++++++++++++++++++-----------------
 1 file changed, 83 insertions(+), 50 deletions(-)

diff --git a/drivers/media/i2c/saa7115.c b/drivers/media/i2c/saa7115.c
index 6b6788c..aa92478 100644
--- a/drivers/media/i2c/saa7115.c
+++ b/drivers/media/i2c/saa7115.c
@@ -1561,46 +1561,103 @@ static const struct v4l2_subdev_ops saa711x_ops = {
 
 /* ----------------------------------------------------------------------- */
 
+/**
+ * saa711x_detect_chip - Detects the saa711x (or clone) variant
+ * @client:		I2C client structure.
+ * @id:			I2C device ID structure.
+ * @name:		Name of the device to be filled.
+ * @size:		Size of the name var.
+ *
+ * Detects the Philips/NXP saa711x chip, or some clone of it.
+ * if 'id' is NULL or id->driver_data is equal to 1, it auto-probes
+ * the analog demod.
+ * If the tuner is not found, it returns -ENODEV.
+ * If auto-detection is disabled and the tuner doesn't match what it was
+ *	requred, it returns -EINVAL and fills 'name'.
+ * If the chip is found, it returns the chip ID and fills 'name'.
+ */
+static int saa711x_detect_chip(struct i2c_client *client,
+			       const struct i2c_device_id *id,
+			       char *name, unsigned size)
+{
+	char chip_ver[size - 1];
+	char chip_id;
+	int i;
+	int autodetect;
+
+	autodetect = !id || id->driver_data == 1;
+
+	/* Read the chip version register */
+	for (i = 0; i < size - 1; i++) {
+		i2c_smbus_write_byte_data(client, 0, i);
+		chip_ver[i] = i2c_smbus_read_byte_data(client, 0);
+		name[i] = (chip_ver[i] & 0x0f) + '0';
+		if (name[i] > '9')
+			name[i] += 'a' - '9' - 1;
+	}
+	name[i] = '\0';
+
+	/* Check if it is a Philips/NXP chip */
+	if (!memcmp(name + 1, "f711", 4)) {
+		chip_id = name[5];
+		snprintf(name, size, "saa711%c", chip_id);
+
+		if (!autodetect && strcmp(name, id->name))
+			return -EINVAL;
+
+		switch (chip_id) {
+		case '1':
+			if (chip_ver[0] & 0xf0) {
+				snprintf(name, size, "saa711%ca", chip_id);
+				v4l_info(client, "saa7111a variant found\n");
+				return V4L2_IDENT_SAA7111A;
+			}
+			return V4L2_IDENT_SAA7111;
+		case '3':
+			return V4L2_IDENT_SAA7113;
+		case '4':
+			return V4L2_IDENT_SAA7114;
+		case '5':
+			return V4L2_IDENT_SAA7115;
+		case '8':
+			return V4L2_IDENT_SAA7118;
+		default:
+			v4l2_info(client,
+				  "WARNING: Philips/NXP chip unknown - Falling back to saa7111\n");
+			return V4L2_IDENT_SAA7111;
+		}
+	}
+
+	/* Chip was not discovered. Return its ID and don't bind */
+	v4l_dbg(1, debug, client, "chip %*ph @ 0x%x is unknown.\n",
+		16, chip_ver, client->addr << 1);
+	return -ENODEV;
+}
+
 static int saa711x_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
 	struct saa711x_state *state;
 	struct v4l2_subdev *sd;
 	struct v4l2_ctrl_handler *hdl;
-	int i;
+	int ident;
 	char name[17];
-	char chip_id;
-	int autodetect = !id || id->driver_data == 1;
 
 	/* Check if the adapter supports the needed features */
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
 		return -EIO;
 
-	for (i = 0; i < 0x0f; i++) {
-		i2c_smbus_write_byte_data(client, 0, i);
-		name[i] = (i2c_smbus_read_byte_data(client, 0) & 0x0f) + '0';
-		if (name[i] > '9')
-			name[i] += 'a' - '9' - 1;
-	}
-	name[i] = '\0';
-
-	chip_id = name[5];
-
-	/* Check whether this chip is part of the saa711x series */
-	if (memcmp(name + 1, "f711", 4)) {
-		v4l_dbg(1, debug, client, "chip found @ 0x%x (ID %s) does not match a known saa711x chip.\n",
-			client->addr << 1, name);
+	ident = saa711x_detect_chip(client, id, name, sizeof(name));
+	if (ident == -EINVAL) {
+		/* Chip exists, but doesn't match */
+		v4l_warn(client, "found %s while %s was expected\n",
+			 name, id->name);
 		return -ENODEV;
 	}
+	if (ident < 0)
+		return ident;
 
-	/* Safety check */
-	if (!autodetect && id->name[6] != chip_id) {
-		v4l_warn(client, "found saa711%c while %s was expected\n",
-			 chip_id, id->name);
-	}
-	snprintf(client->name, sizeof(client->name), "saa711%c", chip_id);
-	v4l_info(client, "saa711%c found (%s) @ 0x%x (%s)\n", chip_id, name,
-		 client->addr << 1, client->adapter->name);
+	strlcpy(client->name, name, sizeof(client->name));
 
 	state = kzalloc(sizeof(struct saa711x_state), GFP_KERNEL);
 	if (state == NULL)
@@ -1637,31 +1694,7 @@ static int saa711x_probe(struct i2c_client *client,
 	state->output = SAA7115_IPORT_ON;
 	state->enable = 1;
 	state->radio = 0;
-	switch (chip_id) {
-	case '1':
-		state->ident = V4L2_IDENT_SAA7111;
-		if (saa711x_read(sd, R_00_CHIP_VERSION) & 0xf0) {
-			v4l_info(client, "saa7111a variant found\n");
-			state->ident = V4L2_IDENT_SAA7111A;
-		}
-		break;
-	case '3':
-		state->ident = V4L2_IDENT_SAA7113;
-		break;
-	case '4':
-		state->ident = V4L2_IDENT_SAA7114;
-		break;
-	case '5':
-		state->ident = V4L2_IDENT_SAA7115;
-		break;
-	case '8':
-		state->ident = V4L2_IDENT_SAA7118;
-		break;
-	default:
-		state->ident = V4L2_IDENT_SAA7111;
-		v4l2_info(sd, "WARNING: Chip is not known - Falling back to saa7111\n");
-		break;
-	}
+	state->ident = ident;
 
 	state->audclk_freq = 48000;
 
-- 
1.8.2.1

Re: [PATCH V2 1/3] saa7115: move the autodetection code out of the probe function

[Ezequiel Garcia]

Hi Jon,

On Mon, Apr 29, 2013 at 10:41:07PM +0200, Jon Arne Jørgensen wrote:
> As we're now seeing other variants from chinese clones, like
> gm1113c, we'll need to add more bits at the detection code.
> 
> So, move it into a separate function.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> Signed-off-by: Jon Arne Jørgensen <jonarne@jonarne.no>

As far as I can see, this patch is identical to the one sent
by Mauro. Therefore, your SOB here is incorrect, since you are not
the author of the patch.

The proper way of re-submitting patches that have been previously
submitted by another developer is this:

--
From: Mauro Carvalho Chehab <mchehab@redhat.com>

Commit message goes here.

Notice how the first line is a 'From:' tagcindicating who's the
real submitter. The SOB tag indicates the patch author, and you
can add your acked-by, tested-by or reported-by if you want.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
Reported-by: Jon Arne Jørgensen <jonarne@jonarne.no>
--

You can read more about this in Documentation/SubmittingPatches.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH V2 1/3] saa7115: move the autodetection code out of the probe function

[Jon Arne Jørgensen]

On Thu, May 02, 2013 at 11:09:14PM -0300, Ezequiel Garcia wrote:
> Hi Jon,
> 
> On Mon, Apr 29, 2013 at 10:41:07PM +0200, Jon Arne Jørgensen wrote:
> > As we're now seeing other variants from chinese clones, like
> > gm1113c, we'll need to add more bits at the detection code.
> > 
> > So, move it into a separate function.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> > Signed-off-by: Jon Arne Jørgensen <jonarne@jonarne.no>
> 
> As far as I can see, this patch is identical to the one sent
> by Mauro. Therefore, your SOB here is incorrect, since you are not
> the author of the patch.
> 
> The proper way of re-submitting patches that have been previously
> submitted by another developer is this:
> 
> --
> From: Mauro Carvalho Chehab <mchehab@redhat.com>
> 
> Commit message goes here.
> 
> Notice how the first line is a 'From:' tagcindicating who's the
> real submitter. The SOB tag indicates the patch author, and you
> can add your acked-by, tested-by or reported-by if you want.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> Reported-by: Jon Arne Jørgensen <jonarne@jonarne.no>
> --
> 
> You can read more about this in Documentation/SubmittingPatches.

Ok, I'll fix this

> -- 
> Ezequiel García, Free Electrons
> Embedded Linux, Kernel and Android Engineering
> http://free-electrons.com

Re: [PATCH V2 1/3] saa7115: move the autodetection code out of the probe function

[Mauro Carvalho Chehab]

Em 03-05-2013 03:32, Jon Arne Jørgensen escreveu:
> On Thu, May 02, 2013 at 11:09:14PM -0300, Ezequiel Garcia wrote:
>> Hi Jon,
>>
>> On Mon, Apr 29, 2013 at 10:41:07PM +0200, Jon Arne Jørgensen wrote:
>>> As we're now seeing other variants from chinese clones, like
>>> gm1113c, we'll need to add more bits at the detection code.
>>>
>>> So, move it into a separate function.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>>> Signed-off-by: Jon Arne Jørgensen <jonarne@jonarne.no>
>>
>> As far as I can see, this patch is identical to the one sent
>> by Mauro. Therefore, your SOB here is incorrect, since you are not
>> the author of the patch.
>>
>> The proper way of re-submitting patches that have been previously
>> submitted by another developer is this:
>>
>> --
>> From: Mauro Carvalho Chehab <mchehab@redhat.com>
>>
>> Commit message goes here.
>>
>> Notice how the first line is a 'From:' tagcindicating who's the
>> real submitter. The SOB tag indicates the patch author, and you
>> can add your acked-by, tested-by or reported-by if you want.
>>
>> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>> Reported-by: Jon Arne Jørgensen <jonarne@jonarne.no>

Please also add here your Tested-by: if you tested (very likely
you did it). The same applies to patch 2/3.

>> --
>>
>> You can read more about this in Documentation/SubmittingPatches.
>
> Ok, I'll fix this


Regards,
Mauro.

Re: [PATCH V2 1/3] saa7115: move the autodetection code out of the probe function

[Jon Arne Jørgensen]

On Thu, May 02, 2013 at 11:09:14PM -0300, Ezequiel Garcia wrote:
> Hi Jon,
> 
> On Mon, Apr 29, 2013 at 10:41:07PM +0200, Jon Arne Jørgensen wrote:
> > As we're now seeing other variants from chinese clones, like
> > gm1113c, we'll need to add more bits at the detection code.
> > 
> > So, move it into a separate function.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> > Signed-off-by: Jon Arne Jørgensen <jonarne@jonarne.no>
> 
> As far as I can see, this patch is identical to the one sent
> by Mauro. Therefore, your SOB here is incorrect, since you are not
> the author of the patch.
> 
> The proper way of re-submitting patches that have been previously
> submitted by another developer is this:
> 
> --
> From: Mauro Carvalho Chehab <mchehab@redhat.com>
> 
> Commit message goes here.
> 
> Notice how the first line is a 'From:' tagcindicating who's the
> real submitter. The SOB tag indicates the patch author, and you
> can add your acked-by, tested-by or reported-by if you want.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> Reported-by: Jon Arne Jørgensen <jonarne@jonarne.no>
> --
> 
> You can read more about this in Documentation/SubmittingPatches.

I just re-read SubmittingPatches.
I couldn't see that there is anything wrong with multiple sign-off's.

Quote:
  The Signed-off-by: tag indicates that the signer was involved in the
  development of the patch, or that he/she was in the patch's delivery
  path.

and:
  (c) The contribution was provided directly to me by some other
      person who certified (a), (b) or (c) and I have not modified
      it.

It's not that important to me, so If you insist, I'll remove the
signed-off-by line :)

> -- 
> Ezequiel García, Free Electrons
> Embedded Linux, Kernel and Android Engineering
> http://free-electrons.com

Re: [PATCH V2 1/3] saa7115: move the autodetection code out of the probe function

[Ezequiel Garcia]

Hi Jon,

On Fri, May 03, 2013 at 08:58:46AM +0200, Jon Arne Jørgensen wrote:
[...]
> > You can read more about this in Documentation/SubmittingPatches.
> 
> I just re-read SubmittingPatches.
> I couldn't see that there is anything wrong with multiple sign-off's.
> 

Indeed there isn't anything wrong with multiple SOBs tags, but they're
used a bit differently than this.

> Quote:
>   The Signed-off-by: tag indicates that the signer was involved in the
>   development of the patch, or that he/she was in the patch's delivery
>   path.
> 
>

Ah, I see your point.

@Mauro, perhaps you can explain this better then me?

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH V2 1/3] saa7115: move the autodetection code out of the probe function

[Mauro Carvalho Chehab]

Em 03-05-2013 08:20, Ezequiel Garcia escreveu:
> Hi Jon,
>
> On Fri, May 03, 2013 at 08:58:46AM +0200, Jon Arne Jørgensen wrote:
> [...]
>>> You can read more about this in Documentation/SubmittingPatches.
>>
>> I just re-read SubmittingPatches.
>> I couldn't see that there is anything wrong with multiple sign-off's.
>>
>
> Indeed there isn't anything wrong with multiple SOBs tags, but they're
> used a bit differently than this.
>
>> Quote:
>>    The Signed-off-by: tag indicates that the signer was involved in the
>>    development of the patch, or that he/she was in the patch's delivery
>>    path.
>>
>>
>
> Ah, I see your point.
>
> @Mauro, perhaps you can explain this better then me?

The SOB is used mainly to describe the patch flow. Each one that touched
on a patch attests that:

        "Developer's Certificate of Origin 1.1

         By making a contribution to this project, I certify that:

         (a) The contribution was created in whole or in part by me and I
             have the right to submit it under the open source license
             indicated in the file; or

         (b) The contribution is based upon previous work that, to the best
             of my knowledge, is covered under an appropriate open source
             license and I have the right under that license to submit that
             work with modifications, whether created in whole or in part
             by me, under the same open source license (unless I am
             permitted to submit under a different license), as indicated
             in the file; or

         (c) The contribution was provided directly to me by some other
             person who certified (a), (b) or (c) and I have not modified
             it.

	(d) I understand and agree that this project and the contribution
	    are public and that a record of the contribution (including all
	    personal information I submit with it, including my sign-off) is
	    maintained indefinitely and may be redistributed consistent with
	    this project or the open source license(s) involved."

In other words, it tracks the custody chain, with is typically one of
the alternatives below[1]:

	Author -> maintainer's tree -> upstream
	Author -> sub-maintainer's tree -> maintainer's tree -> upstream
	Author -> driver's maintainer -> maintainer's tree -> upstream
	Author -> driver's maintainer -> sub-maintainer's tree -> maintainer's tree -> upstream\

In this specific case, as patches 1 and 2 are identical to the ones I submitted,
the right way would be for you both to just reply to my original e-mail with
your tested-by or reviewed-by. That patches will then be applied (either directly
or via Hverkuil's tree, as he is the sub-maintainer for those I2C drivers).

I hope that helps to clarify it.

Regards,
Mauro

[1] when the driver is developed/patched internally on some company's trees,
it is possible to have there also the SOBs for that company's internal
maintainers.

There are also some other corner cases, like patches that are sent in
non-public mailing lists or in private, where everybody in the custody
chain sign it.

Re: [PATCH V2 1/3] saa7115: move the autodetection code out of the probe function

[Jon Arne Jørgensen]

On Fri, May 03, 2013 at 09:08:16AM -0300, Mauro Carvalho Chehab wrote:
> Em 03-05-2013 08:20, Ezequiel Garcia escreveu:
> >Hi Jon,
> >
> >On Fri, May 03, 2013 at 08:58:46AM +0200, Jon Arne Jørgensen wrote:
> >[...]
> >>>You can read more about this in Documentation/SubmittingPatches.
> >>
> >>I just re-read SubmittingPatches.
> >>I couldn't see that there is anything wrong with multiple sign-off's.
> >>
> >
> >Indeed there isn't anything wrong with multiple SOBs tags, but they're
> >used a bit differently than this.
> >
> >>Quote:
> >>   The Signed-off-by: tag indicates that the signer was involved in the
> >>   development of the patch, or that he/she was in the patch's delivery
> >>   path.
> >>
> >>
> >
> >Ah, I see your point.
> >
> >@Mauro, perhaps you can explain this better then me?
> 
> The SOB is used mainly to describe the patch flow. Each one that touched
> on a patch attests that:
> 
>        "Developer's Certificate of Origin 1.1
> 
>         By making a contribution to this project, I certify that:
> 
>         (a) The contribution was created in whole or in part by me and I
>             have the right to submit it under the open source license
>             indicated in the file; or
> 
>         (b) The contribution is based upon previous work that, to the best
>             of my knowledge, is covered under an appropriate open source
>             license and I have the right under that license to submit that
>             work with modifications, whether created in whole or in part
>             by me, under the same open source license (unless I am
>             permitted to submit under a different license), as indicated
>             in the file; or
> 
>         (c) The contribution was provided directly to me by some other
>             person who certified (a), (b) or (c) and I have not modified
>             it.
> 
> 	(d) I understand and agree that this project and the contribution
> 	    are public and that a record of the contribution (including all
> 	    personal information I submit with it, including my sign-off) is
> 	    maintained indefinitely and may be redistributed consistent with
> 	    this project or the open source license(s) involved."
> 
> In other words, it tracks the custody chain, with is typically one of
> the alternatives below[1]:
> 
> 	Author -> maintainer's tree -> upstream
> 	Author -> sub-maintainer's tree -> maintainer's tree -> upstream
> 	Author -> driver's maintainer -> maintainer's tree -> upstream
> 	Author -> driver's maintainer -> sub-maintainer's tree -> maintainer's tree -> upstream\
> 
> In this specific case, as patches 1 and 2 are identical to the ones I submitted,
> the right way would be for you both to just reply to my original e-mail with
> your tested-by or reviewed-by. That patches will then be applied (either directly
> or via Hverkuil's tree, as he is the sub-maintainer for those I2C drivers).
>

The patch you submitted lacked all register setup so there wasn't that much
to test without the stuff I added in the third patch in this
series.

Is there any way to accnowledge this when I reply to your patch with tested-by?
Should I maybe add the third patch in this series into that reply?

Also, when I post the next RFC for my smi2021 driver that
depends on this patch. Are there any correct way to mention that
dependency in the smi2021 patch?

Best,
Jon Arne

> I hope that helps to clarify it.
> 
> Regards,
> Mauro
> 
> [1] when the driver is developed/patched internally on some company's trees,
> it is possible to have there also the SOBs for that company's internal
> maintainers.
> 
> There are also some other corner cases, like patches that are sent in
> non-public mailing lists or in private, where everybody in the custody
> chain sign it.

Re: [PATCH V2 1/3] saa7115: move the autodetection code out of the probe function

[Mauro Carvalho Chehab]

Em Sun, 5 May 2013 10:20:08 +0200
Jon Arne Jørgensen <jonarne@jonarne.no> escreveu:

> On Fri, May 03, 2013 at 09:08:16AM -0300, Mauro Carvalho Chehab wrote:
> > Em 03-05-2013 08:20, Ezequiel Garcia escreveu:
> > >Hi Jon,
> > >
> > >On Fri, May 03, 2013 at 08:58:46AM +0200, Jon Arne Jørgensen wrote:
> > >[...]
> > >>>You can read more about this in Documentation/SubmittingPatches.
> > >>
> > >>I just re-read SubmittingPatches.
> > >>I couldn't see that there is anything wrong with multiple sign-off's.
> > >>
> > >
> > >Indeed there isn't anything wrong with multiple SOBs tags, but they're
> > >used a bit differently than this.
> > >
> > >>Quote:
> > >>   The Signed-off-by: tag indicates that the signer was involved in the
> > >>   development of the patch, or that he/she was in the patch's delivery
> > >>   path.
> > >>
> > >>
> > >
> > >Ah, I see your point.
> > >
> > >@Mauro, perhaps you can explain this better then me?
> > 
> > The SOB is used mainly to describe the patch flow. Each one that touched
> > on a patch attests that:
> > 
> >        "Developer's Certificate of Origin 1.1
> > 
> >         By making a contribution to this project, I certify that:
> > 
> >         (a) The contribution was created in whole or in part by me and I
> >             have the right to submit it under the open source license
> >             indicated in the file; or
> > 
> >         (b) The contribution is based upon previous work that, to the best
> >             of my knowledge, is covered under an appropriate open source
> >             license and I have the right under that license to submit that
> >             work with modifications, whether created in whole or in part
> >             by me, under the same open source license (unless I am
> >             permitted to submit under a different license), as indicated
> >             in the file; or
> > 
> >         (c) The contribution was provided directly to me by some other
> >             person who certified (a), (b) or (c) and I have not modified
> >             it.
> > 
> > 	(d) I understand and agree that this project and the contribution
> > 	    are public and that a record of the contribution (including all
> > 	    personal information I submit with it, including my sign-off) is
> > 	    maintained indefinitely and may be redistributed consistent with
> > 	    this project or the open source license(s) involved."
> > 
> > In other words, it tracks the custody chain, with is typically one of
> > the alternatives below[1]:
> > 
> > 	Author -> maintainer's tree -> upstream
> > 	Author -> sub-maintainer's tree -> maintainer's tree -> upstream
> > 	Author -> driver's maintainer -> maintainer's tree -> upstream
> > 	Author -> driver's maintainer -> sub-maintainer's tree -> maintainer's tree -> upstream\
> > 
> > In this specific case, as patches 1 and 2 are identical to the ones I submitted,
> > the right way would be for you both to just reply to my original e-mail with
> > your tested-by or reviewed-by. That patches will then be applied (either directly
> > or via Hverkuil's tree, as he is the sub-maintainer for those I2C drivers).
> >
> 
> The patch you submitted lacked all register setup so there wasn't that much
> to test without the stuff I added in the third patch in this
> series.

That's by purpose. I didn't want to touch on the things you authored/tested:
the gm7113c special setup. I also didn't want to merge a pure cosmetic
change that re-group the logic with the new table setup for gm7113c, as
we do prefer to have one logical change by patch.

In any case, the patches can be tested as a hole or in separate.

The first one is a "cosmetic" patch, moving things into a separate function.
A tested-by there means that it didn't break anything. The second one detects
the saa7113 clone. Again, a tested by means that it does what it proposes:
detect this new variant.

Of course, your third patch is needed in order to make gm7113c to work.
That's basically why I didn't merge them on my tree yet.

> Is there any way to accnowledge this when I reply to your patch with tested-by?

Feel free to add your notes when replying to the email. We generally
don't add such notes at git history to avoid polluting it, but they
are stored at patchwork, and at the list archives.

For example:
	https://patchwork.linuxtv.org/patch/15524/

In this case, one patch touched on several different drivers. Some
drivers' maintainers replied with their ack, but noted that their
ack are limited to just their own stuff[1].

[1] they did it by cutting-off the diffstat lines for the drivers
they don't maintain. So, Prabhakar acked for DaVinci; Guennadi
acked for sh_vou/soc_camera changes.

> Should I maybe add the third patch in this series into that reply?

Yeah, you can do that. If you're using "git send-email", it
allows you to put the message ID of a message on your reply.
This way, you can bind your patch to an existing patch thread.

You may also add a note on your patch for the maintainer, after
your SOB, like:

	...
	Signed-off-by: my name <my@email>
	
	---

	Note: please apply this one together with the other patches
	...

Everything after "---" are suppressed when merging (such
notes is a transitory condition that doesn't deserve to
be at git history, as it just pollutes its content), but
it is important to bold merge instructions and other related
stuff.

> Also, when I post the next RFC for my smi2021 driver that
> depends on this patch. Are there any correct way to mention that
> dependency in the smi2021 patch?

Yes. See above.

The better is to point there at the note the patchwork URL,
as the (sub)maintainer(s) use it as the patch queue.

> 
> Best,
> Jon Arne

Regards,
Mauro

[PATCH V2 2/3] saa7115: add detection code for gm7113c

[Jon Arne Jørgensen]

Adds a code that (auto)detects gm7113c clones. The auto-detection
here is not perfect, as, on contrary to what it would be expected
by looking into its datasheets some devices would return, instead:

	saa7115 0-0025: chip 10 10 10 10 10 10 10 10 10 10 10 10 10 10 10 10 @ 0x4a is unknown

(found on a device labeled as GM7113C 1145 by Ezequiel Garcia)

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
Signed-off-by: Jon Arne Jørgensen <jonarne@jonarne.no>
---
 drivers/media/i2c/saa7115.c     | 36 ++++++++++++++++++++++++++++++++++++
 include/media/v4l2-chip-ident.h |  2 ++
 2 files changed, 38 insertions(+)

diff --git a/drivers/media/i2c/saa7115.c b/drivers/media/i2c/saa7115.c
index aa92478..eb2c19d 100644
--- a/drivers/media/i2c/saa7115.c
+++ b/drivers/media/i2c/saa7115.c
@@ -1628,6 +1628,36 @@ static int saa711x_detect_chip(struct i2c_client *client,
 		}
 	}
 
+	/* Check if it is a gm7113c */
+	if (!memcmp(name, "0000", 4)) {
+		chip_id = 0;
+		for (i = 0; i < 4; i++) {
+			chip_id = chip_id << 1;
+			chip_id |= (chip_ver[i] & 0x80) ? 1 : 0;
+		}
+
+		/*
+		 * Note: From the datasheet, only versions 1 and 2
+		 * exists. However, tests on a device labeled as:
+		 *	"GM7113C 1145" returned "10" on all 16 chip
+		 *	version (reg 0x00) reads. So, we need to also
+		 *	accept at least verion 0. For now, let's just
+		 *	assume that a device that returns "0000" for
+		 *	the lower nibble is a gm7113c.
+		 */
+
+		snprintf(name, size, "gm7113c");
+
+		if (!autodetect && strcmp(name, id->name))
+			return -EINVAL;
+
+		v4l_dbg(1, debug, client,
+			"It seems to be a %s chip (%*ph) @ 0x%x.\n",
+			name, 16, chip_ver, client->addr << 1);
+
+		return V4L2_IDENT_GM7113C;
+	}
+
 	/* Chip was not discovered. Return its ID and don't bind */
 	v4l_dbg(1, debug, client, "chip %*ph @ 0x%x is unknown.\n",
 		16, chip_ver, client->addr << 1);
@@ -1657,6 +1687,11 @@ static int saa711x_probe(struct i2c_client *client,
 	if (ident < 0)
 		return ident;
 
+	if (ident == V4L2_IDENT_GM7113C) {
+		v4l_warn(client, "%s not yet supported\n", name);
+		return -ENODEV;
+	}
+
 	strlcpy(client->name, name, sizeof(client->name));
 
 	state = kzalloc(sizeof(struct saa711x_state), GFP_KERNEL);
@@ -1744,6 +1779,7 @@ static const struct i2c_device_id saa711x_id[] = {
 	{ "saa7114", 0 },
 	{ "saa7115", 0 },
 	{ "saa7118", 0 },
+	{ "gm7113c", 0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, saa711x_id);
diff --git a/include/media/v4l2-chip-ident.h b/include/media/v4l2-chip-ident.h
index 4ee125b..ef7e1c4 100644
--- a/include/media/v4l2-chip-ident.h
+++ b/include/media/v4l2-chip-ident.h
@@ -52,6 +52,8 @@ enum {
 	V4L2_IDENT_SAA7115 = 105,
 	V4L2_IDENT_SAA7118 = 108,
 
+	V4L2_IDENT_GM7113C = 140,
+
 	/* module saa7127: reserved range 150-199 */
 	V4L2_IDENT_SAA7127 = 157,
 	V4L2_IDENT_SAA7129 = 159,
-- 
1.8.2.1

Re: [PATCH V2 2/3] saa7115: add detection code for gm7113c

[Ezequiel Garcia]

On Mon, Apr 29, 2013 at 10:41:08PM +0200, Jon Arne Jørgensen wrote:
> Adds a code that (auto)detects gm7113c clones. The auto-detection
> here is not perfect, as, on contrary to what it would be expected
> by looking into its datasheets some devices would return, instead:
> 
> 	saa7115 0-0025: chip 10 10 10 10 10 10 10 10 10 10 10 10 10 10 10 10 @ 0x4a is unknown
> 
> (found on a device labeled as GM7113C 1145 by Ezequiel Garcia)
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> Signed-off-by: Jon Arne Jørgensen <jonarne@jonarne.no>

Your SOB doesn't appear to be correct. See my previous comment.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH V2 3/3] saa7115: Add register setup and config for gm7113c

[Jon Arne Jørgensen]

Signed-off-by: Jon Arne Jørgensen <jonarne@jonarne.no>
---
 drivers/media/i2c/saa7115.c | 48 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/saa7115.c b/drivers/media/i2c/saa7115.c
index eb2c19d..77c13dc 100644
--- a/drivers/media/i2c/saa7115.c
+++ b/drivers/media/i2c/saa7115.c
@@ -126,6 +126,8 @@ static int saa711x_has_reg(const int id, const u8 reg)
 		return 0;
 
 	switch (id) {
+	case V4L2_IDENT_GM7113C:
+		return reg != 0x14 && (reg < 0x18 || reg > 0x1e) && reg < 0x20;
 	case V4L2_IDENT_SAA7113:
 		return reg != 0x14 && (reg < 0x18 || reg > 0x1e) && (reg < 0x20 || reg > 0x3f) &&
 		       reg != 0x5d && reg < 0x63;
@@ -447,6 +449,30 @@ static const unsigned char saa7115_cfg_50hz_video[] = {
 
 /* ============== SAA7715 VIDEO templates (end) =======  */
 
+/* ============== GM7113C VIDEO templates =============  */
+static const unsigned char gm7113c_cfg_60hz_video[] = {
+	R_08_SYNC_CNTL, 0x68,			/* 0xBO: auto detection, 0x68 = NTSC */
+	R_0E_CHROMA_CNTL_1, 0x07,		/* video autodetection is on */
+
+#if 0
+	R_5A_V_OFF_FOR_SLICER, 0x06,		/* standard 60hz value for ITU656 line counting */
+#endif
+	0x00, 0x00
+};
+
+static const unsigned char gm7113c_cfg_50hz_video[] = {
+	R_08_SYNC_CNTL, 0x28,			/* 0x28 = PAL */
+	R_0E_CHROMA_CNTL_1, 0x07,
+
+#if 0
+	R_5A_V_OFF_FOR_SLICER, 0x03,		/* standard 50hz value */
+#endif
+	0x00, 0x00
+};
+
+/* ============== GM7113C VIDEO templates (end) =======  */
+
+
 static const unsigned char saa7115_cfg_vbi_on[] = {
 	R_80_GLOBAL_CNTL_1, 0x00,			/* reset tasks */
 	R_88_POWER_SAVE_ADC_PORT_CNTL, 0xd0,		/* reset scaler */
@@ -927,11 +953,17 @@ static void saa711x_set_v4lstd(struct v4l2_subdev *sd, v4l2_std_id std)
 	// This works for NTSC-M, SECAM-L and the 50Hz PAL variants.
 	if (std & V4L2_STD_525_60) {
 		v4l2_dbg(1, debug, sd, "decoder set standard 60 Hz\n");
-		saa711x_writeregs(sd, saa7115_cfg_60hz_video);
+		if (state->ident == V4L2_IDENT_GM7113C)
+			saa711x_writeregs(sd, gm7113c_cfg_60hz_video);
+		else
+			saa711x_writeregs(sd, saa7115_cfg_60hz_video);
 		saa711x_set_size(sd, 720, 480);
 	} else {
 		v4l2_dbg(1, debug, sd, "decoder set standard 50 Hz\n");
-		saa711x_writeregs(sd, saa7115_cfg_50hz_video);
+		if (state->ident == V4L2_IDENT_GM7113C)
+			saa711x_writeregs(sd, gm7113c_cfg_50hz_video);
+		else
+			saa711x_writeregs(sd, saa7115_cfg_50hz_video);
 		saa711x_set_size(sd, 720, 576);
 	}
 
@@ -944,7 +976,8 @@ static void saa711x_set_v4lstd(struct v4l2_subdev *sd, v4l2_std_id std)
 	011 NTSC N (3.58MHz)            PAL M (3.58MHz)
 	100 reserved                    NTSC-Japan (3.58MHz)
 	*/
-	if (state->ident <= V4L2_IDENT_SAA7113) {
+	if (state->ident <= V4L2_IDENT_SAA7113 ||
+	    state->ident == V4L2_IDENT_GM7113C) {
 		u8 reg = saa711x_read(sd, R_0E_CHROMA_CNTL_1) & 0x8f;
 
 		if (std == V4L2_STD_PAL_M) {
@@ -1215,7 +1248,8 @@ static int saa711x_s_routing(struct v4l2_subdev *sd,
 		input, output);
 
 	/* saa7111/3 does not have these inputs */
-	if (state->ident <= V4L2_IDENT_SAA7113 &&
+	if ((state->ident <= V4L2_IDENT_SAA7113 ||
+	     state->ident == V4L2_IDENT_GM7113C) &&
 	    (input == SAA7115_COMPOSITE4 ||
 	     input == SAA7115_COMPOSITE5)) {
 		return -EINVAL;
@@ -1687,11 +1721,6 @@ static int saa711x_probe(struct i2c_client *client,
 	if (ident < 0)
 		return ident;
 
-	if (ident == V4L2_IDENT_GM7113C) {
-		v4l_warn(client, "%s not yet supported\n", name);
-		return -ENODEV;
-	}
-
 	strlcpy(client->name, name, sizeof(client->name));
 
 	state = kzalloc(sizeof(struct saa711x_state), GFP_KERNEL);
@@ -1742,6 +1771,7 @@ static int saa711x_probe(struct i2c_client *client,
 	case V4L2_IDENT_SAA7111A:
 		saa711x_writeregs(sd, saa7111_init);
 		break;
+	case V4L2_IDENT_GM7113C:
 	case V4L2_IDENT_SAA7113:
 		saa711x_writeregs(sd, saa7113_init);
 		break;
-- 
1.8.2.1

Re: [PATCH V2 3/3] saa7115: Add register setup and config for gm7113c

[Ezequiel Garcia]

On Mon, Apr 29, 2013 at 10:41:09PM +0200, Jon Arne Jørgensen wrote:
> 
> Signed-off-by: Jon Arne Jørgensen <jonarne@jonarne.no>

Every patch *must* have a proper commit message indicating what you're doing
and why you're doing it.

In particular, please add as much information as you can about this new clone decoder.

If you know *why* we have to add those quirks for PAL and NTSC, it would
be nice to add that information to the commit message and maybe even
in a comment somewhere in the code.

> ---
>  drivers/media/i2c/saa7115.c | 48 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 39 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/i2c/saa7115.c b/drivers/media/i2c/saa7115.c
> index eb2c19d..77c13dc 100644
> --- a/drivers/media/i2c/saa7115.c
> +++ b/drivers/media/i2c/saa7115.c
> @@ -126,6 +126,8 @@ static int saa711x_has_reg(const int id, const u8 reg)
>  		return 0;
>  
>  	switch (id) {
> +	case V4L2_IDENT_GM7113C:
> +		return reg != 0x14 && (reg < 0x18 || reg > 0x1e) && reg < 0x20;
>  	case V4L2_IDENT_SAA7113:
>  		return reg != 0x14 && (reg < 0x18 || reg > 0x1e) && (reg < 0x20 || reg > 0x3f) &&
>  		       reg != 0x5d && reg < 0x63;
> @@ -447,6 +449,30 @@ static const unsigned char saa7115_cfg_50hz_video[] = {
>  
>  /* ============== SAA7715 VIDEO templates (end) =======  */
>  
> +/* ============== GM7113C VIDEO templates =============  */
> +static const unsigned char gm7113c_cfg_60hz_video[] = {
> +	R_08_SYNC_CNTL, 0x68,			/* 0xBO: auto detection, 0x68 = NTSC */
> +	R_0E_CHROMA_CNTL_1, 0x07,		/* video autodetection is on */
> +
> +#if 0
> +	R_5A_V_OFF_FOR_SLICER, 0x06,		/* standard 60hz value for ITU656 line counting */
> +#endif

What's the meaning of this ifdef? In general it's better to remove dead
code, or at least to put some fat comment about it.

> +	0x00, 0x00
> +};
> +
> +static const unsigned char gm7113c_cfg_50hz_video[] = {
> +	R_08_SYNC_CNTL, 0x28,			/* 0x28 = PAL */
> +	R_0E_CHROMA_CNTL_1, 0x07,
> +
> +#if 0
> +	R_5A_V_OFF_FOR_SLICER, 0x03,		/* standard 50hz value */
> +#endif

Ditto.

I've tested this with an stk1160/gm7113c device and now
it's working fine (NTSC, PAL-M, PAL-Nc).

I also tested against regressions using stk1160/saa7113 and
em28xx/saa7115 devices.

I must admit I'm quite happy to have this issue finally fixed!

Could you please re-send the whole series, taking account of this
comments. You can add my:

Tested-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

to every patch in the series.

Thanks!
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH V2 3/3] saa7115: Add register setup and config for gm7113c

[Jon Arne Jørgensen]

On Thu, May 02, 2013 at 11:24:07PM -0300, Ezequiel Garcia wrote:
> On Mon, Apr 29, 2013 at 10:41:09PM +0200, Jon Arne Jørgensen wrote:
> > 
> > Signed-off-by: Jon Arne Jørgensen <jonarne@jonarne.no>
> 
> Every patch *must* have a proper commit message indicating what you're doing
> and why you're doing it.
> 
> In particular, please add as much information as you can about this new clone decoder.
> 
> If you know *why* we have to add those quirks for PAL and NTSC, it would
> be nice to add that information to the commit message and maybe even
> in a comment somewhere in the code.
>

I'll fix this, I rushed this patch and wasn't sure about what to write.
 
> > ---
> >  drivers/media/i2c/saa7115.c | 48 ++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 39 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/saa7115.c b/drivers/media/i2c/saa7115.c
> > index eb2c19d..77c13dc 100644
> > --- a/drivers/media/i2c/saa7115.c
> > +++ b/drivers/media/i2c/saa7115.c
> > @@ -126,6 +126,8 @@ static int saa711x_has_reg(const int id, const u8 reg)
> >  		return 0;
> >  
> >  	switch (id) {
> > +	case V4L2_IDENT_GM7113C:
> > +		return reg != 0x14 && (reg < 0x18 || reg > 0x1e) && reg < 0x20;
> >  	case V4L2_IDENT_SAA7113:
> >  		return reg != 0x14 && (reg < 0x18 || reg > 0x1e) && (reg < 0x20 || reg > 0x3f) &&
> >  		       reg != 0x5d && reg < 0x63;
> > @@ -447,6 +449,30 @@ static const unsigned char saa7115_cfg_50hz_video[] = {
> >  
> >  /* ============== SAA7715 VIDEO templates (end) =======  */
> >  
> > +/* ============== GM7113C VIDEO templates =============  */
> > +static const unsigned char gm7113c_cfg_60hz_video[] = {
> > +	R_08_SYNC_CNTL, 0x68,			/* 0xBO: auto detection, 0x68 = NTSC */
> > +	R_0E_CHROMA_CNTL_1, 0x07,		/* video autodetection is on */
> > +
> > +#if 0
> > +	R_5A_V_OFF_FOR_SLICER, 0x06,		/* standard 60hz value for ITU656 line counting */
> > +#endif
> 
> What's the meaning of this ifdef? In general it's better to remove dead
> code, or at least to put some fat comment about it.
> 

The gm7113c doesn't care about registers after 0x1f, I had to test that
this wasn't needed and forgot to remove the dead code before submitting.

I shall fix this also :)

> > +	0x00, 0x00
> > +};
> > +
> > +static const unsigned char gm7113c_cfg_50hz_video[] = {
> > +	R_08_SYNC_CNTL, 0x28,			/* 0x28 = PAL */
> > +	R_0E_CHROMA_CNTL_1, 0x07,
> > +
> > +#if 0
> > +	R_5A_V_OFF_FOR_SLICER, 0x03,		/* standard 50hz value */
> > +#endif
> 
> Ditto.
> 
> I've tested this with an stk1160/gm7113c device and now
> it's working fine (NTSC, PAL-M, PAL-Nc).
> 
> I also tested against regressions using stk1160/saa7113 and
> em28xx/saa7115 devices.
> 
> I must admit I'm quite happy to have this issue finally fixed!
> 
> Could you please re-send the whole series, taking account of this
> comments. You can add my:
> 
> Tested-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> 
> to every patch in the series.
> 

Great, thanks.

> Thanks!
> -- 
> Ezequiel García, Free Electrons
> Embedded Linux, Kernel and Android Engineering
> http://free-electrons.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] saa7115: add detection code for gm7113c

[Ezequiel Garcia]

Hi Jon,

On Mon, Apr 29, 2013 at 10:41:06PM +0200, Jon Arne Jørgensen wrote:
> This is the second version of a patch-set previously posted by Mauro,
> the first verseon was posted on 26 April, and can be found here:
> http://www.spinics.net/lists/linux-media/msg63079.html
> 
> The purpose of this patch is to add support for the gm7113c chip in the saa7115 driver.
> The gm7113c chip is a chinese clone of the Philips/NXP saa7113 chip.
> The chip is found in several cheap usb video capture devices.
> 
>  drivers/media/i2c/saa7115.c     | 207 +++++++++++++++++++++++++++++-----------
>  include/media/v4l2-chip-ident.h |   2 +
>  2 files changed, 155 insertions(+), 54 deletions(-)
> 

Good work! Just some minor comments about the way the patchset
has been submitted.

First of all, this is a very ackward cover letter patch (cover letter is
the zero-index patch). I think you will find easier to use
git-format-patch command like this (just an example):
  
# Create a three-patch patchset:
$ git format-patch -3 --cover-letter --subject "PATCH v2" -o my-v2-patchset

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 0/3] saa7115: add detection code for gm7113c

[Jon Arne Jørgensen]

On Thu, May 02, 2013 at 11:00:41PM -0300, Ezequiel Garcia wrote:
> Hi Jon,
> 
> On Mon, Apr 29, 2013 at 10:41:06PM +0200, Jon Arne Jørgensen wrote:
> > This is the second version of a patch-set previously posted by Mauro,
> > the first verseon was posted on 26 April, and can be found here:
> > http://www.spinics.net/lists/linux-media/msg63079.html
> > 
> > The purpose of this patch is to add support for the gm7113c chip in the saa7115 driver.
> > The gm7113c chip is a chinese clone of the Philips/NXP saa7113 chip.
> > The chip is found in several cheap usb video capture devices.
> > 
> >  drivers/media/i2c/saa7115.c     | 207 +++++++++++++++++++++++++++++-----------
> >  include/media/v4l2-chip-ident.h |   2 +
> >  2 files changed, 155 insertions(+), 54 deletions(-)
> > 
> 
> Good work! Just some minor comments about the way the patchset
> has been submitted.
> 
> First of all, this is a very ackward cover letter patch (cover letter is
> the zero-index patch). I think you will find easier to use
> git-format-patch command like this (just an example):
>   
> # Create a three-patch patchset:
> $ git format-patch -3 --cover-letter --subject "PATCH v2" -o my-v2-patchset
>

I rushed this patch a bit, and borked the git send-email command.
I'll improve my send-email skills. 

> -- 
> Ezequiel García, Free Electrons
> Embedded Linux, Kernel and Android Engineering
> http://free-electrons.com