Re: [REVIEW PATCH 11/41] af9035: basic support for IT9135 v2 chips

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 03/22/2013 11:30 AM, Mauro Carvalho Chehab wrote:
Em Fri, 22 Mar 2013 01:45:30 +0200
Antti Palosaari <crope@xxxxxx> escreveu:

On 03/21/2013 11:54 PM, Mauro Carvalho Chehab wrote:
Em Sun, 10 Mar 2013 04:03:03 +0200
Antti Palosaari <crope@xxxxxx> escreveu:
   static struct ite_config af9035_it913x_config = {
-	.chip_ver = 0x01,
+	.chip_ver = 0x02,

@@ -1153,6 +1161,7 @@ static int af9035_tuner_attach(struct dvb_usb_adapter *adap)
   	case AF9033_TUNER_IT9135_38:
   	case AF9033_TUNER_IT9135_51:
   	case AF9033_TUNER_IT9135_52:
+		af9035_it913x_config.chip_ver = 0x01;

Hmmm... aren't you missing a break here? If not, please add a comment, as
otherwise reviewers think that this is a bug.

It is correct as it was set 0x02 by init. And variable was removed
totally few patches later.

Ok, so please send a patch latter adding a notice about that, like:
   	case AF9033_TUNER_IT9135_52:
		af9035_it913x_config.chip_ver = 0x01;
		/* fall trough */
	case ...

This is a very common practice at the Kernel, as it helps to better
document it.

Also I'm pretty sure some janitor would otherwise send us sooner or later a
patch adding a break there.

I totally agree the issue, but it is totally irrelevant currently as the whole piece of code does not exists anymore.

regards
Antti

--
http://palosaari.fi/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux