linux-kernel - Re: [RFC V2 1/3] [smi2021] Add gm7113c chip to the saa7115 driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130425173948.10ff0a2a@redhat.com>
Date:	Thu, 25 Apr 2013 17:39:48 -0300
From:	Mauro Carvalho Chehab <mchehab@...hat.com>
To:	Jon Arne Jørgensen <jonarne@...arne.no>
Cc:	linux-media@...r.kernel.org, jonjon.arnearne@...il.com,
	linux-kernel@...r.kernel.org, hverkuil@...all.nl,
	elezegarcia@...il.com, mkrufky@...uxtv.org, bjorn@...k.no
Subject: Re: [RFC V2 1/3] [smi2021] Add gm7113c chip to the saa7115 driver

Em Thu, 25 Apr 2013 22:33:20 +0200
Jon Arne Jørgensen <jonarne@...arne.no> escreveu:

> On Thu, Apr 25, 2013 at 05:13:28PM -0300, Mauro Carvalho Chehab wrote:
> > Em Thu, 25 Apr 2013 21:10:18 +0200
> > Jon Arne Jørgensen <jonarne@...arne.no> escreveu:
> > 
> > >  	/* Check whether this chip is part of the saa711x series */
> > > -	if (memcmp(name + 1, "f711", 4)) {
> > > +	if (memcmp(id->name + 1, "gm7113c", 7)) {
> > > +		chip_id = 'c';
> > 
> > There are several issues on the above:
> > 1) "id" may be NULL on autodetect mode;
> > 
> > 2) Why are you adding 1 here?
> >    It should be, instead id->name
> > 
> > 3) memcmp returns 0 if matches. So, the test is wrong.
> >    So, It should be instead:
> > 	if (!memcmp(id->name, "gm7113c", 7)) {
> > 
> > 4) Also, while that works, it seems a little hackish...
> > 
> 
> Oh, this is embarrassing.
> I just tried to change as little as possible in this module to make the
> device work.

No problems. On my experience, quick hacks like that are the
ones that generally have more troubles ;)

> > Something like:
> > 
> > static int saa711x_detect_chip(struct i2c_client *client,
> > 			       struct saa711x_state *state,
> > 			       const struct i2c_device_id *id)
> > {
> > 	int i;
> > 	char chip_id, name[16];
> > 
> > 	/*
> > 	 * Check for gm7113c (a saa7113 clone). Currently, there's no
> > 	 * known way to autodetect it, so boards that use will need to
> > 	 * explicitly fill the id->name field.
> > 	 */
> > 	if (id && !memcmp(id->name, "gm7113c", 7)) {
> > 		state->ident = V4L2_IDENT_GM7113C;
> > 		snprintf(client->name, sizeof(client->name), "%s", id->name);
> > 		return 0;
> > 	}
> > 
> > 	/* Check for Philips/NXP original chips */
> > 	for (i = 0; i < sizeof(name); 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';
> > 
> > 	if (memcmp(name + 1, "f711", 4))
> > 		return -ENODEV;
> > 
> > 	chip_id = name[5];
> > 
> > 	snprintf(client->name, sizeof(client->name), "saa711%c", chip_id);
> > 
> > 	/*
> > 	 * Put here the code that fills state->ident for Philips/NXP chips
> > 	 */
> > ...
> > 
> > 	return 0;
> 
> Yes this seems to be a much better way to do it.
> I will fix my code.

Thanks!
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/